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

Prevent collision of imported type with exported declarations in current module #31231

Merged
merged 2 commits into from Sep 27, 2019

Conversation

@collin5
Copy link
Contributor

@collin5 collin5 commented May 3, 2019

Fixes #31031

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Aug 26, 2019

This behavior seems correct; I’m surprised at the behavior asserted by #7591. Let’s see what breaks?

@typescript-bot test this

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Aug 26, 2019

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

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Aug 26, 2019

@collin5 thanks for your patience here! Can you rebase/merge against master so we can see a proper RWC diff? The source branch is too old for me to be able to figure out if this change is actually affecting things.

From reviewing #7591, it does seem like this behavior was intentional, but it’s incredibly unintuitive. I’d personally love to see it removed but we’ll have to see how it goes in our internal test suite and get others on the team to weigh in whether it’s worth a technically breaking change.

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Aug 26, 2019

Ok, I searched closer through the baselines and found that this legitimately creates errors in

  • firebase-firestore
  • glimmer
  • skype
  • vscode

In total, there are 9 errors. @DanielRosenwasser @RyanCavanaugh thoughts?

@collin5
Copy link
Contributor Author

@collin5 collin5 commented Aug 27, 2019

@andrewbranch rebased. Thank you.

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Sep 13, 2019

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Sep 13, 2019

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

@typescript-bot
Copy link
Collaborator

@typescript-bot typescript-bot commented Sep 13, 2019

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

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Sep 13, 2019

@@ -30089,6 +30089,7 @@ namespace ts {
// Based on symbol.flags we can compute a set of excluded meanings (meaning that resolved alias should not have,
// otherwise it will conflict with some local declaration). Note that in addition to normal flags we include matching SymbolFlags.Export*
// in order to prevent collisions with declarations that were exported from the current module (they still contribute to local names).
symbol = getMergedSymbol(symbol.exportSymbol || symbol);
Copy link
Member

@andrewbranch andrewbranch Sep 17, 2019

Choose a reason for hiding this comment

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

Just to fill in some historical context here: I was curious why this was needed when #7583 and #7591 only deal with the flags on the local symbol. Dug around a bit and found #16766. If you look at the changes in the binder there, information about the export used to be attached to the local symbol when it was exporting a type, namespace, or value, but #16766 removed that info for types and namespaces. So, the info we need for this function here used to exist on the local symbol, but now we have to go to the export symbol to get it. This implementation appears correct (although I wonder if #16766 would have been done differently if the flags had been used here).

@andrewbranch andrewbranch merged commit c0573c5 into microsoft:master Sep 27, 2019
6 of 11 checks passed
@Jessidhia
Copy link

@Jessidhia Jessidhia commented Oct 2, 2019

I hate to be the https://xkcd.com/1172/ person but this is actually something I liberally use to work around --isolatedModules-related limitations when re-exporting types under the same name 🤣

// a.ts
export interface A {}

// index.ts
import { A } from './a'
export type A = A

No big deal, it'll just take some, uh, intensive codemodding.


The best way of dealing with this and correctly be able to elide type-only reexports with --isolatedModules / @babel/preset-typescript would probably be to have an export { type A } from './a' syntax, à la Flow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants