Skip to content

Don't offer import completions in non-module files unless "--module" is set #22951

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.

Sign up for GitHub

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

Merged
3 commits merged into from
Apr 2, 2018
Merged

Don't offer import completions in non-module files unless "--module" is set #22951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21f4a16
Don't offer import completions in non-module files unless "--module" …
Mar 28, 2018
6fad887
Merge branch 'master' into shouldOfferImportCompletions
Mar 30, 2018
d88b884
Even smarter shouldOfferImportCompletions
Mar 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 37 additions & 43 deletions src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,9 @@ namespace ts.Completions {

const enum GlobalsSearch { Continue, Success, Fail }

export function getCompletionsAtPosition(
host: LanguageServiceHost,
typeChecker: TypeChecker,
log: Log,
compilerOptions: CompilerOptions,
sourceFile: SourceFile,
position: number,
allSourceFiles: ReadonlyArray<SourceFile>,
preferences: UserPreferences,
): CompletionInfo | undefined {
export function getCompletionsAtPosition(host: LanguageServiceHost, program: Program, log: Log, sourceFile: SourceFile, position: number, preferences: UserPreferences): CompletionInfo | undefined {
const typeChecker = program.getTypeChecker();
const compilerOptions = program.getCompilerOptions();
if (isInReferenceComment(sourceFile, position)) {
const entries = PathCompletions.getTripleSlashReferenceCompletion(sourceFile, position, compilerOptions, host);
return entries && convertPathCompletions(entries);
Expand All @@ -55,7 +48,7 @@ namespace ts.Completions {
return getLabelCompletionAtPosition(contextToken.parent);
}

const completionData = getCompletionData(typeChecker, log, sourceFile, position, allSourceFiles, preferences, compilerOptions.target);
const completionData = getCompletionData(program, log, sourceFile, position, preferences);
if (!completionData) {
return undefined;
}
Expand Down Expand Up @@ -480,16 +473,9 @@ namespace ts.Completions {
previousToken: Node;
readonly isJsxInitializer: IsJsxInitializer;
}
function getSymbolCompletionFromEntryId(
typeChecker: TypeChecker,
log: (message: string) => void,
compilerOptions: CompilerOptions,
sourceFile: SourceFile,
position: number,
{ name, source }: CompletionEntryIdentifier,
allSourceFiles: ReadonlyArray<SourceFile>,
function getSymbolCompletionFromEntryId(program: Program, log: Log, sourceFile: SourceFile, position: number, { name, source }: CompletionEntryIdentifier,
): SymbolCompletion | { type: "request", request: Request } | { type: "none" } {
const completionData = getCompletionData(typeChecker, log, sourceFile, position, allSourceFiles, { includeCompletionsForModuleExports: true, includeCompletionsWithInsertText: true }, compilerOptions.target);
const completionData = getCompletionData(program, log, sourceFile, position, { includeCompletionsForModuleExports: true, includeCompletionsWithInsertText: true });
if (!completionData) {
return { type: "none" };
}
Expand All @@ -505,7 +491,7 @@ namespace ts.Completions {
// completion entry.
return firstDefined<Symbol, SymbolCompletion>(symbols, (symbol): SymbolCompletion => { // TODO: Shouldn't need return type annotation (GH#12632)
const origin = symbolToOriginInfoMap[getSymbolId(symbol)];
const info = getCompletionEntryDisplayNameForSymbol(symbol, compilerOptions.target, origin, completionKind);
const info = getCompletionEntryDisplayNameForSymbol(symbol, program.getCompilerOptions().target, origin, completionKind);
return info && info.name === name && getSourceFromOrigin(origin) === source ? { type: "symbol" as "symbol", symbol, location, symbolToOriginInfoMap, previousToken, isJsxInitializer } : undefined;
}) || { type: "none" };
}
Expand All @@ -525,18 +511,17 @@ namespace ts.Completions {

export function getCompletionEntryDetails(
program: Program,
log: (message: string) => void,
compilerOptions: CompilerOptions,
log: Log,
sourceFile: SourceFile,
position: number,
entryId: CompletionEntryIdentifier,
allSourceFiles: ReadonlyArray<SourceFile>,
host: LanguageServiceHost,
formatContext: formatting.FormatContext,
getCanonicalFileName: GetCanonicalFileName,
preferences: UserPreferences,
): CompletionEntryDetails {
const typeChecker = program.getTypeChecker();
const compilerOptions = program.getCompilerOptions();
const { name } = entryId;

const contextToken = findPrecedingToken(position, sourceFile);
Expand All @@ -548,7 +533,7 @@ namespace ts.Completions {
}

// Compute all the completion symbols again.
const symbolCompletion = getSymbolCompletionFromEntryId(typeChecker, log, compilerOptions, sourceFile, position, entryId, allSourceFiles);
const symbolCompletion = getSymbolCompletionFromEntryId(program, log, sourceFile, position, entryId);
switch (symbolCompletion.type) {
case "request": {
const { request } = symbolCompletion;
Expand All @@ -565,7 +550,7 @@ namespace ts.Completions {
}
case "symbol": {
const { symbol, location, symbolToOriginInfoMap, previousToken } = symbolCompletion;
const { codeActions, sourceDisplay } = getCompletionEntryCodeActionsAndSourceDisplay(symbolToOriginInfoMap, symbol, program, typeChecker, host, compilerOptions, sourceFile, previousToken, formatContext, getCanonicalFileName, allSourceFiles, preferences);
const { codeActions, sourceDisplay } = getCompletionEntryCodeActionsAndSourceDisplay(symbolToOriginInfoMap, symbol, program, typeChecker, host, compilerOptions, sourceFile, previousToken, formatContext, getCanonicalFileName, program.getSourceFiles(), preferences);
return createCompletionDetailsForSymbol(symbol, typeChecker, sourceFile, location, codeActions, sourceDisplay);
}
case "none":
Expand Down Expand Up @@ -642,16 +627,8 @@ namespace ts.Completions {
return { sourceDisplay: [textPart(moduleSpecifier)], codeActions: [codeAction] };
}

export function getCompletionEntrySymbol(
typeChecker: TypeChecker,
log: (message: string) => void,
compilerOptions: CompilerOptions,
sourceFile: SourceFile,
position: number,
entryId: CompletionEntryIdentifier,
allSourceFiles: ReadonlyArray<SourceFile>,
): Symbol | undefined {
const completion = getSymbolCompletionFromEntryId(typeChecker, log, compilerOptions, sourceFile, position, entryId, allSourceFiles);
export function getCompletionEntrySymbol(program: Program, log: Log, sourceFile: SourceFile, position: number, entryId: CompletionEntryIdentifier): Symbol | undefined {
const completion = getSymbolCompletionFromEntryId(program, log, sourceFile, position, entryId);
return completion.type === "symbol" ? completion.symbol : undefined;
}

Expand Down Expand Up @@ -760,14 +737,14 @@ namespace ts.Completions {
}

function getCompletionData(
typeChecker: TypeChecker,
program: Program,
log: (message: string) => void,
sourceFile: SourceFile,
position: number,
allSourceFiles: ReadonlyArray<SourceFile>,
preferences: Pick<UserPreferences, "includeCompletionsForModuleExports" | "includeCompletionsWithInsertText">,
target: ScriptTarget,
): CompletionData | Request | undefined {
const typeChecker = program.getTypeChecker();

let start = timestamp();
let currentToken = getTokenAtPosition(sourceFile, position, /*includeJsDocComment*/ false); // TODO: GH#15853
// We will check for jsdoc comments with insideComment and getJsDocTagAtPosition. (TODO: that seems rather inefficient to check the same thing so many times.)
Expand Down Expand Up @@ -1168,13 +1145,30 @@ namespace ts.Completions {
}
}

// Don't suggest import completions for a commonjs-only module
if (preferences.includeCompletionsForModuleExports && !(sourceFile.commonJsModuleIndicator && !sourceFile.externalModuleIndicator)) {

@mhegazy mhegazy Mar 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am a bit confused now. why was not that working already? in the issue the file is not a module (i.e. no externalModuleIndicator and no commonJsModuleIndicator), so we should not have included the completions, right?

@mhegazy mhegazy Mar 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also should not this just be if (preferences.includeCompletionsForModuleExports && sourceFile.externalModuleIndicator) ?

@ghost ghost Mar 28, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that even in a currently import-less file we should still add import completions if the user has --module set in their tsconfig. (see #22750 (comment))

@ghost ghost Mar 28, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason it wasn't working already is we would always add import completions for a file with no commonjs imports/exports.

@mhegazy mhegazy Mar 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that is what #22880 was doing.. but looking at it again, the condition does not seem right.. the problem seems to be more complicated than i originally thought..

how about somethign like:

function shouldOfferImportCompletions(): boolean {
                if (preferences.includeCompletionsForModuleExports) {
                    // sure to have a module
                    if (sourceFile.externalModuleIndicator) return true;

                    // sure to have a .js file with a require, do not mess with that file
                    if (sourceFile.commonJsModuleIndicator) return false;

                    // either this file is not really a module, or it is a new file added to the project
                    // see if there are other "source" files that we can infer from
                    if (find(program.getSourceFiles(), s => !s.isDeclarationFile && !program.isSourceFileFromExternalLibrary(s) && !!s.externalModuleIndicator))
                        return true;

                    // either this is an empty project, or one with no modules in it
                    // for .js files we should fall on the safe side -- i think :)
                    if (isInJavaScriptFile(sourceFile)) return false;

                    // in the absense of any other info, then may be we can find something in the compiler options
                    if (compilerOptions.module !== undefined) return compilerOptions.module > ModuleKind.None;
                    if (compilerOptions.target >= ScriptTarget.ES2016) return true;
                    if (compilerOptions.noEmit) return true;
                }
                return false;
            }

getSymbolsFromOtherSourceFileExports(symbols, previousToken && isIdentifier(previousToken) ? previousToken.text : "", target);
if (shouldOfferImportCompletions()) {
getSymbolsFromOtherSourceFileExports(symbols, previousToken && isIdentifier(previousToken) ? previousToken.text : "", program.getCompilerOptions().target);
}
filterGlobalCompletion(symbols);
}

function shouldOfferImportCompletions(): boolean {
// If not already a module, must have modules enabled and not currently be in a commonjs module. (TODO: import completions for commonjs)
if (!preferences.includeCompletionsForModuleExports) return false;
// If already using ES6 modules, OK to continue using them.
if (sourceFile.externalModuleIndicator) return true;
// If already using commonjs, don't introduce ES6.
if (sourceFile.commonJsModuleIndicator) return false;
// If some file is using ES6 modules, assume that it's OK to add more.
if (program.getSourceFiles().some(s => !s.isDeclarationFile && !program.isSourceFileFromExternalLibrary(s) && !!s.externalModuleIndicator)) {
return true;
}
// For JS, stay on the safe side.
if (isSourceFileJavaScript(sourceFile)) return false;
// If module transpilation is enabled or we're targeting es6 or above, or not emitting, OK.
const compilerOptions = program.getCompilerOptions();
return !!compilerOptions.module || compilerOptions.target >= ScriptTarget.ES2015 || !!compilerOptions.noEmit;
}

function isSnippetScope(scopeNode: Node): boolean {
switch (scopeNode.kind) {
case SyntaxKind.SourceFile:
Expand Down Expand Up @@ -1268,7 +1262,7 @@ namespace ts.Completions {
function getSymbolsFromOtherSourceFileExports(symbols: Symbol[], tokenText: string, target: ScriptTarget): void {
const tokenTextLowerCase = tokenText.toLowerCase();

codefix.forEachExternalModuleToImportFrom(typeChecker, sourceFile, allSourceFiles, moduleSymbol => {
codefix.forEachExternalModuleToImportFrom(typeChecker, sourceFile, program.getSourceFiles(), moduleSymbol => {
for (let symbol of typeChecker.getExportsOfModule(moduleSymbol)) {
// Don't add a completion for a re-export, only for the original.
// The actual import fix might end up coming from a re-export -- we don't compute that until getting completion details.
Expand Down
15 changes: 2 additions & 13 deletions src/services/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,12 +1404,10 @@ namespace ts {
synchronizeHostData();
return Completions.getCompletionsAtPosition(
host,
program.getTypeChecker(),
program,
log,
program.getCompilerOptions(),
getValidSourceFile(fileName),
position,
program.getSourceFiles(),
fullPreferences);
}

Expand All @@ -1418,11 +1416,9 @@ namespace ts {
return Completions.getCompletionEntryDetails(
program,
log,
program.getCompilerOptions(),
getValidSourceFile(fileName),
position,
{ name, source },
program.getSourceFiles(),
host,
formattingOptions && formatting.getFormatContext(formattingOptions),
getCanonicalFileName,
Expand All @@ -1431,14 +1427,7 @@ namespace ts {

function getCompletionEntrySymbol(fileName: string, position: number, name: string, source?: string): Symbol {
synchronizeHostData();
return Completions.getCompletionEntrySymbol(
program.getTypeChecker(),
log,
program.getCompilerOptions(),
getValidSourceFile(fileName),
position,
{ name, source },
program.getSourceFiles());
return Completions.getCompletionEntrySymbol(program, log, getValidSourceFile(fileName), position, { name, source });
}

function getQuickInfoAtPosition(fileName: string, position: number): QuickInfo {
Expand Down
5 changes: 3 additions & 2 deletions tests/cases/fourslash/completionsImportBaseUrl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
// @Filename: /tsconfig.json
////{
//// "compilerOptions": {
//// "baseUrl": "."
//// "baseUrl": ".",
//// "module": "esnext"
//// }
////}

Expand All @@ -16,6 +17,6 @@
// Test that it prefers a relative import (see sourceDisplay).
goTo.marker("");
verify.completionListContains({ name: "foo", source: "/src/a" }, "const foo: 0", "", "const", undefined, /*hasAction*/ true, {
includeExternalModuleExports: true,
includeCompletionsForModuleExports: true,
sourceDisplay: "./a",
});
40 changes: 40 additions & 0 deletions tests/cases/fourslash/completionsImport_compilerOptionsModule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/// <reference path="fourslash.ts" />

// @allowJs: true
// @module: commonjs

// @Filename: /node_modules/a/index.d.ts
////export const foo = 0;

// @Filename: /b.js
////const a = require("./a");
////fo/*b*/

// @Filename: /c.js
////const x = 0;/*c*/ // Off for JS files (unless a non-declaration external module exists in the project)

// @Filename: /c2.ts
////const x = 0;/*c2*/

// @Filename: /d.js
////const a = import("./a"); // Does not make this an external module
////fo/*d*/

// @Filename: /d2.ts
////const a = import("./a"); // Does not make this an external module
////fo/*d2*/

for (const marker of ["b", "c", "d"]) {
goTo.marker(marker);
verify.not.completionListContains({ name: "foo", source: "/node_modules/a/index" }, undefined, undefined, undefined, undefined, undefined, {
includeCompletionsForModuleExports: true
});
}

for (const marker of ["c2", "d2"]) {
goTo.marker(marker);
verify.completionListContains({ name: "foo", source: "/node_modules/a/index" }, "const foo: 0", "", "const", /*spanIndex*/ undefined, /*hasAction*/ true, {
includeCompletionsForModuleExports: true,
sourceDisplay: "a",
});
}
1 change: 1 addition & 0 deletions tests/cases/fourslash/completionsImport_default_anonymous.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/// <reference path="fourslash.ts" />

// Use `/src` to test that directory names are not included in conversion from module path to identifier.
// @module: esnext

// @Filename: /src/foo-bar.ts
////export default 0;
Expand Down
2 changes: 1 addition & 1 deletion tests/cases/fourslash/completionsImport_default_didNotExistBefore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/// <reference path="fourslash.ts" />

// @noLib: true
// @module: esnext

// @Filename: /a.ts
////export default function foo() {}
Expand Down
2 changes: 2 additions & 0 deletions tests/cases/fourslash/completionsImport_default_exportDefaultIdentifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

// Tests that we use the name "foo".

// @module: esnext

// @Filename: /a.ts
////const foo = 0;
////export default foo;
Expand Down
2 changes: 2 additions & 0 deletions tests/cases/fourslash/completionsImport_fromAmbientModule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/// <reference path="fourslash.ts" />

// @module: esnext

// @Filename: /a.ts
////declare module "m" {
//// export const x: number;
Expand Down
2 changes: 2 additions & 0 deletions tests/cases/fourslash/completionsImport_matching.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/// <reference path="fourslash.ts" />

// @module: esnext

// @Filename: /a.ts
// Not included:
////export function abcde() {}
Expand Down
2 changes: 2 additions & 0 deletions tests/cases/fourslash/completionsImport_multipleWithSameName.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/// <reference path="fourslash.ts" />

// @module: esnext

// @Filename: /global.d.ts
// A local variable would prevent import completions (see `completionsImport_shadowedByLocal.ts`), but a global doesn't.
////declare var foo: number;
Expand Down
1 change: 1 addition & 0 deletions tests/cases/fourslash/completionsImport_named_didNotExistBefore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/// <reference path="fourslash.ts" />


// @Filename: /a.ts
////export function Test1() {}
////export function Test2() {}
Expand Down
2 changes: 2 additions & 0 deletions tests/cases/fourslash/completionsImport_named_exportEqualsNamespace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/// <reference path="fourslash.ts" />

// @module: esnext

// @Filename: /a.d.ts
////declare namespace N {
//// export const foo = 0;
Expand Down
2 changes: 2 additions & 0 deletions tests/cases/fourslash/completionsImport_named_exportEqualsNamespace_merged.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/// <reference path="fourslash.ts" />

// @module: esnext

// @Filename: /b.d.ts
////declare namespace N {
//// export const foo: number;
Expand Down
2 changes: 2 additions & 0 deletions tests/cases/fourslash/completionsImport_notFromUnrelatedNodeModules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/// <reference path="fourslash.ts" />

// @module: esnext

// @Filename: /unrelated/node_modules/@types/foo/index.d.ts
////export function foo() {}

Expand Down
2 changes: 2 additions & 0 deletions tests/cases/fourslash/completionsImport_ofAlias.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// Tests that we don't filter out a completion for an alias,
// so long as it's not an alias to a different module.

// @module: esnext

// @Filename: /a.ts
////const foo = 0;
////export { foo };
Expand Down
2 changes: 2 additions & 0 deletions tests/cases/fourslash/completionsImport_quoteStyle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/// <reference path="fourslash.ts" />

// @module: esnext

// @Filename: /a.ts
////export const foo = 0;

Expand Down
3 changes: 3 additions & 0 deletions tests/cases/fourslash/completionsImport_reExportDefault.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
/// <reference path="fourslash.ts" />

// @module: esnext
// @moduleResolution: node

// @Filename: /a/b/impl.ts
////export default function foo() {}

Expand Down
Loading