Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Methods are not accepted as implementation of abstract members with function type #51261

Open
RedBeard0531 opened this issue Oct 21, 2022 · 6 comments
Labels
Experience Enhancement Noncontroversial enhancements Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@RedBeard0531
Copy link

Bug Report

I am declaring a named type with the signature for an abstract method(ish)* on a base class. But I am not able to use the Signature type directly to declare the method, instead I need to pluck off Parameters<Sig> and ReturnType<Sig> to declare a "real" method. Given how methods usually behave in TS interfaces, this is certainly surprising.

  • To expand on the motivation for doing this: The real-world use case involves a "pluggable" method where some subclasses have a call method that implements the interface, and others do constructor(public readonly call: Sig, ...otherArgs) so that they have an injectable call member. In either case, the consumer of the api just does myObj.call(blah, blah, blah) and it Does The Right Thing. I am declaring the named Sig to avoid repeating it everywhere.

🔎 Search Terms

abstract method member function TS2425

🕗 Version & Regression Information

When did you start seeing this bug occur?

Repros in oldest and nightly in playground.

This is the behavior in every version I tried, and I reviewed the FAQ for entries about _________

"Bugs" that have existed in TS for a long time are very likely to be FAQs; refer to https://github.com/Microsoft/TypeScript/wiki/FAQ#common-bugs-that-arent-bugs

I'll admit this is certainly similar to the third from the last issue in that list:

  • A method and a function property of the same type behave differently.
    • Methods are always bivariant in their argument, while function properties are contravariant in their argument under strictFunctionTypes. More discussion here.

However, that summary and the linked issue are all about bivariance/invariance/contravariance which controls matching semantics for functions with different signatures. In this case, the signatures are identical, so that difference doesn't apply. This appears to be a different difference (at least to me). If this is intentional, perhaps the FAQ should be updated to focus less on variance. Ideally, explaining why even identical signatures are rejected.

Issue #27965 also seems related, but that case is a bit different. It was declaring an actual member, which may exist in JS land on the base, while this issue is specifically about adding an abstract member, which I would expect to be more like adding something to the interface portion of a class type, since it explicitly isn't set by the base in JS. Also that case tried to use any as the "function" type of the member, and I am using an exact-matching function type.

⏯ Playground Link

Playground link with relevant code

💻 Code

type Sig = (n: number) => number

abstract class Base {
    abstract member: Sig;
    abstract method(...args: Parameters<Sig>): ReturnType<Sig> // This works, but is something only a type theorist would love.
}

class DerivedMethods extends Base {
    member(n: number) { return n; } // <---- Error here ☹
    method(n: number) { return n; }
}

class DerivedMembers extends Base {
    member!: Sig;
    method!: Sig; // Note: this works in the opposite direction!
}

🙁 Actual behavior

Class 'Base' defines instance member property 'member', but extended class 'DerivedMethods' defines it as instance member function.

🙂 Expected behavior

Compile without error

RedBeard0531 added a commit to realm/realm-js that referenced this issue Oct 21, 2022
@MartinJohns
Copy link
Contributor

This is working as intended. Methods and properties behave different. The base class defines a property, your implementation is a method.

@RyanCavanaugh
Copy link
Member

I don't think we need to enforce this rule for abstract members -- it doesn't really make any sense.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Help Wanted You can do this Experience Enhancement Noncontroversial enhancements labels Oct 21, 2022
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Oct 21, 2022
@MartinJohns
Copy link
Contributor

@RyanCavanaugh Can't formulate my thoughts properly about this right now (stupid covid), but wouldn't this risk the implementation screwing up an intended variance?

If you allow this, wouldn't it allow this to compile?

abstract class Base {
  abstract member: (n: string | number) => number;
}

class Derived extends Base {
  member(x: number): void {}
}

@whzx5byb
Copy link

@MartinJohns

If you allow this, wouldn't it allow this to compile?

(x: number) => void is not assignable to (n: string | number) => number so it will always error even with function parameter bivariance. I guess you want to write member(x: number): number, right?

@RyanCavanaugh
Copy link
Member

It wouldn't compile. Method bivariance is determined by the target type, not the source type (I had to look this up). You can try it today and see that that error, along with the one about methodness/propertyness, are both issued.

kraenhansen pushed a commit to realm/realm-js that referenced this issue Oct 24, 2022
kraenhansen pushed a commit to realm/realm-js that referenced this issue Oct 26, 2022
RedBeard0531 added a commit to realm/realm-js that referenced this issue Nov 15, 2022
RedBeard0531 added a commit to realm/realm-js that referenced this issue Nov 23, 2022
takameyer pushed a commit to realm/realm-js that referenced this issue Dec 28, 2022
@SystemParadox
Copy link

It may also be worth noting that whilst Parameters and ReturnType work fine for a single call signature, it doesn't work well at all if you want to pass an overloaded function signature.

kraenhansen pushed a commit to realm/realm-js that referenced this issue Mar 1, 2023
kraenhansen pushed a commit to realm/realm-js that referenced this issue Mar 3, 2023
RedBeard0531 added a commit to realm/realm-core that referenced this issue Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experience Enhancement Noncontroversial enhancements Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants