[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 accepted this revision.
xbolva00 added a comment.
This revision is now accepted and ready to land.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8192
+def warn_unreachable_stmt_in_switch : Warning<
+  "statement will be never executed">, 
InGroup>;
 def warn_bool_switch_condition : Warning<

aaron.ballman wrote:
> 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.
Agreed.



Comment at: lib/Sema/SemaStmt.cpp:727
+   unsigned UnpromotedWidth, bool UnpromotedSign,
+   bool ) {
   // In C++11 onwards, this is checked by the language rules.

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.



Comment at: test/SemaCXX/warn-unreachable-stmt-switch.cpp:29
+  label3:
+x++;
+  case 4:

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.



Comment at: test/SemaCXX/warn-unreachable-stmt-switch.cpp:49
+  switch (x) {
+;
+  case 7:

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;
  // ...
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8192
+def warn_unreachable_stmt_in_switch : Warning<
+  "statement will be never executed">, 
InGroup>;
 def warn_bool_switch_condition : Warning<

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.



Comment at: test/SemaCXX/warn-unreachable-stmt-switch.cpp:1-4
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wswitch-unreachable %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wswitch-unreachable %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify  %s

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a subscriber: jfb.
xbolva00 added a comment.

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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)..


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Ping @rsmith


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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`).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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!)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D63139#1561891 , @rjmccall wrote:

> Are the `CaseStmt`s 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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Yes, CaseStmt is dropped in that case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Are the `CaseStmt`s 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

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 
>   |   |   |-ReturnStmt 0x55f5b7d68060 
>   |   |   | `-IntegerLiteral 0x55f5b7d68040  'int' 1
>   |   |   |-ReturnStmt 0x55f5b7d68120 
>   |   |   | `-IntegerLiteral 0x55f5b7d68100  'int' 2
>   |   |   |-ReturnStmt 0x55f5b7d68210 
>   |   |   | `-IntegerLiteral 0x55f5b7d681f0  'int' 3
>   |   |   |-ReturnStmt 0x55f5b7d683c0 
>   |   |   | `-IntegerLiteral 0x55f5b7d683a0  'int' 4
>   |   |   `-CaseStmt 0x55f5b7d68408 
>   |   | |-ConstantExpr 0x55f5b7d683f0  'int'
>   |   | | `-IntegerLiteral 0x55f5b7d683d0  'int' 2147483647
>   |   | `-ReturnStmt 0x55f5b7d68450 
>   |   |   `-IntegerLiteral 0x55f5b7d68430  '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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 205295.
xbolva00 added a comment.

Addressed some notes. Thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaStmt.cpp
  test/Sema/implicit-decl-c90.c
  test/SemaCXX/array-bounds.cpp
  test/SemaCXX/warn-unreachable-stmt-switch.cpp

Index: test/SemaCXX/warn-unreachable-stmt-switch.cpp
===
--- test/SemaCXX/warn-unreachable-stmt-switch.cpp
+++ test/SemaCXX/warn-unreachable-stmt-switch.cpp
@@ -0,0 +1,152 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wswitch-unreachable %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wswitch-unreachable %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify  %s
+
+void g(int x);
+
+void foo(int x) {
+  int b = 0;
+  switch (x) {
+  label:
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+x++; // expected-warning {{statement will be never executed}}
+  label2:
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+  label3:
+x++;
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+  case 4:
+return;
+  }
+
+  switch (x) {
+b = x; // expected-warning {{statement will be never executed}}
+  case 7:
+g(b);
+break;
+  }
+
+  switch (x) {
+;
+  case 7:
+g(b);
+break;
+  }
+
+  switch (x) {
+break; // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+return; // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+x++;  // expected-warning {{statement will be never executed}}
+g(x); // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+x++; // expected-warning {{statement will be never executed}}
+  label4:
+g(x);
+  case 7:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 1:
+  case 2:
+  case 3:
+break;
+  }
+
+  switch (x) {
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  switch (x) {
+  case 1:
+break;
+  default:
+break;
+  }
+
+  switch (x) {
+b = x; // expected-warning {{statement will be never executed}}
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  }
+
+  switch (x)
+b = x; // expected-warning {{statement will be never executed}}
+
+  switch (x)
+g(x); // expected-warning {{statement will be never executed}}
+
+  switch (x)
+  label5:
+g(x);
+
+  switch (x) {
+  label6:
+g(x);
+  }
+
+  switch (x)
+  case 5:
+g(x);
+
+  switch (x) {
+  case 5:
+g(x);
+  }
+
+  switch (x)
+  default:
+g(x);
+
+  switch (x) {
+  default:
+g(x);
+  }
+}
Index: test/SemaCXX/array-bounds.cpp
===
--- test/SemaCXX/array-bounds.cpp
+++ test/SemaCXX/array-bounds.cpp
@@ -164,6 +164,7 @@
 static enum enumB myVal = enumB_X;
 void test_nested_switch() {
   switch (enumA_E) { // expected-warning {{no case matching constant}}
+// expected-warning@+1 {{statement will be never executed}}
 switch (myVal) { // expected-warning {{enumeration values 'enumB_X' and 'enumB_Z' not handled in switch}}
   case enumB_Y: ;
 }
Index: test/Sema/implicit-decl-c90.c
===
--- test/Sema/implicit-decl-c90.c
+++ test/Sema/implicit-decl-c90.c
@@ -15,7 +15,7 @@
 void t1(int x) {
   int (*p)();
   switch (x) {
-g();
+g(); // expected-warning {{statement will be never executed}}
   case 0:
 x = h() + 1;
 break;
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -723,7 +723,8 @@
 /// Check the specified case value is in range for the given unpromoted switch
 /// type.
 static void checkCaseValue(Sema , SourceLocation Loc, const llvm::APSInt ,
-   unsigned UnpromotedWidth, bool UnpromotedSign) {
+   unsigned UnpromotedWidth, bool UnpromotedSign,
+   bool ) {
   // In C++11 onwards, this is checked by the language rules.
   if (S.getLangOpts().CPlusPlus11)
 return;
@@ -738,9 +739,11 @@
 // FIXME: Use different diagnostics for overflow  in conversion to promoted
 // type versus "switch expression cannot have this value". Use proper
 // IntRange checking rather than just looking at the unpromoted type here.
-if (ConvVal != Val)
-  S.Diag(Loc, diag::warn_case_value_overflow) << Val.toString(10)
-  << 

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

There is a bug in Clang AST. @rsmith

Check testcase test/Sema/switch-1.c. In C++ mode we have AST:
CompoundStmt 0x55f5b7d68460 

|  |   | -ReturnStmt 
0x55f5b7d68060|
|  |   | `-IntegerLiteral 
0x55f5b7d68040  'int' 1 |
|  |   | -ReturnStmt 
0x55f5b7d68120|
|  |   | `-IntegerLiteral 
0x55f5b7d68100  'int' 2 |
|  |   | -ReturnStmt 
0x55f5b7d68210|
|  |   | `-IntegerLiteral 
0x55f5b7d681f0  'int' 3 |
|  |   | -ReturnStmt 
0x55f5b7d683c0|
|  |   | `-IntegerLiteral 
0x55f5b7d683a0  'int' 4 |
|  | `-CaseStmt 0x55f5b7d68408  |
|  |   | -ConstantExpr 
0x55f5b7d683f0  'int'  |
|  |   | `-IntegerLiteral 
0x55f5b7d683d0  'int' 2147483647 |
|  | `-ReturnStmt 0x55f5b7d68450|
|  | `-IntegerLiteral 0x55f5b7d68430  'int' 0  |
|

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: include/clang/Basic/DiagnosticGroups.td:541
 def SwitchEnum : DiagGroup<"switch-enum">;
+def SwitchUnreachable : DiagGroup<"switch-unreachable">;
 def Switch : DiagGroup<"switch">;

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.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8192
+def warn_unreachable_stmt_in_switch : Warning<
+  "statement will be never executed">, InGroup, 
DefaultIgnore;
 def warn_bool_switch_condition : Warning<

`InGroup>` and drop the `DefaultIgnore`.



Comment at: lib/Sema/SemaStmt.cpp:864
 
+  if (CompoundStmt *CS = dyn_cast(BodyStmt)) {
+for (auto It = CS->body_begin(); It != CS->body_end(); ++It) {

`const auto *`



Comment at: lib/Sema/SemaStmt.cpp:865-866
+  if (CompoundStmt *CS = dyn_cast(BodyStmt)) {
+for (auto It = CS->body_begin(); It != CS->body_end(); ++It) {
+  auto *S = *It;
+  if (isa(S) || isa(S) || isa(S))

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



Comment at: lib/Sema/SemaStmt.cpp:871-876
+  } else if (isa(BodyStmt) || isa(BodyStmt) ||
+ isa(BodyStmt)) {
+// No warning
+  } else {
+Diag(BodyStmt->getBeginLoc(), diag::warn_unreachable_stmt_in_switch);
+  }

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



Comment at: test/Sema/switch_unreachable.c:2
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wswitch-unreachable %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 204254.
xbolva00 added a comment.

Attached forgotten tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaStmt.cpp
  test/Sema/switch_unreachable.c
  test/SemaCXX/switch_unreachable.cpp

Index: test/SemaCXX/switch_unreachable.cpp
===
--- test/SemaCXX/switch_unreachable.cpp
+++ test/SemaCXX/switch_unreachable.cpp
@@ -0,0 +1,143 @@
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wswitch-unreachable %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+
+void g(int x);
+
+void foo(int x) {
+  int b = 0;
+  switch (x) {
+  label:
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+x++; // expected-warning {{statement will be never executed}}
+  label2:
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+  label3:
+x++;
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+  case 4:
+return;
+  }
+
+  switch (x) {
+b = x; // expected-warning {{statement will be never executed}}
+  case 7:
+g(b);
+break;
+  }
+
+  switch (x) {
+break; // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+return; // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+x++;  // expected-warning {{statement will be never executed}}
+g(x); // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+x++; // expected-warning {{statement will be never executed}}
+  label4:
+g(x);
+  case 7:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 1:
+  case 2:
+  case 3:
+break;
+  }
+
+  switch (x) {
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  switch (x) {
+  case 1:
+break;
+  default:
+break;
+  }
+
+  switch (x) {
+b = x; // expected-warning {{statement will be never executed}}
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  }
+
+  switch (x)
+b = x; // expected-warning {{statement will be never executed}}
+
+  switch (x)
+g(x); // expected-warning {{statement will be never executed}}
+
+  switch (x)
+  label5:
+g(x);
+
+  switch (x) {
+  label6:
+g(x);
+  }
+
+  switch (x)
+  case 5:
+g(x);
+
+  switch (x) {
+  case 5:
+g(x);
+  }
+
+  switch (x)
+  default:
+g(x);
+
+  switch (x) {
+  default:
+g(x);
+  }
+}
Index: test/Sema/switch_unreachable.c
===
--- test/Sema/switch_unreachable.c
+++ test/Sema/switch_unreachable.c
@@ -0,0 +1,143 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wswitch-unreachable %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+
+void g(int x);
+
+void foo(int x) {
+  int b = 0;
+  switch (x) {
+  label:
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+x++; // expected-warning {{statement will be never executed}}
+  label2:
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+  label3:
+x++;
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+  case 4:
+return;
+  }
+
+  switch (x) {
+b = x; // expected-warning {{statement will be never executed}}
+  case 7:
+g(b);
+break;
+  }
+
+  switch (x) {
+break; // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+return; // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+x++;  // expected-warning {{statement will be never executed}}
+g(x); // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+x++; // expected-warning {{statement will be never executed}}
+  label4:
+g(x);
+  case 7:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 1:
+  case 2:
+  case 3:
+break;
+  }
+
+  switch (x) {
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  switch (x) {
+  case 1:
+break;
+  default:
+break;
+  }
+
+  switch (x) {
+b = x; // expected-warning {{statement will be never executed}}
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  }
+
+  switch (x)
+b = x; // expected-warning {{statement will be never executed}}
+
+  switch (x)
+g(x); // expected-warning {{statement 

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 204253.
xbolva00 added a comment.

Warn in more cases. Added many new tests.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaStmt.cpp


Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -861,6 +861,20 @@
   typedef std::vector > CaseRangesTy;
   CaseRangesTy CaseRanges;
 
+  if (CompoundStmt *CS = dyn_cast(BodyStmt)) {
+for (auto It = CS->body_begin(); It != CS->body_end(); ++It) {
+  auto *S = *It;
+  if (isa(S) || isa(S) || isa(S))
+break;
+  Diag(S->getBeginLoc(), diag::warn_unreachable_stmt_in_switch);
+}
+  } else if (isa(BodyStmt) || isa(BodyStmt) ||
+ isa(BodyStmt)) {
+// No warning
+  } else {
+Diag(BodyStmt->getBeginLoc(), diag::warn_unreachable_stmt_in_switch);
+  }
+
   DefaultStmt *TheDefaultStmt = nullptr;
 
   bool CaseListIsErroneous = false;
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -8188,6 +8188,8 @@
 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, 
DefaultIgnore;
 def warn_bool_switch_condition : Warning<
   "switch condition has boolean value">, InGroup;
 def warn_case_value_overflow : Warning<
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -538,6 +538,7 @@
 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]>;
@@ -842,7 +843,7 @@
 // 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]>;


Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -861,6 +861,20 @@
   typedef std::vector > CaseRangesTy;
   CaseRangesTy CaseRanges;
 
+  if (CompoundStmt *CS = dyn_cast(BodyStmt)) {
+for (auto It = CS->body_begin(); It != CS->body_end(); ++It) {
+  auto *S = *It;
+  if (isa(S) || isa(S) || isa(S))
+break;
+  Diag(S->getBeginLoc(), diag::warn_unreachable_stmt_in_switch);
+}
+  } else if (isa(BodyStmt) || isa(BodyStmt) ||
+ isa(BodyStmt)) {
+// No warning
+  } else {
+Diag(BodyStmt->getBeginLoc(), diag::warn_unreachable_stmt_in_switch);
+  }
+
   DefaultStmt *TheDefaultStmt = nullptr;
 
   bool CaseListIsErroneous = false;
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -8188,6 +8188,8 @@
 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, DefaultIgnore;
 def warn_bool_switch_condition : Warning<
   "switch condition has boolean value">, InGroup;
 def warn_case_value_overflow : Warning<
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -538,6 +538,7 @@
 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]>;
@@ -842,7 +843,7 @@
 // Note that putting warnings in -Wall will not disable 

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 created this revision.
xbolva00 added reviewers: aaron.ballman, rsmith, rjmccall.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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


Repository:
  rC Clang

https://reviews.llvm.org/D63139

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaStmt.cpp
  test/Sema/switch_unreachable.c
  test/SemaCXX/switch_unreachable.cpp

Index: test/SemaCXX/switch_unreachable.cpp
===
--- test/SemaCXX/switch_unreachable.cpp
+++ test/SemaCXX/switch_unreachable.cpp
@@ -0,0 +1,103 @@
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wswitch-unreachable %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+
+void g(int x);
+
+void foo(int x) {
+  int b = 0;
+  if (x == 7)
+goto label;
+  if (x == 3)
+goto label2;
+  if (x == 4)
+goto label3;
+  if (x == 1)
+goto label4;
+
+  switch (x) {
+  label:
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+x++; // expected-warning {{statement will be never executed}}
+  label2:
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+  label3:
+x++;
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+  case 4:
+return;
+  }
+
+  switch (x) {
+b = x; // expected-warning {{statement will be never executed}}
+  case 7:
+g(b);
+break;
+  }
+
+  switch (x) {
+break; // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+return; // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+x++;  // expected-warning {{statement will be never executed}}
+g(x); // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+x++; // expected-warning {{statement will be never executed}}
+  label4:
+g(x);
+  case 7:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 1:
+  case 2:
+  case 3:
+break;
+  }
+}
Index: test/Sema/switch_unreachable.c
===
--- test/Sema/switch_unreachable.c
+++ test/Sema/switch_unreachable.c
@@ -0,0 +1,97 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wswitch-unreachable %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+
+void g(int x);
+
+void foo(int x) {
+  int b = 0;
+  if (x == 7)
+goto label;
+  if (x == 3)
+goto label2;
+  if (x == 4)
+goto label3;
+  if (x == 1)
+goto label4;
+
+  switch (x) {
+  label:
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+x++; // expected-warning {{statement will be never executed}}
+  label2:
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+  label3:
+x++;
+  case 4:
+break;
+  default:
+return;
+  }
+
+  switch (x) {
+  case 4:
+return;
+  }
+
+  switch (x) {
+b = x; // expected-warning {{statement will be never executed}}
+  case 7:
+g(b);
+break;
+  }
+
+  switch (x) {
+break; // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+return; // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+x++;  // expected-warning {{statement will be never executed}}
+g(x); // expected-warning {{statement will be never executed}}
+  case 7:
+break;
+  }
+
+  switch (x) {
+x++; // expected-warning {{statement will be never executed}}
+  label4:
+g(x);
+  case 7:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 1:
+break;
+  case 2:
+break;
+  }
+
+  switch (x) {
+g(x); // expected-warning {{statement will be never executed}}
+  case 1:
+  case 2:
+  case 3:
+break;
+  }
+}
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -861,6 +861,16 @@
   typedef std::vector > CaseRangesTy;
   CaseRangesTy CaseRanges;
 
+  CompoundStmt *CS = dyn_cast(BodyStmt);
+  if (CS && !CS->body_empty()) {
+for (auto It = CS->body_begin(); It != CS->body_end(); ++It) {
+  auto *S = *It;
+  if (isa(S) || isa(S) || isa(S))
+break;
+  Diag(S->getBeginLoc(), diag::warn_unreachable_stmt_in_switch);
+}
+  }
+
   DefaultStmt *TheDefaultStmt = nullptr;
 
   bool CaseListIsErroneous = false;
Index: include/clang/Basic/DiagnosticSemaKinds.td