Skip to content

Instantiate original target type in substituteIndexedMappedType #49205

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 1 commit into from
May 23, 2022

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented May 22, 2022

No tests in this PR as it only affects performance. Have manually verified that issue in #49136 is gone (we go from >4M instantiations to just 172 when compiling the original example).

Fixes #49136.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels May 22, 2022
@ahejlsberg ahejlsberg requested a review from RyanCavanaugh May 22, 2022 15:20
@@ -15798,7 +15798,7 @@ namespace ts {
function substituteIndexedMappedType(objectType: MappedType, index: Type) {
const mapper = createTypeMapper([getTypeParameterFromMappedType(objectType)], [index]);
const templateMapper = combineTypeMappers(objectType.mapper, mapper);
return instantiateType(getTemplateTypeFromMappedType(objectType), templateMapper);
return instantiateType(getTemplateTypeFromMappedType(objectType.target as MappedType || objectType), templateMapper);
Copy link
Contributor

@Andarist Andarist May 22, 2022

Choose a reason for hiding this comment

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

Is .target like a "link" to the original type if you end up creating a new instantiated type from something? 🤔 Would in such a case first "call" to this lack the .target but all future calls would be able to use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, an instantiated type has a target property that links to the original mapped type and a mapper property that provides a mapping from type parameters to corresponding type arguments. The original mapped type doesn't have target and mapper properties. It's target is itself and it's type parameters are also its type arguments, so to speak.

@ahejlsberg ahejlsberg merged commit c592ee7 into main May 23, 2022
@ahejlsberg ahejlsberg deleted the fix49136 branch May 23, 2022 18:07
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Should getTemplateTypeFromMappedType just always look up the .target and it's template first?

@ahejlsberg
Copy link
Member Author

Should getTemplateTypeFromMappedType just always look up the .target and it's template first?

It could, but it would just add another code path and not save anything.

@weswigham
Copy link
Member

I just bring it up because only about half the callers do this target following right now, and I'm unsure if the other half should be, as a rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Perf cliff when computing a base constraint of a circular mapped type
5 participants