Fix "Cannot find name 'global'. Did you mean 'global'?" #42262

mkantor · 2021-01-08T21:36:23Z

Avoid suggesting the global keyword (from global scope augmentation declarations) as typo fix when resolving names.

ghost · 2021-01-08T21:36:37Z

All CLA requirements met.

mkantor · 2021-01-08T21:36:50Z

As I noted in a comment on #42209 this is my first foray into the compiler so I'm not sure this is the best approach. This check could also happen inside getSuggestedSymbolForNonexistentSymbol, for example, or maybe there's a more general notion of "things that shouldn't be suggested for value positions" that should be captured somewhere, or maybe some other fix I haven't considered.

Let me know!

ExE-Boss · 2021-01-11T13:22:25Z

tests/cases/compiler/spellingSuggestionGlobal1.ts

@@ -0,0 +1,3 @@
+export {}
+declare global { const x: any }
+global.x // should not suggest `global` (GH#42209)


In this case, it should probably suggest globalThis.

Did you miss my reply after you suggested that on the issue?

globalThis.x is not valid here.

It is valid when you do:

declare global { var x: any }

So that case should be covered.

Let's make sure we're on the same page:

Are you saying spellingSuggestionGlobal1.ts (as written, with const x) should produce the error message Cannot find name 'global'. Did you mean 'globalThis'??

If so, I disagree. Users will follow that suggestion and change global.x to globalThis.x, then get another error (Property 'x' does not exist on type 'typeof globalThis'.), then be confused/frustrated and have to research why it doesn't work, then probably end up ignoring the suggestion anyway and write x instead of globalThis.x.

Or are you saying that in situations where x is a known property of globalThis and the user writes global.x that it would be nice to say Cannot find name 'global'. Did you mean 'globalThis'??

If so, I agree, but unfortunately that would be tricky to implement (unless I'm missing something). I'd bet that this situation occurs rarely in the wild, so it might not be worth the complexity cost to maintain a dedicated suggestion for it. I could be wrong, though.

In any case it seems orthogonal to the bugfix in this pull request. Currently if you write something like global.Math the error message won't suggest globalThis, either.

Regardless, I should add a test case that uses var in the global declaration. I'll push a commit for that soon.

src/compiler/checker.ts

sandersn · 2021-01-20T01:08:04Z

src/compiler/checker.ts

+                            // Avoid suggesting "global" if it refers to a global scope augmentation declaration.
+                            if (suggestion && suggestion.valueDeclaration && isAmbientModule(suggestion.valueDeclaration) && isGlobalScopeAugmentation(suggestion.valueDeclaration)) {
+                                suggestion = undefined;
+                            }
                            if (suggestion) {


Since getSuggestedLibForNonExistentName doesn't make sense for global, I would just invert the predicate and move it here. Or possibly return undefined.

Either way, I think it would be better to extract the predicate to its own variable -- that way you can probably delete the comment on the first line of the change.

It's true that getSuggestedLibForNonExistentName doesn't make sense for global, but execution needs to fall through to the else case on line 2049/2053 in order to get any error message at all.

I don't like overwriting suggestion in-place either, though. Maybe it'd be better to duplicate the error call up here?

const isGlobalScopeAugmentationDeclaration = suggestion && suggestion.valueDeclaration && isAmbientModule(suggestion.valueDeclaration) && isGlobalScopeAugmentation(suggestion.valueDeclaration); if (isGlobalScopeAugmentationDeclaration) { if (nameArg) { error(errorLocation, nameNotFoundMessage, diagnosticName(nameArg)); } } else if (suggestion) { // ... }

ooh, I forgot that there weren't any other error calls afterward. I guess it's better to keep it the way it is.

mkantor · 2021-01-20T19:23:14Z

src/compiler/checker.ts

@@ -2028,6 +2028,10 @@ namespace ts {
                        let suggestion: Symbol | undefined;
                        if (suggestedNameNotFoundMessage && suggestionCount < maximumSuggestionCount) {
                            suggestion = getSuggestedSymbolForNonexistentSymbol(originalLocation, name, meaning);
+                            const isGlobalScopeAugmentationDeclaration = suggestion && suggestion.valueDeclaration && isAmbientModule(suggestion.valueDeclaration) && isGlobalScopeAugmentation(suggestion.valueDeclaration);


In hindsight I wish I'd named this suggestionIs..., but the name is already obnoxiously long.

Maybe suggestionIsGlobalDeclaration? suggestionIsGlobalModule? Something else?

in context, I think this is a pretty good name.

sandersn · 2021-01-20T23:19:23Z

src/compiler/checker.ts

@@ -2028,6 +2028,10 @@ namespace ts {
                        let suggestion: Symbol | undefined;
                        if (suggestedNameNotFoundMessage && suggestionCount < maximumSuggestionCount) {
                            suggestion = getSuggestedSymbolForNonexistentSymbol(originalLocation, name, meaning);
+                            const isGlobalScopeAugmentationDeclaration = suggestion && suggestion.valueDeclaration && isAmbientModule(suggestion.valueDeclaration) && isGlobalScopeAugmentation(suggestion.valueDeclaration);


in context, I think this is a pretty good name.

sandersn · 2021-01-20T23:22:37Z

src/compiler/checker.ts

+                            // Avoid suggesting "global" if it refers to a global scope augmentation declaration.
+                            if (suggestion && suggestion.valueDeclaration && isAmbientModule(suggestion.valueDeclaration) && isGlobalScopeAugmentation(suggestion.valueDeclaration)) {
+                                suggestion = undefined;
+                            }
                            if (suggestion) {


ooh, I forgot that there weren't any other error calls afterward. I guess it's better to keep it the way it is.

mkantor added 2 commits January 8, 2021 13:01

Add tests for "Cannot find name 'global'. Did you mean 'global'?"

23631d4

Fix "Cannot find name 'global'. Did you mean 'global'?"

f0af90f

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jan 8, 2021

mkantor mentioned this pull request Jan 8, 2021

Incorrect suggestion: "Cannot find name 'global'. Did you mean 'global'?" #42209

Closed

ExE-Boss reviewed Jan 11, 2021

View reviewed changes

Add an additional test case for spelling suggestions of "global".

da03b72

sandersn requested changes Jan 20, 2021

View reviewed changes

mkantor added 2 commits January 20, 2021 09:50

Merge branch 'master' into global-suggestion

b92e803

Name the boolean for suggestions being global scope augmentations.

fb3af21

mkantor commented Jan 20, 2021

View reviewed changes

sandersn approved these changes Jan 20, 2021

View reviewed changes

sandersn merged commit 424b805 into microsoft:master Jan 20, 2021

mkantor deleted the global-suggestion branch January 22, 2021 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix "Cannot find name 'global'. Did you mean 'global'?" #42262

Fix "Cannot find name 'global'. Did you mean 'global'?" #42262

Uh oh!

mkantor commented Jan 8, 2021

Uh oh!

ghost commented Jan 8, 2021 •

edited by ghost

Loading

Uh oh!

mkantor commented Jan 8, 2021

Uh oh!

ExE-Boss Jan 11, 2021

Uh oh!

mkantor Jan 11, 2021

Uh oh!

ExE-Boss Jan 11, 2021 •

edited

Loading

Uh oh!

mkantor Jan 12, 2021

Uh oh!

Uh oh!

sandersn Jan 20, 2021

Uh oh!

mkantor Jan 20, 2021

Uh oh!

sandersn Jan 20, 2021

Uh oh!

mkantor Jan 20, 2021 •

edited

Loading

Uh oh!

sandersn Jan 20, 2021

Uh oh!

sandersn Jan 20, 2021

Uh oh!

sandersn Jan 20, 2021

Uh oh!

Uh oh!

Fix "Cannot find name 'global'. Did you mean 'global'?" #42262

Fix "Cannot find name 'global'. Did you mean 'global'?" #42262

Uh oh!

Conversation

mkantor commented Jan 8, 2021

Uh oh!

ghost commented Jan 8, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkantor commented Jan 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ExE-Boss Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkantor Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Jan 8, 2021 •

edited by ghost

Loading

ExE-Boss Jan 11, 2021 •

edited

Loading

mkantor Jan 20, 2021 •

edited

Loading