Don’t offer this. completions on self, window, global, globalThis. Disambiguate this. completions from others in details requests. (#37652)

andrewbranch · web-flow · commit 23b500cadfb1 · 2020-03-31T12:40:57.000-07:00
* Special-case window, self, global, globalThis methods

* Disambiguate global and this property completions in details requests

* Hide the Map&lt;boolean&gt; implementation

* Update old tests

* Replace SymbolOriginKind stringification with dedicated enum
diff --git a/src/compiler/types.ts b/src/compiler/types.ts
@@ -3608,7 +3608,7 @@ namespace ts {
          * Does not include properties of primitive types.
          */
         /* @internal */ getAllPossiblePropertiesOfTypes(type: readonly Type[]): Symbol[];
-        /* @internal */ resolveName(name: string, location: Node, meaning: SymbolFlags, excludeGlobals: boolean): Symbol | undefined;
+        /* @internal */ resolveName(name: string, location: Node | undefined, meaning: SymbolFlags, excludeGlobals: boolean): Symbol | undefined;
         /* @internal */ getJsxNamespace(location?: Node): string;
 
         /**
diff --git a/src/harness/fourslashImpl.ts b/src/harness/fourslashImpl.ts
@@ -322,6 +322,14 @@ namespace FourSlash {
                 if (!resolvedResult.isLibFile) {
                     this.languageServiceAdapterHost.addScript(Harness.Compiler.defaultLibFileName,
                         Harness.Compiler.getDefaultLibrarySourceFile()!.text, /*isRootFile*/ false);
+
+                    compilationOptions.lib?.forEach(fileName => {
+                        const libFile = Harness.Compiler.getDefaultLibrarySourceFile(fileName);
+                        ts.Debug.assertIsDefined(libFile, `Could not find lib file '${fileName}'`);
+                        if (libFile) {
+                            this.languageServiceAdapterHost.addScript(fileName, libFile.text, /*isRootFile*/ false);
+                        }
+                    });
                 }
             }
             else {
@@ -334,6 +342,14 @@ namespace FourSlash {
                 if (!compilationOptions.noLib) {
                     this.languageServiceAdapterHost.addScript(Harness.Compiler.defaultLibFileName,
                         Harness.Compiler.getDefaultLibrarySourceFile()!.text, /*isRootFile*/ false);
+
+                    compilationOptions.lib?.forEach(fileName => {
+                        const libFile = Harness.Compiler.getDefaultLibrarySourceFile(fileName);
+                        ts.Debug.assertIsDefined(libFile, `Could not find lib file '${fileName}'`);
+                        if (libFile) {
+                            this.languageServiceAdapterHost.addScript(fileName, libFile.text, /*isRootFile*/ false);
+                        }
+                    });
                 }
             }
 
@@ -879,19 +895,19 @@ namespace FourSlash {
                 }
             }
 
-            assert.equal(actual.hasAction, hasAction, `Expected 'hasAction' value '${actual.hasAction}' to equal '${hasAction}'`);
-            assert.equal(actual.isRecommended, isRecommended, `Expected 'isRecommended' value '${actual.isRecommended}' to equal '${isRecommended}'`);
-            assert.equal(actual.source, source, `Expected 'source' value '${actual.source}' to equal '${source}'`);
+            assert.equal(actual.hasAction, hasAction, `Expected 'hasAction' properties to match`);
+            assert.equal(actual.isRecommended, isRecommended, `Expected 'isRecommended' properties to match'`);
+            assert.equal(actual.source, source, `Expected 'source' values to match`);
             assert.equal(actual.sortText, sortText || ts.Completions.SortText.LocationPriority, this.messageAtLastKnownMarker(`Actual entry: ${JSON.stringify(actual)}`));
 
             if (text !== undefined) {
                 const actualDetails = this.getCompletionEntryDetails(actual.name, actual.source)!;
-                assert.equal(ts.displayPartsToString(actualDetails.displayParts), text);
-                assert.equal(ts.displayPartsToString(actualDetails.documentation), documentation || "");
+                assert.equal(ts.displayPartsToString(actualDetails.displayParts), text, "Expected 'text' property to match 'displayParts' string");
+                assert.equal(ts.displayPartsToString(actualDetails.documentation), documentation || "", "Expected 'documentation' property to match 'documentation' display parts string");
                 // TODO: GH#23587
                 // assert.equal(actualDetails.kind, actual.kind);
-                assert.equal(actualDetails.kindModifiers, actual.kindModifiers);
-                assert.equal(actualDetails.source && ts.displayPartsToString(actualDetails.source), sourceDisplay);
+                assert.equal(actualDetails.kindModifiers, actual.kindModifiers, "Expected 'kindModifiers' properties to match");
+                assert.equal(actualDetails.source && ts.displayPartsToString(actualDetails.source), sourceDisplay, "Expected 'sourceDisplay' property to match 'source' display parts string");
                 assert.deepEqual(actualDetails.tags, tags);
             }
             else {
diff --git a/src/harness/fourslashInterfaceImpl.ts b/src/harness/fourslashInterfaceImpl.ts
@@ -845,6 +845,7 @@ namespace FourSlashInterface {
     }
     export namespace Completion {
         export import SortText = ts.Completions.SortText;
+        export import CompletionSource = ts.Completions.CompletionSource;
 
         const functionEntry = (name: string): ExpectedCompletionEntryObject => ({
             name,
diff --git a/src/services/completions.ts b/src/services/completions.ts
@@ -11,6 +11,22 @@ namespace ts.Completions {
     }
     export type Log = (message: string) => void;
 
+    /**
+     * Special values for `CompletionInfo['source']` used to disambiguate
+     * completion items with the same `name`. (Each completion item must
+     * have a unique name/source combination, because those two fields
+     * comprise `CompletionEntryIdentifier` in `getCompletionEntryDetails`.
+     *
+     * When the completion item is an auto-import suggestion, the source
+     * is the module specifier of the suggestion. To avoid collisions,
+     * the values here should not be a module specifier we would ever
+     * generate for an auto-import.
+     */
+    export enum CompletionSource {
+        /** Completions that require `this.` insertion text */
+        ThisProperty = "ThisProperty/"
+    }
+
     const enum SymbolOriginInfoKind {
         ThisType            = 1 << 0,
         SymbolMember        = 1 << 1,
@@ -52,6 +68,11 @@ namespace ts.Completions {
         return !!(origin.kind & SymbolOriginInfoKind.Nullable);
     }
 
+    interface UniqueNameSet {
+        add(name: string): void;
+        has(name: string): boolean;
+    }
+
     /**
      * Map from symbol id -> SymbolOriginInfo.
      * Only populated for symbols that come from other modules.
@@ -298,7 +319,7 @@ namespace ts.Completions {
     function getJSCompletionEntries(
         sourceFile: SourceFile,
         position: number,
-        uniqueNames: Map<true>,
+        uniqueNames: UniqueNameSet,
         target: ScriptTarget,
         entries: Push<CompletionEntry>): void {
         getNameTable(sourceFile).forEach((pos, name) => {
@@ -307,7 +328,8 @@ namespace ts.Completions {
                 return;
             }
             const realName = unescapeLeadingUnderscores(name);
-            if (addToSeen(uniqueNames, realName) && isIdentifierText(realName, target)) {
+            if (!uniqueNames.has(realName) && isIdentifierText(realName, target)) {
+                uniqueNames.add(realName);
                 entries.push({
                     name: realName,
                     kind: ScriptElementKind.warning,
@@ -429,7 +451,12 @@ namespace ts.Completions {
     }
 
     function getSourceFromOrigin(origin: SymbolOriginInfo | undefined): string | undefined {
-        return origin && originIsExport(origin) ? stripQuotes(origin.moduleSymbol.name) : undefined;
+        if (originIsExport(origin)) {
+            return stripQuotes(origin.moduleSymbol.name);
+        }
+        if (origin?.kind === SymbolOriginInfoKind.ThisType) {
+            return CompletionSource.ThisProperty;
+        }
     }
 
     export function getCompletionEntriesFromSymbols(
@@ -448,21 +475,21 @@ namespace ts.Completions {
         recommendedCompletion?: Symbol,
         symbolToOriginInfoMap?: SymbolOriginInfoMap,
         symbolToSortTextMap?: SymbolSortTextMap,
-    ): Map<true> {
+    ): UniqueNameSet {
         const start = timestamp();
         // Tracks unique names.
-        // We don't set this for global variables or completions from external module exports, because we can have multiple of those.
-        // Based on the order we add things we will always see locals first, then globals, then module exports.
+        // Value is set to false for global variables or completions from external module exports, because we can have multiple of those;
+        // true otherwise. Based on the order we add things we will always see locals first, then globals, then module exports.
         // So adding a completion for a local will prevent us from adding completions for external module exports sharing the same name.
-        const uniques = createMap<true>();
+        const uniques = createMap<boolean>();
         for (const symbol of symbols) {
             const origin = symbolToOriginInfoMap ? symbolToOriginInfoMap[getSymbolId(symbol)] : undefined;
             const info = getCompletionEntryDisplayNameForSymbol(symbol, target, origin, kind, !!jsxIdentifierExpected);
             if (!info) {
                 continue;
             }
             const { name, needsConvertPropertyAccess } = info;
-            if (uniques.has(name)) {
+            if (uniques.get(name)) {
                 continue;
             }
 
@@ -484,16 +511,22 @@ namespace ts.Completions {
                 continue;
             }
 
-            // Latter case tests whether this is a global variable.
-            if (!origin && !(symbol.parent === undefined && !some(symbol.declarations, d => d.getSourceFile() === location!.getSourceFile()))) { // TODO: GH#18217
-                uniques.set(name, true);
-            }
+            /** True for locals; false for globals, module exports from other files, `this.` completions. */
+            const shouldShadowLaterSymbols = !origin && !(symbol.parent === undefined && !some(symbol.declarations, d => d.getSourceFile() === location!.getSourceFile()));
+            uniques.set(name, shouldShadowLaterSymbols);
 
             entries.push(entry);
         }
 
         log("getCompletionsAtPosition: getCompletionEntriesFromSymbols: " + (timestamp() - start));
-        return uniques;
+
+        // Prevent consumers of this map from having to worry about
+        // the boolean value. Externally, it should be seen as the
+        // set of all names.
+        return {
+            has: name => uniques.has(name),
+            add: name => uniques.set(name, true),
+        };
     }
 
     function getLabelCompletionAtPosition(node: BreakOrContinueStatement): CompletionInfo | undefined {
@@ -1359,7 +1392,7 @@ namespace ts.Completions {
             // Need to insert 'this.' before properties of `this` type, so only do that if `includeInsertTextCompletions`
             if (preferences.includeCompletionsWithInsertText && scopeNode.kind !== SyntaxKind.SourceFile) {
                 const thisType = typeChecker.tryGetThisTypeAt(scopeNode, /*includeGlobalThis*/ false);
-                if (thisType) {
+                if (thisType && !isProbablyGlobalType(thisType, sourceFile, typeChecker)) {
                     for (const symbol of getPropertiesForCompletion(thisType, typeChecker)) {
                         symbolToOriginInfoMap[getSymbolId(symbol)] = { kind: SymbolOriginInfoKind.ThisType };
                         symbols.push(symbol);
@@ -2723,13 +2756,22 @@ namespace ts.Completions {
         }
     }
 
-    function isNonGlobalDeclaration(declaration: Declaration) {
-        const sourceFile = declaration.getSourceFile();
-        // If the file is not a module, the declaration is global
-        if (!sourceFile.externalModuleIndicator && !sourceFile.commonJsModuleIndicator) {
-            return false;
+    /** Determines if a type is exactly the same type resolved by the global 'self', 'global', or 'globalThis'. */
+    function isProbablyGlobalType(type: Type, sourceFile: SourceFile, checker: TypeChecker) {
+        // The type of `self` and `window` is the same in lib.dom.d.ts, but `window` does not exist in
+        // lib.webworker.d.ts, so checking against `self` is also a check against `window` when it exists.
+        const selfSymbol = checker.resolveName("self", /*location*/ undefined, SymbolFlags.Value, /*excludeGlobals*/ false);
+        if (selfSymbol && checker.getTypeOfSymbolAtLocation(selfSymbol, sourceFile) === type) {
+            return true;
+        }
+        const globalSymbol = checker.resolveName("global", /*location*/ undefined, SymbolFlags.Value, /*excludeGlobals*/ false);
+        if (globalSymbol && checker.getTypeOfSymbolAtLocation(globalSymbol, sourceFile) === type) {
+            return true;
+        }
+        const globalThisSymbol = checker.resolveName("globalThis", /*location*/ undefined, SymbolFlags.Value, /*excludeGlobals*/ false);
+        if (globalThisSymbol && checker.getTypeOfSymbolAtLocation(globalThisSymbol, sourceFile) === type) {
+            return true;
         }
-        // If the file is a module written in TypeScript, it still might be in a `declare global` augmentation
-        return isInJSFile(declaration) || !findAncestor(declaration, isGlobalScopeAugmentation);
+        return false;
     }
 }
diff --git a/src/services/utilities.ts b/src/services/utilities.ts
@@ -2775,5 +2775,19 @@ namespace ts {
         return name.charCodeAt(0) === CharacterCodes._;
     }
 
+    export function isGlobalDeclaration(declaration: Declaration) {
+        return !isNonGlobalDeclaration(declaration);
+    }
+
+    export function isNonGlobalDeclaration(declaration: Declaration) {
+        const sourceFile = declaration.getSourceFile();
+        // If the file is not a module, the declaration is global
+        if (!sourceFile.externalModuleIndicator && !sourceFile.commonJsModuleIndicator) {
+            return false;
+        }
+        // If the file is a module written in TypeScript, it still might be in a `declare global` augmentation
+        return isInJSFile(declaration) || !findAncestor(declaration, isGlobalScopeAugmentation);
+    }
+
     // #endregion
 }
diff --git a/tests/cases/fourslash/completionsJsxAttributeInitializer.ts b/tests/cases/fourslash/completionsJsxAttributeInitializer.ts
@@ -9,8 +9,8 @@ verify.completions({
     marker: "",
     includes: [
         { name: "x", text: "(parameter) x: number", kind: "parameter", insertText: "{x}" },
-        { name: "p", text: "(JSX attribute) p: number", kind: "JSX attribute", insertText: "{this.p}", sortText: completion.SortText.SuggestedClassMembers },
-        { name: "a b", text: '(JSX attribute) "a b": number', kind: "JSX attribute", insertText: '{this["a b"]}', sortText: completion.SortText.SuggestedClassMembers },
+        { name: "p", text: "(JSX attribute) p: number", kind: "JSX attribute", insertText: "{this.p}", sortText: completion.SortText.SuggestedClassMembers, source: completion.CompletionSource.ThisProperty },
+        { name: "a b", text: '(JSX attribute) "a b": number', kind: "JSX attribute", insertText: '{this["a b"]}', sortText: completion.SortText.SuggestedClassMembers, source: completion.CompletionSource.ThisProperty },
     ],
     preferences: {
         includeInsertTextCompletions: true,
diff --git a/tests/cases/fourslash/completionsThisProperties_globalSameName.ts b/tests/cases/fourslash/completionsThisProperties_globalSameName.ts
@@ -0,0 +1,51 @@
+/// <reference path="fourslash.ts" />
+
+// @Filename: globals.d.ts
+//// declare var foot: string;
+
+// @Filename: index.ts
+//// class Service {
+////   foot!: number;
+////   serve() {
+////     foot/**/
+////   }
+//// }
+
+
+verify.completions({
+  marker: "",
+  exact: [
+    "arguments",
+    completion.globalThisEntry,
+    ...completion.globalsVars,
+    {
+      name: "foot",
+      insertText: undefined,
+      kind: "var",
+      kindModifiers: "declare",
+      sortText: completion.SortText.GlobalsOrKeywords,
+      text: "var foot: string"
+    },
+    "Service",
+    completion.undefinedVarEntry,
+    {
+      name: "foot",
+      insertText: "this.foot",
+      kind: "property",
+      sortText: completion.SortText.SuggestedClassMembers,
+      source: completion.CompletionSource.ThisProperty,
+      text: "(property) Service.foot: number"
+    },
+    {
+      name: "serve",
+      insertText: "this.serve",
+      kind: "method",
+      sortText: completion.SortText.SuggestedClassMembers,
+      source: completion.CompletionSource.ThisProperty
+    },
+    ...completion.insideMethodKeywords
+  ],
+  preferences: {
+    includeInsertTextCompletions: true
+  }
+});
diff --git a/tests/cases/fourslash/completionsThisProperties_globalType.ts b/tests/cases/fourslash/completionsThisProperties_globalType.ts
@@ -0,0 +1,32 @@
+/// <reference path="fourslash.ts" />
+
+// #37091
+
+// @lib: dom
+// @allowJs: true
+
+// @Filename: globals.d.ts
+//// declare var global: { console: Console };
+
+// @Filename: index.js
+//// window.f = function() { console/*1*/ }
+//// self.g = function() { console/*2*/ }
+//// global.h = function() { console/*3*/ }
+//// globalThis.i = function() { console/*4*/ }
+
+test.markerNames().forEach(marker => {
+  verify.completions({
+    marker,
+    includes: [{
+      name: "console",
+      insertText: undefined,
+      kind: "var",
+      kindModifiers: "declare",
+      sortText: completion.SortText.GlobalsOrKeywords
+    }]
+  }, {
+    preferences: {
+      includeInsertTextCompletions: true
+    }
+  });
+});
diff --git a/tests/cases/fourslash/completionsThisType.ts b/tests/cases/fourslash/completionsThisType.ts
@@ -14,15 +14,15 @@ verify.completions(
     {
         marker: "",
         includes: [
-            { name: "xyz", text: "(method) C.xyz(): any", kind: "method", insertText: "this.xyz", sortText: completion.SortText.SuggestedClassMembers },
-            { name: "foo bar", text: '(property) C["foo bar"]: number', kind: "property", insertText: 'this["foo bar"]', sortText: completion.SortText.SuggestedClassMembers },
+            { name: "xyz", text: "(method) C.xyz(): any", kind: "method", insertText: "this.xyz", sortText: completion.SortText.SuggestedClassMembers, source: completion.CompletionSource.ThisProperty },
+            { name: "foo bar", text: '(property) C["foo bar"]: number', kind: "property", insertText: 'this["foo bar"]', sortText: completion.SortText.SuggestedClassMembers, source: completion.CompletionSource.ThisProperty },
         ],
         isNewIdentifierLocation: true,
         preferences,
     },
     {
         marker: "f",
-        includes: { name: "x", text: "(property) x: number", kind: "property", insertText: "this.x", sortText: completion.SortText.SuggestedClassMembers },
+        includes: { name: "x", text: "(property) x: number", kind: "property", insertText: "this.x", sortText: completion.SortText.SuggestedClassMembers, source: completion.CompletionSource.ThisProperty },
         preferences,
     },
 );
diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts
@@ -753,6 +753,9 @@ declare namespace completion {
         AutoImportSuggestions = "5",
         JavascriptIdentifiers = "6"
     }
+    export const enum CompletionSource {
+        ThisProperty = "ThisProperty/"
+    }
     export const globalThisEntry: Entry;
     export const undefinedVarEntry: Entry;
     export const globals: ReadonlyArray<Entry>;

Original file line number	Diff line number	Diff line change
`@@ -845,6 +845,7 @@ namespace FourSlashInterface {`
`845`	`845`	`}`
`846`	`846`	`export namespace Completion {`
`847`	`847`	`export import SortText = ts.Completions.SortText;`
	`848`	`+ export import CompletionSource = ts.Completions.CompletionSource;`
`848`	`849`
`849`	`850`	`const functionEntry = (name: string): ExpectedCompletionEntryObject => ({`
`850`	`851`	`name,`