Skip to content

Fix inference from enum object type to generic mapped type #30944

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 17, 2019
Merged

Fix inference from enum object type to generic mapped type #30944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cc2ea68
Fix inference from enum object type to generic mapped type
ahejlsberg Apr 15, 2019
83f3d2e
Add regression test
ahejlsberg Apr 15, 2019
436f606
Accept new baselines
ahejlsberg Apr 15, 2019
efa16ac
Address CR feedback
ahejlsberg Apr 17, 2019
3435451
Even more succinct
ahejlsberg Apr 17, 2019
50fdecc
One more iteration
ahejlsberg Apr 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14960,12 +14960,11 @@ namespace ts {
}
// If no inferences can be made to K's constraint, infer from a union of the property types
// in the source to the template type X.
const valueTypes = compact([
getIndexTypeOfType(source, IndexKind.String),
getIndexTypeOfType(source, IndexKind.Number),
...map(getPropertiesOfType(source), getTypeOfSymbol)
]);
inferFromTypes(getUnionType(valueTypes), getTemplateTypeFromMappedType(target));
const propTypes = map(getPropertiesOfType(source), getTypeOfSymbol);
Copy link
Member

@weswigham weswigham Apr 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than all this stuff, we shouldn't we just filter the enumNumberIndexInfo singleton from the result of getIndexTypeOfType(source, IndexKind.Number) so intersection-based inferences still work out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that works.

const stringIndexType = getIndexTypeOfType(source, IndexKind.String);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't including the index signature type in the union with the named property types, whereas it was before. Doesn't seem intentional and is potentially an undesirably break, since now properties will be ignored for inference when an index signature exists.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It matches what we do for keyof T, e.g. if T has a numeric index signature, we include number in the index type (and thus forget about any numerically named properties).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but that's not how we operate with homorphic constraints in play, and that erasure of information is typically undesirable during inference. Take when we infer to interface F<T extends string> { kind: T; [idx: string]: string} - never inferring to the kind field's value seems like a mistake to me; even though the index signature "makes the property redundant" in the key space, the information is still there and still quite useful.

IMO, rather than producing a union we should probably be inferring from each value (indexes and properties alike) we find to the target template, rather than producing a union to infer with (and adjusting the inference priority for these inferences to allow a union result to handle the intersection-with-incompatable-index case).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But here we specifically know we're inferring to a non-homomorphic mapped type. And, we're in the final part where we're inferring to the template type, not specific properties.

Copy link
Member

@weswigham weswigham Apr 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but the source is just some object type. If I'm inferring from {x: "x", [idx: string]: string} to {[X in K]: Foo<X, T>}, while technically {[X in string | "x"]: Foo<X, T>} producing {[idx: string]: Foo<string, T>} is... ok, {x: Foo<"x", T>; [idx: string]: Foo<string, T>;} would preserve more data for contextual typing and completions, which is always desirable. And to top that off, if we infer from {x: 42} & {[idx: number]: string} to {[X in K]: Foo<X, T>}, we'd want to be effectively producing {[X in "x" | number]: Foo<X, T>} which is {x: Foo<"x", T>; [idx: number]: Foo<number, T>} and inferring to both is just straight up required.

Especially if you consider Foo<X, T> could be defined along the lines of X extends "x" ? T : never, so the presence of that explicit "x" prop could change its meaning (and thus our final inference result) quite a bit!

const numberIndexInfo = getNonEnumNumberIndexInfo(source);
const numberIndexType = numberIndexInfo && numberIndexInfo.type;
inferFromTypes(getUnionType(append(append(propTypes, stringIndexType), numberIndexType)), getTemplateTypeFromMappedType(target));
return true;
}
return false;
Expand Down
14 changes: 14 additions & 0 deletions tests/baselines/reference/mappedToToIndexSignatureInference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,21 @@
declare const fn: <K extends string, V>(object: { [Key in K]: V }) => object;
declare const a: { [index: string]: number };
fn(a);

// Repro from #30218

declare function enumValues<K extends string, V extends string>(e: Record<K, V>): V[];

enum E { A = 'foo', B = 'bar' }

let x: E[] = enumValues(E);


//// [mappedToToIndexSignatureInference.js]
fn(a);
var E;
(function (E) {
E["A"] = "foo";
E["B"] = "bar";
})(E || (E = {}));
var x = enumValues(E);
23 changes: 23 additions & 0 deletions tests/baselines/reference/mappedToToIndexSignatureInference.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,26 @@ fn(a);
>fn : Symbol(fn, Decl(mappedToToIndexSignatureInference.ts, 0, 13))
>a : Symbol(a, Decl(mappedToToIndexSignatureInference.ts, 1, 13))

// Repro from #30218

declare function enumValues<K extends string, V extends string>(e: Record<K, V>): V[];
>enumValues : Symbol(enumValues, Decl(mappedToToIndexSignatureInference.ts, 2, 6))
>K : Symbol(K, Decl(mappedToToIndexSignatureInference.ts, 6, 28))
>V : Symbol(V, Decl(mappedToToIndexSignatureInference.ts, 6, 45))
>e : Symbol(e, Decl(mappedToToIndexSignatureInference.ts, 6, 64))
>Record : Symbol(Record, Decl(lib.es5.d.ts, --, --))
>K : Symbol(K, Decl(mappedToToIndexSignatureInference.ts, 6, 28))
>V : Symbol(V, Decl(mappedToToIndexSignatureInference.ts, 6, 45))
>V : Symbol(V, Decl(mappedToToIndexSignatureInference.ts, 6, 45))

enum E { A = 'foo', B = 'bar' }
>E : Symbol(E, Decl(mappedToToIndexSignatureInference.ts, 6, 86))
>A : Symbol(E.A, Decl(mappedToToIndexSignatureInference.ts, 8, 8))
>B : Symbol(E.B, Decl(mappedToToIndexSignatureInference.ts, 8, 19))

let x: E[] = enumValues(E);
>x : Symbol(x, Decl(mappedToToIndexSignatureInference.ts, 10, 3))
>E : Symbol(E, Decl(mappedToToIndexSignatureInference.ts, 6, 86))
>enumValues : Symbol(enumValues, Decl(mappedToToIndexSignatureInference.ts, 2, 6))
>E : Symbol(E, Decl(mappedToToIndexSignatureInference.ts, 6, 86))

19 changes: 19 additions & 0 deletions tests/baselines/reference/mappedToToIndexSignatureInference.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,22 @@ fn(a);
>fn : <K extends string, V>(object: { [Key in K]: V; }) => object
>a : { [index: string]: number; }

// Repro from #30218

declare function enumValues<K extends string, V extends string>(e: Record<K, V>): V[];
>enumValues : <K extends string, V extends string>(e: Record<K, V>) => V[]
>e : Record<K, V>

enum E { A = 'foo', B = 'bar' }
>E : E
>A : E.A
>'foo' : "foo"
>B : E.B
>'bar' : "bar"

let x: E[] = enumValues(E);
>x : E[]
>enumValues(E) : E[]
>enumValues : <K extends string, V extends string>(e: Record<K, V>) => V[]
>E : typeof E

8 changes: 8 additions & 0 deletions tests/cases/compiler/mappedToToIndexSignatureInference.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
declare const fn: <K extends string, V>(object: { [Key in K]: V }) => object;
declare const a: { [index: string]: number };
fn(a);

// Repro from #30218

declare function enumValues<K extends string, V extends string>(e: Record<K, V>): V[];

enum E { A = 'foo', B = 'bar' }

let x: E[] = enumValues(E);