-
Notifications
You must be signed in to change notification settings - Fork 13.1k
Don’t offer this. completions on self, window, global, globalThis. Disambiguate this. completions from others in details requests.
#37652
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
Conversation
amcasey
left a comment
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.
| } | ||
| }); | ||
| if (!compilationOptions.noLib) { | ||
| this.languageServiceAdapterHost.addScript(Harness.Compiler.defaultLibFileName, |
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 below code is identical to the front, is there any possibilities to reuse it?
@amcasey for the purposes of this PR, at least, forget about the conversation I started in Teams. I didn’t really sense that any of us were in agreement about the best behavior, so I left the sorting and the shadowing as it’s always been. There is a rule baked into the architecture of This PR does only two things, despite being related to the other ideas we talked about:
|
| */ | ||
| export enum CompletionSource { | ||
| /** Completions that require `this.` insertion text */ | ||
| ThisProperty = "ThisProperty/" |
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.
is "/" intentional at the end?
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.
Yeah, it’s explained in the comment—we need these values to be impossible values for an auto-import specifier. If someone had a folder named ThisProperty and a baseUrl set, we could conceivably generate an auto-import suggestion whose source is ThisProperty.

Fixes #37091
Fixes #37651
#37091 is interesting, because it exposes that the symbols for, say,
consoleandwindow.consoleare completely different symbols that just happen to have the same type. There’s really no way to know generally that a member on a giventhistype is actually the same thing as a symbol that’s declared globally. And as far as I can tell, there’s nothing special in the type system about symbols likewindow,self, orglobal, so there’s no mechanism to determine that a given type is the global object type.So, I’ve introduced a special case heuristic to reject
this.completions when thethistype is the type of a global by the nameself,global, orglobalThis. It’s possible to construct a strange environment for yourself where this is wrong, but the stakes are fairly low—you won’t getthis.-inserting completions in methods on that type.I also discovered and fixed #37651 along the way.