Page MenuHomePhabricator

[Diagnostics] Implement -Wswitch-unreachable
AbandonedPublic

Authored by xbolva00 on Jun 11 2019, 7:12 AM.

Details

Summary

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

Diff Detail

Event Timeline

xbolva00 created this revision.Jun 11 2019, 7:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 7:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
xbolva00 updated this revision to Diff 204253.Jun 12 2019, 3:57 AM

Warn in more cases. Added many new tests.

xbolva00 updated this revision to Diff 204254.Jun 12 2019, 3:59 AM

Attached forgotten tests

aaron.ballman added inline comments.Jun 17 2019, 10:56 AM
include/clang/Basic/DiagnosticGroups.td
541 ↗(On Diff #204254)

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
880

const auto *

881–882

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

887–892

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 added a comment.EditedJun 18 2019, 3:20 AM

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 updated this revision to Diff 205295.Jun 18 2019, 3:49 AM

Addressed some notes. Thanks.

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?

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.

Yes, CaseStmt is dropped in that case.

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?

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.

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

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

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 added a subscriber: jfb.Sat, Jul 27, 10:24 AM

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

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

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 added inline comments.Mon, Jul 29, 10:01 PM
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

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

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 accepted this revision.Tue, Jul 30, 2:05 PM

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

This revision is now accepted and ready to land.Tue, Jul 30, 2:05 PM
xbolva00 abandoned this revision.Tue, Jul 30, 2:06 PM