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

DT regression from Improved type inference for object literals #19613

Closed
mhegazy opened this issue Oct 31, 2017 · 6 comments
Closed

DT regression from Improved type inference for object literals #19613

mhegazy opened this issue Oct 31, 2017 · 6 comments
Assignees

Comments

@mhegazy
Copy link
Contributor

mhegazy commented Oct 31, 2017

#19513 broke a DefinitelyTyped test. (Tested by building locally both with this commit and with the previous commit 4895d16) Go to DefinitelyTyped/types/baconjs and run tsc to see the error.

EDIT: Same for d3-selection-multi.

It's hard to say what exactly changed. Something that used to be assignable is no longer assignable.
In baconjs the error is:

baconjs-tests.ts(187,28): error TS2345: Argument of type '(groupedStream: EventStream<string, { id: number; type: string; val?: number; }>) => EventStream<...' is not assignable to parameter of type '(groupedStream: EventStream<{}, { id: number; type: string; val: number; } | { id: number; type: ...'.
  Types of parameters 'groupedStream' and 'groupedStream' are incompatible.
    Type 'EventStream<{}, { id: number; type: string; val: number; } | { id: number; type: string; val?: un...' is not assignable to type 'EventStream<string, { id: number; type: string; val?: number; }>'.
      Type '{}' is not assignable to type 'string'.
baconjs-tests.ts(188,22): error TS7006: Parameter 'groupedStream' implicitly has an 'any' type.
baconjs-tests.ts(188,62): error TS7006: Parameter 'acc' implicitly has an 'any' type.
baconjs-tests.ts(188,67): error TS7006: Parameter 'x' implicitly has an 'any' type.
baconjs-tests.ts(189,22): error TS7006: Parameter 'sum' implicitly has an 'any' type.

In d3-selection-multi the error is:

d3-selection-multi-tests.ts(37,29): error TS2345: Argument of type '(this: HTMLAnchorElement, d: string, i: number, g: ArrayLike<HTMLAnchorElement> | HTMLAnchorEleme...' is not assignable to parameter of type 'ValueFn<HTMLAnchorElement, string, ValueMap<HTMLAnchorElement, string>>'.
  Type '{ id?: undefined; } | { id: string; }' is not assignable to type 'ValueMap<HTMLAnchorElement, string>'.
    Type '{ id?: undefined; }' is not assignable to type 'ValueMap<HTMLAnchorElement, string>'.
      Property 'id' is incompatible with index signature.
        Type 'undefined' is not assignable to type 'string | number | boolean | ValueFn<HTMLAnchorElement, string, string | number | boolean | null> ...'.
d3-selection-multi-tests.ts(92,34): error TS2345: Argument of type '(this: HTMLAnchorElement, d: string, i: number, g: ArrayLike<HTMLAnchorElement> | HTMLAnchorEleme...' is not assignable to parameter of type 'ValueFn<HTMLAnchorElement, string, ValueMap<HTMLAnchorElement, string>>'.
  Type '{ href?: undefined; } | { href: string; }' is not assignable to type 'ValueMap<HTMLAnchorElement, string>'.
    Type '{ href?: undefined; }' is not assignable to type 'ValueMap<HTMLAnchorElement, string>'.
      Property 'href' is incompatible with index signature.
        Type 'undefined' is not assignable to type 'string | number | boolean | ValueFn<HTMLAnchorElement, string, string | number | boolean | null> ...'.
d3-selection-multi-tests.ts(121,31): error TS2345: Argument of type '(this: HTMLAnchorElement, d: string, i: number, g: ArrayLike<HTMLAnchorElement> | HTMLAnchorEleme...' is not assignable to parameter of type 'ValueFn<HTMLAnchorElement, string, ValueMap<HTMLAnchorElement, string>>'.
  Type '{ id?: undefined; } | { id: string; }' is not assignable to type 'ValueMap<HTMLAnchorElement, string>'.
    Type '{ id?: undefined; }' is not assignable to type 'ValueMap<HTMLAnchorElement, string>'.
      Property 'id' is incompatible with index signature.
        Type 'undefined' is not assignable to type 'string | number | boolean | ValueFn<HTMLAnchorElement, string, string | number | boolean | null> ...'.
@mhegazy
Copy link
Contributor Author

mhegazy commented Oct 31, 2017

 var events = [
                {id: 1, type: "add", val: 3},
                {id: 2, type: "add", val: -1},
                {id: 1, type: "add", val: 2},
                {id: 2, type: "cancel"},
                {id: 3, type: "add", val: 2},
                {id: 3, type: "cancel"},
                {id: 1, type: "add", val: 1},
                {id: 1, type: "add", val: 2},
                {id: 1, type: "cancel"}
            ];

events used to be inferred to { id: number; type: string; } | { id: number; type: string; val: number} now it is { id: number; type: string; val: number} | { id: number; type: string; val?: undefined}. which is not assignable from { id: number; type: string; val?: number} any more

seems like the new type is worse..

@ahejlsberg
Copy link
Member

The new type is fine and in fact more correct. The real issue is that we don't consider { ... , p: T | U } assignable to { ... , p: T } | { ... , p: U }. We've gotten this request several times before, it may be time to get it done.

BTW, it's interesting to observe that an object type with one or more properties that have union types should behave equivalently to a union of object types with all possible permutations of those property types. This of course expands exponentially, so real life scenarios typically only have one property for which the expansion takes place, and it may be that those are the only ones we really care about.

@ghost
Copy link

ghost commented Oct 31, 2017

@ahejlsberg The error in d3-selection-multi looks different -- we are inferring a type for a callback and then issuing an error that the inferred type isn't assignable to the declared callback type.

@weswigham
Copy link
Member

weswigham commented Oct 31, 2017

BTW, it's interesting to observe that an object type with one or more properties that have union types should behave equivalently to a union of object types with all possible permutations of those property types. This of course expands exponentially, so real life scenarios typically only have one property for which the expansion takes place, and it may be that those are the only ones we really care about.

#14865 Is open to track this already.

@mhegazy
Copy link
Contributor Author

mhegazy commented Oct 31, 2017

closing in favor of #14865

@mhegazy mhegazy closed this as completed Oct 31, 2017
@ahejlsberg
Copy link
Member

The three new errors in d3-selection-multi-tests.ts are all correct. We now infer a return type of { id: string } | { id?: undefined } where previously we just inferred {}. The more precise return type then causes an error because undefined isn't a permitted property value (probably an oversight since null appears to be permitted).

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants