-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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.
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
Add code fix to remove unreachable code #24028
Conversation
7b9ac54
to
539a43d
Compare
539a43d
to
88b7948
Compare
const container = (isBlock(statement.parent) ? statement.parent : statement).parent; | ||
switch (container.kind) { | ||
case SyntaxKind.IfStatement: | ||
return (container as IfStatement).elseStatement ? statement : container as IfStatement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the statement was not in a block, that will leave the the ifStatement syntactically incorrect.. e.g.
if (false) console.log();
else console.log();
case SyntaxKind.ForStatement: | ||
return container as WhileStatement | ForStatement; | ||
default: | ||
return statement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you want to return all statemetns from this one untill the end of the block, e.g.:
function f() {
return 0;
// both should be removed, error is only on the first.
var x = 0;
console.log(x);
}
break; | ||
default: | ||
if (isBlock(statement.parent)) { | ||
split(sliceAfter(statement.parent.statements, statement), s => !isFunctionDeclaration(s), (start, end) => changes.deleteNodeRange(sourceFile, start, end)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably also want to preserve type alias declarations, interfaces, const enums, type only namespaces, var statements, ...
// Don't remove statements that can validly be used before they appear. | ||
return !isFunctionDeclaration(s) && !isPurelyTypeDeclaration(s) && | ||
// `var x;` may declare a variable used above | ||
!(isVariableStatement(s) && !(getCombinedNodeFlags(s) & (NodeFlags.Let | NodeFlags.Const)) && s.declarationList.declarations.some(d => !d.initializer)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it is a var statement it make sure it is not ambient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's ambient, it should not have an initializer so we won't remove it, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
declare let x;
would be removed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right.
Actually, I can't think of a way a declare
statement would be unreachable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions.
newFileContent: | ||
` | ||
// No good way to delete just the 'if' part | ||
if (false) { } else b; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a perfect world, the output would be b;
?
case SyntaxKind.IfStatement: | ||
if ((container as IfStatement).elseStatement) { | ||
if (isBlock(statement.parent)) { | ||
changes.deleteNodeRange(sourceFile, first(statement.parent.statements), last(statement.parent.statements)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to eliminate the whole else-clause?
} | ||
|
||
function shouldRemove(s: Statement): boolean { | ||
// Don't remove statements that can validly be used before they appear. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing something - why would it be reported as unused if it's used above?
split(sliceAfter(statement.parent.statements, statement), shouldRemove, (start, end) => changes.deleteNodeRange(sourceFile, start, end)); | ||
} | ||
else { | ||
changes.deleteNode(sourceFile, statement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will a switch case ever be reported as unused? Does that need special handling? What about a catch block?
Mentioned in #24026