[analysis server] Improve dead code removal to prepare for sound-flow-analysis.
The following improvements are made to the "remove dead code" quick fix:
- Dead code removal can now replace conditional expressions like
`condition ? live : dead` or `condition ? dead : live` with just the
live subexpression, provided that the condition appears free of side
effects.
- Dead code removal can now replace `if` statements like `if
(condition) live; else dead;` or `if (condition) dead; else live;`
with just the live statement, provided that the condition appears
free of side effects.
- Dead code removal can now fully remove `if` statements like `if
(condition) dead;`, provided that the condition appears free of side
effects.
Extra care is taken around opening and closing braces to try to
preserve reasonable formatting as much as possible.
These features were chosen by examining the set of new dead code
warnings that pop up inside google3 if the sound-flow-analysis
language feature is enabled. They are sufficient to allow 98% of those
new warnings to be addressed by the quick fix. This should help
customers adjust their code quickly when moving to a language version
that enables this language feature.
The notion that a condition "appears free of side effects" is defined
to include casts, non-null assertions, and getter invocations. It is
of course possible that a getter invocation, a cast, or a non-null
assertion could have a side effect that is important, but it would be
difficult to build the necessary static analysis to detect these
cases, and in practice, they don't happen very often. Considering that
the "remove dead code" quick fix is not usable in an automated fashion
(it must be explicitly requested by the user in their editor for each
piece of affected dead code), I believe this is a good tradeoff.
Bug: https://github.com/dart-lang/sdk/issues/60438
Change-Id: Ied43500c4b7a729bbd686708ab10e80e72f22088
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/425180
Commit-Queue: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
diff --git a/pkg/analysis_server/lib/src/services/correction/dart/remove_dead_code.dart b/pkg/analysis_server/lib/src/services/correction/dart/remove_dead_code.dart
index 18646177..5a46cd7 100644
--- a/pkg/analysis_server/lib/src/services/correction/dart/remove_dead_code.dart
+++ b/pkg/analysis_server/lib/src/services/correction/dart/remove_dead_code.dart
@@ -160,6 +160,67 @@
await _addEdit((builder) {
builder.addDeletion(deletionRange);
});
+ case Expression(
+ parent: ConditionalExpression(
+ parent: var grandParent,
+ :var condition,
+ :var thenExpression,
+ :var elseExpression,
+ ) &&
+ var parent,
+ )
+ when (node == thenExpression || node == elseExpression) &&
+ _looksSideEffectFree(condition):
+ // Then expression is `condition ? live : dead` or
+ // `condition ? dead : live`, with a condition that is free of side
+ // effects (or so we presume--see `_looksSideEffectFree` for details).
+ // So the conditional expression can be replaced with the `live`
+ // subexpression.
+ var nodeToKeep =
+ node == thenExpression ? elseExpression : thenExpression;
+ if (nodeToKeep is ThrowExpression &&
+ grandParent is CascadeExpression &&
+ grandParent.target == parent) {
+ // It's not safe to transform something like
+ // `a ? b : throw c..cascadeSection` into `throw c..cascadeSection`,
+ // because the former parses as `(a ? b : throw c)..cascadeSection`,
+ // and the latter parses as `throw (c..cascadeSection)`. This is
+ // unlikely to arise in practice, so just bail out and don't generate
+ // a correction.
+ return;
+ }
+ await _addEdit((builder) {
+ builder.addDeletion(range.startStart(parent, nodeToKeep));
+ builder.addDeletion(range.endEnd(nodeToKeep, parent));
+ });
+ case Statement(
+ parent: IfStatement(
+ :var expression,
+ caseClause: null,
+ :var thenStatement,
+ :var elseStatement,
+ ) &&
+ var ifStatement,
+ )
+ when _looksSideEffectFree(expression):
+ if (node == thenStatement) {
+ if (elseStatement == null) {
+ // The "if" statement is `if (expression) dead;`, so it can be
+ // removed in its entirety.
+ await _removeStatement(ifStatement);
+ } else {
+ // The "if" statement is `if (expression) dead; else live;`, so it
+ // can be replaced with `live;`.
+ await _replaceStatementWithInnerStatement(
+ ifStatement,
+ elseStatement,
+ );
+ }
+ } else if (node == elseStatement) {
+ // The "if" statement is `if (expression) live; else dead;`, so it can
+ // be replaced with `live;`.
+ await _replaceStatementWithInnerStatement(ifStatement, thenStatement);
+ }
case Statement(:Block parent):
var lastStatementInBlock = parent.statements.last;
assert(
@@ -189,6 +250,64 @@
}
}
+ /// Determines whether [expression] is side effect free, under certain
+ /// presumptions.
+ ///
+ /// The presumptions are:
+ /// - A getter invocation is presumed to be side effect free, because (a) in
+ /// practice, most getter invocations are side effect free, and (b) it would
+ /// take a lot of analysis to establish with certainty whether a particular
+ /// getter invocation is side effect free.
+ /// - A use of a non-null assertion (`!`) or cast (`as`) is presumed to be
+ /// side effect free, because in practice these constructs are only used
+ /// when their operand is already known by the user to be of the appropriate
+ /// type. This enables dead code like `if (a.b!.c == null) { ... }` to be
+ /// eliminated (assuming `c` has a non-null type).
+ ///
+ /// These assumptions aren't necessarily going to be true of all code. We rely
+ /// on the user to verify these assumptions by inspecting the results of dead
+ /// code removal.
+ bool _looksSideEffectFree(Expression expression) {
+ switch (expression) {
+ case AsExpression(:var expression):
+ // A cast is presumed to be side effect free (provided that its operand
+ // is).
+ return _looksSideEffectFree(expression);
+ case BinaryExpression(:var leftOperand, :var rightOperand):
+ return _looksSideEffectFree(leftOperand) &&
+ _looksSideEffectFree(rightOperand);
+ case SimpleIdentifier():
+ // A simple identifier might be a local variable reference or a method
+ // tear-off, in which case it's definitely side effect free. Or it might
+ // be a call to a getter, in which case we assume it's side effect free.
+ return true;
+ case Literal():
+ return true;
+ case ParenthesizedExpression(:var expression):
+ return _looksSideEffectFree(expression);
+ case PostfixExpression(
+ :var operand,
+ operator: Token(type: TokenType.BANG),
+ ):
+ // A non-null assertion is presumed to be side effect free (provided
+ // that its operand is).
+ return _looksSideEffectFree(operand);
+ case PrefixedIdentifier(prefix: Expression? target):
+ case PropertyAccess(:var target):
+ // If the target isn't side effect free, then the property access isn't.
+ if (target != null && !_looksSideEffectFree(target)) return false;
+ // A property access might be a method tear-off, in which case it's
+ // definitely side effect free. Or it might be a call to a getter, in
+ // which case we assume it's side effect free.
+ return true;
+ case SuperExpression():
+ return true;
+ default:
+ // Anything else is conservatively assumed to have side effects.
+ return false;
+ }
+ }
+
/// In the case where the dead code range starts somewhere inside
/// [partiallyDeadNode], and may continue past it, removes any code after
/// [partiallyDeadNode] that is included in the dead code range.
@@ -244,4 +363,63 @@
}
}
}
+
+ Future<void> _removeStatement(Statement statement) async {
+ var parent = statement.parent;
+ switch (parent) {
+ case Block():
+ await _addEdit((builder) {
+ _deleteLineRange(builder, range.entity(statement));
+ });
+ case IfStatement(:var thenStatement, :var elseStatement)
+ when statement == elseStatement:
+ // The code looks something like:
+ // if (condition) live; else if (false) dead;
+ // thenStatement-^^^^^ ^^^^^^^^^^^^^^^^-elseStatement
+ // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-parent
+ // So drop the `else` and the `elseStatement` to produce:
+ // if (condition) live;
+ await _addEdit((builder) {
+ builder.addDeletion(range.endEnd(thenStatement, parent));
+ });
+ default:
+ // In general it's not safe to just remove a statement, since it affects
+ // how the surrounding code will be parsed. So don't generate a
+ // correction.
+ break;
+ }
+ }
+
+ Future<void> _replaceStatementWithInnerStatement(
+ Statement statement,
+ Statement innerStatement,
+ ) async {
+ var parent = statement.parent;
+ if (parent is Block && innerStatement is Block) {
+ // The code looks something like:
+ // { ... if (condition) { live; } ... }
+ // ^^^^^^^^^-innerStatement
+ // ^^^^^^^^^^^^^^^^^^^^^^^^-statement
+ // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-parent
+ // So drop the opening and closing brace of `innerStatement` to produce:
+ // { ... live; ... }
+ await _addEdit((builder) {
+ _deleteLineRange(
+ builder,
+ range.startEnd(statement, innerStatement.leftBracket),
+ );
+ _deleteLineRange(
+ builder,
+ range.startEnd(innerStatement.rightBracket, statement),
+ );
+ });
+ } else {
+ // Don't do anything special with braces. Just replace `statement` with
+ // `innerStatement`.
+ await _addEdit((builder) {
+ builder.addDeletion(range.startStart(statement, innerStatement));
+ builder.addDeletion(range.endEnd(innerStatement, statement));
+ });
+ }
+ }
}
diff --git a/pkg/analysis_server/test/src/services/correction/fix/remove_dead_code_test.dart b/pkg/analysis_server/test/src/services/correction/fix/remove_dead_code_test.dart
index 773ae54..aa2bf78 100644
--- a/pkg/analysis_server/test/src/services/correction/fix/remove_dead_code_test.dart
+++ b/pkg/analysis_server/test/src/services/correction/fix/remove_dead_code_test.dart
@@ -122,6 +122,54 @@
''');
}
+ Future<void> test_conditionalExpression_falseCondition() async {
+ await resolveTestCode('''
+void f(int i, int j) => false ? i : j;
+''');
+ await assertHasFix('''
+void f(int i, int j) => j;
+''');
+ }
+
+ Future<void>
+ test_conditionalExpression_falseCondition_cascadeException() async {
+ // It's not safe to transform something like
+ // `a ? b : throw c..cascadeSection` into `throw c..cascadeSection`, because
+ // the former parses as `(a ? b : throw c)..cascadeSection`, and the latter
+ // parses as `throw (c..cascadeSection)`.
+ var code = '''
+void f(int i, int j) => false ? i : throw j..abs();
+''';
+ await resolveTestCode(code);
+ await assertNoFix(
+ errorFilter: (error) => error.offset == code.indexOf('i :'),
+ );
+ }
+
+ Future<void> test_conditionalExpression_trueCondition() async {
+ await resolveTestCode('''
+void f(int i, int j) => true ? i : j;
+''');
+ await assertHasFix('''
+void f(int i, int j) => i;
+''');
+ }
+
+ Future<void>
+ test_conditionalExpression_trueCondition_cascadeException() async {
+ // It's not safe to transform something like
+ // `a ? throw b : c..cascadeSection` into `throw b..cascadeSection`, because
+ // the former parses as `(a ? throw b : c)..cascadeSection`, and the latter
+ // parses as `throw (b..cascadeSection)`.
+ var code = '''
+void f(int i, int j) => true ? throw i : j..abs();
+''';
+ await resolveTestCode(code);
+ await assertNoFix(
+ errorFilter: (error) => error.offset == code.indexOf('j..'),
+ );
+ }
+
Future<void> test_do_returnInBody() async {
await resolveTestCode('''
void f(bool c) {
@@ -283,6 +331,25 @@
''', errorFilter: (error) => error.length == 12);
}
+ Future<void> test_else_if_false() async {
+ await resolveTestCode('''
+void f(bool b) {
+ if (b) {
+ print('');
+ } else if (false) {
+ return;
+ }
+}
+''');
+ await assertHasFix('''
+void f(bool b) {
+ if (b) {
+ print('');
+ }
+}
+''');
+ }
+
Future<void> test_emptyStatement() async {
await resolveTestCode('''
void f() {
@@ -462,20 +529,206 @@
''');
}
- Future<void> test_if_false_return() async {
+ Future<void> test_if_false_block() async {
+ await resolveTestCode('''
+void f() {
+ if (false) {
+ return;
+ }
+ print('');
+}
+''');
+ await assertHasFix('''
+void f() {
+ print('');
+}
+''');
+ }
+
+ Future<void> test_if_false_block_else_block() async {
+ await resolveTestCode('''
+void f() {
+ if (false) {
+ return;
+ } else {
+ print('1');
+ }
+ print('2');
+}
+''');
+ await assertHasFix('''
+void f() {
+ print('1');
+ print('2');
+}
+''');
+ }
+
+ Future<void> test_if_false_block_else_noBlock() async {
+ await resolveTestCode('''
+void f() {
+ if (false) {
+ return;
+ } else print('1');
+ print('2');
+}
+''');
+ await assertHasFix('''
+void f() {
+ print('1');
+ print('2');
+}
+''');
+ }
+
+ Future<void> test_if_false_noBlock() async {
await resolveTestCode('''
void f() {
if (false) return;
print('');
}
''');
- // No fix. It's not safe to remove the `return;` statement, because then the
- // `if (false)` would cover the `print('');` statement.
- // TODO(paulberry): add the ability to recognize that `false` has no effect,
- // and thus it is safe to remove the entire `if` statement.
+ await assertHasFix('''
+void f() {
+ print('');
+}
+''');
+ }
+
+ Future<void> test_if_false_noBlock_else_block() async {
+ await resolveTestCode('''
+void f() {
+ if (false) return; else {
+ print('1');
+ }
+ print('2');
+}
+''');
+ await assertHasFix('''
+void f() {
+ print('1');
+ print('2');
+}
+''');
+ }
+
+ Future<void> test_if_false_noBlock_else_noBlock() async {
+ await resolveTestCode('''
+void f() {
+ if (false) return; else print('1');
+ print('2');
+}
+''');
+ await assertHasFix('''
+void f() {
+ print('1');
+ print('2');
+}
+''');
+ }
+
+ Future<void> test_if_false_notInBlock() async {
+ // Normally a statement like `if (true) { live; } else { dead; }` will be
+ // fixed by removing the opening and closing braces around `live;`. But if
+ // the parent of the if statement is not a block, this is not safe to do,
+ // so the braces are kept.
+ await resolveTestCode('''
+void f() {
+ while (true) if (true) {
+ print('1');
+ } else {
+ return;
+ }
+}
+''');
+ await assertHasFix('''
+void f() {
+ while (true) {
+ print('1');
+ }
+}
+''');
+ }
+
+ Future<void> test_if_false_notRemovable() async {
+ // Normally a statement like `if (false) dead;` can be removed. But if the
+ // parent is not a block, it's not safe to do so.
+ await resolveTestCode('''
+void f() {
+ while (true) if (false) return;
+}
+''');
await assertNoFix();
}
+ Future<void> test_if_true_block_else_block() async {
+ await resolveTestCode('''
+void f() {
+ if (true) {
+ print('1');
+ } else {
+ return;
+ }
+ print('2');
+}
+''');
+ await assertHasFix('''
+void f() {
+ print('1');
+ print('2');
+}
+''');
+ }
+
+ Future<void> test_if_true_block_else_noBlock() async {
+ await resolveTestCode('''
+void f() {
+ if (true) {
+ print('1');
+ } else return;
+ print('2');
+}
+''');
+ await assertHasFix('''
+void f() {
+ print('1');
+ print('2');
+}
+''');
+ }
+
+ Future<void> test_if_true_noBlock_else_block() async {
+ await resolveTestCode('''
+void f() {
+ if (true) print('1'); else {
+ return;
+ }
+ print('2');
+}
+''');
+ await assertHasFix('''
+void f() {
+ print('1');
+ print('2');
+}
+''');
+ }
+
+ Future<void> test_if_true_noBlock_else_noBlock() async {
+ await resolveTestCode('''
+void f() {
+ if (true) print('1'); else return;
+ print('2');
+}
+''');
+ await assertHasFix('''
+void f() {
+ print('1');
+ print('2');
+}
+''');
+ }
+
Future<void> test_statements_one() async {
await resolveTestCode('''
int f() {
@@ -656,6 +909,248 @@
@override
List<String> get experiments => [...super.experiments, 'sound-flow-analysis'];
+ Future<void> test_conditional_unnecessaryNullCheck_equalsNull() async {
+ await resolveTestCode(r'''
+class C {
+ String? s;
+ void f(int i) {
+ s = i == null ? null : '$i';
+ }
+}
+''');
+ await assertHasFix(r'''
+class C {
+ String? s;
+ void f(int i) {
+ s = '$i';
+ }
+}
+''', errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_conditional_unnecessaryNullCheck_notEqualsNull() async {
+ await resolveTestCode(r'''
+class C {
+ String? s;
+ void f(int i) {
+ s = i != null ? '$i' : null;
+ }
+}
+''');
+ await assertHasFix(r'''
+class C {
+ String? s;
+ void f(int i) {
+ s = '$i';
+ }
+}
+''', errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_castEqualsNull() async {
+ // Casts are presumed to be side effect free, so it is safe to remove
+ // `if ((i as int) == null) return null;`
+ await resolveTestCode(r'''
+String? f(int? i) {
+ if ((i as int) == null) return null;
+ return '$i';
+}
+''');
+ await assertHasFix(r'''
+String? f(int? i) {
+ return '$i';
+}
+''', errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_castEqualsNull_withTargetSideEffect() async {
+ // Casts are presumed to be side effect free, but only if the target of the
+ // cast is side effect free. So it is not safe to remove
+ // `if ((g(i) as int) == null) return null;`, because `g(i)` may have side
+ // effects.
+ await resolveTestCode(r'''
+String? f(int i, int? Function(int) g) {
+ if ((g(i) as int) == null) return null;
+ return '$i';
+}
+''');
+ await assertNoFix(errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_identifierEqualsNull() async {
+ await resolveTestCode(r'''
+String? f(int i) {
+ if (i == null) return null;
+ return '$i';
+}
+''');
+ await assertHasFix(r'''
+String? f(int i) {
+ return '$i';
+}
+''', errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_nonNullAssertEqualsNull() async {
+ // Non-null assertions are presumed to be side effect free, so it is safe to
+ // remove `if (i! == null) return null;`
+ await resolveTestCode(r'''
+String? f(int? i) {
+ if (i! == null) return null;
+ return '$i';
+}
+''');
+ await assertHasFix(r'''
+String? f(int? i) {
+ return '$i';
+}
+''', errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_nonNullAssertEqualsNull_withTargetSideEffect() async {
+ // Non-null assertions are presumed to be side effect free, but only if the
+ // target of the assertion is side effect free. So it is not safe to remove
+ // `if (g(i)! == null) return null;`, because `g(i)` may have side effects.
+ await resolveTestCode(r'''
+String? f(int i, int? Function(int) g) {
+ if (g(i)! == null) return null;
+ return '$i';
+}
+''');
+ await assertNoFix(errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_nullEqualsPropertyAccess_withTargetSideEffect() async {
+ // Getter invocations are presumed to be side effect free, but only if the
+ // target of the invocation is side effect free. So it is not safe to remove
+ // `if (null == g(i).isEven) return null;`, because `g(i)` may have side
+ // effects.
+ await resolveTestCode(r'''
+String? f(int i, int Function(int) g) {
+ if (null == g(i).isEven) return null;
+ return '$i';
+}
+''');
+ await assertNoFix(errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_postfixOperatorEqualsNull() async {
+ // Postfix operators other than `!` are not side effect free, so it is not
+ // safe to remove `if (i++ == null) return null;`.
+ await resolveTestCode(r'''
+String? f(int i) {
+ if (i++ == null) return null;
+ return '$i';
+}
+''');
+ await assertNoFix(errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_prefixedIdentifierEqualsNull() async {
+ // Getter invocations are presumed to be side effect free, so it is safe to
+ // remove `if (i.isEven == null) return null;`.
+ await resolveTestCode(r'''
+String? f(int i) {
+ if (i.isEven == null) return null;
+ return '$i';
+}
+''');
+ await assertHasFix(r'''
+String? f(int i) {
+ return '$i';
+}
+''', errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_propertyAccessEqualsNull() async {
+ // Getter invocations are presumed to be side effect free, so it is safe to
+ // remove `if ((i).isEven == null) return null;`.
+ // Note: parentheses around `i` to force `.isEven` to be represented as a
+ // PropertyAccess rather than a PrefixedIdentifier.
+ await resolveTestCode(r'''
+String? f(int i) {
+ if ((i).isEven == null) return null;
+ return '$i';
+}
+''');
+ await assertHasFix(r'''
+String? f(int i) {
+ return '$i';
+}
+''', errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_propertyAccessEqualsNull_nullAware() async {
+ // Getter invocations are presumed to be side effect free, so it is safe to
+ // remove `if ((i?.isEven)! == null) return null;`.
+ await resolveTestCode(r'''
+String? f(int i) {
+ if ((i?.isEven)! == null) return null;
+ return '$i';
+}
+''');
+ await assertHasFix(r'''
+String? f(int i) {
+ return '$i';
+}
+''', errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void>
+ test_if_propertyAccessEqualsNull_parenthesizedWithTargetSideEffect() async {
+ // Getter invocations are presumed to be side effect free, but only if the
+ // target of the invocation is side effect free. So it is not safe to remove
+ // `if ((g(i)).isEven == null) return null;`, because `(g(i))` may have side
+ // effects.
+ await resolveTestCode(r'''
+String? f(int i, int Function(int) g) {
+ if ((g(i)).isEven == null) return null;
+ return '$i';
+}
+''');
+ await assertNoFix(errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_propertyAccessEqualsNull_super() async {
+ // Getter invocations are presumed to be side effect free, so it is safe to
+ // remove `if (super.foo == null) return null;`.
+ await resolveTestCode(r'''
+class B {
+ int get foo => 0;
+}
+class C extends B {
+ String? f() {
+ if (super.foo == null) return null;
+ return '${super.foo}';
+ }
+}
+''');
+ await assertHasFix(r'''
+class B {
+ int get foo => 0;
+}
+class C extends B {
+ String? f() {
+ return '${super.foo}';
+ }
+}
+''', errorFilter: _ignoreNullSafetyWarnings);
+ }
+
+ Future<void> test_if_propertyAccessEqualsNull_withTargetSideEffect() async {
+ // Getter invocations are presumed to be side effect free, but only if the
+ // target of the invocation is side effect free. So it is not safe to remove
+ // `if (g(i).isEven == null) return null;`, because `g(i)` may have side
+ // effects.
+ await resolveTestCode(r'''
+String? f(int i, int Function(int) g) {
+ if (g(i).isEven == null) return null;
+ return '$i';
+}
+''');
+ await assertNoFix(errorFilter: _ignoreNullSafetyWarnings);
+ }
+
Future<void> test_ifNull() async {
await resolveTestCode('''
void f(int i, int j) => i ?? j;
@@ -668,5 +1163,9 @@
/// Error filter ignoring warnings that frequently occur in conjunction with
/// code that is dead due to sound flow analysis.
bool _ignoreNullSafetyWarnings(AnalysisError error) =>
- error.errorCode.name != 'DEAD_NULL_AWARE_EXPRESSION';
+ !const {
+ 'DEAD_NULL_AWARE_EXPRESSION',
+ 'INVALID_NULL_AWARE_OPERATOR',
+ 'UNNECESSARY_NULL_COMPARISON',
+ }.contains(error.errorCode.name);
}
