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

Experiment with always using parameters from base types for derived methods #23911

Open
DanielRosenwasser opened this issue May 4, 2018 · 20 comments · May be fixed by #50542
Open

Experiment with always using parameters from base types for derived methods #23911

DanielRosenwasser opened this issue May 4, 2018 · 20 comments · May be fixed by #50542
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented May 4, 2018

Note some related issues for doing this with properties rather than methods: #3667, #6118, #1373.

Goal

We want to make it easier for users to derive types without rewriting the same parameter signature every time

class Base {
  method(x: number) {
    // ...
  }
}

class Derived extends Base {
  method(x) {
    // 'x' should have the type 'number' here.
  }
}

Potential ideas

  • Only enable in noImplicitAny (doesn't work for default initializers 😞)
  • Revert to any in all locations, opt in with another strictness flag (😞)
  • Something else? 😕

Potential issues

Default initializers with more capable derived types

class A {
  a = 1;
}

class B extends A {
  b = 2;
}

class Base {
  method(x: A) {
    // ...
  }
}

class Derived extends Base {
  method(x = new B) {
    x.b;
    // Today, 'x' has type 'B' which is technically unsound
    // but that's just what we do. Does changing this to 'A' break things?
  }
}

Default initializers that become less-capable via contextual types

class Base {
  method(x: "a" | "b") {
    // ...
  }
}

class Derived extends Base {
  method(x = "a") {
    // We have to make sure 'x' doesn't have type '"a"'
    // which is both unsound and less useful.
  }
}

Distinction between properties and methods

Would this work?

class Base {
  method = (x: number) => {
    // ...
  }
}

class Derived extends Base {
  method(x) {
    // Does 'x' have the type 'number' here?
  }
}

What about this?

class Base {
  method(x: number) {
    // ...
  }
}

class Derived extends Base {
  method = (x) => {
    // Does 'x' have the type 'number' here?
  }
}

Keywords: base type derived contextual contextually inherit methods implicit any

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels May 4, 2018
@DanielRosenwasser DanielRosenwasser changed the title Experiment with always using parameters from base types for all derived methods Experiment with always using parameters from base types for derived methods May 4, 2018
@sandersn
Copy link
Member

sandersn commented May 7, 2018

Couple of quick observations:

  1. Methods have the same implement/override ambiguity that properties do, as discussed in Contextually type implemented properties #6118. The core of any solution to both problems is the balancing of the majority who (I think) want to implement a method strictly against the twin minorities who alternately want to override a method, and those who want to implement a method, but unsoundly. I'm not convinced that restricting the domain to methods helps us choose the right tradeoffs, but it's a good idea to have a discussion since a couple of years have passed.
  2. I'm allergic to any function/method syntax distinctions unless everything else works out and there's no way around it. We should start with the assumption that there is no distinction between properties and methods.

@weswigham
Copy link
Member

I'm allergic to any function/method syntax distinctions unless everything else works out and there's no way around it. We should start with the assumption that there is no distinction between properties and methods.

I agree - non-explicit syntactic switches are not something I enjoy.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented May 16, 2018

class Base {
  method = (x: number) => {
    // ...
  }
}

class Derived extends Base {
  method(x) {
    // Does 'x' have the type 'number' here?
  }
}

Conveniently, this code is illegal. The converse example would just be a case for #10570

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented May 16, 2018

I really want to take this and #10570 as part of a "3.0 is a numerological excuse to make some good breaks" bundle.

Regarding this example

class Base {
  method(x: "a" | "b") {
    // ...
  }
}

class Derived extends Base {
  method(x = "a") {
    // We have to make sure 'x' doesn't have type '"a"'
    // which is both unsound and less useful.
  }
}

I think in any world, Derived#method has the signature (x: string): void. Parameter default expressions have to be widened, and we don't want to contextually type them (i.e. with a literal union type here) because that results in the type of a contextually-typed expression being observable (rules 1 and 2 of contextual typing: don't do that!). And it's certainly legal to write a derived type with a wider method signature than its base, so this code is already quite valid today and it'd be unwise to break it by invalidating anyone calling derived.method("hi")

@donaldpipowitch
Copy link
Contributor

a "3.0 is a numerological excuse to make some good breaks" bundle

Interesting! Are there more breaks in this "bundle"? (Looking for example at #13971)

@unional
Copy link
Contributor

unional commented Feb 3, 2019

Referencing a similar question regarding interfaces on SO

and complication on implementing multiple interfaces:

interface X { foo(i: string): string }
interface Y { foo(x: number): number }

class K implements X, Y {
  foo(x: number): number
  foo(x: string): string
  foo(x: number | string) {
    return x
  }
}

@nevir
Copy link

nevir commented Feb 6, 2019

I'm definitely curious about whether there's been any more progress on this?

At least from my perspective, it seems like inference of method parameters from the interface would be a pretty big ergonomics win (and happens relatively frequently, especially in library/framework code)

@nevir
Copy link

nevir commented Apr 11, 2019

Re:

  1. Methods have the same implement/override ambiguity that properties do, as discussed in Contextually type implemented properties #6118. The core of any solution to both problems is the balancing of the majority who (I think) want to implement a method strictly against the twin minorities who alternately want to override a method, and those who want to implement a method, but unsoundly. I'm not convinced that restricting the domain to methods helps us choose the right tradeoffs, but it's a good idea to have a discussion since a couple of years have passed.

Would a less strict take on it be more palatable? From what I can gather from the various issues/PRs towards this, the primary motivation seems to be improving the ergonomics, and is not so much about strictly checking classes to their parent types (we already have good assertions covering those cases).

For example: infer the types of method arguments when extending or implementsing—but do not check compatibility when explicitly overriding a parameter?

E.g.

interface Base { doThing(a: number): number }

class Foo implements Base {
  doThing(a /* inferred to be number */) {
    return a;
  }
}

class Bar implements Base {
  // already an error here: 
  // "Property 'doThing' in type 'Bar' is not assignable to the same property in base type 'Base'."
  doThing(a: string /* not an error here */) {
    return parseInt(a);
  }
}

class Baz implements Base {
  doThing(a: 1 | 2 | 3 /* not widened to number*/) {
    return a;
  }
}

@nickofthyme
Copy link

Any movement on this issue?

@tadhgmister
Copy link

This much like a lot of very similar suggestions only consider implicit type behaviour changing fairly subtlety. #36165 suggests a way to explicitly say "the same type as expected by base class" which solves the issue of derive types without rewriting the same parameter signature every time.

@falsandtru
Copy link
Contributor

Copying and following upstream definitions is complicated as this: https://github.com/falsandtru/spica/blob/master/src/promise.ts

@svicalifornia
Copy link

svicalifornia commented Aug 2, 2020

@RyanCavanaugh wrote:

Regarding this example

class Base {
  method(x: "a" | "b") {
    // ...
  }
}

class Derived extends Base {
  method(x = "a") {
    // We have to make sure 'x' doesn't have type '"a"'
    // which is both unsound and less useful.
  }
}

I think in any world, Derived#method has the signature (x: string): void. Parameter default expressions have to be widened…

I don't see it that way. Per my long comment on #32082, type inheritance should use the nearest explicitly defined type from the ancestor classes, further narrowed by any interfaces implemented by the subclass. (See the linked comment for the full methodology and rationale.)

If you follow that logic, then Derived#method would have the same signature explicitly defined in superclass Base, which is method(x: "a" | "b") with an inferred return type of void. Parameter x should not simply take any or string — such a presumption forces each subclass to explicitly repeat the narrower inherited type, which is not DRY and indicates that type inheritance provides no value to method parameter types if they can be arbitrarily "widened" by default values. (And why would we widen the value to string and not to any or some other aribitrary type? All of these supersets of the explicit type "a" | "b" are arbitrary, and none of them represent correct type inheritance in classical OOP.)

My proposal for this issue of parameter types of overridden methods in derived classes follows the logic of my comment on #32082. Such methods should be allowed to specify explicit parameter types that are compatible with the class's ancestor classes and interfaces, but in the absence of explicit types, then the types should be inherited from those ancestor classes and interfaces as I proposed, and only inferred from default values if the ancestor classes and interfaces have nothing to say about this method name.

I really want to take this and #10570 as part of a "3.0 is a numerological excuse to make some good breaks" bundle.

This sentiment I agree with. 😄 So… how about TypeScript 4.0?

andreialecu added a commit to andreialecu/reactive-forms that referenced this issue Jan 1, 2021
The readme was asserting something that isn't in the TypeScript language. The issue for tracking support for this would be: microsoft/TypeScript#23911 (comment)

See this playground for confirmation that `val` is inferred as `any` and not `boolean`:
https://www.typescriptlang.org/play?ts=4.1.3#code/IYIwzgLgTsDGEAJYBthjAgYge2wHgBUA+BAbwCgEFRIZ4EB3KASwgFMA1YZAVzYAoAbtwBcCAgEoxg7MwAmAbnIBfcuRRoMAIWBQEbAB7sAdnIw58IXMjbBjJClSatO3PkO4SylKgh+rlIA
@darklight9811
Copy link

Any updates on this?

@haskelcurry
Copy link

Any updates on this?

@muratgozel
Copy link

No Typescript expert but would love to see Typescript implement this somehow. There is also a gain in this when it comes to using module.js and module.d.ts separately, and I don't mean standalone modules or packages, smaller modules inside projects codebase.

Code example for more vibe:

// utility.d.ts

abstract class Utility {
  dirname(importMetaUrl: string): string;
  resolvePath(importMetaUrl: string, relativePath: string): string;

  // ...
}

declare const utility: Utility

export {utility, Utility}
// utility.js

import { URL, fileURLToPath } from 'url'
import path from 'path'

class Utility {
  /* Error: TS7006: Parameter 'importMetaUrl' implicitly has an 'any' type. */
  dirname(importMetaUrl) {
    const __filename = fileURLToPath(importMetaUrl)
    return path.dirname(__filename)
  }

  /* Error: TS7006: Parameter 'importMetaUrl' implicitly has an 'any' type. */
  /* Error: TS7006: Parameter 'relativePath' implicitly has an 'any' type. */
  resolvePath(importMetaUrl, relativePath) {
    return path.resolve(this.dirname(importMetaUrl), relativePath)
  }

  // ...
}

export const utility = new Utility()

@clshortfuse
Copy link

clshortfuse commented Nov 29, 2022

The JSDoc workaround is to use @type on the function itself:

class Joiner {
  /** @param {string[]} strings*/
  join(...strings) { return strings.join('') }
}

class CommaJoiner extends Joiner {
  /** @type {Joiner['join']} */
  join(...strings) {
    return strings.join(',');
  }
}

class NewLineJoiner extends Joiner {
  /** @type {Joiner['join']} */
  join(...strings) {
    return strings.join('\n');
  }
}

Though it would be nice to be able to use super['join'] since the above doesn't work when using mixins.

Not sure what kind of cast works on TS though. None of these do:

interface IJoiner {
  join: (...strings:string[]) => string;
};

class Joiner implements IJoiner {
  join(...strings:string[]) { return strings.join('') }
}

class CommaJoiner extends Joiner implements IJoiner {
  join(...strings){
    return strings.join(',');
  }
}

class NewLineJoiner extends Joiner {
  join!: Joiner['join'];

  join(...strings) {
    return strings.join('\n');
  }
}

The third one is the closest, but TS says it's a duplicate identifier without being able to ignore. Maybe relaxing is the key? If TS could also take hints from JSDocs that could be a solution.

@IlyaSemenov
Copy link

This is already possible:

class NewLineJoiner extends Joiner {
	join: Joiner['join'] = (...strings) => {
		return strings.join('\n')
	}
}

Despite using the arrow function, this supports this and super inside the override method, both in typings and in runtime.

@jonlepage
Copy link

This is already possible:

class NewLineJoiner extends Joiner {
	join: Joiner['join'] = (...strings) => {
		return strings.join('\n')
	}
}

Despite using the arrow function, this supports this and super inside the override method, both in typings and in runtime.

look your memory app 😋 you lost prototype reference and class optimisation for a type feature, this is not suitable.
Type should not affect code base

@tadhgmister
Copy link

This is already possible:

class NewLineJoiner extends Joiner {
	join: Joiner['join'] = (...strings) => {
		return strings.join('\n')
	}
}

Despite using the arrow function, this supports this and super inside the override method, both in typings and in runtime.

look your memory app 😋 you lost prototype reference and class optimisation for a type feature, this is not suitable. Type should not affect code base

then apply it to the arguments and return type:

class NewLineJoiner extends Joiner {
	join(...strings: Parameters<Joiner["join"]>): ReturnType<Joiner["join"]> {
		return strings.join('\n');
	}
}

playground link. And if that feels way to verbose and repetative then possibly support #36165.

myndzi added a commit to Noita-Together/lobby-server that referenced this issue Nov 6, 2023
This appears to be a longstanding TypeScript feature request that's
not seeing much action: microsoft/TypeScript#23911

What I wanted was to automatically gain the type inference for the
message type from the mapped type we're implementing, but what I
got was a lack of reuse of the functions themselves.

The extra redundancy is painful, but these really should be proper methods.
@hediet
Copy link
Member

hediet commented Apr 26, 2024

I think a first implementation could just cover the case when override is present to avoid various problems.
A simple reference implementation could be this:

Whenever this pattern matches syntactically:

class MySubClass extends MyBaseClass {
       ...
	override myMethodName(arg1, ..., argN) {
		...
	}
	...
}

infer the types like this:

class MySubClass extends MyBaseClass {
       ...
	override myMethodName(arg1: Parameters<Joiner["join"]>[1], ..., argN: Parameters<Joiner["join"]>[N]): ReturnType<Joiner["join"]> {
		...
	}
	...
}

Of course this would have to be implemented in a more performant way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet