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 14 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
23 changes: 22 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,25 @@ 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 && isImportClause(referenceImport)) {
info = { ...info, fix: { ...info.fix, importKind: ImportKind.Default, addAsTypeOnly: isTypeOnlyImportDeclaration(referenceImport) ? AddAsTypeOnly.Required : AddAsTypeOnly.NotAllowed } };
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 isValidTypeOnlyUseSite should be consulted here in addition to the reference import. Also, what if the reference import is a type-only namespace import? It looks like we're only considering the type-onlyness of the import if it's a default import.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what the rules are supposed to be for setting addAsTypeOnly? I don't quite understand the last commit.

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 believe when isValidTypeOnlyUseSite is true, then if isTypeOnlyImportDeclaration(referenceImport) is true then it should be AddAsTypeOnly.Required and if not, then it should be AddAsTypeOnly.Allowed.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, and if isValidTypeOnlyUseSite is false, then it has to be AddAsTypeOnly.NotAllowed. That makes sense to me!

}
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
18 changes: 13 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 @@ -964,7 +964,7 @@ function inferNewFileName(importsFromNewFile: Map<Symbol, unknown>, movedSymbols
function forEachReference(node: Node, checker: TypeChecker, enclosingRange: TextRange | undefined, onReference: (s: Symbol, isValidTypeOnlyUseSite: boolean) => void) {
node.forEachChild(function cb(node) {
if (isIdentifier(node) && !isDeclarationName(node)) {
if (enclosingRange && !rangeContainsRange(enclosingRange, node)) {
if (enclosingRange && !rangeContainsRange(enclosingRange, { pos: node.getStart(), end: node.getEnd() })) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (enclosingRange && !rangeContainsRange(enclosingRange, { pos: node.getStart(), end: node.getEnd() })) {
if (enclosingRange && !rangeContainsRange(enclosingRange, createTextRangeFromNode(node, sourceFile))) {

And declare sourceFile outside the recursive function so it doesn’t have to be retrieved with a parent walk each time

return;
}
const sym = checker.getSymbolAtLocation(node);
Expand Down Expand Up @@ -1125,18 +1125,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