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

Generate a unique type parameter name for each nested type parameter #31544

Merged
merged 5 commits into from
Jul 11, 2019

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented May 22, 2019

Previously we'd generate a name for each declaration we saw, but this neglected to account of the possibility that we see multiple unique instantiations of the same symbol. Now the name is generated in the node builder, rather than deferring to the emitter, so we can have a unique name for each type.

Fixes half of the issues brought to light by #30732.

Fixes #31605.

@ajafff
Copy link
Contributor

ajafff commented Jun 3, 2019

Will this also fix #30373? It looks similar to the other issue.

@weswigham
Copy link
Member Author

Quite possibly. Quickinfo does use the same machinery for printing types.

@weswigham
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 26, 2019

Heya @weswigham, I've started to run the extended test suite on this PR at 6aa8ea4. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 26, 2019

Heya @weswigham, I've started to run the community code test suite on this PR at 6aa8ea4. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 26, 2019

Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 6aa8ea4. You can monitor the build here. It should now contribute to this PR's status checks.

@weswigham
Copy link
Member Author

RWC changes are minor - we now use some _1's in the printout whereas before we didn't (the node builder doesn't have a concept of sibling scopes, so generated names are reserved globally within a node builder invocation).

@weswigham
Copy link
Member Author

DT failure is unrelated (a module seems to have gone missing from DT), so looks good.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@weswigham
Copy link
Member Author

User suite failure is just a new package in the azure-sdk-for-js that hasn't made it into master yet, so is clean. Tests are 💯

@weswigham weswigham requested a review from sandersn July 10, 2019 23:19
@weswigham weswigham merged commit 6839973 into microsoft:master Jul 11, 2019
@weswigham weswigham deleted the revise-type-param-namegen branch July 11, 2019 00:12
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.

Incorrect Recursive Union Emmission
4 participants