Skip to content

Implement ts.OrganizeImports.removeUnusedImports#22007

Merged
amcasey merged 2 commits into
microsoft:masterfrom
amcasey:UnusedImports
Feb 21, 2018
Merged

Implement ts.OrganizeImports.removeUnusedImports#22007
amcasey merged 2 commits into
microsoft:masterfrom
amcasey:UnusedImports

Conversation

@amcasey
Copy link
Copy Markdown
Member

@amcasey amcasey commented Feb 17, 2018

TODO: Still need to add support for organizing imports in ambient
modules

@amcasey amcasey requested review from a user and mhegazy February 17, 2018 02:14
return true;
}

const entries = FindAllReferences.getReferenceEntriesForNode(identifier.pos, identifier, program, [sourceFile], {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I have a repro where this returns entries from other files. I have yet to figure out why.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I can just add .filter(e => e.type === "node" && e.node.getSourceFile() === sourceFile), but first I'd like to confirm that this isn't a bug in FAR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's coming in as a related symbol. @mhegazy @Andy-MS Would it be preferable to filter at the call site (as above) or add a property to ts.FindAllReferences.Options?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Per our offline discussion, I've added filtering at the call site.

@amcasey
Copy link
Copy Markdown
Member Author

amcasey commented Feb 20, 2018

Failures are, once again, due to uppercase/lowercase mismatch. Investigating.

TODO: Still need to add support for organizing imports in ambient
modules

return usedImports;

function isDeclarationUsed(identifier: Identifier) {
Copy link
Copy Markdown

@ghost ghost Feb 20, 2018

Choose a reason for hiding this comment

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

return !!program.getTypeChecker().getSymbolAtLocation(identifier)!.isReferenced;, and at top, program.getSemanticDiagnostics(sourceFile); // Ensure we've checked this file so 'isReferenced' is defined on symbols.. Also must change check isUse && result && nameNotFoundMessage && noUnusedIdentifiers in checker.ts to not include && noUnusedIdentifiers, so we always set isReferenced accurately.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll defer to @mhegazy, who asked me to do it this way.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Any progress towards reaching a consensus on how to identify unused imports?


const usedImports: ImportDeclaration[] = [];

for (const importDecl of oldImports) {
Copy link
Copy Markdown

@ghost ghost Feb 20, 2018

Choose a reason for hiding this comment

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

return mapDefined(oldImports, importDecl => <<<importDecl, updated import, or undefined>>>)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand the advantage of doing it that way.

Copy link
Copy Markdown

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Have some ideas for how to improve the findAllRefs use but I'll do that in a later PR.

@amcasey amcasey merged commit 72a7194 into microsoft:master Feb 21, 2018
@amcasey amcasey deleted the UnusedImports branch February 21, 2018 21:35
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant