Fix crash pulling on global types before they're initialized #46471
Conversation
src/compiler/checker.ts
Outdated
| const namespaceName = getFullyQualifiedName(namespace); | ||
| const declarationName = declarationNameToString(right); | ||
| const suggestionForNonexistentModule = getSuggestedSymbolForNonexistentModule(right, namespace); | ||
| const exportedTypeSymbol = getMergedSymbol(getSymbol(getExportsOfSymbol(namespace), right.escapedText, SymbolFlags.Type)); |
There was a problem hiding this comment.
So the original change also added this. This means that while we're doing symbol merging, we're asking for the merged symbol. Is this also a problem?
src/compiler/checker.ts
Outdated
| const containingQualifiedName = isQualifiedName(name) && getContainingQualifiedNameNode(name); | ||
| const canSuggestTypeof = containingQualifiedName && !isTypeOfExpression(containingQualifiedName.parent) && tryGetQualifiedNameAsValue(containingQualifiedName); | ||
| // Can't pull on types if global types aren't initialized yet | ||
| const canSuggestTypeof = !!globalObjectType && containingQualifiedName && !isTypeOfExpression(containingQualifiedName.parent) && tryGetQualifiedNameAsValue(containingQualifiedName); |
There was a problem hiding this comment.
It's also weird that we never check for the appropriate SymbolFlags being passed in, right? There's no way you would've done this work if we strictly were looking for SymbolFlags.Type. I think this was overlooked in the follow-up code since there is a check for meaning & SymbolFlags.Namespace below.
There was a problem hiding this comment.
Yeah. In order to trigger the bug, the symbol has to not exist by the meaning of the original lookup, but exist in the value meaning, and it has to be part of a qualified name on the right-hand of an import equals, at least in the stack trace we're exclusively seeing in reports. I think the implicit assumption in this code was that if the symbol wasn't found in the lookup but exists by value, we must have been looking for a type, which of course is not true when we’re looking for namespaces exclusively.
There is definitely some logical cleanup that should ideally happen around that, but I’m a little concerned because it would fix this particular repro, but is pretty far removed from the root cause, so I’m not confident it would fix all instances of this crash. There could be some other entrypoint to it that doesn’t come from resolving an import = entity name.
Did this in the latest commit, though I’m still not convinced checking for |
…ft#46471) * Add failing test * Dumb fix * Compute error message info more lazily * One more laziness
2 participants
Fixes #45874, which was a regression of #45354. The hard work of this PR was reverse engineering a repro. The fix here is a little dirty and maybe narrower than we want—mostly posting this to get the failing test up and start deciding collectively how we want to address it.