Fix reference marking of values merged with unresolvable type-only imports #54799
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
andrewbranch
commented
Jun 27, 2023
src/compiler/checker.ts
Outdated
Comment on lines
4461
to
4469
| const typeOnlyDeclaration = excludeTypeOnlyMeanings && getTypeOnlyAliasDeclaration(symbol); | ||
| const typeOnlyResolution = typeOnlyDeclaration && resolveAlias(typeOnlyDeclaration.symbol); | ||
| let flags = symbol.flags; | ||
| let seenSymbols; | ||
| while (symbol.flags & SymbolFlags.Alias) { | ||
| const target = resolveAlias(symbol); | ||
| if (target === typeOnlyResolution) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
The indentation on this function was messed up, so I recommend viewing it with whitespace changes hidden. These few lines are the core change that I mentioned in the PR description.
There was a problem hiding this comment.
My thoughts exactly 👀
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
andrewbranch
commented
Jun 27, 2023
| exports.x = void 0; | ||
| exports.x = exports.A = void 0; | ||
| var A = a.A; // Error | ||
| exports.A = A; |
There was a problem hiding this comment.
Some illegal merges that were previously counted as unreferenced for purposes of emit are now counted as referenced. Stuff is going to crash either way, and I don’t really think one is particularly more correct than the other.
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 4d07d5f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 4d07d5f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 4d07d5f. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 4d07d5f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 4d07d5f. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:
CompilerComparison Report - main..54799
System
Hosts
Scenarios
TSServerComparison Report - main..54799
System
Hosts
Scenarios
StartupComparison Report - main..54799
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. |
sandersn
approved these changes
Jul 24, 2023
| symbol = target; | ||
| } | ||
| return flags; | ||
| function getSymbolFlags(symbol: Symbol, excludeTypeOnlyMeanings?: boolean, excludeLocalMeanings?: boolean): SymbolFlags { |
There was a problem hiding this comment.
this name makes it sound like all uses of symbol.flags should become getSymbolFlags(symbol). Is that true? It doesn't look like it from the diff.
There was a problem hiding this comment.
I tried to audit all uses of symbol.flags when I first wrote getAllSymbolFlags and did make many substitutions. The remaining uses, as best I could tell, intentionally only look at local meanings. The name getAllSymbolFlags no longer felt appropriate with arguments that explicitly filter them down from “all.” Perhaps a way to say getSymbolFlagsIncludingNonLocalOnes would make a better name.
| typeOnlyDeclarationIsExportStar | ||
| ? resolveExternalModuleName(typeOnlyDeclaration.moduleSpecifier, typeOnlyDeclaration.moduleSpecifier, /*ignoreErrors*/ true) | ||
| : resolveAlias(typeOnlyDeclaration.symbol)); | ||
| const typeOnlyExportStarTargets = typeOnlyDeclarationIsExportStar && typeOnlyResolution ? getExportsOfModule(typeOnlyResolution) : undefined; |
There was a problem hiding this comment.
This name is a little confusing since it's hard to notice that it's the exports of a module. But maybe the property that it's targets of resolution is more important.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In #50853, I identified several cases where a symbol was incorrectly being reported as unusable in emit locations due to being a type-only import, when it was actually a local value merged with a type-only import:
The original code simply looked to see if the symbol resolved to any type-only import or export, which was obviously insufficient for merged symbols. Ideally, we wanted to know which declarations contributed which meanings to the merged symbol’s flags. That information isn’t tracked, though, so in #50853 I implemented a bit of proxy logic to get close: if the yet-unresolved alias symbol has a value meaning, and the type-only import symbol doesn’t, then we know the value meaning could not have come from the type-only import, so it’s safe to use. In code, that was something like
So in the example above,
symbol.flagswould haveAlias|Variable(andVariablecounts as a Value), and the resolution ofAstarting at the type-only import would haveInterface. The new logic correctly allowedAto be referenced as a value.So we were kind of picking apart the pieces of the merged symbol, performing some alias resolution independently on the parts, and using the process of elimination to determine where the
SymbolFlags.Valuemeaning must have come from. (The key thing that makes this work is that separate values are not allowed to merge across modules through an import—e.g., even though a function and a namespace can merge in the same file, you can’t import a functionfand then declare an instantiatednamespace f. That’s always been an error.)However, this logic breaks down if the type-only import cannot be resolved, as in
ts.transpileModule. The symbol returned byresolveAliasfor an unresolvable alias isunknownSymbol, which has all symbol flags, to denote that it could be anything. So looking at the example through the old code again:symbol.flags & SymbolFlags.Valueis still true, but this time,resolveAlias(getTypeOnlyAliasDeclaration(symbol).symbol)isunknownSymbol, which also hasSymbolFlags.Value, so the incorrect assumption is that the value meaning forAmust have come from the type-only import.This PR replaces that strategy with one that collects flags starting at the merged symbol, and stops just before it gets to the resolution of the type-only import.
Fixes #54526