fixUnusedIdentifier: Handle destructure with all bindings unused#23805
fixUnusedIdentifier: Handle destructure with all bindings unused#238055 commits merged intomasterfrom
Conversation
| const message = isTypeDeclaration(declaration) ? Diagnostics._0_is_declared_but_never_used : Diagnostics._0_is_declared_but_its_value_is_never_read; | ||
| addDiagnostic(UnusedKind.Local, createDiagnosticForNodeSpan(getSourceFileOfNode(declaration), declaration, node, message, name)); | ||
| } | ||
| const message = isTypeDeclaration(declaration) ? Diagnostics._0_is_declared_but_never_used : Diagnostics._0_is_declared_but_its_value_is_never_read; |
There was a problem hiding this comment.
but even with this, we still have a comma probelm.. like:
declare var o: any;
var { x, y } = o;
y;
var { a, b } = o;
a;still convert to:
declare var o: any;
var {, y } = o;
y;
var { a, } = o;
a;The first one is a syntax error. the second is not what a human would have written either.
There was a problem hiding this comment.
so were you intending this change to address that, or this will be in a separate change?
There was a problem hiding this comment.
and if this is handled with a different change, then do we still need this one?
There was a problem hiding this comment.
I think so, even if we correctly delete every element from the list, that's different from deleting the entire list.
There was a problem hiding this comment.
that is another issue.. deleting the entire list may not be correct in the general case.. for instance var { x } = foo() if foo had side effects, then deleting it would be wrong..
One option is to convert it to var {} = foo() which is what i was thinking about when i asked about commas earlier. but i do agree that this solution would be ugly.
Alternatively, we gonna just make it foo(), and if the expression was a single identifier or a property access we can remove it. We have in the checker a function isSideEffectFree, we could leverage this to decide whether to delete the whole statement or not.
There was a problem hiding this comment.
Another question, how does this apply to functions. I see in this change you are only handling variable statements..
There was a problem hiding this comment.
for instance var { x } = foo() if foo had side effects, then deleting it would be wrong..
But that's exactly what we do for var x = foo()... I think it's better to assume that any side effects are unwanted, since the function was being called for a result before and not merely for a side effect -- the situation is similar to #17624.
There was a problem hiding this comment.
mind filing a bug for removing nodes with possible side-effects
ghost
mentioned this pull request
| return startToken.kind === SyntaxKind.ImportKeyword ? tryCast(startToken.parent, isImportDeclaration) : undefined; | ||
| } | ||
|
|
||
| function tryDeleteFullDestructure(changes: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number): boolean { |
There was a problem hiding this comment.
add a test for for(var {} of o) ;
ghost
force-pushed
the
codeFixUnusedIdentifier_destructure
branch
from
0afb217 to
18effd2
Compare
1 participant
Fixes one of the issues from #22330. Similar to #22386.