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

Option strictMethodType: strictFunctionType for method syntax. #57783

Closed
5 of 6 tasks
Conaclos opened this issue Mar 14, 2024 · 11 comments
Closed
5 of 6 tasks

Option strictMethodType: strictFunctionType for method syntax. #57783

Conaclos opened this issue Mar 14, 2024 · 11 comments

Comments

@Conaclos
Copy link

Conaclos commented Mar 14, 2024

πŸ” Search Terms

  • strictFunctionType
  • strictMethodType
  • bivariant method

βœ… Viability Checklist

⭐ Suggestion

While the TypeScript team has deprecated legacy options in 5.0 and is looking to deprecate other options, I would like to revisit the decision to exclude method signatures from `strictFunctionType'.

The TS community is now more used to the strict subset of TypeScript. And this is now the standard and recommended way to use TypeScript. StrictFunctionType brought a lot of security, but it didn't apply to methods because it broke too much code at the time.

The difference between function properties and methods is not clear to many users. The bivariance of methods can even be seen as a bug from a normal user's point of view. It is also more convenient to use the method syntax, because it is shorter and seems more idiomatic. For these reasons, users don't get the security of `strictFunctionType' when writing interfaces, classes or object types.

I think it is time to introduce a new option strictMethodType, which applies strict variance to methods.
This could also be the opportunity to make method readonly by default under this new option.

To ensure backwards compatibility, the option could be disabled by default for the time being.

πŸ“ƒ Motivating Example

interface Comparer<T> {
    compare(a: T, b: T): number;
}

declare let animalComparer: Comparer<Animal>;
declare let dogComparer: Comparer<Dog>;

animalComparer = dogComparer;  // Error under `strictMethodSyntax`
dogComparer = animalComparer;  // Ok

πŸ’» Use Cases

I want to get the safety benefit of strictFunctionType without loosing readability when writing interfaces and classes.

@Conaclos Conaclos changed the title strictMethodType: strictFunctionType for method syntax. Option strictMethodType: strictFunctionType for method syntax. Mar 14, 2024
@jcalz
Copy link
Contributor

jcalz commented Mar 15, 2024

This wouldn't be a breaking change in existing TypeScript/JavaScript code

Anyone who turns on such a flag would undoubtedly see lots of breakage. Arrays are (unsoundly but conveniently) covariant in TypeScript and lots of code depends on that.

I love soundness but if I introduced a flag that broke everything when turned on, I don't think saying "it'll be off by default" would help much. This would be breaky.

@fatcerberus
Copy link

Particularly since, because of how TS works, enabling the flag for your own project probably requires any dependencies you have to successfully compile against it, even if they don’t need it themselves. With options like this, in a lot of ways they’re only ”optional” in theory.

@Conaclos
Copy link
Author

Anyone who turns on such a flag would undoubtedly see lots of breakage. Arrays are (unsoundly but conveniently) covariant in TypeScript and lots of code depends on that.

I checked This wouldn't be a breaking change [...] because I propose to gate this behind an option.

My guess is that this should be less of an issue now than it was 7 years ago when `strictFunctionType' was introduced.

Arrays are (unsoundly but conveniently) covariant in TypeScript and lots of code depends on that.

Have you some practical examples? I am curious about that.

I love soundness but if I introduced a flag that broke everything when turned on, I don't think saying "it'll be off by default" would help much. This would be breaky.

Still, it makes sense for many projects to have this option enabled.

Particularly since, because of how TS works, enabling the flag for your own project probably requires any dependencies you have to successfully compile against it, even if they don’t need it themselves. With options like this, in a lot of ways they’re only ”optional” in theory.

I am unsure to follow you. This could be checked at usage site? Similarly to strictFunctionType?

@ehoogeveen-medweb
Copy link

Assuming that enabling such an option doesn't break everything, having the option at least allows users to file issues and PRs on dependencies that don't work with it and work toward making the ecosystem compatible.

@jcalz
Copy link
Contributor

jcalz commented Mar 15, 2024

Have you some practical examples? I am curious about that.

So, you're presumably aware that Array has a push() method like

interface Array<T> {
  push(...items: T[]): number;
}

And this would be contravariant with your proposed flag, making Array<T> invariant in T . Then the following idiomatic TS code would break:

interface Animal { name: string }
function animalNames(animals: Animal[]) { return animals.map(x => x.name) }
const animals = [{ name: "Fido", age: 10 }, { name: "Snoopy", age: 5 }];
console.log(animalNames(animals));
// -------------------> ~~~~~~~

because even though every element of animals is an Animal, the array animals would not be an Animal[]. That is sound but would break lots of stuff. Indeed the standard way to avoid making a distributive conditional type is to wrap the type parameter in a one-tuple like

type DistributiveFoo<T> = T extends Bar ? Baz<T> : Qux;
type NonDistributiveFoo<T> = [T] extends [Bar] ? Baz<T> : Qux

And that would stop working and break all the things.

In the docs for --strictFunctionTypes it says:

During development of this feature, we discovered a large number of inherently unsafe class hierarchies, including some in the DOM. Because of this, the setting only applies to functions written in function syntax, not to those in method syntax.

So it's not just Array (does anyone have a DOM example to put in here)?


I checked This wouldn't be a breaking change [...] because I propose to gate this behind an option.

I love soundness but if I introduced a flag that broke everything when turned on, I don't think saying "it'll be off by default" would help much. This would be breaky.

Still, it makes sense for many projects to have this option enabled.

You apparently didn't get my point. Having the flag off by default doesn't mean it's not a breaking change. Anyone who wants to use your option would see breakage. Just having the option exist means that large amounts of TypeScript code (including the standard libraries provided with TypeScript itself) would need to change so that people who turn on the flag only see problems in their own code and not in a large chunk of the libraries they depend on. You're not proposing merely adding a flag; you're proposing adding a flag and rewriting a lot of TypeScript declarations. It really doesn't help if the only way for this not to be a breaking change is if the flag is off. We can do that right now by not introducing the flag.

And again, I'm not saying "this is a bad suggestion", I'm saying "this would be a breaking change". TypeScript does sometimes make breaking changes. Even --strictFunctionTypes was a breaking change. Breaking changes happen. I'm saying that we should be clear about the effects of introducing such a flag.

@RyanCavanaugh
Copy link
Member

Assuming that enabling such an option doesn't break everything

This actually does break everything; it would make it roughly impossible to obtain an array or similar "actually invariant" object.

For example, you can't write

const arr: number[] = [];

because the RHS is never[], so it's illegal to push a number into it.

This can't be fixed with contextual typing of literals since there are other cases like:

// Correct program rejected --
// Invalid, RHS is number[]
const arr: unknown[] = [1, 2, 3].slice();

The underlying problem is there's no available distinction between these two methods:

class ArrayHolder {
  private arr: number[] = [];
  // Safe to covariantly alias the return of this function
  getCopy() {
    return arr.slice();
  }

  // Not safe to covariantly alias the return of this function
  getSelf() {
    return arr;
  }
}

Both are just "number[]" even though they have fundamentally different sound aliasing semantics.

For this flag to even be plausible, there'd need to be a reasonable way to solve this problem.

@Conaclos
Copy link
Author

Most of its issues could be solved by using readonly arrays and carefully redesigning some function signatures (for example replacing T in includes by unknown or by a type parameter that extends T) in order to make readonly arrays covariant.

However, I accept that this could involve large breaking changes. Another possibility could create an exception for arrays. This is what Java is doing by making arrays covariant.

Given the current philosophy of TypeScript, I am no longer sure that this issue has any chance of being accepted.

@ritschwumm
Copy link

@jcalz i'd love to see the breakage in my code. i've carefully avoided bivariance where possible, but i don't feel confident at all that i didn't accidentally mess up somewhere the compiler didn't want to check for me.

@RyanCavanaugh
Copy link
Member

i've carefully avoided bivariance where possible

Your dependencies on bivariance are almost certainly implicit.

Out of the gate, the core lib is broken:

../../github/TypeScript/built/local/lib.es2015.generator.d.ts:21:11 - error TS2430: Interface 'Generator<T, TReturn, TNext>' incorrectly extends interface 'Iterator<T, TReturn, TNext>'.
  Types of property 'return' are incompatible.
    Type '(value: TReturn) => IteratorResult<T, TReturn>' is not assignable to type '(value?: TReturn | undefined) => IteratorResult<T, TReturn>'.
      Types of parameters 'value' and 'value' are incompatible.
        Type 'TReturn | undefined' is not assignable to type 'TReturn'.
          'TReturn' could be instantiated with an arbitrary type which could be unrelated to 'TReturn | undefined'.

21 interface Generator<T = unknown, TReturn = any, TNext = unknown> extends Iterator<T, TReturn, TNext> {
             ~~~~~~~~~

../../github/TypeScript/built/local/lib.es2018.asyncgenerator.d.ts:21:11 - error TS2430: Interface 'AsyncGenerator<T, TReturn, TNext>' incorrectly extends interface 'AsyncIterator<T, TReturn, TNext>'.
  Types of property 'return' are incompatible.
    Type '(value: TReturn | PromiseLike<TReturn>) => Promise<IteratorResult<T, TReturn>>' is not assignable to type '(value?: TReturn | PromiseLike<TReturn> | undefined) => Promise<IteratorResult<T, TReturn>>'.
      Types of parameters 'value' and 'value' are incompatible.
        Type 'TReturn | PromiseLike<TReturn> | undefined' is not assignable to type 'TReturn | PromiseLike<TReturn>'.
          Type 'undefined' is not assignable to type 'TReturn | PromiseLike<TReturn>'.

21 interface AsyncGenerator<T = unknown, TReturn = any, TNext = unknown> extends AsyncIterator<T, TReturn, TNext> {
             ~~~~~~~~~~~~~~

../../github/TypeScript/built/local/lib.es5.d.ts:331:11 - error TS2430: Interface 'CallableFunction' incorrectly extends interface 'Function'.
  Types of property 'apply' are incompatible.
    Type '{ <T, R>(this: (this: T) => R, thisArg: T): R; <T, A extends any[], R>(this: (this: T, ...args: A) => R, thisArg: T, args: A): R; }' is not assignable to type '(this: Function, thisArg: any, argArray?: any) => any'.
      The 'this' types of each signature are incompatible.
        Type 'Function' is not assignable to type '(this: any) => any'.
          Type 'Function' provides no match for the signature '(this: any): any'.

331 interface CallableFunction extends Function {
              ~~~~~~~~~~~~~~~~

../../github/TypeScript/built/local/lib.es5.d.ts:368:11 - error TS2430: Interface 'NewableFunction' incorrectly extends interface 'Function'.
  Types of property 'apply' are incompatible.
    Type '{ <T>(this: new () => T, thisArg: T): void; <T, A extends any[]>(this: new (...args: A) => T, thisArg: T, args: A): void; }' is not assignable to type '(this: Function, thisArg: any, argArray?: any) => any'.
      The 'this' types of each signature are incompatible.
        Type 'Function' is not assignable to type 'new () => any'.
          Type 'Function' provides no match for the signature 'new (): any'.

368 interface NewableFunction extends Function {
              ~~~~~~~~~~~~~~~

The DOM also has hundreds of errors because Events are not meant to be called from supertype aliases. It makes HTMLElement not assignable to Node due to addEventListener, so any DOM-related code gets broken quite hard.

This also breaks correct assumptions about array variance, specifically for readonly arrays. This code is sound by inspection:

interface MiniatureReadonlyArray<T> {
    getMutableCopy(): MiniatureMutableArray<T>;
    [i: number]: T;
}
interface MiniatureMutableArray<T> extends MiniatureReadonlyArray<T> {
    push(arg: T): void;
}
declare const sn_mini: MiniatureReadonlyArray<string | number>;
// Fails (wrong)
let snb_mini: MiniatureReadonlyArray<string | number | boolean> = sn_mini;

But it errors because from TS's perspective, a method returning an invariant type must make the entire type invariant.

We'd need a number of fairly invasive scaffolding features to make this work palatably. As-is, "just" making methods covariant creates a lot of incorrect errors.

You can try out this branch. https://github.com/RyanCavanaugh/TypeScript/tree/strictMethodTypes . It creates a new option "demoStrictMethodTypes"

@Conaclos
Copy link
Author

Conaclos commented Mar 19, 2024

Alternatively we could require explicit notation o make a parameter/return type bivariant.
For instance, we could mark a parameter as covariant (contravariant could be implied) and a return type as contravariant (covariance could be implied):

interface I {
  f(param: out Type): in boolean
}

I proposed a similar mechanism for Eiffel ten years ago.

@ethanresnick
Copy link
Contributor

Would it make sense to do this just for method syntax in object literals?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants