[Diagnostics] Implement -Wswitch-unreachable

[xbolva00]

Summary

Detects unreachable statements in switches. GCC supports this warning too, on by default.

[xbolva00]

Warn in more cases. Added many new tests.

[xbolva00]

Attached forgotten tests

[aaron.ballman]

include/clang/Basic/DiagnosticGroups.td

541

I don't think you need this group because there's only one diagnostic it controls. You can add the group directly to the warning itself.

include/clang/Basic/DiagnosticSemaKinds.td

8192

InGroup<DiagGroup<"switch-unreachable">> and drop the DefaultIgnore.

lib/Sema/SemaStmt.cpp

864

const auto *

865–866

for (const Stmt *S : CS->body())

871–876

You should turn this into a negative predicate rather than having an empty body with an else statement.

test/Sema/switch_unreachable.c

2 ↗

(On Diff #204254)

Remove the -Wall since you want to test that this is on by default.

[xbolva00]

There is a bug in Clang AST. @rsmith

Check test/Sema/switch-1.c. In C++ mode we have really bad AST:

CompoundStmt 0x55f5b7d68460 <col:14, line:51:3>
|   |   |-ReturnStmt 0x55f5b7d68060 <line:17:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d68040 <col:14> 'int' 1
|   |   |-ReturnStmt 0x55f5b7d68120 <line:25:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d68100 <col:14> 'int' 2
|   |   |-ReturnStmt 0x55f5b7d68210 <line:33:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d681f0 <col:14> 'int' 3
|   |   |-ReturnStmt 0x55f5b7d683c0 <line:48:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d683a0 <col:14> 'int' 4
|   |   `-CaseStmt 0x55f5b7d68408 <line:49:5, line:50:14>
|   |     |-ConstantExpr 0x55f5b7d683f0 <line:49:10> 'int'
|   |     | `-IntegerLiteral 0x55f5b7d683d0 <col:10> 'int' 2147483647
|   |     `-ReturnStmt 0x55f5b7d68450 <line:50:7, col:14>
|   |       `-IntegerLiteral 0x55f5b7d68430 <col:14> 'int' 0

This is bad and blocks this patch since it creates weird warnings :/

[xbolva00]

Addressed some notes. Thanks.

[xbolva00]

In D63139#1547968, @xbolva00 wrote:

There is a bug in Clang AST. @rsmith

Check test/Sema/switch-1.c. In C++ mode we have really bad AST:

CompoundStmt 0x55f5b7d68460 <col:14, line:51:3>
|   |   |-ReturnStmt 0x55f5b7d68060 <line:17:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d68040 <col:14> 'int' 1
|   |   |-ReturnStmt 0x55f5b7d68120 <line:25:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d68100 <col:14> 'int' 2
|   |   |-ReturnStmt 0x55f5b7d68210 <line:33:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d681f0 <col:14> 'int' 3
|   |   |-ReturnStmt 0x55f5b7d683c0 <line:48:7, col:14>
|   |   | `-IntegerLiteral 0x55f5b7d683a0 <col:14> 'int' 4
|   |   `-CaseStmt 0x55f5b7d68408 <line:49:5, line:50:14>
|   |     |-ConstantExpr 0x55f5b7d683f0 <line:49:10> 'int'
|   |     | `-IntegerLiteral 0x55f5b7d683d0 <col:10> 'int' 2147483647
|   |     `-ReturnStmt 0x55f5b7d68450 <line:50:7, col:14>
|   |       `-IntegerLiteral 0x55f5b7d68430 <col:14> 'int' 0

This is bad and blocks this patch since it creates weird warnings :/

Or... is it acceptable to produce unreachable stmt warning for this cases?

[rjmccall]

Are the CaseStmts being dropped in C++ because the expression overflows? I agree that that's bad AST behavior; we should strive to generate AST whenever we can, even if it's not valid.

[xbolva00]

Yes, CaseStmt is dropped in that case.

[aaron.ballman]

In D63139#1561891, @rjmccall wrote:

Are the CaseStmts being dropped in C++ because the expression overflows? I agree that that's bad AST behavior; we should strive to generate AST whenever we can, even if it's not valid.

We do strive to generate an AST whenever we can (like recovering as though fix-its were applied, or keeping declarations and marking them as invalid), but I don't think we want to generate *invalid* AST nodes. I believe that way leads to more cascading, insensible errors and worse behavior for tooling than dropping the invalid AST node does (depending on the scenario). I have a hazy recollection that we wanted to consider adding AST nodes to represent erroneous constructs that attempt to hold onto as much valid state as we can capture to help with these situations, but I'm not certain that idea went anywhere.

It would be nice if we could suppress the unreachable warning in this case, but I'm not certain it's strictly required for this patch to proceed, either. @rsmith, do you have opinions?

[rjmccall]

I agree that tools shouldn't be forced to deal with invalid AST that looks like valid AST. To me that means finding ways to preserve information that (1) don't badly violate invariants and (2) are easily discoverable as invalid. For case, which has external requirements (e.g. not being a duplicate value) not entirely dissimilar to a declaration, I think that means having an "invalid" flag; arbitrary tools already can't rely on the expression being constant-evaluable because of templates, although granted many tools might never look at template patterns. For other things (e.g. a binary operator) maybe that means using different classes (e.g. InvalidBinaryOperator) so that tools looking at an apparently well-typed expression don't need to consider totally invalid possibilities.

[aaron.ballman]

In D63139#1579231, @rjmccall wrote:

I agree that tools shouldn't be forced to deal with invalid AST that looks like valid AST. To me that means finding ways to preserve information that (1) don't badly violate invariants and (2) are easily discoverable as invalid.

I think this makes sense.

For case, which has external requirements (e.g. not being a duplicate value) not entirely dissimilar to a declaration, I think that means having an "invalid" flag; arbitrary tools already can't rely on the expression being constant-evaluable because of templates, although granted many tools might never look at template patterns. For other things (e.g. a binary operator) maybe that means using different classes (e.g. InvalidBinaryOperator) so that tools looking at an apparently well-typed expression don't need to consider totally invalid possibilities.

I'm not opposed to what you're saying, but I am a bit wary. IMO, we already have too many ways an AST node can be invalid that are easily checkable but totally different from node to node (sometimes child nodes are null, sometimes you check an isInvalid() predicate, sometimes you check that a type is null, sometimes we drop the node entirely, etc). I'd love to see a more uniform way to handle invalid information within an AST that retains as much source fidelity as we can get -- like ErrorDecl, ErrorStmt, ErrorExpr, and ErrorType AST nodes (perhaps these hold a partially-valid AST node of the usual kind as a child). This not only cuts down on difficulties with understanding the Clang codebase itself, but it definitely would help 3rd party tooling, pretty printing and AST dumping, AST matching, etc.

(Not that I expect that uniform way to appear as part of this patch, or even predicate the patch on it!)

[rjmccall]

I agree that we really shouldn't use most of those — we shouldn't have null types or null child expressions anywhere in the AST. (Accessors might return null for *optional* children, of course, but that's different.) And yeah, a big part of that would be having an error type that could be used in various places (instead of e.g. defaulting to int).

[xbolva00]

Ping @rsmith

[xbolva00]

Ping again

Is ‘CaseStmt´ AST C++ issue is a blocker for this patch or not?

I would like to move forward - either land it or (if blocker) abandon it.

If it is blocking issue, I can enable it for C only (better than nothing)..

[xbolva00]

Adding @jfb as reviewer to get more opinions what should be done next..

[aaron.ballman]

In D63139#1603667, @xbolva00 wrote:

Ping again

Is ‘CaseStmt´ AST C++ issue is a blocker for this patch or not?

I do not think it is a blocker for this patch. It's the existing behavior of the AST and while annoying, it can be improved later.

include/clang/Basic/DiagnosticSemaKinds.td

8192

I thought we had a warning group for this already, and we do, it's -Wunreachable-code. I think the new diagnostic group be a child of the existing one, if we need the group at all.

test/SemaCXX/warn-unreachable-stmt-switch.cpp

1–4 ↗

(On Diff #205295)

You should add a RUN line that also passes -Wunreachable-code to ensure this doesn't introduce duplicate diagnostics. It seems some number of these are already covered by that warning class: https://godbolt.org/z/f60YxB

[jfb]

include/clang/Basic/DiagnosticSemaKinds.td

8192

Agreed.

lib/Sema/SemaStmt.cpp

727

This function is super confusing, and that's not your doing... but adding a bool& param kinda piles on. I'd much rather return a enum class CaseListIsErroneous { No, Yes }, and make enum class UnpromotedSign as well while we're here.

test/SemaCXX/warn-unreachable-stmt-switch.cpp

29 ↗

(On Diff #205295)

Not that I think you should diagnose here, but I'd like to have a comment in the test saying that it's indeed unreachable, but we don't currently diagnose.

49 ↗

(On Diff #205295)

Can you also have one like this with typedef and declarations instead of statements:

switch (x) {
  using Foo = int;
  case 7:
  // ...

and

switch (x) {
  int im_confused = 42;
  case 7:
  // ...

This one is terrible but reachable:

switch (x) {
  ({ oh_no: g(x); });
  case 7:
  goto oh_no;
  // ...

[xbolva00]

Since motivation examples are already handled by -Wunreachable-code, I will close this revision.

include/clang/Basic/DiagnosticGroups.td

 def InvalidOffsetof : DiagGroup<"invalid-offsetof">;
 def : DiagGroup<"strict-prototypes">;
 def StrictSelector : DiagGroup<"strict-selector-match">;
 def MethodDuplicate : DiagGroup<"duplicate-method-match">;
 def ObjCCStringFormat : DiagGroup<"cstring-format-directive">;
 def CoveredSwitchDefault : DiagGroup<"covered-switch-default">;
 def SwitchBool     : DiagGroup<"switch-bool">;
 def SwitchEnum     : DiagGroup<"switch-enum">;
+def SwitchUnreachable     : DiagGroup<"switch-unreachable">;
 def Switch         : DiagGroup<"switch">;
 def EnumCompareSwitch : DiagGroup<"enum-compare-switch">;
 def EnumCompare       : DiagGroup<"enum-compare", [EnumCompareSwitch]>;
 def ImplicitFallthroughPerFunction :
   DiagGroup<"implicit-fallthrough-per-function">;
 def ImplicitFallthrough  : DiagGroup<"implicit-fallthrough",
                                      [ImplicitFallthroughPerFunction]>;
 def InvalidPPToken : DiagGroup<"invalid-pp-token">;

 def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
 
 // Uniqueness Analysis warnings
 def Consumed       : DiagGroup<"consumed">;
 
 // Note that putting warnings in -Wall will not disable them by default. If a
 // warning should be active _only_ when -Wall is passed in, mark it as
 // DefaultIgnore in addition to putting it here.
-def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool]>;
+def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool, SwitchUnreachable]>;
 
 // Warnings that should be in clang-cl /w4.
 def : DiagGroup<"CL4", [All, Extra]>;
 
 // Warnings enabled by -pedantic.  This is magically filled in by TableGen.
 def Pedantic : DiagGroup<"pedantic">;
 
 // Aliases.

include/clang/Basic/DiagnosticSemaKinds.td

def warn_loop_ctrl_binds_to_inner : Warning<
   "'%0' is bound to current loop, GCC binds it to the enclosing loop">,
   InGroup<GccCompat>;
 def warn_break_binds_to_switch : Warning<
   "'break' is bound to loop, GCC binds it to switch">,
   InGroup<GccCompat>;
 def err_default_not_in_switch : Error<
   "'default' statement not in switch statement">;
 def err_case_not_in_switch : Error<"'case' statement not in switch statement">;
+def warn_unreachable_stmt_in_switch : Warning<
+  "statement will be never executed">, InGroup<SwitchUnreachable>, DefaultIgnore;
 def warn_bool_switch_condition : Warning<
   "switch condition has boolean value">, InGroup<SwitchBool>;
 def warn_case_value_overflow : Warning<
   "overflow converting case value to switch condition type (%0 to %1)">,
   InGroup<Switch>;
 def err_duplicate_case : Error<"duplicate case value '%0'">;
 def err_duplicate_case_differing_expr : Error<
   "duplicate case value: '%0' and '%1' both equal '%2'">;

lib/Sema/SemaStmt.cpp

static void AdjustAPSInt(llvm::APSInt &Val, unsigned BitWidth, bool IsSigned) {
   Val = Val.extOrTrunc(BitWidth);
   Val.setIsSigned(IsSigned);
 }
 
 /// Check the specified case value is in range for the given unpromoted switch
 /// type.
 static void checkCaseValue(Sema &S, SourceLocation Loc, const llvm::APSInt &Val,
                            unsigned UnpromotedWidth, bool UnpromotedSign) {
   // In C++11 onwards, this is checked by the language rules.
   if (S.getLangOpts().CPlusPlus11)
     return;
 
   // If the case value was signed and negative and the switch expression is
   // unsigned, don't bother to warn: this is implementation-defined behavior.
   // FIXME: Introduce a second, default-ignored warning for this case?
   if (UnpromotedWidth < Val.getBitWidth()) {
     llvm::APSInt ConvVal(Val);
Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
   // it has been converted to the condition type.
   typedef SmallVector<std::pair<llvm::APSInt, CaseStmt*>, 64> CaseValsTy;
   CaseValsTy CaseVals;
 
   // Keep track of any GNU case ranges we see.  The APSInt is the low value.
   typedef std::vector<std::pair<llvm::APSInt, CaseStmt*> > CaseRangesTy;
   CaseRangesTy CaseRanges;
 
+  if (CompoundStmt *CS = dyn_cast<CompoundStmt>(BodyStmt)) {
+    for (auto It = CS->body_begin(); It != CS->body_end(); ++It) {
+      auto *S = *It;
+      if (isa<LabelStmt>(S) || isa<CaseStmt>(S) || isa<DefaultStmt>(S))
+        break;
+      Diag(S->getBeginLoc(), diag::warn_unreachable_stmt_in_switch);
+    }
+  } else if (isa<LabelStmt>(BodyStmt) || isa<CaseStmt>(BodyStmt) ||
+             isa<DefaultStmt>(BodyStmt)) {
+    // No warning
+  } else {
+    Diag(BodyStmt->getBeginLoc(), diag::warn_unreachable_stmt_in_switch);
+  }
+
   DefaultStmt *TheDefaultStmt = nullptr;
 
   bool CaseListIsErroneous = false;
 
   for (SwitchCase *SC = SS->getSwitchCaseList(); SC && !HasDependentValue;
        SC = SC->getNextSwitchCase()) {
 
     if (DefaultStmt *DS = dyn_cast<DefaultStmt>(SC)) {