[clang][Parse] Diagnose useless null statements / empty init-statements

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

Additionally:
```
if(; // <- empty init-statement
   true)
  ;

switch (; // empty init-statement
        x) {
  ...
}

for (; // <- empty init-statement
     int y : S())
  ;
}

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 [[ https://bugs.llvm.org/show_bug.cgi?id=39111 | PR39111 ]]

Reviewers: rsmith, aaron.ballman, efriedma

Reviewed By: aaron.ballman

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D52695

llvm-svn: 347339

clang/docs/ReleaseNotes.rst

@@ -55,6 +55,63 @@ Major New Features
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- ``-Wextra-semi-stmt`` is a new diagnostic that diagnoses extra semicolons,
+  much like ``-Wextra-semi``. This new diagnostic diagnoses all *unnecessary*
+  null statements (expression statements without an expression), unless: the
+  semicolon directly follows a macro that was expanded to nothing or if the
+  semicolon is within the macro itself. This applies to macros defined in system
+  headers as well as user-defined macros.
+
+  .. code-block:: c++
+
+      #define MACRO(x) int x;
+      #define NULLMACRO(varname)
+
+      void test() {
+        ; // <- warning: ';' with no preceding expression is a null statement
+
+        while (true)
+          ; // OK, it is needed.
+
+        switch (my_enum) {
+        case E1:
+          // stuff
+          break;
+        case E2:
+          ; // OK, it is needed.
+        }
+
+        MACRO(v0;) // Extra semicolon, but within macro, so ignored.
+
+        MACRO(v1); // <- warning: ';' with no preceding expression is a null statement
+
+        NULLMACRO(v2); // ignored, NULLMACRO expanded to nothing.
+      }
+
+- ``-Wempty-init-stmt`` is a new diagnostic that diagnoses empty init-statements
+  of ``if``, ``switch``, ``range-based for``, unless: the semicolon directly
+  follows a macro that was expanded to nothing or if the semicolon is within the
+  macro itself (both macros from system headers, and normal macros). This
+  diagnostic is in the ``-Wextra-semi-stmt`` group and is enabled in
+  ``-Wextra``.
+
+  .. code-block:: c++
+
+      void test() {
+        if(; // <- warning: init-statement of 'if' is a null statement
+           true)
+          ;
+
+        switch (; // <- warning: init-statement of 'switch' is a null statement
+                x) {
+          ...
+        }
+
+        for (; // <- warning: init-statement of 'range-based for' is a null statement
+             int y : S())
+          ;
+      }
+
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------

clang/include/clang/Basic/DiagnosticGroups.td

@@ -162,6 +162,8 @@ def GNUEmptyStruct : DiagGroup<"gnu-empty-struct">;
 def ExtraTokens : DiagGroup<"extra-tokens">;
 def CXX98CompatExtraSemi : DiagGroup<"c++98-compat-extra-semi">;
 def CXX11ExtraSemi : DiagGroup<"c++11-extra-semi">;
+def EmptyInitStatement : DiagGroup<"empty-init-stmt">;
+def ExtraSemiStmt : DiagGroup<"extra-semi-stmt", [EmptyInitStatement]>;
 def ExtraSemi : DiagGroup<"extra-semi", [CXX98CompatExtraSemi,
                                          CXX11ExtraSemi]>;
 
@@ -768,7 +770,8 @@ def Extra : DiagGroup<"extra", [
     MissingMethodReturnType,
     SignCompare,
     UnusedParameter,
-    NullPointerArithmetic
+    NullPointerArithmetic,
+    EmptyInitStatement
   ]>;
 
 def Most : DiagGroup<"most", [

clang/include/clang/Basic/DiagnosticParseKinds.td

@@ -53,6 +53,10 @@ def ext_extra_semi_cxx11 : Extension<
 def warn_extra_semi_after_mem_fn_def : Warning<
   "extra ';' after member function definition">,
   InGroup<ExtraSemi>, DefaultIgnore;
+def warn_null_statement : Warning<
+  "empty expression statement has no effect; "
+  "remove unnecessary ';' to silence this warning">,
+  InGroup<ExtraSemiStmt>, DefaultIgnore;
 
 def ext_thread_before : Extension<"'__thread' before '%0'">;
 def ext_keyword_as_ident : ExtWarn<
@@ -554,6 +558,9 @@ def ext_for_range_init_stmt : ExtWarn<
 def warn_cxx17_compat_for_range_init_stmt : Warning<
   "range-based for loop initialization statements are incompatible with "
   "C++ standards before C++2a">, DefaultIgnore, InGroup<CXXPre2aCompat>;
+def warn_empty_init_statement : Warning<
+  "empty initialization statement of '%select{if|switch|range-based for}0' "
+  "has no effect">, InGroup<EmptyInitStatement>, DefaultIgnore;
 
 // C++ derived classes
 def err_dup_virtual : Error<"duplicate 'virtual' in base specifier">;

clang/include/clang/Parse/Parser.h

@@ -1888,6 +1888,7 @@ private:
   StmtResult ParseCompoundStatement(bool isStmtExpr,
                                     unsigned ScopeFlags);
   void ParseCompoundStatementLeadingPragmas();
+  bool ConsumeNullStmt(StmtVector &Stmts);
   StmtResult ParseCompoundStatementBody(bool isStmtExpr = false);
   bool ParseParenExprOrCondition(StmtResult *InitStmt,
                                  Sema::ConditionResult &CondResult,

clang/lib/Parse/ParseExprCXX.cpp

@@ -1773,7 +1773,13 @@ Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt,
     //   if (; true);
     if (InitStmt && Tok.is(tok::semi)) {
       WarnOnInit();
-      SourceLocation SemiLoc = ConsumeToken();
+      SourceLocation SemiLoc = Tok.getLocation();
+      if (!Tok.hasLeadingEmptyMacro() && !SemiLoc.isMacroID()) {
+        Diag(SemiLoc, diag::warn_empty_init_statement)
+            << (CK == Sema::ConditionKind::Switch)
+            << FixItHint::CreateRemoval(SemiLoc);
+      }
+      ConsumeToken();
       *InitStmt = Actions.ActOnNullStmt(SemiLoc);
       return ParseCXXCondition(nullptr, Loc, CK);
     }

clang/lib/Parse/ParseStmt.cpp

@@ -930,6 +930,34 @@ void Parser::ParseCompoundStatementLeadingPragmas() {
 
 }
 
+/// Consume any extra semi-colons resulting in null statements,
+/// returning true if any tok::semi were consumed.
+bool Parser::ConsumeNullStmt(StmtVector &Stmts) {
+  if (!Tok.is(tok::semi))
+    return false;
+
+  SourceLocation StartLoc = Tok.getLocation();
+  SourceLocation EndLoc;
+
+  while (Tok.is(tok::semi) && !Tok.hasLeadingEmptyMacro() &&
+         Tok.getLocation().isValid() && !Tok.getLocation().isMacroID()) {
+    EndLoc = Tok.getLocation();
+
+    // Don't just ConsumeToken() this tok::semi, do store it in AST.
+    StmtResult R = ParseStatementOrDeclaration(Stmts, ACK_Any);
+    if (R.isUsable())
+      Stmts.push_back(R.get());
+  }
+
+  // Did not consume any extra semi.
+  if (EndLoc.isInvalid())
+    return false;
+
+  Diag(StartLoc, diag::warn_null_statement)
+      << FixItHint::CreateRemoval(SourceRange(StartLoc, EndLoc));
+  return true;
+}
+
 /// ParseCompoundStatementBody - Parse a sequence of statements and invoke the
 /// ActOnCompoundStmt action.  This expects the '{' to be the current token, and
 /// consume the '}' at the end of the block.  It does not manipulate the scope
@@ -992,6 +1020,9 @@ StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
       continue;
     }
 
+    if (ConsumeNullStmt(Stmts))
+      continue;
+
     StmtResult R;
     if (Tok.isNot(tok::kw___extension__)) {
       R = ParseStatementOrDeclaration(Stmts, ACK_Any);
@@ -1588,10 +1619,15 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
   ParsedAttributesWithRange attrs(AttrFactory);
   MaybeParseCXX11Attributes(attrs);
 
+  SourceLocation EmptyInitStmtSemiLoc;
+
   // Parse the first part of the for specifier.
   if (Tok.is(tok::semi)) {  // for (;
     ProhibitAttributes(attrs);
     // no first part, eat the ';'.
+    SourceLocation SemiLoc = Tok.getLocation();
+    if (!Tok.hasLeadingEmptyMacro() && !SemiLoc.isMacroID())
+      EmptyInitStmtSemiLoc = SemiLoc;
     ConsumeToken();
   } else if (getLangOpts().CPlusPlus && Tok.is(tok::identifier) &&
              isForRangeIdentifier()) {
@@ -1723,6 +1759,11 @@ StmtResult Parser::ParseForStatement(SourceLocation *TrailingElseLoc) {
                    : diag::ext_for_range_init_stmt)
               << (FirstPart.get() ? FirstPart.get()->getSourceRange()
                                   : SourceRange());
+          if (EmptyInitStmtSemiLoc.isValid()) {
+            Diag(EmptyInitStmtSemiLoc, diag::warn_empty_init_statement)
+                << /*for-loop*/ 2
+                << FixItHint::CreateRemoval(EmptyInitStmtSemiLoc);
+          }
         }
       } else {
         ExprResult SecondExpr = ParseExpression();

clang/test/Parser/extra-semi-resulting-in-nullstmt-in-init-statement.cpp

@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -std=c++2a -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wempty-init-stmt -std=c++2a -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -fixit %t
+// RUN: %clang_cc1 -x c++ -Wempty-init-stmt -std=c++2a -Werror %t
+
+struct S {
+  int *begin();
+  int *end();
+};
+
+void naive(int x) {
+  if (; true) // expected-warning {{empty initialization statement of 'if' has no effect}}
+    ;
+
+  switch (; x) { // expected-warning {{empty initialization statement of 'switch' has no effect}}
+  }
+
+  for (; int y : S()) // expected-warning {{empty initialization statement of 'range-based for' has no effect}}
+    ;
+
+  for (;;) // OK
+    ;
+}
+
+#define NULLMACRO
+
+void with_null_macro(int x) {
+  if (NULLMACRO; true)
+    ;
+
+  switch (NULLMACRO; x) {
+  }
+
+  for (NULLMACRO; int y : S())
+    ;
+}
+
+#define SEMIMACRO ;
+
+void with_semi_macro(int x) {
+  if (SEMIMACRO true)
+    ;
+
+  switch (SEMIMACRO x) {
+  }
+
+  for (SEMIMACRO int y : S())
+    ;
+}
+
+#define PASSTHROUGHMACRO(x) x
+
+void with_passthrough_macro(int x) {
+  if (PASSTHROUGHMACRO(;) true)
+    ;
+
+  switch (PASSTHROUGHMACRO(;) x) {
+  }
+
+  for (PASSTHROUGHMACRO(;) int y : S())
+    ;
+}

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

@@ -0,0 +1,96 @@
+// RUN: %clang_cc1 -fsyntax-only -Wextra-semi-stmt -verify %s
+// RUN: cp %s %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -fixit %t
+// RUN: %clang_cc1 -x c++ -Wextra-semi-stmt -Werror %t
+
+#define GOODMACRO(varname) int varname
+#define BETTERMACRO(varname) GOODMACRO(varname);
+#define NULLMACRO(varname)
+
+enum MyEnum {
+  E1,
+  E2
+};
+
+void test() {
+  ; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+  ;
+
+  // This removal of extra semi also consumes all the comments.
+  // clang-format: off
+  ;;;
+  // clang-format: on
+
+  // clang-format: off
+  ;NULLMACRO(ZZ);
+  // clang-format: on
+
+  {}; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+  {
+    ; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+  }
+
+  if (true) {
+    ; // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+  }
+
+  GOODMACRO(v0); // OK
+
+  GOODMACRO(v1;) // OK
+
+  BETTERMACRO(v2) // OK
+
+  BETTERMACRO(v3;) // Extra ';', but within macro expansion, so ignored.
+
+  BETTERMACRO(v4); // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+  BETTERMACRO(v5;); // expected-warning {{empty expression statement has no effect; remove unnecessary ';' to silence this warning}}
+
+  NULLMACRO(v6) // OK
+
+  NULLMACRO(v7); // OK. This could be either GOODMACRO() or BETTERMACRO() situation, so we can't know we can drop it.
+
+  if (true)
+    ; // OK
+
+  while (true)
+    ; // OK
+
+  do
+    ; // OK
+  while (true);
+
+  for (;;) // OK
+    ;      // OK
+
+  MyEnum my_enum;
+  switch (my_enum) {
+  case E1:
+    // stuff
+    break;
+  case E2:; // OK
+  }
+
+  for (;;) {
+    for (;;) {
+      goto contin_outer;
+    }
+  contin_outer:; // OK
+  }
+}
+
+;
+
+namespace NS {};
+
+void foo(int x) {
+  switch (x) {
+  case 0:
+    [[fallthrough]];
+  case 1:
+    return;
+  }
+}
+
+[[]];