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

Strict function types #18654

Merged
merged 25 commits into from Oct 2, 2017

Conversation

Projects
None yet
@ahejlsberg
Member

ahejlsberg commented Sep 21, 2017

With this PR we introduce a --strictFunctionTypes mode in which function type parameter positions are checked contravariantly instead of bivariantly. The stricter checking applies to all function types, except those originating in method or construcor declarations. Methods are excluded specifically to ensure generic classes and interfaces (such as Array<T>) continue to mostly relate covariantly. The impact of strictly checking methods would be a much bigger breaking change as a large number of generic types would become invariant (even so, we may continue to explore this stricter mode).

The --strictFunctionTypes switch is part of the --strict family of switches, meaning that it defaults to on in --strict mode. This PR is therefore a breaking change only in --strict mode.

Consider the following example in which Animal is the supertype of Dog and Cat:

declare let f1: (x: Animal) => void;
declare let f2: (x: Dog) => void;
declare let f3: (x: Cat) => void;
f1 = f2;  // Error with --strictFunctionTypes
f2 = f1;  // Ok
f2 = f3;  // Error

The first assignment is permitted in default type checking mode, but flagged as an error in strict function types mode. Intuitively, the default mode permits the assignment because it is possibly sound, whereas strict function types mode makes it an error because it isn't provably sound. In either mode the third assignment is an error because it is never sound.

Another way to describe the example is that the type (x: T) => void is bivariant (i.e. covariant or contravariant) for T in default type checking mode, but contravariant for T in strict function types mode.

Another example:

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

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

animalComparer = dogComparer;  // Ok because of bivariance
dogComparer = animalComparer;  // Ok

In --strictFunctionTypes mode the first assignment is still permitted because compare is declared as a method. Effectively, T is bivariant in Comparer<T> because it is used only in method parameter positions. However, changing compare to be a property with a function type causes stricter checking to take effect:

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

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

animalComparer = dogComparer;  // Error
dogComparer = animalComparer;  // Ok

The first assignment is now an error. Effectively, T is contravariant in Comparer<T> because it is used only in function type parameter positions.

By the way, note that whereas some languages (e.g. C# and Scala) require variance annotations (out/in or +/-), variance emerges naturally from the actual use of a type parameter within a generic type due to TypeScript's structural type system.

We also improve type inference involving contravariant positions in this PR:

function combine<T>(...funcs: ((x: T) => void)[]): (x: T) => void {
    return x => {
        for (const f of funcs) f(x);
    }
}

function animalFunc(x: Animal) {}
function dogFunc(x: Dog) {}

let combined = combine(animalFunc, dogFunc);  // (x: Dog) => void

Above, all inferences for T originate in contravariant positions, and we therefore infer the best common subtype for T. This contrasts with inferences from covariant positions, where we infer the best common supertype. Note that inferences from covariant positions have precedence over inferences from contravariant positions.

Fixes #6102.
Fixes #7472.
Fixes #9514.
Fixes #9765.
Fixes #12498.
Fixes #13248.
Fixes #15579.
Fixes #18337.
Fixes #18466.

Also related are #10717 and #14973.

@ahejlsberg ahejlsberg requested review from mhegazy and DanielRosenwasser Sep 21, 2017

@ahejlsberg ahejlsberg added this to the TypeScript 2.6 milestone Sep 21, 2017

@ahejlsberg ahejlsberg requested review from sandersn and RyanCavanaugh Sep 21, 2017

@sandersn

This comment has been minimized.

Show comment
Hide comment
@sandersn

sandersn Sep 21, 2017

Member

I ran RWC on this and saw changes only in elaborations and order of printing union types. No errors were added or removed, and no types changed.

The changes appear to result from better type caching leading to less elaboration. The only interesting result was from the Azure Framework tests, where one incorrectly duplicated error disappeared entirely, maybe because the whole error was already issued just prior.

I'll run DefinitelyTyped next and look for diffs.

Member

sandersn commented Sep 21, 2017

I ran RWC on this and saw changes only in elaborations and order of printing union types. No errors were added or removed, and no types changed.

The changes appear to result from better type caching leading to less elaboration. The only interesting result was from the Azure Framework tests, where one incorrectly duplicated error disappeared entirely, maybe because the whole error was already issued just prior.

I'll run DefinitelyTyped next and look for diffs.

@ahejlsberg

This comment has been minimized.

Show comment
Hide comment
@ahejlsberg

ahejlsberg Sep 21, 2017

Member

@sandersn When running DefinitelyTyped, be sure to run both with and without --strict. I'm particularly interested in the results with --strict.

Member

ahejlsberg commented Sep 21, 2017

@sandersn When running DefinitelyTyped, be sure to run both with and without --strict. I'm particularly interested in the results with --strict.

@sandersn

This comment has been minimized.

Show comment
Hide comment
@sandersn

sandersn Sep 21, 2017

Member

Ah, good point. I forgot --strict the first time around. First I compiled everything using 0ac8406, the commit before this PR. Then I compiled again with this PR's commits and diffed the changes. Interestingly, a few packages on DefinitelyTyped still have more errors even without --strictFunctionTypes. The errors look like they come from tighter variance checking too, so it's likely that some changes are escaping from behind the flag.

Take a look at the test files for recompose, rx-lite and rx.wamp (which depends on rx-lite). Recompose is clearer to see (the rx* errors are obscured by overloads in rx-lite):

recompose-tests.tsx(248,7): error TS2345: Argument of type 'InferableComponentEnhancerWithProps<{ count: number; } & { update: (state: number) => number; }, {}>' is not assignable to parameter of type 'ComponentEnhancer<any, any> | InferableComponentEnhancerWithProps<{}, {}>'.
  Type 'InferableComponentEnhancerWithProps<{ count: number; } & { update: (state: number) => number; }, {}>' is not assignable to type 'InferableComponentEnhancerWithProps<{}, {}>'.
    Type '{}' is not assignable to type '{ count: number; } & { update: (state: number) => number; }'.
      Type '{}' is not assignable to type '{ count: number; }'.
        Property 'count' is missing in type '{}'.

Notice that the direction of assignability switches, I think because of contravariance.

In total, I noticed changes with

  • recompose
  • rx-lite
  • rx.wamp (for nearly the same reason as rx-lite)
  • jasmine
  • a few other projects had different line numbers for errors in lib.d.ts, which doesn't make sense to me.

I'll take a look at the results with --strictFunctionChecks turned on next. It's only halfway done, but there are already lots of failures, so I'll only provide a summary.

Member

sandersn commented Sep 21, 2017

Ah, good point. I forgot --strict the first time around. First I compiled everything using 0ac8406, the commit before this PR. Then I compiled again with this PR's commits and diffed the changes. Interestingly, a few packages on DefinitelyTyped still have more errors even without --strictFunctionTypes. The errors look like they come from tighter variance checking too, so it's likely that some changes are escaping from behind the flag.

Take a look at the test files for recompose, rx-lite and rx.wamp (which depends on rx-lite). Recompose is clearer to see (the rx* errors are obscured by overloads in rx-lite):

recompose-tests.tsx(248,7): error TS2345: Argument of type 'InferableComponentEnhancerWithProps<{ count: number; } & { update: (state: number) => number; }, {}>' is not assignable to parameter of type 'ComponentEnhancer<any, any> | InferableComponentEnhancerWithProps<{}, {}>'.
  Type 'InferableComponentEnhancerWithProps<{ count: number; } & { update: (state: number) => number; }, {}>' is not assignable to type 'InferableComponentEnhancerWithProps<{}, {}>'.
    Type '{}' is not assignable to type '{ count: number; } & { update: (state: number) => number; }'.
      Type '{}' is not assignable to type '{ count: number; }'.
        Property 'count' is missing in type '{}'.

Notice that the direction of assignability switches, I think because of contravariance.

In total, I noticed changes with

  • recompose
  • rx-lite
  • rx.wamp (for nearly the same reason as rx-lite)
  • jasmine
  • a few other projects had different line numbers for errors in lib.d.ts, which doesn't make sense to me.

I'll take a look at the results with --strictFunctionChecks turned on next. It's only halfway done, but there are already lots of failures, so I'll only provide a summary.

@ahejlsberg

This comment has been minimized.

Show comment
Hide comment
@ahejlsberg

ahejlsberg Sep 22, 2017

Member

@sandersn With the latest commits we revert to always performing a structural comparison when the relationships indicated by getVariances don't hold. In effect, the variance information computed by getVariances is now an indication of which relationships to try first as an optimization, but it doesn't preclude other relationships. In the default checking mode we now behave exactly the same way as before, so I would expect no changes to RWC baselines or DefinitelyTyped. I would expect to see new errors only in --strictFunctionTypes mode.

Member

ahejlsberg commented Sep 22, 2017

@sandersn With the latest commits we revert to always performing a structural comparison when the relationships indicated by getVariances don't hold. In effect, the variance information computed by getVariances is now an indication of which relationships to try first as an optimization, but it doesn't preclude other relationships. In the default checking mode we now behave exactly the same way as before, so I would expect no changes to RWC baselines or DefinitelyTyped. I would expect to see new errors only in --strictFunctionTypes mode.

@@ -2517,7 +2522,7 @@ namespace ts {
return typeReferenceToTypeNode(<TypeReference>type);
}
if (type.flags & TypeFlags.TypeParameter || objectFlags & ObjectFlags.ClassOrInterface) {
const name = symbolToName(type.symbol, context, SymbolFlags.Type, /*expectsIdentifier*/ false);
const name = type.symbol ? symbolToName(type.symbol, context, SymbolFlags.Type, /*expectsIdentifier*/ false) : createIdentifier("?");

This comment has been minimized.

@RyanCavanaugh

RyanCavanaugh Sep 22, 2017

Member

When does this case occur?

@RyanCavanaugh

RyanCavanaugh Sep 22, 2017

Member

When does this case occur?

This comment has been minimized.

@ahejlsberg

ahejlsberg Sep 26, 2017

Member

This happens when we have a synthetic type parameter, such as the type parameters we create for tuple types and the markerXXXType type parameters we use for variance determination.

@ahejlsberg

ahejlsberg Sep 26, 2017

Member

This happens when we have a synthetic type parameter, such as the type parameters we create for tuple types and the markerXXXType type parameters we use for variance determination.

@RyanCavanaugh

This comment has been minimized.

Show comment
Hide comment
@RyanCavanaugh

RyanCavanaugh Sep 22, 2017

Member

LGTM but I'd like to see what happens if we run RWC with this forced on

Member

RyanCavanaugh commented Sep 22, 2017

LGTM but I'd like to see what happens if we run RWC with this forced on

@sandersn

This comment has been minimized.

Show comment
Hide comment
@sandersn

sandersn Sep 22, 2017

Member

With the latest commits, both RWC and DT crash much more due to running out of memory. I only get about 2/3 of the RWC tests to succeed, and DT crashes at 'bookshelf'.

Member

sandersn commented Sep 22, 2017

With the latest commits, both RWC and DT crash much more due to running out of memory. I only get about 2/3 of the RWC tests to succeed, and DT crashes at 'bookshelf'.

@ahejlsberg

This comment has been minimized.

Show comment
Hide comment
@ahejlsberg

ahejlsberg Sep 22, 2017

Member

@sandersn Is that with or without --strictFunctionTypes?

Member

ahejlsberg commented Sep 22, 2017

@sandersn Is that with or without --strictFunctionTypes?

@sandersn

This comment has been minimized.

Show comment
Hide comment
@sandersn

sandersn Sep 22, 2017

Member

With --strictFunctionTypes

Member

sandersn commented Sep 22, 2017

With --strictFunctionTypes

@ahejlsberg

This comment has been minimized.

Show comment
Hide comment
@ahejlsberg

ahejlsberg Sep 22, 2017

Member

@sandersn It is likely because we do more structural comparisons of types. There are enough edge cases (e.g. any or void type arguments) where types don't act exactly according to the gathered variance information, so if the optimized first check of the type arguments doesn't succeed we need to do a structural comparison. We'll have to look at the specific cases, but it's not clear what else we could do.

Member

ahejlsberg commented Sep 22, 2017

@sandersn It is likely because we do more structural comparisons of types. There are enough edge cases (e.g. any or void type arguments) where types don't act exactly according to the gathered variance information, so if the optimized first check of the type arguments doesn't succeed we need to do a structural comparison. We'll have to look at the specific cases, but it's not clear what else we could do.

@ahejlsberg ahejlsberg merged commit 884c72e into master Oct 2, 2017

5 checks passed

TypeScript Test Run typescript_node.4 Build finished.
Details
TypeScript Test Run typescript_node.6 Build finished.
Details
TypeScript Test Run typescript_node.stable Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@ahejlsberg ahejlsberg deleted the strictFunctionTypes branch Oct 2, 2017

@goodmind

This comment has been minimized.

Show comment
Hide comment
@goodmind

goodmind Oct 8, 2017

@ahejlsberg do you have feedback on my latest example?

goodmind commented Oct 8, 2017

@ahejlsberg do you have feedback on my latest example?

@xnnkmd

This comment has been minimized.

Show comment
Hide comment
@xnnkmd

xnnkmd Oct 10, 2017

@ahejlsberg Even though you mention only 3% of type packages are affected, I think the real negative impact is in much higher considering that major packages like angular, express, hapi, cucumber, selenium-webdriver etc. are affected. Hopefully the undocummented "skipLibCheck" option can rescue some projects.

xnnkmd commented Oct 10, 2017

@ahejlsberg Even though you mention only 3% of type packages are affected, I think the real negative impact is in much higher considering that major packages like angular, express, hapi, cucumber, selenium-webdriver etc. are affected. Hopefully the undocummented "skipLibCheck" option can rescue some projects.

@ahejlsberg

This comment has been minimized.

Show comment
Hide comment
@ahejlsberg

ahejlsberg Oct 12, 2017

Member

@xnnkmd The list above doesn't show it, but in practically all cases (including all of the ones you mention) the errors are in the tests associated with the type package, not in the package itself. So, there should be no need to use --skipLibCheck.

Member

ahejlsberg commented Oct 12, 2017

@xnnkmd The list above doesn't show it, but in practically all cases (including all of the ones you mention) the errors are in the tests associated with the type package, not in the package itself. So, there should be no need to use --skipLibCheck.

@styfle

This comment has been minimized.

Show comment
Hide comment
@styfle

styfle Oct 12, 2017

Contributor

the errors are in the tests associated with the type package, not in the package itself

@ahejlsberg Can you post a list of packages where only the package itself is affected? (not the tests)

Contributor

styfle commented Oct 12, 2017

the errors are in the tests associated with the type package, not in the package itself

@ahejlsberg Can you post a list of packages where only the package itself is affected? (not the tests)

@aleksey-bykov

This comment has been minimized.

Show comment
Hide comment
@aleksey-bykov

aleksey-bykov Oct 12, 2017

gentlemen, pardon me, but i don't understand your attitude towards these mighty new features... even if we break 97% of all code out there, even then:

  • it's under the flag --strictFunctionTypes, so if you are scared of it, stay away from it! so if you did stay away from it, you are in the old safe TypeScript and nothing brakes

  • secondly, the code that breaks is ill and it's a good thing that you see it breaks, it means you know where you weak parts are, so please be grateful, will you?

aleksey-bykov commented Oct 12, 2017

gentlemen, pardon me, but i don't understand your attitude towards these mighty new features... even if we break 97% of all code out there, even then:

  • it's under the flag --strictFunctionTypes, so if you are scared of it, stay away from it! so if you did stay away from it, you are in the old safe TypeScript and nothing brakes

  • secondly, the code that breaks is ill and it's a good thing that you see it breaks, it means you know where you weak parts are, so please be grateful, will you?

@sandersn

This comment has been minimized.

Show comment
Hide comment
@sandersn

sandersn Oct 12, 2017

Member

DefinitelyTyped/DefinitelyTyped#20219 fixes the packages on DefinitelyTyped that break with --strictFunctionTypes. Plus it updates the tests for many "popular" packages whose tests will break so you can take a look to see how you might have to update your code.

Packages fixed by DefinitelyTyped/DefinitelyTyped#20219:

  • backbone
  • bootbox
  • cordova-plugin-appbrowser
  • ibm-mobilefirst
  • jointjs
  • jquery
  • knockout
  • lodash
  • ramda
  • react

I also fixed tests in

  • node
  • log4js
  • q
  • react-native
  • react-redux
  • sharepoint
  • webpack

Notes:

  • Here, "popular" just means a random selection of packages I have heard of. I didn't fix hapi's tests, for example, even though I think it's fairly popular.
  • I didn't submit PRs to projects that bundles types with their packages. I don't have a good way to scan all Typescript that eventually ends up on npm.
Member

sandersn commented Oct 12, 2017

DefinitelyTyped/DefinitelyTyped#20219 fixes the packages on DefinitelyTyped that break with --strictFunctionTypes. Plus it updates the tests for many "popular" packages whose tests will break so you can take a look to see how you might have to update your code.

Packages fixed by DefinitelyTyped/DefinitelyTyped#20219:

  • backbone
  • bootbox
  • cordova-plugin-appbrowser
  • ibm-mobilefirst
  • jointjs
  • jquery
  • knockout
  • lodash
  • ramda
  • react

I also fixed tests in

  • node
  • log4js
  • q
  • react-native
  • react-redux
  • sharepoint
  • webpack

Notes:

  • Here, "popular" just means a random selection of packages I have heard of. I didn't fix hapi's tests, for example, even though I think it's fairly popular.
  • I didn't submit PRs to projects that bundles types with their packages. I don't have a good way to scan all Typescript that eventually ends up on npm.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.