Lookup retained type nodes in the node builder using correct, specific SymbolFlags meaning #57887
Conversation
…c SymbolFlags meaning
typescript-bot
added
Author: Team
For Milestone Bug
| (entityName.parent.kind === SyntaxKind.QualifiedName && (entityName.parent as QualifiedName).left === entityName) || | ||
| (entityName.parent.kind === SyntaxKind.PropertyAccessExpression && (entityName.parent as PropertyAccessExpression).expression === entityName) || | ||
| (entityName.parent.kind === SyntaxKind.ElementAccessExpression && (entityName.parent as ElementAccessExpression).expression === entityName) |
There was a problem hiding this comment.
I don't know if this is even a good thought, but could entityName.parent.left/entityName.parent.expression be yet another one of these nodes has a left/expression that satisfies these rules, e.g. does this need to be recursively unwrapped to check for the innermost thing?
There was a problem hiding this comment.
These are so if you're the A in A.B, you get a Namespace meaning. Since we always get the leftmost identifier for the lookup, we only ever actually lookup the A. Even in an A.B.C, we only lookup the A. Elsewhere, we use getQualifiedLeftMeaning on the "input" meaning for the whole dotted name to map type/value meaning to a namespace/value meaning, but when we're using this function, we're usually in a state where we don't even have an "input" meaning yet.
There was a problem hiding this comment.
That this codepath is somewhat bespoke is... unfortunate. It basically has to distill "what meaning is done by a resolveName call where elsewhere in the checker for this node" into a single function, so declaration emit can redo the lookup with alias collection on.
There was a problem hiding this comment.
For example, getResolvedSymbol, as used by checkIdentifier (for expression identifiers), uses SymbolFlags.Value | SymbolFlags.ExportValue, while getTypeFromTypeReference uses SymbolFlags.Type* (and SymbolFlags.Namespace for anything left of the last dot), while checkPropertyAccessExpressionOrQualifiedName just expects the LHS namespace to already be resolved and looks up members on it directly, so doesn't concern itself with meanings.
*Unless you're in JSDoc and it gets very complicated with Value meanings mixing in a fallback lookup.
| if (!isDeclarationName(node)) { | ||
| introducesError = true; | ||
| } | ||
| } | ||
| else { | ||
| context.tracker.trackSymbol(sym, context.enclosingDeclaration, SymbolFlags.All); | ||
| context.tracker.trackSymbol(sym, context.enclosingDeclaration, meaning); |
There was a problem hiding this comment.
Just as an FYI to anyone who wants to know why this is the fix for the issue, this call here passes the symbol off to the declaration emitter, which then does a isSymbolAccessible with shouldComputeAliasesToMakeVisible set to true (unlike the speculative one above, where it's false), and uses that alias list to determine what to preserve in the output. When the meaning is inaccurate, you get the wrong symbol from the resolveEntityName lookup, and collect the wrong set of aliases.
|
@DanielRosenwasser I guess this fixes a 5.4 regression, so is a candidate for a patch? |
|
@typescript-bot run dt |
|
Hey @weswigham, the results of running the DT tests are ready. |
|
@weswigham Here are the results of running the user test suite comparing Everything looks good! |
|
@weswigham Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here are the results of running the top-repos suite comparing Everything looks good! |
weswigham
deleted the
fix-declaration-emit-imports-referenced-by-retained-nodes
branch
|
@typescript-bot cherry-pick this to release-5.4 |
|
Hey, @jakebailey! I was unable to cherry-pick this PR. Check the logs at: https://github.com/microsoft/TypeScript/actions/runs/8380436348 |
|
@typescript-bot cherry-pick this to release-5.4 |
|
Hey, @jakebailey! I was unable to cherry-pick this PR. Check the logs at: https://github.com/microsoft/TypeScript/actions/runs/8381864069 |
…c SymbolFlags meaning (microsoft#57887)
|
The cherry-pick for this onto the release branch is... big? #57898 |
Fixes #57861