-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Fix checker initialization crash #46973
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The work we do when producing diagnostics sometimes has side effects, like pushing things into caches, which can affect other calculations down the line, so I'm usually wary against produceDiagnostics
checks that aren't immediately around an error
call, as it can the a source of reeeeeaaally hard to track bugs where hover info and error info on a symbol don't match (hover data comes from a non-diagnostics producing checkers, while errors do). Specifically here, there's a nested conditional checkResolvedBlockScopedVariable
that might toss something into a cache which now we'd avoid doing - it's probably fine until proven otherwise, since everything in the guarded branch is supposed to just be to generate an error message, I'd just keep it in mind in case something weird crops up in the future.
@typescript-bot cherry-pick this to release-4.5 |
Heya @andrewbranch, I've started to run the task to cherry-pick this into |
Hey @andrewbranch, I've opened #47005 for you. |
Component commits: 0612e18 Fix checker initialization crash 2064b74 Move checks to a place that makes more sense Co-authored-by: Andrew Branch <[email protected]>
excludeGlobals = false): Symbol | undefined { | ||
return resolveNameHelper(location, name, meaning, nameNotFoundMessage, nameArg, isUse, excludeGlobals, getSymbol); | ||
excludeGlobals = false, | ||
getSpellingSuggstions = true): Symbol | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm obviously very late to this PR but is this argument ironically named? 😛
* Fix checker initialization crash * Move checks to a place that makes more sense
Fixes #46587
The auto import provider was trying to generate spelling suggestions for
IArguments
since it runs withnoLib: true
. Not sure why this happens only sometimes, but (a) there’s no point in trying to come up with spelling suggestions for global symbols, and (b) there’s no point in trying to come up with spelling suggestions for anything inside a non-diagnostics-producing checker.