[clang][Parse] Diagnose useless null statements (PR39111)
Needs ReviewPublic

Authored by lebedev.ri on Sat, Sep 29, 10:43 AM.

Details

Summary

clang has -Wextra-semi (D43162), which is not dictated by the currently selected standard.
While that is great, there is at least one more source of need-less semis - 'null statements'.
Sometimes, they are needed:

for(int x = 0; continueToDoWork(x); x++)
  ; // Ugly code, but the semi is needed here.

But sometimes they are just there for no reason:

switch(X) {
case 0:
  return -2345;
case 5:
  return 0;
default:
  return 42;
}; // <- oops

;;;;;;;;;;; <- OOOOPS, still not diagnosed. Clearly this is junk.

As usual, things may or may not go sideways in the presence of macros.
While evaluating this diag on my codebase of interest, it was unsurprisingly
discovered that Google Test macros are *very* prone to this.
And it seems many issues are deep within the GTest itself, not
in the snippets passed from the codebase that uses GTest.

So after some thought, i decided not do issue a diagnostic if the semi
is within *any* macro, be it either from the normal header, or system header.

Fixes PR39111

Diff Detail

Repository
rC Clang
lebedev.ri created this revision.Sat, Sep 29, 10:43 AM

Again, I'm not sure we really want this... we don't really like adding new off-by-default warnings, and we don't usually add diagnostics to enforce style.

include/clang/Basic/DiagnosticGroups.td
163

We usually only use "pedantic" in the names of warnings which are enabled by the "-pedantic" flag.

rsmith added a comment.Mon, Oct 1, 3:49 PM

Like Eli, I don't see much value in this warning. But it doesn't seem much different from -Wextra-semi in that regard for languages in which such extra semicolons are generally valid -- I expect both warnings will generally diagnose typos and little else.

Does this warn on:

switch (my_enum) {
case E1:
  // stuff
  break;
case E2:
  ; // nothing to do, but a label requires a following statement
}

and

for (/*...outer...*/) {
  for (/*...inner...*/) {
    goto contin_outer;
  }
  contin_outer: ;
}

? We shouldn't warn on a semicolon after a label, because a statement is required there, and use of a null statement is idiomatic.

include/clang/Basic/DiagnosticGroups.td
163

Maybe -Wextra-semi-stmt or -Wredundant-null-stmt or similar would be more appropriate?

lib/Parse/ParseStmt.cpp
237

I'm a little concerned that checking whether the scope is a compound statement isn't really checking the right thing -- what you care about is whether the syntactic context is a compound statement, not which scope a declaration at this level would be injected into. (Example of the difference: back in the pre-standard times when { for (int x = 0; x < 10; ++x) //... injected x into the enclosing scope, the first substatement in that for statement would be in a compound-statement scope, but we definitely shouldn't warn on a stray ; there.) If you moved this check into ParseCompoundStatementBody, there'd be no risk of such problems.

test/Parser/extra-semi-resulting-in-nullstmt.cpp
60

It'd seem reasonable to warn on a null-statement as the *init-statement* of an if / switch / range-based for.

lebedev.ri marked 4 inline comments as done.

Thank you for taking a look!

  • Move it into ParseCompoundStatementBody(), thus fixing false-positives with case X: ; e.g.
  • Rename to -Wextra-semi-stmt
  • Add -Wempty-init-stmt (tentatively included into -Wextra too), to diagnose empty init-statements of if/switch/range-based for
lib/Parse/ParseStmt.cpp
237

Great idea, thank you!
Testcases added, that fixed them.

test/Parser/extra-semi-resulting-in-nullstmt.cpp
60

If you so insist :)

rsmith added inline comments.Tue, Oct 2, 1:34 PM
include/clang/Basic/DiagnosticGroups.td
164

I think this should either be folded into -Wextra-semi-stmt or should perhaps be a subgroup of it.

167–168

I would prefer to keep -Wextra-semi and -Wextra-semi-stmt separate. -Wextra-semi is for those cases that are ill-formed in C++98, whereas your new warning is only about code style / typo detection.

lib/Parse/ParseStmt.cpp
1622

You can just use a SourceLocation here; it already has an 'invalid' state.

lebedev.ri updated this revision to Diff 168017.Tue, Oct 2, 1:47 PM
lebedev.ri marked 3 inline comments as done.
  • Moved -Wempty-init-stmt into -Wextra-semi-stmt
  • Moved -Wextra-semi-stmt out of -Wextra-semi
  • Tentatively enabled -Wextra-semi-stmt in -Wextra (and removed -Wempty-init-stmt from -Wextra, since it will be transitively enabled by -Wextra-semi-stmt)
include/clang/Basic/DiagnosticGroups.td
167–168

Hmm, does that mean ExtraSemiStmt[+EmptyInitStatement] should be in -Wextra?

lib/Parse/ParseStmt.cpp
1622

Aha.

lebedev.ri updated this revision to Diff 168020.Tue, Oct 2, 1:54 PM

Slightly improved test coverage for macros in extra-semi-resulting-in-nullstmt-in-init-statement.cpp

lebedev.ri added inline comments.Tue, Oct 2, 2:06 PM
include/clang/Basic/DiagnosticGroups.td
770

I'm really unsure of this. Maybe this should only be EmptyInitStatement.

lebedev.ri marked 4 inline comments as done.

After looking at stage-2 of LLVM, remove -Wextra-semi-stmt from -Wextra, and add -Wempty-init-stmt back into -Wextra.
Anything else?

rsmith added a comment.Wed, Oct 3, 5:39 PM

I feel indifferent about this warning, but clearly you care about it enough to work on it, and it's a pretty low-cost feature from a maintenance perspective, so I suppose it's fine.

include/clang/Basic/DiagnosticParseKinds.td
56–58

The warning should explain why we're warning: what the problem is, and why it warrants action from the user. I would expect people seeing this warning to think "yes, but so what?". Something like "statement has no effect" might be better, maybe? (A customized warning for ;; might be nice too.)

lebedev.ri updated this revision to Diff 168240.Thu, Oct 4, 1:47 AM
lebedev.ri marked an inline comment as done.

Thank you for taking a look!

  • Reworded the diag as suggested a little
  • Do consume consequtive semis as one
    • Do still record each one of them in AST. I'm not sure if we want to do that, but else existing tests break.