Code fix for missing imports #11768

paulvanbrenk · 2016-10-20T23:48:23Z

Implement codefix for adding missing imports.

mhegazy · 2016-10-26T04:09:16Z

src/services/codefixes/importFixes.ts

+            if (sourceFile.statements) {
+                for (const statement of sourceFile.statements) {
+                    if (statement.kind === SyntaxKind.ImportDeclaration) {
+                        imports.push(<ImportDeclaration>statement);


use soruceFile.resolvedModules instead of rewalking the tree.

The closest thing I can find is the sourceFIle.Imports, however I don't know how to get the import declarations from the module names. Maybe I can call getSymbolAtLocation? Or do you know there are better ways.

mhegazy · 2016-10-26T04:11:38Z

src/services/codefixes/importFixes.ts

+            function getCodeActionForImport(moduleName: string, isEqual: (a: string, b: string) => Comparison = compareStrings): CodeAction {
+                // Check to see if there are already imports being made from this source in the current file
+                const existing = forEach(imports, (importDeclaration) => {
+                    if (isEqual(removeQuotes(importDeclaration.moduleSpecifier.getText()), moduleName) === Comparison.EqualTo) {


the fileName is not guaranteed to be the same as the module name. e.g. path mapping. use sourcefile.resolvedModules, build a reverse lookup map and use it here.

mhegazy · 2016-10-26T04:15:21Z

src/services/codefixes/importFixes.ts

+
+            // Check if a matching symbol is exported by any files known to the compiler
+            for (const file of allFiles) {
+                const exports = file.symbol && file.symbol.exports;


check cancellation toke for every file.

mhegazy · 2016-10-26T04:17:01Z

src/services/codefixes/importFixes.ts

+            for (const file of allFiles) {
+                const exports = file.symbol && file.symbol.exports;
+                if (exports) {
+                    for (const exported in exports) {


why? just check exports[name]

actually this is not accurate, you want to use checker.ts::getExportsOfModule here to make sure export * from "module" are reflected here.

Do you mean instead of file.symbol && file.symbol.exports, use getExportsOfModule(file.symbol)?

mhegazy · 2016-10-26T04:39:27Z

src/services/codefixes/importFixes.ts

+                                moduleSpecifier = removeFileExtension(isRootedOrRelative ? pathName : combinePaths(".", pathName));
+                            }
+
+                            allActions.push(getCodeActionForImport(moduleSpecifier, (a, b) =>


You could get the same symbol from multiple imports, we need to filter them and only include the "best" for a symbol. where best is:

a module you already imported

a module in your source

a module that is "closer" in path to the current file

a module in a declaration file

I think we should split this into phases, first phase is to find all [exported symbol, module] that match the name we are looking for, then the next phase is to generate the action for each symbol.

… pvb/fixmissingimports

zhengbli · 2016-11-03T08:31:15Z

@mhegazy updated, can you take another look

mhegazy · 2016-11-03T23:47:09Z

src/services/codefixes/codeFixProvider.ts

 namespace ts {
    export interface CodeFix {
        errorCodes: number[];
-        getCodeActions(context: CodeFixContext): CodeAction[] | undefined;
+        getCodeActions(context: CodeFixContext, cancellationToken?: CancellationToken): CodeAction[] | undefined;


the context has the host which has the cancelation token anyways.

mhegazy · 2016-11-03T23:49:20Z

src/services/codefixes/importFixes.ts

+            let allPotentialModules: Symbol[] = [];
+
+            const ambientModules = checker.getAmbientModules();
+            if (ambientModules) {


getAmbientModules should always return an array.

mhegazy · 2016-11-03T23:52:57Z

src/services/codefixes/importFixes.ts

+            for (const moduleSymbol of allPotentialModules) {
+                cancellationToken.throwIfCancellationRequested();
+
+                const exports = checker.getExportsOfModule(moduleSymbol) || [];


i would recommend either exposing the getExportsOfModule that returns a symbolTable or adding a new getExportOfModule(name): Symbol | undefined instead of the loop.

mhegazy · 2016-11-03T23:56:22Z

src/services/codefixes/importFixes.ts

+                const exports = checker.getExportsOfModule(moduleSymbol) || [];
+                for (const exported of exports) {
+                    if (exported.name === name) {
+                        allActions.push(getCodeActionForImport(moduleSymbol));


i would add an extra check for getMeaningFromDeclaration(exportedSymbol) & getMeaningFromLocation(token) to filter the list more.

We also need to handel the default export correctelly:

if (result = moduleExports["default"]) { const localSymbol = getLocalSymbolForExportDefault(result); if (localSymbol && (result.flags & meaning) && localSymbol.name === name) { // found it } // otherwise it is not the right match }

// a.ts Foo // b.ts export default class Foo { }

mhegazy · 2016-11-03T23:58:18Z

src/services/codefixes/importFixes.ts

+                }
+            }
+
+            return allActions;


worth investigating the order of the result. possibly by the shortest path.

mhegazy · 2016-11-04T00:01:46Z

src/services/codefixes/importFixes.ts

+                                 * become "ns.foo"
+                                 */
+                                const ns = (<NamespaceImport>namedBindings).name.getText();
+                                return createCodeAction(


i would just add an import as in getCodeActionForNewImport

we should also handle import equals declarations the same way.

After trying it I find it quite annoying if we insert a new line for every single imports. I tried webstorm, and they do consolidate imports from the same file together.

mhegazy · 2016-11-04T00:15:23Z

src/services/codefixes/importFixes.ts

+                    const moduleSpecifier = declaration.moduleSpecifier.getText();
+
+                    // We have to handle all of the different import declaration forms
+                    if (declaration.importClause) {


we need to handle the case where the name of the exported symbol is "default" but the local is a different name.

options:

import {default as Foo} from "mod";

or

import default from "mod";

mhegazy · 2016-11-04T00:18:39Z

src/services/codefixes/importFixes.ts

+                        //
+                        // for case 1 and 2.2, we would return the relative file path as the module specifier;
+                        // for case 2.1, we would just use the module name instead.
+                        const sourceDir = getDirectoryPath(sourceFile.fileName);


we should be using paths here and not file names

mhegazy · 2016-11-04T00:32:37Z

src/services/codefixes/importFixes.ts

+                        // case 2.2
+                        // If this is a node module, check to see if the given file is the main export of the module or not. If so,
+                        // it can be referenced by just the module name.
+                        const moduleDir = getDirectoryPath(modulePath);


we need to handle @types/typeRoots. so use getEffictiveTypeRoots to get that

also i would move the check for index.d.ts to be before reading package.json

… pvb/fixmissingimports

mhegazy · 2016-11-09T20:59:57Z

src/compiler/checker.ts

@@ -102,7 +102,8 @@ namespace ts {
            isImplementationOfOverload,
            getAliasedSymbol: resolveAlias,
            getEmitResolver,
-            getExportsOfModule: getExportsOfModuleAsArray,


we can not rename it as this is a published API and ppl are using it already. consider adding a new method instead getExportOfModule(moduleSymbol, exportName): Symbol

mhegazy · 2016-11-09T21:28:27Z

src/services/codefixes/importFixes.ts

+
+                function getCodeActionForExistingImport(declaration: ImportDeclaration | ImportEqualsDeclaration): CodeAction {
+                    if (declaration.kind === SyntaxKind.ImportDeclaration) {
+                        const namedBindings = declaration.importClause && declaration.importClause.namedBindings;


we need to handel the default import case, if you already have:

import Foo, { other } from "./b"; import Foo, * as ns from "./b";

mhegazy · 2016-11-09T21:29:04Z

src/services/codefixes/importFixes.ts

+                        }
+
+                        return {
+                            newText: `,${oneImportPerLine ? context.newLineCharacter : ""}${name}`,


as for default

mhegazy · 2016-11-09T21:33:19Z

src/services/codefixes/importFixes.ts

+                         *
+                         * Because there is no import list, we alter the reference to include the
+                         * namespace instead of altering the import declaration. For example, "foo" would
+                         * become "ns.foo"


i would rather we add a new import defalcation regardless.

I ended up providing two code actions to the user, and he/she can do either one. I think C# has the same behavior.

mhegazy · 2016-11-09T21:39:33Z

src/services/codefixes/importFixes.ts

+                            }
+                        }
+
+                        function tryGetModuleNameFromExistingUses(): string {


i would say this not worth the work here.

mhegazy · 2016-11-09T21:41:33Z

src/services/codefixes/importFixes.ts

+                        }
+
+                        function tryGetModuleNameFromTypeRoots() {
+                            const typesRoots = getEffectiveTypeRoots(options, context.host);


make sure these are normalized paths

mhegazy · 2016-11-09T21:43:53Z

src/services/codefixes/importFixes.ts

+
+                            relativeFileName = removeFileExtension(relativeFileName);
+
+                            if (startsWith(relativeFileName, "@types/")) {


would not this already be handled by tryGetModuleNameFromTypeRoots?

billti · 2016-11-10T00:44:32Z

src/services/codefixes/importFixes.ts

+
+            const allPotentialModules = checker.getAmbientModules();
+            for (const otherSourceFile of allSourceFiles) {
+                if (otherSourceFile !== sourceFile && otherSourceFile.symbol) {


Is the presence of ".symbol" an indicator this is a module then? Is the helpful function isExternalModule not usable here? (It is more readable intuitive if so).

(Or perhaps isExternalOrCommonJSModule if we might want this for imports from JavaScript/Node files as some point too).

billti · 2016-11-10T00:56:42Z

src/services/codefixes/importFixes.ts

+                    const localSymbol = getLocalSymbolForExportDefault(defaultExport);
+                    if (localSymbol && localSymbol.name === name && checkSymbolHasMeaning(localSymbol, currentTokenMeaning)) {
+                        allActions.push(getCodeActionForImport(moduleSymbol, /*isDefaultExport*/ true));
+                    }


I don't understand what the above few lines do. Can you add some explanatory comments? In what scenario can we detect that you need to import a default export? It has no name to match on, as the local name for the 'default' export is provided by the import statement.

(I think I see the scenario in the *ImportDefault0.ts test-case, but is that a real scenario? How often is a default export a named function, and how often is that the identifier you just happened to try and refer to it as?)

I don't have data, but a quick search in the DefinitelyTyped repo did give me some library files with named export default, for example the material-ui library. And in the latest implementation I return multiple code fixes to the user, so that he/she can choose which one he/she likes the best.

billti · 2016-11-10T01:11:49Z

src/services/codefixes/importFixes.ts

+                        }
+
+                        // insert a new import statement in a new lines
+                        return getCodeActionForNewImport(declaration.moduleSpecifier.getText(), declaration.getEnd());


I don't understand why this is here. You already have an existing import for the module, why would you need to add another statement importing from the same module? Is this only if the existing statement contains a namespace import, and you want to add an import list import? (Is that valid/recommended?)

Yes it is used when we want to add a new import list, while the existing one is a namespace import.

And in the latest implementation, when you have:

import * as ns from "module"; foo // foo is a member from ns

We provide two options:
1. change your code: so foo becomes ns.foo
2. add import list, so it becomes:

import * as ns from "module"; import { foo } from "module"; foo // foo is a member from ns

The user can choose from these two.

It is valid code, and as we cannot delete any user's code, I think these are the only options.

billti · 2016-11-10T01:17:22Z

src/services/codefixes/importFixes.ts

+                            const endLine = getLineOfLocalPosition(sourceFile, importList.getEnd());
+                            oneImportPerLine = endLine - startLine >= 2;
+                        }
+                        else {


Do you need this else check? Seems if the if condition was just .length > 0 the test is valid for any number of elements.

(and actually, you already return on line 115 if length was 0, so you don't even need an "if" check).

billti · 2016-11-10T01:34:06Z

src/services/codefixes/importFixes.ts

+                                lastModuleSpecifierEnd = end;
+                            }
+                        }
+                        insertPos = lastModuleSpecifierEnd > 0 ? lastModuleSpecifierEnd + 1 : sourceFile.getStart();


If there are no existing imports, it would be good to place the first prior to the first statement (i.e. after any comments), rather than the start of the file, as a file will often start with a license header.

Correct, and it is the current behavior, as the test importNameCodeFixNewImportAmbient2.ts shows.

zhengbli · 2016-11-11T23:31:32Z

ping @mhegazy @billti @paulvanbrenk @aozgaa

… pvb/fixmissingimports

zhengbli · 2016-11-15T01:01:28Z

src/services/codefixes/importFixes.ts

+/* @internal */
+namespace ts.codefix {
+
+    type ImportCodeActionKind = "CodeChange" | "InsertingIntoExistingImport" | "NewImport";


They are used to filter the code actions later. For example, inserting new item to an existing import list should take higher priority to adding a new import statement. In reality it is possible that we can get both kinds of code actions if the code has multiple existing imports

They are only used internally for sorting / filtering. So no need to expose them

mhegazy · 2016-11-15T19:55:05Z

src/services/codefixes/importFixes.ts

+                // check exports with the same name
+                const exportWithIdenticalName = checker.tryGetMemberInModuleExports(name, moduleSymbol);
+                if (exportWithIdenticalName && checkSymbolHasMeaning(exportWithIdenticalName, currentTokenMeaning)) {
+                    addRange(allActions, getCodeActionForImport(moduleSymbol));


i would say instead of calculating the actions here, then filtering them out, is do the filtering on the symbols, then generate the code actions at the end.

this way you do not have to figure out if they are the same symbol or not.

so, consider building a map keyed off the getSymbolId(checker.getAliasedSymbol(symbol)) this way you twiddle down the list to unique symbols. if you have seen this symbol before then check the relative path length to the including file, and only keep the shorter one.

this way you do not need the kind as well. you can just keep them all at this point.

mhegazy · 2016-11-15T19:59:31Z

src/services/codefixes/importFixes.ts

+                            tryGetModuleNameAsNodeModule() ||
+                            removeFileExtension(getRelativePath(moduleFileName, sourceDirectory));
+
+                        function tryGetModuleNameFromAmbientModule(): string {


this does not seem to be needed. you do pass the module specifier if it is an ambient module any ways.

The module specifier is passed only when there is already an existing import declaration in this file, otherwise this is still needed.

mhegazy · 2016-11-15T20:50:06Z

src/services/codefixes/importFixes.ts

+                    // in this case we should provie 2 actions:
+                    //     1. change "member3" to "ns.member3"
+                    //     2. add "member3" to the second import statement's import list
+                    // and it is up to the user to decide which one fits best.


mm... i am not sure why this would be better than just picking the "best" one.

when would the user need to choose one over the other?

Say the user already has a import * as ns in his code, which means he intends to use it somewhere, then the first choice might be good. And as you previously suggested, the second option might solve more errors because it addresses all identifiers with the same name in this file. It all comes down to personal preferences. However, I feel there are really no objective criteria regarding which one is "always" better, just provide two choices might be better than enforcing our own understanding.

… pvb/fixmissingimports

Paul van Brenk added 2 commits October 20, 2016 09:59

Add codefix for missing imports + tests

0389207

Re-order and cleanup

eb46886

msftclas added the cla-not-required label Oct 20, 2016

mhegazy suggested changes Oct 26, 2016

View reviewed changes

mhegazy mentioned this pull request Oct 26, 2016

[ES6] Auto imports with completion #7849

Closed

zhengbli self-assigned this Nov 2, 2016

zhengbli added 3 commits November 2, 2016 23:41

refactor

80d7f5c

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

a6807ab

… pvb/fixmissingimports

make tests pass

1502f75

mhegazy reviewed Nov 3, 2016

View reviewed changes

mhegazy suggested changes Nov 4, 2016

View reviewed changes

zhengbli added 4 commits November 7, 2016 23:23

Make import specifier for new imports more comprehensive

e72f0c8

Fix existing import cases

d273308

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

35d668a

… pvb/fixmissingimports

refactor

eb892d3

mhegazy suggested changes Nov 9, 2016

View reviewed changes

billti reviewed Nov 10, 2016

View reviewed changes

zhengbli added 3 commits November 9, 2016 23:11

Fix multiple import statement case

6d024dc

add multiple code fixes and code action filtering and polishing

26cd24a

not using the generic verify method for import fixes.

a282fc0

zhengbli changed the title ~~Pvb/fixmissingimports~~ Code fix for missing imports Nov 11, 2016

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

1f82ce7

… pvb/fixmissingimports

zhengbli force-pushed the pvb/fixmissingimports branch from ab3342d to 1f82ce7 Compare November 12, 2016 00:28

Correct insert position for new imports

d83c292

zhengbli reviewed Nov 15, 2016

View reviewed changes

mhegazy suggested changes Nov 15, 2016

View reviewed changes

zhengbli added 4 commits November 15, 2016 22:43

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

7568f6f

… pvb/fixmissingimports

improve the code action filtering logic

aa2891c

Fix line ending issue

055d20f

cache where we can

11ea6b3

mhegazy approved these changes Nov 16, 2016

View reviewed changes

zhengbli merged commit 52ec508 into master Nov 17, 2016

zhengbli deleted the pvb/fixmissingimports branch November 17, 2016 00:01

mhegazy mentioned this pull request Dec 20, 2016

Provide Visual Studio LightBulb style fixup support for <reference> and import statements #2893

Closed

jeffrey-easyesi mentioned this pull request Jan 9, 2017

Please add feature to fix imports Everlaw/nbts#49

Closed

microsoft locked and limited conversation to collaborators Jun 19, 2018

typescript-bot unassigned zhengbli Jun 19, 2018


		relativeFileName = removeFileExtension(relativeFileName);

		if (startsWith(relativeFileName, "@types/")) {

Code fix for missing imports #11768

Code fix for missing imports #11768

Conversation

paulvanbrenk commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengbli commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengbli commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengbli Nov 16, 2016 • edited Loading

Choose a reason for hiding this comment

zhengbli Nov 16, 2016 •

edited

Loading