Auto-imports: fix some exports being incorrectly stored as re-exports of others due to key conflict #45792

andrewbranch · 2021-09-09T03:34:35Z

There’s some explanation in review comments

andrewbranch · 2021-09-09T17:29:21Z

typescript-bot · 2021-09-09T17:29:24Z

Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 57220f2. You can monitor the build here.

typescript-bot · 2021-09-09T17:52:07Z

Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/110109/artifacts?artifactName=tgz&fileId=244C0951A81B1898389C45E97CA686C5006F6BDB37BAF2ED2F2D91492665B67E02&fileName=/typescript-4.5.0-insiders.20210909.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.5.0-pr-45792-2".;

sandersn

A couple of questions to check my understanding.

tests/cases/fourslash/autoImportSameNameDefaultExported.ts

src/services/exportInfoMap.ts

andrewbranch · 2021-09-30T15:36:53Z

The reason I haven’t explained what the heck is happening in this PR is I’m hoping to have some time to do a more thorough fix by saving a reference to the originally-exporting module and the name of the export in the module’s symbol table at the end of alias resolution in the checker. This is precisely the information we need to be able to identify every unique export, and the checker already has that information during alias resolution, but it discards it.

johnsoncodehk · 2021-10-06T03:21:49Z

Can this PR fix #46115?

andrewbranch · 2021-10-06T17:10:40Z

Probably, feel free to try it

…here

callmeberzerker · 2021-10-08T07:32:50Z

I just tested this and it works fine for my react project. useEffect, useState etc. are not wrongly imported from react-transition-group anymore.

andrewbranch · 2021-10-08T19:25:56Z

src/services/completions.ts

-                        const origin: SymbolOriginInfoExport = {
-                            kind: getNullableSymbolOriginInfoKind(SymbolOriginInfoKind.SymbolMemberExport),
+                        const fileName = isExternalModuleNameRelative(stripQuotes(moduleSymbol.name)) ? getSourceFileOfModule(moduleSymbol)?.fileName : undefined;
+                        const { moduleSpecifier } = codefix.getModuleSpecifierForBestExportInfo([{
+                            exportKind: ExportKind.Named,
+                            moduleFileName: fileName,
+                            isFromPackageJson: false,
                            moduleSymbol,
-                            isDefaultExport: false,
-                            symbolName: firstAccessibleSymbol.name,
-                            exportName: firstAccessibleSymbol.name,
-                            fileName: isExternalModuleNameRelative(stripQuotes(moduleSymbol.name)) ? cast(moduleSymbol.valueDeclaration, isSourceFile).fileName : undefined,
-                        };
-                        symbolToOriginInfoMap[index] = origin;
+                            symbol: firstAccessibleSymbol,
+                            targetFlags: skipAlias(firstAccessibleSymbol, typeChecker).flags,
+                        }], sourceFile, program, host, preferences) || {};
+
+                        if (moduleSpecifier) {
+                            const origin: SymbolOriginInfoResolvedExport = {
+                                kind: getNullableSymbolOriginInfoKind(SymbolOriginInfoKind.SymbolMemberExport),
+                                moduleSymbol,
+                                isDefaultExport: false,
+                                symbolName: firstAccessibleSymbol.name,
+                                exportName: firstAccessibleSymbol.name,
+                                fileName,
+                                moduleSpecifier,
+                            };
+                            symbolToOriginInfoMap[index] = origin;
+                        }


This is one of the larger set of changes in the PR, but it’s kind of a drive-by fix for a probable ancient bug that the real change, the addition of exportMapKey to SymbolOriginInfoExport, forced me to consider. This has always been a weird ad-hoc auto-import for when you need to import a symbol in order to do a property completion, as shown in this test.

I think there have likely been cases where fetching the actual auto-import (in completion details) for this completion would have failed due to some oversimplified assumptions here. I’m now guarding against that a bit more by resolving its module specifier up front and ensuring that the symbol can definitely be re-retrieved by looking for its name in the exports of the module. I could have sort of ignored this problem, but it felt better to deal with it.

andrewbranch · 2021-10-08T19:33:26Z

src/services/exportInfoMap.ts

@@ -46,8 +46,8 @@ namespace ts {
        isUsableByFile(importingFile: Path): boolean;
        clear(): void;
        add(importingFile: Path, symbol: Symbol, key: __String, moduleSymbol: Symbol, moduleFile: SourceFile | undefined, exportKind: ExportKind, isFromPackageJson: boolean, scriptTarget: ScriptTarget, checker: TypeChecker): void;
-        get(importingFile: Path, importedName: string, symbol: Symbol, moduleName: string, checker: TypeChecker): readonly SymbolExportInfo[] | undefined;
-        forEach(importingFile: Path, action: (info: readonly SymbolExportInfo[], name: string, isFromAmbientModule: boolean) => void): void;
+        get(importingFile: Path, key: string): readonly SymbolExportInfo[] | undefined;


The central insight of this PR is that the cache key does not actually need to be derivable from an updated program later on, because the only thing that ever calls get is continuePreviousIncompleteResponse in completions.ts, and rather than recomputing the key from symbols it currently has access to, we can just save the key we originally came up with on CompletionEntryData and use that to access the cache in continuePreviousIncompleteResponse. This means the only criteria that the key needs to satisfy is that it produces proper grouping while building the map. This opens the door to using getSymbolId(skipAlias(someExport)) in the key, which doesn't have the same very weird pitfalls as trying to find a way to uniquely identify the target symbol that remains stable over program updates.

amcasey

I'm happy with the concept and the protocol changes and I'm satisfied that we have an escape hatch if it breaks. I agree with @DanielRosenwasser that a little more baking time wouldn't hurt.

typescript-bot assigned andrewbranch Sep 9, 2021

typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 9, 2021

andrewbranch mentioned this pull request Sep 9, 2021

Auto-imports offered from another package’s node_modules #45784

Closed

typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Sep 9, 2021

microsoft deleted a comment from typescript-bot Sep 9, 2021

andrewbranch marked this pull request as ready for review September 9, 2021 17:29

andrewbranch mentioned this pull request Sep 16, 2021

Auto import working inconsistently after updating to v1.60.1 #45921

Closed

This was referenced Sep 22, 2021

Exclude specific files from auto import suggestions #35395

Closed

Debug failure in TSServer autocomplete #46024

Closed

sandersn requested changes Sep 29, 2021

View reviewed changes

tests/cases/fourslash/autoImportSameNameDefaultExported.ts Show resolved Hide resolved

src/services/exportInfoMap.ts Outdated Show resolved Hide resolved

andrewbranch added the Experiment A fork with an experimental idea which might not make it into master label Sep 30, 2021

andrewbranch added 4 commits October 6, 2021 11:06

Ensure symbol key unique when target is a local symbol exported elsew…

ba90719

…here

Add test

00e2c65

Support targets without declarations

106e1a9

Best key yet

20676c7

andrewbranch mentioned this pull request Oct 7, 2021

Intellisense for vue is broken #46115

Closed

A-ha moment

fef7e8c

andrewbranch force-pushed the bug/45763 branch from 57220f2 to fef7e8c Compare October 8, 2021 18:16

andrewbranch added 3 commits October 8, 2021 12:17

Clean up types

6d35017

Update API

dcd0ed8

Update unit test

c4283e3

andrewbranch commented Oct 8, 2021

View reviewed changes

andrewbranch requested review from sandersn, amcasey and DanielRosenwasser October 8, 2021 20:55

andrewbranch removed the Experiment A fork with an experimental idea which might not make it into master label Oct 8, 2021

amcasey approved these changes Oct 8, 2021

View reviewed changes

andrewbranch merged commit 64b8172 into microsoft:main Oct 8, 2021

andrewbranch deleted the bug/45763 branch October 8, 2021 22:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-imports: fix some exports being incorrectly stored as re-exports of others due to key conflict #45792

Auto-imports: fix some exports being incorrectly stored as re-exports of others due to key conflict #45792

andrewbranch commented Sep 9, 2021 •

edited

Loading

andrewbranch commented Sep 9, 2021

typescript-bot commented Sep 9, 2021 •

edited

Loading

typescript-bot commented Sep 9, 2021 •

edited

Loading

sandersn left a comment

andrewbranch commented Sep 30, 2021

johnsoncodehk commented Oct 6, 2021

andrewbranch commented Oct 6, 2021

callmeberzerker commented Oct 8, 2021

andrewbranch Oct 8, 2021

andrewbranch Oct 8, 2021

amcasey left a comment

Auto-imports: fix some exports being incorrectly stored as re-exports of others due to key conflict #45792

Auto-imports: fix some exports being incorrectly stored as re-exports of others due to key conflict #45792

Conversation

andrewbranch commented Sep 9, 2021 • edited Loading

andrewbranch commented Sep 9, 2021

typescript-bot commented Sep 9, 2021 • edited Loading

typescript-bot commented Sep 9, 2021 • edited Loading

sandersn left a comment

Choose a reason for hiding this comment

andrewbranch commented Sep 30, 2021

johnsoncodehk commented Oct 6, 2021

andrewbranch commented Oct 6, 2021

callmeberzerker commented Oct 8, 2021

andrewbranch Oct 8, 2021

Choose a reason for hiding this comment

andrewbranch Oct 8, 2021

Choose a reason for hiding this comment

amcasey left a comment

Choose a reason for hiding this comment

andrewbranch commented Sep 9, 2021 •

edited

Loading

typescript-bot commented Sep 9, 2021 •

edited

Loading

typescript-bot commented Sep 9, 2021 •

edited

Loading