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.

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

Jump to bottom

Fixing namespace import debug failure #59004

Closed
wants to merge 17 commits into from
Closed

Fixing namespace import debug failure #59004

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e31bb29
Fixes namespace debug failure but test 2 fails
navya9singh Jun 20, 2024
c4d86b8
fixing test 2
navya9singh Jun 24, 2024
1347b9f
fixing formatting
navya9singh Jun 24, 2024
45af3f0
fixing baselines
navya9singh Jun 24, 2024
9e4a30a
Merge branch 'main' of https://github.com/microsoft/TypeScript into e…
navya9singh Jun 24, 2024
e35b0f7
Adding more fixes and tests
navya9singh Jun 27, 2024
c0aaae4
Removing previous changes
navya9singh Jun 27, 2024
dc452e8
deleting old baselines
navya9singh Jun 27, 2024
d47ff25
default import fixes
navya9singh Jul 11, 2024
ba4c3dd
fixing baselines
navya9singh Jul 11, 2024
fb453ee
changing baselines
navya9singh Jul 11, 2024
ba364e8
Merge branch 'main' of https://github.com/microsoft/TypeScript into e…
navya9singh Jul 11, 2024
b21b4a1
fixing tests and baselines
navya9singh Jul 15, 2024
b23e6d8
removing baseline for deleted test
navya9singh Jul 15, 2024
cb06e8f
fixes
navya9singh Jul 18, 2024
2a432e4
addressing pr comments
navya9singh Jul 20, 2024
e3deb17
addAsTypeOnly changes
navya9singh Jul 22, 2024
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
25 changes: 22 additions & 3 deletions src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ export interface ImportAdder {
addImportFromExportedSymbol: (exportedSymbol: Symbol, isValidTypeOnlyUseSite?: boolean, referenceImport?: ImportOrRequireAliasDeclaration) => void;
addImportForNonExistentExport: (exportName: string, exportingFileName: string, exportKind: ExportKind, exportedMeanings: SymbolFlags, isImportUsageValidAsTypeOnly: boolean) => void;
addImportForUnresolvedIdentifier: (context: CodeFixContextBase, symbolToken: Identifier, useAutoImportProvider: boolean) => void;
addImportForExternalModuleSymbol: (symbol: Symbol, originalSymbol: Symbol, isValidTypeOnlyUseSite: boolean) => void;
addVerbatimImport: (declaration: AnyImportOrRequireStatement | ImportOrRequireAliasDeclaration) => void;
removeExistingImport: (declaration: ImportOrRequireAliasDeclaration) => void;
writeFixes: (changeTracker: textChanges.ChangeTracker, oldFileQuotePreference?: QuotePreference) => void;
Expand Down Expand Up @@ -255,7 +256,7 @@ function createImportAdderWorker(sourceFile: SourceFile | FutureSourceFile, prog
type NewImportsKey = `${0 | 1}|${string}`;
/** Use `getNewImportEntry` for access */
const newImports = new Map<NewImportsKey, Mutable<ImportsCollection & { useRequire: boolean; }>>();
return { addImportFromDiagnostic, addImportFromExportedSymbol, writeFixes, hasFixes, addImportForUnresolvedIdentifier, addImportForNonExistentExport, removeExistingImport, addVerbatimImport };
return { addImportFromDiagnostic, addImportFromExportedSymbol, writeFixes, hasFixes, addImportForUnresolvedIdentifier, addImportForNonExistentExport, addImportForExternalModuleSymbol, removeExistingImport, addVerbatimImport };

function addVerbatimImport(declaration: AnyImportOrRequireStatement | ImportOrRequireAliasDeclaration) {
verbatimImports.add(declaration);
Expand All @@ -274,9 +275,10 @@ function createImportAdderWorker(sourceFile: SourceFile | FutureSourceFile, prog
}

function addImportFromExportedSymbol(exportedSymbol: Symbol, isValidTypeOnlyUseSite?: boolean, referenceImport?: ImportOrRequireAliasDeclaration) {
const checker = program.getTypeChecker();
const useRequire = shouldUseRequire(sourceFile, program);
const moduleSymbol = Debug.checkDefined(exportedSymbol.parent);
const symbolName = getNameForExportedSymbol(exportedSymbol, getEmitScriptTarget(compilerOptions));
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 (!exportInfo) {
Expand All @@ -285,7 +287,6 @@ function createImportAdderWorker(sourceFile: SourceFile | FutureSourceFile, prog
Debug.assert(preferences.autoImportFileExcludePatterns?.length);
return;
}
const useRequire = shouldUseRequire(sourceFile, program);
let fix = getImportFixForSymbol(sourceFile, exportInfo, program, /*position*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
if (fix) {
const localName = tryCast(referenceImport?.name, isIdentifier)?.text ?? symbolName;
Expand Down Expand Up @@ -356,6 +357,24 @@ function createImportAdderWorker(sourceFile: SourceFile | FutureSourceFile, prog
}
}

function addImportForExternalModuleSymbol(symbol: Symbol, originalSymbol: Symbol, isValidTypeOnlyUseSite: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

symbol/originalSymbol are confusing names here. It looks like originalSymbol is the actual module symbol, and symbol is just an existing alias for the module that’s only being used to see what name to use. Instead, try just passing in the module symbol and the name you want to use?

const useRequire = shouldUseRequire(sourceFile, program);
if (originalSymbol.valueDeclaration) {
Copy link
Member

Choose a reason for hiding this comment

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

What is this check for? When does an external module symbol not have a valueDeclaration?

const moduleSpecifier = moduleSpecifiers.getLocalModuleSpecifierBetweenFileNames(
sourceFile,
originalSymbol.valueDeclaration.getSourceFile().fileName,
compilerOptions,
createModuleSpecifierResolutionHost(program, host),
);
const info: FixInfo = {
fix: { kind: ImportFixKind.AddNew, importKind: ImportKind.Namespace, addAsTypeOnly: isValidTypeOnlyUseSite ? AddAsTypeOnly.Allowed : AddAsTypeOnly.NotAllowed, useRequire, moduleSpecifierKind: undefined, moduleSpecifier },
Copy link
Member

Choose a reason for hiding this comment

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

Since there may be multiple different forms of import that can reference a whole module, you were asking about whether it’s ok to unconditionally coerce them into a namespace import. That might be fine, but it seems like it would make sense for this function to take an optional referenceImport like addImportFromExportedSymbol, and copy its kind and type-only status where appropriate.

symbolName: symbol.name,
errorIdentifierText: undefined,
};
addImport(info);
}
}

function removeExistingImport(declaration: ImportOrRequireAliasDeclaration) {
if (declaration.kind === SyntaxKind.ImportClause) {
Debug.assertIsDefined(declaration.name, "ImportClause should have a name if it's being removed");
Expand Down
3 changes: 2 additions & 1 deletion src/services/refactors/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
identifierToKeywordKind,
isAnyImportOrRequireStatement,
isClassLike,
isExternalModuleSymbol,
isPrivateIdentifier,
isPropertyAccessExpression,
ModuleBlock,
Expand Down Expand Up @@ -81,7 +82,7 @@ export function addTargetFileImports(
importAdder.addVerbatimImport(Debug.checkDefined(declaration ?? findAncestor(symbol.declarations?.[0], isAnyImportOrRequireStatement)));
}
else {
importAdder.addImportFromExportedSymbol(targetSymbol, isValidTypeOnlyUseSite, declaration);
isExternalModuleSymbol(targetSymbol) ? importAdder.addImportForExternalModuleSymbol(symbol, targetSymbol, isValidTypeOnlyUseSite) : importAdder.addImportFromExportedSymbol(targetSymbol, isValidTypeOnlyUseSite, declaration);
}
});

Expand Down
14 changes: 11 additions & 3 deletions src/services/refactors/moveToFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,14 +1131,22 @@ export function getExistingLocals(sourceFile: SourceFile, statements: readonly S
const declaration = importFromModuleSpecifier(moduleSpecifier);
if (
isImportDeclaration(declaration) && declaration.importClause &&
declaration.importClause.namedBindings && isNamedImports(declaration.importClause.namedBindings)
declaration.importClause.namedBindings
) {
for (const e of declaration.importClause.namedBindings.elements) {
const symbol = checker.getSymbolAtLocation(e.propertyName || e.name);
if (isNamespaceImport(declaration.importClause.namedBindings)) {
const symbol = checker.getSymbolAtLocation(declaration.importClause.namedBindings.name);
Copy link
Member

Choose a reason for hiding this comment

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

I think there’s no need for checker.getSymbolAtLocation anywhere in this function. Declarations always have a .symbol; you can just do declaration.importClause.namedBindings.symbol here, and .symbol on each ImportSpecifier below. Even the existing variable declaration / binding pattern stuff further down can lose the getSymbolAtLocation.

if (symbol) {
existingLocals.add(skipAlias(symbol, checker));
}
}
else if (isNamedImports(declaration.importClause.namedBindings)) {
for (const e of declaration.importClause.namedBindings.elements) {
const symbol = checker.getSymbolAtLocation(e.propertyName || e.name);
if (symbol) {
existingLocals.add(skipAlias(symbol, checker));
}
}
}
}
if (isVariableDeclarationInitializedToRequire(declaration.parent) && isObjectBindingPattern(declaration.parent.name)) {
for (const e of declaration.parent.name.elements) {
Expand Down
Loading
Loading