When every import is unused, error on the entire import declaration#22386
When every import is unused, error on the entire import declaration#223864 commits merged intomasterfrom
Conversation
ghost
force-pushed
the
unusedImports_entireImportDeclaration
branch
from
2e25654 to
142d881
Compare
ghost
force-pushed
the
unusedImports_entireImportDeclaration
branch
from
142d881 to
bda9140
Compare
src/compiler/checker.ts
Outdated
| const importClause = decl.kind === SyntaxKind.ImportClause ? decl : decl.kind === SyntaxKind.NamespaceImport ? decl.parent : decl.parent.parent; | ||
| if (!forEachImportedDeclaration(importClause, d => d !== decl && !contains(unusedImports, d))) { | ||
| forEachImportedDeclaration(importClause, d => unorderedRemoveItem(unusedImports, d)); | ||
| error(importClause.parent, Diagnostics.No_import_of_0_is_used, showModuleSpecifier(importClause.parent)); |
There was a problem hiding this comment.
-
if there is only one import e.g. default or one named import, the other error is superior. i would suggests such extending the span and using the error message with he name of the import.
-
I think the new error message is inaccurate. we just know that the import declaration is not needed, but there may be other import declarations for the same module that are still in use. so possibly remove the name of the module from the error message and say
Import declaration unusedorall imports in import declaration are unused
src/compiler/checker.ts
Outdated
| for (const declaration of local.declarations) { | ||
| if (isAmbientModule(declaration)) continue; | ||
| if (isImportedDeclaration(declaration)) { | ||
| unusedImports.push(declaration); |
There was a problem hiding this comment.
Personally, I find the forEach code below a little hard to follow. Another way to express this might be to build a multi-map from import clauses to the unused declarations they contain. Then, for each import clause in the map, you could either report a single diagnostic or a list of diagnostics according to whether or not the import clause contained a declaration not in the corresponding unused list. I don't feel strong about this.
There was a problem hiding this comment.
Thanks, helped me remove the count variable.
| case fixIdDelete: | ||
| tryDeleteDeclaration(changes, sourceFile, token); | ||
| if (diag.code === Diagnostics.No_import_of_0_is_used.code) { | ||
| changes.deleteNode(sourceFile, getImportDeclarationAtErrorLocation(token)); |
There was a problem hiding this comment.
Which adjacent trivia does this delete?
There was a problem hiding this comment.
Looks like it removes leading but not trailing trivia by default.
| ////import a, { b } from "mod"; | ||
|
|
||
| verify.codeFix({ | ||
| description: "Remove declaration for: 'mod'", |
There was a problem hiding this comment.
This wording is confusing. What about something like "Remove import from 'mod'"?
|
@Andy-MS looks like you might need to regenerate baselines - there's a conflict with your expanded range change. |
amcasey
left a comment
amcasey
left a comment
There was a problem hiding this comment.
Thanks! I find it a lot clearer with the map.
src/compiler/checker.ts
Outdated
| }); | ||
|
|
||
| unusedImports.forEach(unuseds => { | ||
| const importClause = importClauseFromImported(first(unuseds)); // others will have the same clause |
There was a problem hiding this comment.
Why recompute this? Isn't it the map key?
There was a problem hiding this comment.
Unfortunately we don't support non-string keys for maps for compatibility with ES5. See shimMap.
There was a problem hiding this comment.
We could always make the map value be a pair though...
There was a problem hiding this comment.
I must have been thinking of another context. This makes sense, given that limitation.
ghost
mentioned this pull request
2 participants
Part of #22361 (comment)