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 all 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
33 changes: 32 additions & 1 deletion src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ import {
isFullSourceFile,
isIdentifier,
isImportableFile,
isImportClause,
isImportDeclaration,
isImportEqualsDeclaration,
isIntrinsicJsxName,
Expand Down Expand Up @@ -227,6 +228,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, symbolName: string, isValidTypeOnlyUseSite: boolean, referenceImport?: ImportOrRequireAliasDeclaration) => void;
addVerbatimImport: (declaration: AnyImportOrRequireStatement | ImportOrRequireAliasDeclaration) => void;
removeExistingImport: (declaration: ImportOrRequireAliasDeclaration) => void;
writeFixes: (changeTracker: textChanges.ChangeTracker, oldFileQuotePreference?: QuotePreference) => void;
Expand Down Expand Up @@ -255,7 +257,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 Down Expand Up @@ -356,6 +358,35 @@ function createImportAdderWorker(sourceFile: SourceFile | FutureSourceFile, prog
}
}

function addImportForExternalModuleSymbol(symbol: Symbol, symbolName: string, isValidTypeOnlyUseSite: boolean, referenceImport?: ImportOrRequireAliasDeclaration) {
const useRequire = shouldUseRequire(sourceFile, program);
const moduleSpecifier = moduleSpecifiers.getLocalModuleSpecifierBetweenFileNames(
sourceFile,
Debug.checkDefined(symbol.valueDeclaration).getSourceFile().fileName,
compilerOptions,
createModuleSpecifierResolutionHost(program, host),
);
let info: FixInfo = {
fix: { kind: ImportFixKind.AddNew, importKind: ImportKind.Namespace, addAsTypeOnly: isValidTypeOnlyUseSite ? AddAsTypeOnly.Allowed : AddAsTypeOnly.NotAllowed, useRequire, moduleSpecifierKind: undefined, moduleSpecifier },
symbolName,
errorIdentifierText: undefined,
};
if (referenceImport && isValidTypeOnlyUseSite && isImportClause(referenceImport)) {
info = {
...info,
fix: {
kind: ImportFixKind.AddNew,
importKind: isNamespaceImport(referenceImport) ? ImportKind.Namespace : ImportKind.Default,
Copy link
Member

@andrewbranch andrewbranch Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This condition can never be true based on the if statement it's in. Also, it looks like we don't use the referenceImport to decide between default/namespace unless isValidTypeOnlyUseSite is true, which is an unrelated piece of information. I think all this code can be reduced to a single assignment to info with conditional properties, which would help untangle the two separate things you're trying to control.

addAsTypeOnly: isTypeOnlyImportDeclaration(referenceImport) ? AddAsTypeOnly.Required : AddAsTypeOnly.Allowed,
useRequire,
moduleSpecifierKind: undefined,
moduleSpecifier,
},
};
}
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(targetSymbol, symbol.name, isValidTypeOnlyUseSite, declaration) : importAdder.addImportFromExportedSymbol(targetSymbol, isValidTypeOnlyUseSite, declaration);
}
});

Expand Down
20 changes: 15 additions & 5 deletions src/services/refactors/moveToFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
contains,
createFutureSourceFile,
createModuleSpecifierResolutionHost,
createTextRangeFromNode,
createTextRangeFromSpan,
Debug,
Declaration,
Expand Down Expand Up @@ -962,9 +963,10 @@ function inferNewFileName(importsFromNewFile: Map<Symbol, unknown>, movedSymbols
}

function forEachReference(node: Node, checker: TypeChecker, enclosingRange: TextRange | undefined, onReference: (s: Symbol, isValidTypeOnlyUseSite: boolean) => void) {
const sourceFile = node.getSourceFile();
node.forEachChild(function cb(node) {
if (isIdentifier(node) && !isDeclarationName(node)) {
if (enclosingRange && !rangeContainsRange(enclosingRange, node)) {
if (enclosingRange && !rangeContainsRange(enclosingRange, createTextRangeFromNode(node, sourceFile))) {
return;
}
const sym = checker.getSymbolAtLocation(node);
Expand Down Expand Up @@ -1125,18 +1127,26 @@ 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 = declaration.importClause.namedBindings.symbol;
if (symbol) {
existingLocals.add(skipAlias(symbol, checker));
}
}
else if (isNamedImports(declaration.importClause.namedBindings)) {
for (const e of declaration.importClause.namedBindings.elements) {
const symbol = e.symbol;
if (symbol) {
existingLocals.add(skipAlias(symbol, checker));
}
}
}
}
if (isVariableDeclarationInitializedToRequire(declaration.parent) && isObjectBindingPattern(declaration.parent.name)) {
for (const e of declaration.parent.name.elements) {
const symbol = checker.getSymbolAtLocation(e.propertyName || e.name);
const symbol = e.symbol;
if (symbol) {
existingLocals.add(skipAlias(symbol, checker));
}
Expand Down
Loading