Flow: Regression: Unable to override statics of class

[jcready]

declare class Foo<R> extends Array<R>{
  static bind(): Foo<R>;
}

This makes it impossible to provide a flow libdef for Bluebird:

declare class Bluebird$Promise<+R> extends Promise<R>{
  static bind(ctx: any): Bluebird$Promise<void>;
}

http://bluebirdjs.com/docs/api/promise.bind.html

[vkurchatkin]

Indeed, something like this is inherently unsafe, although in this particular case it is a side effect of bind being declared as a property, not as a method, so it might be fixable

[jcready]

Sorry, what is unsafe about it? Bluebird also defines its own bind method (in addition to the static property) which I also don’t understand how that would be unsafe. Extending a class doesn’t just mean adding new properties/methods, overriding existing ones is something people do.

When you say it might be fixable, you mean something in Flow itself would need to be fixed?

[vkurchatkin]

For example:

class A {
    static foo() {}
}

class B extends A {
  static foo(x: string) {
      x.toLowerCase();
    }
}


const X: Class<A> =  B;

X.foo();

If this was allowed, this would lead to a runtime error.

[jcready]

I agree doing something like that would be unsafe, however I never tried to cast Foo to an Array (or B to an A in your example), so I’m still not getting how simply extending a class and changing the signature of a method or static property is unsafe.

[vkurchatkin]

Either assigning B to Class<A> shouldn't be allowed, or overriding a static method. At the moment it is the latter.

[jcready]

So then this is a bug since the latter isn’t unsafe and is a thing people expect to be able to do?

[vkurchatkin]

No, this is not a bug. The latter IS unsafe, if the former is allowed

[jcready]

But the former is the only part that is actually unsafe and not something I wish to do. The latter is something that is A) safe by itself, B) something people expect to be able to do (since it is safe), C) is depended on by existing flow-typed libdefs, and D) used to work in flow but no longer does. I’m not sure how this isn’t considered a regression.

I completely agree that the former is unsafe and I would expect flow to complain if you attempted to write code like that, but that doesn’t make the latter any less safe by itself.

[vkurchatkin]

Again: both are safe on their own, but not together. It's not obvious which one to chose,but if Flow decides to support this, it would have to drop the other rule and associated use cases

[jcready]

if Flow decides to support this, it would have to drop the other rule and associated use cases

Flow did support this. It’s a regression. What are the “other rules and associated use cases” that would need to be dropped?

Doing either should be allowed by flow and should only cause an error if you try to do both. Neither one on their own should cause errors. Being able to extend a class while overriding type signatures of static properties or methods is safe by itself. Being able to assign a class to the type of the class it extended is safe by itself. Flow should only error when you do both.

[jcready]

Flow isn't even consistent in this regard. Every class extends Object implicitly, yet this does not cause any flow errors: Try Flow

declare class Foo {
  static bind(): Foo;
  valueOf(): "flow, you're drunk";
};

class Bar {
  static bind() { return new Bar }
  valueOf() { return 'go home' }
}

But this does: Try Flow

declare class Foo extends Object { // error
  static bind(): Foo;              // error
  valueOf(): "flow, you're drunk"; // error
};

class Bar extends Object {         // error
  static bind() { return new Bar } // error
  valueOf() { return 'go home' }   // error
}

[arabold]

I ran into this problem a while back and due to this problem we're still stuck at flow-bin 0.54.1. From @jcready 's previous comment the behavior of flow doesn't seem right, does it? My question is if there any plan on addressing this in flow or do we have to find a workaround in the flow-typed definition of Bluebird?

[mike-marcacci]

I’d like to seek clarification here on whether or not this is actually being considered a bug. While the current behavior seems completely out of line with the way the extends keyword works (to me at least), it sounds like perhaps this is how the flow team wants to keep it? Is this correct?

[kesor]

have the same problem, this is silly.

import { Transform } from 'stream'

class MyTransformer extends Transform {
  constructor() {
    super({ objectMode: true })
  }
  _transform(chunk: Object, encoding: string, callback: (error: ?Error, data?: Object) => void): void {
    // ...
  }
}

getting the error

 15: export class MyTransformer extends Transform {
                  ^^^^^^^^^ MyTransformer. Cannot extend Transform
  Property `_transform` is incompatible:
           v---------------------------------------------------------------------------------------------------
     31:   _transform (obj: Object, encoding: string, callback: (error: ?Error, data?: Object) => void): void {
    ...:
     37:   }
           ^ function. This type is incompatible with
           v-----------
     21:   _transform (
     22:     chunk: Buffer | string,
     23:     encoding: string,
     24:     (error: ?Error, data?: Buffer | string) => void): void {
    ...:
     27:   }

I don't care that it is not compatible by flow standards, in Node.js this IS the way to use this class - extend and override.

[goodmind]

Workaround is to use mixins instead of extends

[nnmrts]

Okay, then let me turn this error off globally at least. Is that possible? Because this is just nonsense now. You should be able to override class properties in "child" classes by definition. This is one of the major reasons we even have extends. To take a class, copy it and then change a little detail of it.

EDIT:
This isn't a feature request, this is a bug.

class MyClass {
    probablyTrue: boolean;
}

class MyMorePreciseClass extends MyClass {
    probablyTrue: true;
}

The example above errors for no reason whatsoever.

[goodmind]

@nnmrts this errors correctly, it is liskov violation

[nnmrts]

Yeah, it is a Liskov violation, but JavaScript doesn't care about Liskov.

In 99.9% of cases, this:

class MyClass {
    myMethod: (param: string) => number;
    probablyTrue: boolean;
}

class MyMorePreciseClass {
    myMethod: (param: string) => number;
    probablyTrue: true;
}

is the same as this:

class MyClass {
    myMethod: (param: string) => number;
    probablyTrue: boolean;
}

class MyMorePreciseClass extends MyClass {
    probablyTrue: true;
}

In the second example though, I don't have to write down myMethod twice.

JavaScript, eslint and babel isn't complaining here, but flow is. Why?

No one knows, because it's perfectly fine JavaScript.

[goodmind]

Because of variance and Liskov violation, it's supposed to find bugs, not allow them, why even use typechecker then

[Meazythaboss]

Sent from Yahoo Mail on Android

On Sun, Oct 13, 2019 at 5:26 PM, [e-mail] wrote:
Because of variance and Liskov violation, it's supposed to find bugs, not allow them, why even use typechecker then

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

[Meazythaboss]

Sent from Yahoo Mail on Android

On Sun, Oct 13, 2019 at 10:03 PM, [e-mail] wrote:

Sent from Yahoo Mail on Android

On Sun, Oct 13, 2019 at 5:26 PM, [e-mail] wrote:
Because of variance and Liskov violation, it's supposed to find bugs, not allow them, why even use typechecker then

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

[jcready]

I think the misunderstanding here is that the flow maintainers falsely believe that a class which extends another means one is a subtype of the other. This is not the case.