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

[lebedev.ri]

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

[efriedma]

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

165

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

[rsmith]

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

165

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]

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]

include/clang/Basic/DiagnosticGroups.td

166

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

169–170

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]

• 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

169–170

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

lib/Parse/ParseStmt.cpp

1622

Aha.

[lebedev.ri]

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

[lebedev.ri]

include/clang/Basic/DiagnosticGroups.td

774

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

[lebedev.ri]

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

[rsmith]

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]

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.

[lebedev.ri]

Ping.

[lebedev.ri]

Ping.

[lebedev.ri]

ping

[lebedev.ri]

Ping.

[lebedev.ri]

ping.
I wonder if @aaron.ballman has any opinion on this.

[lebedev.ri]

Ping. Is even the -Wempty-init-stmt is not interesting?

[aaron.ballman]

Sorry about the delayed review on this -- I had intended to provide comments earlier, but somehow this fell off my radar.

docs/ReleaseNotes.rst

58

, which is, much like -Wextra-semi, diagnoses extra semicolons. -> that diagnoses extra semicolons, much like -Wextra-semi.

60

fires on all -> diagnoses

61

semi -> semicolon

62

nothing; the semi is -> nothing or if the semicolon

I'd make the parenthetical a stand-alone sentence. Something like: "This applies to macros defined in system headers as well as user-defined macros."

70–73

I'd drop this from the example.

96

diagnostic, which is diagnoses empty -> diagnostic that diagnoses empty

97

semi -> semicolon

98–99

nothing; the semi is -> nothing or if the semicolon

I'd make the parenthetical a stand-alone sentence. Something like: "This applies to macros defined in system headers as well as user-defined macros."

100

diagnostic in -> diagnostic is in the

Drop the comma.

include/clang/Basic/DiagnosticParseKinds.td

57–58

How about: empty expression statement has no effect; remove unnecessary ';' to silence this warning ?

562–563

How about: empty initialization statement of '%select{if|switch|range-based for}0' has no effect?

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

82

There are four additional tests I'd like to see: 1) a semicolon at global scope, 2) a semicolon after a namespace declaration (e.g., namespace foo {};), 3) a [[fallthrough]]; null statement, 4) a [[]]; null statement.

[lebedev.ri]

Address @aaron.ballman review notes.

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

82

None of these warn, added.

[aaron.ballman]

Aside from some small nits, LGTM!

docs/ReleaseNotes.rst

58

diagnostic, that -> diagnostic that

93

expanded nothing -> expanded to nothing

include/clang/Basic/DiagnosticParseKinds.td

57

Spurious whitespace at the start of the diagnostic text.

[lebedev.ri]

In D52695#1304225, @aaron.ballman wrote:

Aside from some small nits, LGTM!

Thank you for the review!
Will address nits and land.

docs/ReleaseNotes.rst

- Clang supports use of a profile remapping file, which permits
   profile data captured for one version of a program to be applied
   when building another version where symbols have changed (for
   example, due to renaming a class or namespace).
   See the :doc:`UsersManual` for details.
 
 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
 -------------------------------------------------
 
 - ...
 
 New Compiler Flags
 ------------------

include/clang/Basic/DiagnosticGroups.td

 def EmptyBody : DiagGroup<"empty-body">;
 def Exceptions : DiagGroup<"exceptions">;
 
 def GNUEmptyInitializer : DiagGroup<"gnu-empty-initializer">;
 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]>;
 
 def GNUFlexibleArrayInitializer : DiagGroup<"gnu-flexible-array-initializer">;
 def GNUFlexibleArrayUnionMember : DiagGroup<"gnu-flexible-array-union-member">;
 def GNUFoldingConstant : DiagGroup<"gnu-folding-constant">;
 def FormatExtraArgs : DiagGroup<"format-extra-args">;
 def FormatZeroLength : DiagGroup<"format-zero-length">;
 
 def InvalidIOSDeploymentTarget : DiagGroup<"invalid-ios-deployment-target">;
 
 def CXX17CompatMangling : DiagGroup<"c++17-compat-mangling">;

 def Extra : DiagGroup<"extra", [
     MissingFieldInitializers,
     IgnoredQualifiers,
     InitializerOverrides,
     SemiBeforeMethodBody,
     MissingMethodReturnType,
     SignCompare,
     UnusedParameter,
-    NullPointerArithmetic
+    NullPointerArithmetic,
+    EmptyInitStatement
   ]>;
 
 def Most : DiagGroup<"most", [
     CharSubscript,
     Comment,
     DeleteNonVirtualDtor,
     ForLoopAnalysis,
     Format,

include/clang/Basic/DiagnosticParseKinds.td

def ext_extra_semi : Extension<
   "after member function definition}0">,
   InGroup<ExtraSemi>;
 def ext_extra_semi_cxx11 : Extension<
   "extra ';' outside of a function is a C++11 extension">,
   InGroup<CXX11ExtraSemi>;
 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<
   "keyword '%0' will be made available as an identifier "
   "%select{here|for the remainder of the translation unit}1">,
   InGroup<KeywordCompat>;
 
 def ext_nullability : Extension<
def warn_cxx14_compat_init_statement : Warning<
   "%select{if|switch}0 initialization statements are incompatible with "
   "C++ standards before C++17">, DefaultIgnore, InGroup<CXXPre17Compat>;
 def ext_for_range_init_stmt : ExtWarn<
   "range-based for loop initialization statements are a C++2a extension">,
   InGroup<CXX2a>;
 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">;
 
 // C++ operator overloading
 def err_literal_operator_string_prefix : Error<
   "string literal after 'operator' cannot have an encoding prefix">;
 def err_literal_operator_string_not_empty : Error<

include/clang/Parse/Parser.h

private:
   StmtResult ParseLabeledStatement(ParsedAttributesWithRange &attrs);
   StmtResult ParseCaseStatement(bool MissingCase = false,
                                 ExprResult Expr = ExprResult());
   StmtResult ParseDefaultStatement();
   StmtResult ParseCompoundStatement(bool isStmtExpr = false);
   StmtResult ParseCompoundStatement(bool isStmtExpr,
                                     unsigned ScopeFlags);
   void ParseCompoundStatementLeadingPragmas();
+  bool ConsumeNullStmt(StmtVector &Stmts);
   StmtResult ParseCompoundStatementBody(bool isStmtExpr = false);
   bool ParseParenExprOrCondition(StmtResult *InitStmt,
                                  Sema::ConditionResult &CondResult,
                                  SourceLocation Loc,
                                  Sema::ConditionKind CK);
   StmtResult ParseIfStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseSwitchStatement(SourceLocation *TrailingElseLoc);
   StmtResult ParseWhileStatement(SourceLocation *TrailingElseLoc);

lib/Parse/ParseExprCXX.cpp

Sema::ConditionResult Parser::ParseCXXCondition(StmtResult *InitStmt,
   switch (isCXXConditionDeclarationOrInitStatement(InitStmt, FRI)) {
   case ConditionOrInitStatement::Expression: {
     ProhibitAttributes(attrs);
 
     // We can have an empty expression here.
     //   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);
     }
 
     // Parse the expression.
     ExprResult Expr = ParseExpression(); // expression
     if (Expr.isInvalid())
       return Sema::ConditionError();

lib/Parse/ParseStmt.cpp

Retry:
   case tok::kw_default:             // C99 6.8.1: labeled-statement
     return ParseDefaultStatement();
 
   case tok::l_brace:                // C99 6.8.2: compound-statement
     return ParseCompoundStatement();
   case tok::semi: {                 // C99 6.8.3p3: expression[opt] ';'
     bool HasLeadingEmptyMacro = Tok.hasLeadingEmptyMacro();
     return Actions.ActOnNullStmt(ConsumeToken(), HasLeadingEmptyMacro);
   }
 
   case tok::kw_if:                  // C99 6.8.4.1: if-statement
     return ParseIfStatement(TrailingElseLoc);
   case tok::kw_switch:              // C99 6.8.4.2: switch-statement
     return ParseSwitchStatement(TrailingElseLoc);
 
   case tok::kw_while:               // C99 6.8.5.1: while-statement
     return ParseWhileStatement(TrailingElseLoc);
while (checkForPragmas) {
     default:
       checkForPragmas = false;
       break;
     }
   }
 
 }
 
+/// 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
 /// stack.
 StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
   PrettyStackTraceLoc CrashInfo(PP.getSourceManager(),
                                 Tok.getLocation(),
                                 "in compound statement ('{}')");
StmtResult Parser::ParseCompoundStatementBody(bool isStmtExpr) {
 
   while (!tryParseMisplacedModuleImport() && Tok.isNot(tok::r_brace) &&
          Tok.isNot(tok::eof)) {
     if (Tok.is(tok::annot_pragma_unused)) {
       HandlePragmaUnused();
       continue;
     }
 
+    if (ConsumeNullStmt(Stmts))
+      continue;
+
     StmtResult R;
     if (Tok.isNot(tok::kw___extension__)) {
       R = ParseStatementOrDeclaration(Stmts, ACK_Any);
     } else {
       // __extension__ can start declarations and it can also be a unary
       // operator for expressions.  Consume multiple __extension__ markers here
       // until we can determine which is which.
       // FIXME: This loses extension expressions in the AST!
Actions.CodeCompleteOrdinaryName(getCurScope(),
                                                    : Sema::PCC_Expression);
     cutOffParsing();
     return StmtError();
   }
 
   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()) {
     ProhibitAttributes(attrs);
     IdentifierInfo *Name = Tok.getIdentifierInfo();
     SourceLocation Loc = ConsumeToken();
     MaybeParseCXX11Attributes(attrs);
 
if (Tok.is(tok::semi)) {  // for (...;;
         if (ForRangeInfo.ParsedForRangeDecl()) {
           Diag(FirstPart.get() ? FirstPart.get()->getBeginLoc()
                                : ForRangeInfo.ColonLoc,
                getLangOpts().CPlusPlus2a
                    ? diag::warn_cxx17_compat_for_range_init_stmt
                    : 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();
         if (SecondExpr.isInvalid())
           SecondPart = Sema::ConditionError();
         else
           SecondPart =
               Actions.ActOnCondition(getCurScope(), ForLoc, SecondExpr.get(),

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

+// 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())
+    ;
+}

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

+// 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;
+  }
+}
+
+[[]];