Skip to content
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

fix55816: exclude files with re-exports if excluded by preferences.autoImportFileExcludePatterns #58537

Merged
merged 7 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
16 changes: 14 additions & 2 deletions src/services/codefixes/importFixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
FutureSymbolExportInfo,
getAllowSyntheticDefaultImports,
getBaseFileName,
getDeclarationOfKind,
getDefaultExportInfoWorker,
getDefaultLikeExportInfo,
getDirectoryPath,
Expand All @@ -49,6 +50,7 @@ import {
getEmitScriptTarget,
getExportInfoMap,
getImpliedNodeFormatForEmitWorker,
getIsFileExcluded,
getMeaningFromDeclaration,
getMeaningFromLocation,
getNameForExportedSymbol,
Expand Down Expand Up @@ -284,8 +286,11 @@ function createImportAdderWorker(sourceFile: SourceFile | FutureSourceFile, prog
const checker = program.getTypeChecker();
const symbol = checker.getMergedSymbol(skipAlias(exportedSymbol, checker));
const exportInfo = getAllExportInfoForSymbol(sourceFile, symbol, symbolName, moduleSymbol, /*preferCapitalized*/ false, program, host, preferences, cancellationToken);

// If no exportInfo is found, this means the export could not be resolved, so we should not generate an import
if (!exportInfo) return;
iisaduan marked this conversation as resolved.
Show resolved Hide resolved
const useRequire = shouldUseRequire(sourceFile, program);
let fix = getImportFixForSymbol(sourceFile, Debug.checkDefined(exportInfo), program, /*position*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
let fix = getImportFixForSymbol(sourceFile, exportInfo, program, /*position*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
if (fix) {
const localName = tryCast(referenceImport?.name, isIdentifier)?.text ?? symbolName;
if (
Expand Down Expand Up @@ -864,9 +869,16 @@ function codeFixActionToCodeAction({ description, changes, commands }: CodeFixAc

function getAllExportInfoForSymbol(importingFile: SourceFile | FutureSourceFile, symbol: Symbol, symbolName: string, moduleSymbol: Symbol, preferCapitalized: boolean, program: Program, host: LanguageServiceHost, preferences: UserPreferences, cancellationToken: CancellationToken | undefined): readonly SymbolExportInfo[] | undefined {
const getChecker = createGetChecker(program, host);
const isFileExcluded = preferences.autoImportFileExcludePatterns && getIsFileExcluded(host, preferences);
const mergedModuleSymbol = program.getTypeChecker().getMergedSymbol(moduleSymbol);
const moduleSourceFile = isFileExcluded && mergedModuleSymbol.declarations && getDeclarationOfKind(mergedModuleSymbol, SyntaxKind.SourceFile);
const moduleSymbolExcluded = moduleSourceFile && isFileExcluded(moduleSourceFile as SourceFile);
return getExportInfoMap(importingFile, host, program, preferences, cancellationToken)
.search(importingFile.path, preferCapitalized, name => name === symbolName, info => {
if (skipAlias(info[0].symbol, getChecker(info[0].isFromPackageJson)) === symbol && info.some(i => i.moduleSymbol === moduleSymbol || i.symbol.parent === moduleSymbol)) {
if (
getChecker(info[0].isFromPackageJson).getMergedSymbol(skipAlias(info[0].symbol, getChecker(info[0].isFromPackageJson))) === symbol
&& (moduleSymbolExcluded || info.some(i => i.moduleSymbol === moduleSymbol || i.symbol.parent === moduleSymbol))
) {
return info;
}
});
Expand Down
48 changes: 31 additions & 17 deletions src/services/exportInfoMap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -429,12 +429,7 @@ export function forEachExternalModuleToImportFrom(
cb: (module: Symbol, moduleFile: SourceFile | undefined, program: Program, isFromPackageJson: boolean) => void,
) {
const useCaseSensitiveFileNames = hostUsesCaseSensitiveFileNames(host);
const excludePatterns = preferences.autoImportFileExcludePatterns && mapDefined(preferences.autoImportFileExcludePatterns, spec => {
// The client is expected to send rooted path specs since we don't know
// what directory a relative path is relative to.
const pattern = getSubPatternFromSpec(spec, "", "exclude");
return pattern ? getRegexFromPattern(pattern, useCaseSensitiveFileNames) : undefined;
});
const excludePatterns = preferences.autoImportFileExcludePatterns && getIsExcludedPatterns(preferences, useCaseSensitiveFileNames);

forEachExternalModule(program.getTypeChecker(), program.getSourceFiles(), excludePatterns, host, (module, file) => cb(module, file, program, /*isFromPackageJson*/ false));
const autoImportProvider = useAutoImportProvider && host.getPackageJsonAutoImportProvider?.();
Expand All @@ -454,9 +449,33 @@ export function forEachExternalModuleToImportFrom(
}
}

function getIsExcludedPatterns(preferences: UserPreferences, useCaseSensitiveFileNames: boolean) {
return mapDefined(preferences.autoImportFileExcludePatterns, spec => {
// The client is expected to send rooted path specs since we don't know
// what directory a relative path is relative to.
const pattern = getSubPatternFromSpec(spec, "", "exclude");
return pattern ? getRegexFromPattern(pattern, useCaseSensitiveFileNames) : undefined;
});
}

function forEachExternalModule(checker: TypeChecker, allSourceFiles: readonly SourceFile[], excludePatterns: readonly RegExp[] | undefined, host: LanguageServiceHost, cb: (module: Symbol, sourceFile: SourceFile | undefined) => void) {
const isExcluded = excludePatterns && getIsExcluded(excludePatterns, host);

for (const ambient of checker.getAmbientModules()) {
if (!ambient.name.includes("*") && !(excludePatterns && ambient.declarations?.every(d => isExcluded!(d.getSourceFile())))) {
cb(ambient, /*sourceFile*/ undefined);
}
}
for (const sourceFile of allSourceFiles) {
if (isExternalOrCommonJsModule(sourceFile) && !isExcluded?.(sourceFile)) {
cb(checker.getMergedSymbol(sourceFile.symbol), sourceFile);
}
}
}

function getIsExcluded(excludePatterns: readonly RegExp[], host: LanguageServiceHost) {
const realpathsWithSymlinks = host.getSymlinkCache?.().getSymlinkedDirectoriesByRealpath();
const isExcluded = excludePatterns && (({ fileName, path }: SourceFile) => {
return (({ fileName, path }: SourceFile) => {
if (excludePatterns.some(p => p.test(fileName))) return true;
if (realpathsWithSymlinks?.size && pathContainsNodeModules(fileName)) {
let dir = getDirectoryPath(fileName);
Expand All @@ -470,17 +489,12 @@ function forEachExternalModule(checker: TypeChecker, allSourceFiles: readonly So
}
return false;
});
}

for (const ambient of checker.getAmbientModules()) {
if (!ambient.name.includes("*") && !(excludePatterns && ambient.declarations?.every(d => isExcluded!(d.getSourceFile())))) {
cb(ambient, /*sourceFile*/ undefined);
}
}
for (const sourceFile of allSourceFiles) {
if (isExternalOrCommonJsModule(sourceFile) && !isExcluded?.(sourceFile)) {
cb(checker.getMergedSymbol(sourceFile.symbol), sourceFile);
}
}
/** @internal */
export function getIsFileExcluded(host: LanguageServiceHost, preferences: UserPreferences) {
if (!preferences.autoImportFileExcludePatterns) return () => false;
return getIsExcluded(getIsExcludedPatterns(preferences, hostUsesCaseSensitiveFileNames(host)), host);
}

/** @internal */
Expand Down
44 changes: 44 additions & 0 deletions tests/cases/fourslash/autoImportFileExcludePatterns10.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/// <reference path="fourslash.ts" />

// @Filename: /src/vs/test.ts
//// import { Parts } from './parts';
//// export class /**/Extended implements Parts {
//// }

// @Filename: /src/vs/parts.ts
//// import { Event } from '../event/event';
////
//// export interface Parts {
//// readonly options: Event;
//// }

// @Filename: /src/event/event.ts
//// export interface Event {
//// (): string;
//// }

// @Filename: /src/thing.ts
//// import { Event } from '../event/event';
//// export { Event };

// @Filename: /src/a.ts
//// import './thing'
//// declare module './thing' {
//// interface Event {
//// c: string;
//// }
//// }


verify.codeFix({
description: "Implement interface 'Parts'",
newFileContent:
`import { Event } from '../event/event';
import { Parts } from './parts';
export class Extended implements Parts {
options: Event;
}`,
preferences: {
autoImportFileExcludePatterns: ["src/thing.ts"],
}
});
43 changes: 43 additions & 0 deletions tests/cases/fourslash/autoImportFileExcludePatterns11.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/// <reference path="fourslash.ts" />

// @Filename: /src/vs/test.ts
//// import { Parts } from './parts';
//// export class /**/Extended implements Parts {
//// }

// @Filename: /src/vs/parts.ts
//// import { Event } from '../thing';
//// export interface Parts {
//// readonly options: Event;
//// }

// @Filename: /src/event/event.ts
//// export interface Event {
//// (): string;
//// }

// @Filename: /src/thing.ts
//// import { Event } from './event/event';
//// export { Event };

// @Filename: /src/a.ts
//// import './thing'
//// declare module './thing' {
//// interface Event {
//// c: string;
//// }
//// }


verify.codeFix({
description: "Implement interface 'Parts'",
newFileContent:
`import { Event } from '../event/event';
Copy link
Member

Choose a reason for hiding this comment

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

Huh, this one with the broken import ended up working? What’s different from when we debugged it yesterday and this was crashing? Is there a test case that exercises if (!exportInfo) return;?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the broken import that we fixed was in thing,ts, and the file that the codefix was triggered in is in a different part of the folder tree than thing

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to make cases with other file structures, since I was trying to test if the codefix would still crash in situations where the import map resolved to a different preferred file to generate the import from

import { Parts } from './parts';
export class Extended implements Parts {
options: Event;
}`,
preferences: {
autoImportFileExcludePatterns: ["src/thing.ts"],
}
});
33 changes: 33 additions & 0 deletions tests/cases/fourslash/autoImportFileExcludePatterns4.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/// <reference path="fourslash.ts" />

// @Filename: /src/vs/workbench/test.ts
//// import { Parts } from './parts';
//// export class /**/EditorParts implements Parts { }

// @Filename: /src/vs/event/event.ts
//// export interface Event {
//// (): string;
//// }

// @Filename: /src/vs/workbench/parts.ts
//// import { Event } from '../event/event';
//// export interface Parts {
//// readonly options: Event;
//// }

// @Filename: /src/vs/workbench/workbench.ts
//// import { Event } from '../event/event';
//// export { Event };

verify.codeFix({
description: "Implement interface 'Parts'",
newFileContent:
`import { Event } from '../event/event';
import { Parts } from './parts';
export class EditorParts implements Parts {
options: Event;
}`,
preferences: {
autoImportFileExcludePatterns: ["src/vs/workbench/workbench.ts"],
}
});
37 changes: 37 additions & 0 deletions tests/cases/fourslash/autoImportFileExcludePatterns5.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/// <reference path="fourslash.ts" />

// @Filename: /src/vs/workbench/test.ts
//// import { Parts } from './parts';
//// export class /**/EditorParts implements Parts { }

// @Filename: /src/vs/event/event.ts
//// export interface Event {
//// (): string;
//// }

// @Filename: /src/vs/workbench/parts.ts
//// import { Event } from '../event/event';
//// export interface Parts {
//// readonly options: Event;
//// }

// @Filename: /src/vs/workbench/workbench.ts
//// import { Event } from '../event/event';
//// export { Event };

// @Filename: /src/vs/workbench/canImport.ts
//// import { Event } from '../event/event';
//// export { Event };

verify.codeFix({
description: "Implement interface 'Parts'",
newFileContent:
`import { Event } from './canImport';
import { Parts } from './parts';
export class EditorParts implements Parts {
options: Event;
}`,
preferences: {
autoImportFileExcludePatterns: ["src/vs/workbench/workbench.ts"],
}
});
37 changes: 37 additions & 0 deletions tests/cases/fourslash/autoImportFileExcludePatterns6.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/// <reference path="fourslash.ts" />

// @Filename: /src/vs/workbench/test.ts
//// import { Parts } from './parts';
//// export class /**/EditorParts implements Parts { }

// @Filename: /src/vs/event/event.ts
//// export interface Event {
//// (): string;
//// }

// @Filename: /src/vs/workbench/parts.ts
//// import { Event } from '../event/event';
//// export interface Parts {
//// readonly options: Event;
//// }

// @Filename: /src/vs/workbench/workbench.ts
//// import { Event } from './canImport';
//// export { Event };

// @Filename: /src/vs/workbench/canImport.ts
//// import { Event } from '../event/event';
//// export { Event };

verify.codeFix({
description: "Implement interface 'Parts'",
newFileContent:
`import { Event } from './canImport';
import { Parts } from './parts';
export class EditorParts implements Parts {
options: Event;
}`,
preferences: {
autoImportFileExcludePatterns: ["src/vs/workbench/workbench.ts"],
}
});
37 changes: 37 additions & 0 deletions tests/cases/fourslash/autoImportFileExcludePatterns7.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/// <reference path="fourslash.ts" />

// @Filename: /src/vs/workbench/test.ts
//// import { Parts } from './parts';
//// export class /**/EditorParts implements Parts { }

// @Filename: /src/vs/event/event.ts
//// export interface Event {
//// (): string;
//// }

// @Filename: /src/vs/workbench/parts.ts
//// import { Event } from '../event/event';
//// export interface Parts {
//// readonly options: Event;
//// }

// @Filename: /src/vs/workbench/workbench.ts
//// import { Event } from '../event/event';
//// export { Event };

// @Filename: /src/vs/workbench/workbench2.ts
//// import { Event } from '../event/event';
//// export { Event };

verify.codeFix({
description: "Implement interface 'Parts'",
newFileContent:
`import { Event } from '../event/event';
import { Parts } from './parts';
export class EditorParts implements Parts {
options: Event;
}`,
preferences: {
autoImportFileExcludePatterns: ["src/vs/workbench/workbench*"],
}
});
37 changes: 37 additions & 0 deletions tests/cases/fourslash/autoImportFileExcludePatterns8.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/// <reference path="fourslash.ts" />

// @Filename: /src/vs/workbench/test.ts
//// import { Parts } from './parts';
//// export class /**/EditorParts implements Parts { }

// @Filename: /src/vs/event/event.ts
//// export interface Event {
//// (): string;
//// }

// @Filename: /src/vs/workbench/parts.ts
//// import { Event } from '../event/event';
//// export interface Parts {
//// readonly options: Event;
//// }

// @Filename: /src/vs/workbench/workbench.ts
//// import { Event } from './workbench2';
//// export { Event };

// @Filename: /src/vs/workbench/workbench2.ts
//// import { Event } from '../event/event';
//// export { Event };

verify.codeFix({
description: "Implement interface 'Parts'",
newFileContent:
`import { Event } from '../event/event';
import { Parts } from './parts';
export class EditorParts implements Parts {
options: Event;
}`,
preferences: {
autoImportFileExcludePatterns: ["src/vs/workbench/workbench*"],
}
});
Loading