Add UNREACHABLE_SWITCH_DEFAULT warning to the analyzer.

This warning is similar to the existing `UNREACHABLE_SWITCH_CASE`
warning, except that it warns if the `default` clause of a switch
statement is unreachable due to all the `case` clasuses fully
exhausting the switched type.

To make the implementation easier, I changed the API for the
`reportExhaustiveness` method in `_fe_analyzer_shared` (which is the
primary entry point to the shared exhaustiveness checker). Previously,
this method returned a list of `ExhaustivenessError`, where each list
element was either an `UnreachableCaseError` (indicating that a
certain case was unreachable) or a `NonExhaustiveError` (indicating
that the entire switch statement was not exhaustive). If the caller
passed in `false` for `computeUnreachable`, `UnreachableCaseError`s
would not be returned, so the returned list would either be empty or
contain a single `NonExhaustiveError`.

The new API renames the types for clarity:

- `NonExhaustiveError` becomes `NonExhaustiveness`, to highlight the
  fact that it's not necessarily an error for the switch's cases to be
  non-exhaustive; it's only an error if the scrutinee's static type is
  an "always exhaustive" type and there is no `default` clause.

- `UnreachableCaseError` becomes `CaseUnreachability`, to highlight
  the fact that it's not an error for a case to be unreachable; it's a
  warning.

Also, the new API adds instances of `CaseUnreachability` to an
optional user-provided list instead of returning a newly created list;
this allows callers to communicate that they don't need to see
`CaseUnreachability` information by passing `null`. This frees up the
return type to simply be an instance of `NonExhaustiveness` (if the
cases are not exhaustive) or `null` (if they are exhaustive). This
makes it easier for the analyzer to decide whether to issue the new
warning, because it doesn't have to dig around the list looking for an
instance of `NonExhaustiveness`.

The new warning has an associated quick fix (remove the unreachable
`default` clause). This quick fix uses the same `RemoveDeadCode` logic
in the analysis server that the existing `UNREACHABLE_SWITCH_CASE`
warning uses.

Fixes #54575.

Bug: #54575
Change-Id: I18b6b7c5249d77d28ead7488b4aae4ea65c4b664
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/378960
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Paul Berry <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
Reviewed-by: Erik Ernst <[email protected]>

Author: stereotype441

pkg/_fe_analyzer_shared/lib/src/exhaustiveness/exhaustive.dart

@@ -21,37 +21,44 @@ bool isExhaustive(ObjectPropertyLookup fieldLookup, Space valueSpace,
 /// checks to see if any case can't be matched because it's covered by previous
 /// cases.
 ///
-/// Returns a list of any unreachable case or non-exhaustive match errors.
-/// Returns an empty list if all cases are reachable and the cases are
-/// exhaustive.
-List<ExhaustivenessError> reportErrors(
+/// If any unreachable cases are found, information about them is appended to
+/// [caseUnreachabilities]. (If `null` is passed for [caseUnreachabilities],
+/// then no information about unreachable cases is generated).
+///
+/// If the switch cases are not fully exhaustive, details about how they fail to
+/// be exhaustive are returned using a data structure of type
+/// [NonExhaustiveness]; otherwise `null` is returned.
+///
+/// Note that if a non-null value is returned, that doesn't necessarily mean
+/// that an error should be reported; the caller still must check whether the
+/// switch has a `default` clause and whether the scrutinee type is an "always
+/// exhaustive" type.
+NonExhaustiveness? computeExhaustiveness(
     ObjectPropertyLookup fieldLookup, StaticType valueType, List<Space> cases,
-    {required bool computeUnreachable}) {
+    {List<CaseUnreachability>? caseUnreachabilities}) {
   _Checker checker = new _Checker(fieldLookup);
 
-  List<ExhaustivenessError> errors = <ExhaustivenessError>[];
-
   Space valuePattern = new Space(const Path.root(), valueType);
   List<List<Space>> caseRows = cases.map((space) => [space]).toList();
 
-  if (computeUnreachable) {
+  if (caseUnreachabilities != null) {
     for (int i = 1; i < caseRows.length; i++) {
       // See if this case is covered by previous ones.
       if (checker._unmatched(caseRows.sublist(0, i), caseRows[i],
               returnMultipleWitnesses: false) ==
           null) {
-        errors.add(new UnreachableCaseError(valueType, cases, i));
+        caseUnreachabilities.add(new CaseUnreachability(valueType, cases, i));
       }
     }
   }
 
   List<Witness>? witnesses = checker._unmatched(caseRows, [valuePattern],
       returnMultipleWitnesses: true);
   if (witnesses != null) {
-    errors.add(new NonExhaustiveError(valueType, cases, witnesses));
+    return new NonExhaustiveness(valueType, cases, witnesses);
+  } else {
+    return null;
   }
-
-  return errors;
 }
 
 /// Determines if [cases] is exhaustive over all values contained by
@@ -357,28 +364,26 @@ List<StaticType> checkingOrder(StaticType type, Set<Key> keysOfInterest) {
   return result;
 }
 
-class ExhaustivenessError {}
-
-class NonExhaustiveError implements ExhaustivenessError {
+class NonExhaustiveness {
   final StaticType valueType;
 
   final List<Space> cases;
 
   final List<Witness> witnesses;
 
-  NonExhaustiveError(this.valueType, this.cases, this.witnesses);
+  NonExhaustiveness(this.valueType, this.cases, this.witnesses);
 
   @override
   String toString() =>
       '$valueType is not exhaustively matched by ${cases.join('|')}.';
 }
 
-class UnreachableCaseError implements ExhaustivenessError {
+class CaseUnreachability {
   final StaticType valueType;
   final List<Space> cases;
   final int index;
 
-  UnreachableCaseError(this.valueType, this.cases, this.index);
+  CaseUnreachability(this.valueType, this.cases, this.index);
 
   @override
   String toString() => 'Case #${index + 1} ${cases[index]} is unreachable.';

pkg/_fe_analyzer_shared/lib/src/exhaustiveness/test_helper.dart

@@ -80,25 +80,20 @@ String? typesToText(Iterable<StaticType> types) {
   return sb.toString();
 }
 
-String errorToText(ExhaustivenessError error) {
-  if (error is NonExhaustiveError) {
-    StringBuffer sb = new StringBuffer();
-    sb.write('non-exhaustive:');
-    String delimiter = '';
-    for (Witness witness in error.witnesses) {
-      sb.write(delimiter);
-      String witnessText = witness.asWitness;
-      String correctionText = witness.asCorrection;
-      if (witnessText != correctionText) {
-        sb.write('$witnessText/$correctionText');
-      } else {
-        sb.write(witnessText);
-      }
-      delimiter = ';';
+String nonExhaustivenessToText(NonExhaustiveness nonExhaustiveness) {
+  StringBuffer sb = new StringBuffer();
+  sb.write('non-exhaustive:');
+  String delimiter = '';
+  for (Witness witness in nonExhaustiveness.witnesses) {
+    sb.write(delimiter);
+    String witnessText = witness.asWitness;
+    String correctionText = witness.asCorrection;
+    if (witnessText != correctionText) {
+      sb.write('$witnessText/$correctionText');
+    } else {
+      sb.write(witnessText);
     }
-    return sb.toString();
-  } else {
-    assert(error is UnreachableCaseError);
-    return 'unreachable';
+    delimiter = ';';
   }
+  return sb.toString();
 }

pkg/_fe_analyzer_shared/test/exhaustiveness/report_errors_test.dart

@@ -36,54 +36,54 @@ void main() {
 
     test('exhaustiveness', () {
       // Case matching top type covers all subtypes.
-      expectReportErrors(env, a, [a]);
-      expectReportErrors(env, b, [a]);
-      expectReportErrors(env, d, [a]);
+      expectExhaustiveness(env, a, [a]);
+      expectExhaustiveness(env, b, [a]);
+      expectExhaustiveness(env, d, [a]);
 
       // Case matching subtype doesn't cover supertype.
-      expectReportErrors(env, a, [b], 'A is not exhaustively matched by B.');
-      expectReportErrors(env, b, [b]);
-      expectReportErrors(env, d, [b]);
-      expectReportErrors(env, e, [b]);
+      expectExhaustiveness(env, a, [b], 'A is not exhaustively matched by B.');
+      expectExhaustiveness(env, b, [b]);
+      expectExhaustiveness(env, d, [b]);
+      expectExhaustiveness(env, e, [b]);
 
       // Matching subtypes of sealed type is exhaustive.
-      expectReportErrors(env, a, [b, c]);
-      expectReportErrors(env, a, [d, e, f]);
-      expectReportErrors(env, a, [b, f]);
-      expectReportErrors(
+      expectExhaustiveness(env, a, [b, c]);
+      expectExhaustiveness(env, a, [d, e, f]);
+      expectExhaustiveness(env, a, [b, f]);
+      expectExhaustiveness(
           env, a, [c, d], 'A is not exhaustively matched by C|D.');
-      expectReportErrors(
+      expectExhaustiveness(
           env, f, [g, h], 'F is not exhaustively matched by G|H.');
     });
 
     test('unreachable case', () {
       // Same type.
-      expectReportErrors(env, b, [b, b], 'Case #2 B is unreachable.');
+      expectExhaustiveness(env, b, [b, b], 'Case #2 B is unreachable.');
 
       // Previous case is supertype.
-      expectReportErrors(env, b, [a, b], 'Case #2 B is unreachable.');
+      expectExhaustiveness(env, b, [a, b], 'Case #2 B is unreachable.');
 
       // Previous subtype cases cover sealed supertype.
-      expectReportErrors(env, a, [b, c, a], 'Case #3 A is unreachable.');
-      expectReportErrors(env, a, [d, e, f, a], 'Case #4 A is unreachable.');
-      expectReportErrors(env, a, [b, f, a], 'Case #3 A is unreachable.');
-      expectReportErrors(env, a, [c, d, a]);
+      expectExhaustiveness(env, a, [b, c, a], 'Case #3 A is unreachable.');
+      expectExhaustiveness(env, a, [d, e, f, a], 'Case #4 A is unreachable.');
+      expectExhaustiveness(env, a, [b, f, a], 'Case #3 A is unreachable.');
+      expectExhaustiveness(env, a, [c, d, a]);
 
       // Previous subtype cases do not cover unsealed supertype.
-      expectReportErrors(env, f, [g, h, f]);
+      expectExhaustiveness(env, f, [g, h, f]);
     });
 
     test('covered record destructuring |', () {
       var r = env.createRecordType({x: a, y: a, z: a});
 
       // Wider field is not covered.
-      expectReportErrors(env, r, [
+      expectExhaustiveness(env, r, [
         ty(r, {x: b}),
         ty(r, {x: a}),
       ]);
 
       // Narrower field is covered.
-      expectReportErrors(
+      expectExhaustiveness(
           env,
           r,
           [
@@ -107,40 +107,45 @@ void main() {
       var e = env.createClass('E', inherits: [c]);
 
       // Must cover null.
-      expectReportErrors(env, a.nullable, [b, d, e],
+      expectExhaustiveness(env, a.nullable, [b, d, e],
           'A? is not exhaustively matched by B|D|E.');
 
       // Can cover null with any nullable subtype.
-      expectReportErrors(env, a.nullable, [b.nullable, c]);
-      expectReportErrors(env, a.nullable, [b, c.nullable]);
-      expectReportErrors(env, a.nullable, [b, d.nullable, e]);
-      expectReportErrors(env, a.nullable, [b, d, e.nullable]);
+      expectExhaustiveness(env, a.nullable, [b.nullable, c]);
+      expectExhaustiveness(env, a.nullable, [b, c.nullable]);
+      expectExhaustiveness(env, a.nullable, [b, d.nullable, e]);
+      expectExhaustiveness(env, a.nullable, [b, d, e.nullable]);
 
       // Can cover null with a null space.
-      expectReportErrors(env, a.nullable, [b, c, StaticType.nullType]);
-      expectReportErrors(env, a.nullable, [b, d, e, StaticType.nullType]);
+      expectExhaustiveness(env, a.nullable, [b, c, StaticType.nullType]);
+      expectExhaustiveness(env, a.nullable, [b, d, e, StaticType.nullType]);
 
       // Nullable covers the non-null.
-      expectReportErrors(
+      expectExhaustiveness(
           env, a.nullable, [a.nullable, a], 'Case #2 A is unreachable.');
-      expectReportErrors(
+      expectExhaustiveness(
           env, b.nullable, [a.nullable, b], 'Case #2 B is unreachable.');
 
       // Nullable covers null.
-      expectReportErrors(env, a.nullable, [a.nullable, StaticType.nullType],
+      expectExhaustiveness(env, a.nullable, [a.nullable, StaticType.nullType],
           'Case #2 Null is unreachable.');
-      expectReportErrors(env, b.nullable, [a.nullable, StaticType.nullType],
+      expectExhaustiveness(env, b.nullable, [a.nullable, StaticType.nullType],
           'Case #2 Null is unreachable.');
     });
   });
 }
 
-void expectReportErrors(ObjectPropertyLookup objectFieldLookup,
+void expectExhaustiveness(ObjectPropertyLookup objectFieldLookup,
     StaticType valueType, List<Object> cases,
     [String errors = '']) {
+  List<CaseUnreachability> caseUnreachabilities = [];
+  var nonExhaustiveness = computeExhaustiveness(
+      objectFieldLookup, valueType, parseSpaces(cases),
+      caseUnreachabilities: caseUnreachabilities);
   expect(
-      reportErrors(objectFieldLookup, valueType, parseSpaces(cases),
-              computeUnreachable: true)
-          .join('\n'),
+      [
+        ...caseUnreachabilities,
+        if (nonExhaustiveness != null) nonExhaustiveness
+      ].join('\n'),
       errors);
 }

pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml

@@ -3739,6 +3739,8 @@ WarningCode.UNNECESSARY_WILDCARD_PATTERN:
   status: hasFix
 WarningCode.UNREACHABLE_SWITCH_CASE:
   status: hasFix
+WarningCode.UNREACHABLE_SWITCH_DEFAULT:
+  status: hasFix
 WarningCode.UNUSED_CATCH_CLAUSE:
   status: hasFix
 WarningCode.UNUSED_CATCH_STACK:

pkg/analysis_server/lib/src/services/correction/fix_internal.dart

@@ -1824,6 +1824,9 @@ final _builtInNonLintProducers = <ErrorCode, List<ProducerGenerator>>{
   WarningCode.UNREACHABLE_SWITCH_CASE: [
     RemoveDeadCode.new,
   ],
+  WarningCode.UNREACHABLE_SWITCH_DEFAULT: [
+    RemoveDeadCode.new,
+  ],
   WarningCode.UNUSED_CATCH_CLAUSE: [
     RemoveUnusedCatchClause.new,
   ],

pkg/analysis_server/test/src/services/correction/fix/remove_dead_code_test.dart

@@ -539,6 +539,52 @@ void f() {
 ''');
   }
 
+  Future<void> test_switchDefault_sharedStatements() async {
+    await resolveTestCode('''
+enum E { e1, e2 }
+void f(E e) {
+  switch(e) {
+    case E.e1:
+    case E.e2:
+    default:
+      break;
+  }
+}
+''');
+    await assertHasFix('''
+enum E { e1, e2 }
+void f(E e) {
+  switch(e) {
+    case E.e1:
+    case E.e2:
+    break;
+  }
+}
+''');
+  }
+
+  Future<void> test_switchDefault_uniqueStatements() async {
+    await resolveTestCode('''
+enum E { e1, e2 }
+void f(E e) {
+  switch(e) {
+    case E.e1: print('e1');
+    case E.e2: print('e2');
+    default: print('e3');
+  }
+}
+''');
+    await assertHasFix('''
+enum E { e1, e2 }
+void f(E e) {
+  switch(e) {
+    case E.e1: print('e1');
+    case E.e2: print('e2');
+    }
+}
+''');
+  }
+
   @FailingTest(issue: 'https://github.com/dart-lang/sdk/issues/50950')
   Future<void> test_switchExpression() async {
     await resolveTestCode('''