Skip to content

Cache JS inferred class type symbol#33010

Merged
sandersn merged 4 commits into
masterfrom
cache-jsmerged-symbol
Aug 21, 2019
Merged

Cache JS inferred class type symbol#33010
sandersn merged 4 commits into
masterfrom
cache-jsmerged-symbol

Conversation

@sandersn
Copy link
Copy Markdown
Member

@sandersn sandersn commented Aug 21, 2019

Note that many sources merge into a single target, so the source [links] is the one that caches the merged target.

The reason this is a problem is not that many sources merge into a single target, but that both getTypeOfSymbol and getDeclaredTypeOfSymbol
end up calling mergeJSSymbols with the same [source,target] pair. The merge should not happen twice.

Fixes #32997

Note that many sources merge into a single target, so the *source*
[links] is the one that caches the merged target.

The reason this is a problem is not that many sources merge into a
single target, but that both getTypeOfSymbol and getDeclaredTypeOfSymbol
end up calling mergeJSSymbols with the same [source,target] pair. The
merge should not happen twice.
Comment thread src/compiler/types.ts Outdated
typeParameters?: TypeParameter[]; // Type parameters of type alias (undefined if non-generic)
outerTypeParameters?: TypeParameter[]; // Outer type parameters of anonymous object type
instantiations?: Map<Type>; // Instantiations of generic type alias (undefined if non-generic)
inferredClassSymbol?: TransientSymbol; // Symbol of an inferred ES5 constructor function
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ignore whitespace to highlight the actual change
(I added some whitespace on the other lines)

@sandersn
Copy link
Copy Markdown
Member Author

Uggg breaks chainedPrototypeAssignment.ts

A.prototype = B.prototype = { ... }

I'm not convinced this is sane code, though. Accepting the baselines for now while I think about it.

@sandersn
Copy link
Copy Markdown
Member Author

I thought about it. It's not real code, just a test case I came up with, so I don't feel bad about breaking it.

@sandersn
Copy link
Copy Markdown
Member Author

Adding @andrewbranch and @orta in case they want to think about caching in the compiler. =)

Copy link
Copy Markdown
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.

So long as the change to chainedPrototypeAssignment.types seems good to you, I think this is good.

@weswigham
Copy link
Copy Markdown
Member

I'm not convinced this is sane code, though. Accepting the baselines for now while I think about it.

I think the new behavior is arguably more sane - references to both A and B now both reflect the same symbol.

Actually, wait.... they're still different constructors, just with separate prototypes, which means... ick. If A and B have differing signatures, this is going to wholly substitute A with B - ick.

@sandersn I think the inferredClassType cache may need to be a map with the appropriate target symbol id, so the same source can be merged into multiple targets.

@sandersn
Copy link
Copy Markdown
Member Author

@weswigham mind taking another look?

>b : A
>new mod.B() : A
>b : B
>new mod.B() : B
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks better 😄

@weswigham weswigham self-requested a review August 21, 2019 22:23
Copy link
Copy Markdown
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.

❤️

@sandersn sandersn merged commit 3c42760 into master Aug 21, 2019
@sandersn sandersn deleted the cache-jsmerged-symbol branch August 21, 2019 22:36
@sandersn
Copy link
Copy Markdown
Member Author

Hooray! My insane test case continues to work~~ 👽 🦑 🦀

timsuchanek pushed a commit to timsuchanek/TypeScript that referenced this pull request Sep 11, 2019
* Cache JS inferred class type symbol

Note that many sources merge into a single target, so the *source*
[links] is the one that caches the merged target.

The reason this is a problem is not that many sources merge into a
single target, but that both getTypeOfSymbol and getDeclaredTypeOfSymbol
end up calling mergeJSSymbols with the same [source,target] pair. The
merge should not happen twice.

* Remove more verbose debug assertion message

* Fix isJSConstructor check + update baselines

* inferClassSymbol cache now track multiple targets
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JS Crash: global type reference to constructor function

3 participants