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.

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

Conversation

ahejlsberg
Copy link
Member

This PR fixes a break introduced by #29253. We now exclude index signatures when inferring from an enum object type to a generic mapped type.

Fixes #30218.

...map(getPropertiesOfType(source), getTypeOfSymbol)
]);
inferFromTypes(getUnionType(valueTypes), getTemplateTypeFromMappedType(target));
const indexType = source.symbol && source.symbol.flags & SymbolFlags.Enum && getEnumKind(source.symbol) === EnumKind.Literal ?
Copy link
Member

@weswigham weswigham Apr 16, 2019

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.

]);
inferFromTypes(getUnionType(valueTypes), getTemplateTypeFromMappedType(target));
const indexInfo = getIndexInfoOfType(source, IndexKind.String) || getNonEnumNumberIndexInfo(source);
const sourcePropsType = indexInfo && indexInfo.type || getUnionType(map(getPropertiesOfType(source), getTypeOfSymbol));
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

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!

@ahejlsberg ahejlsberg merged commit a40b08d into master Apr 17, 2019
@ahejlsberg ahejlsberg deleted the fixInferenceToMappedType branch April 17, 2019 22:10
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

Successfully merging this pull request may close these issues.

Inference of the Enum object as a Record fails in TS3.3.3333
2 participants