[clang-tidy] find/fix unneeded trailing semicolons in macros

[trixirt]

Summary

clang's -Wextra-semi-stmt shows a large number of warnings that
should be cleaned up in the linux kernel. clang's fixer removes
just the semicolon. This is not always the best solution because
it either causes other whitespace problems or there is a better
semicolon to remove.

This change introduces linuxkernel-extra-semi as a place to
collect special purpose fixers. Starting with two fixers set by
the option 'Fixer'

'Switch'
switch() {} ; <-- the ';' is not needed.

The fixer takes care of formatting issues of having an empty line
when the ';' is removed.

'TrailingMacro'
#define M(a) a++; <-- clang-tidy fixes problem here
int f() {

int v = 0;
M(v); <-- clang reports problem here
return v;

}

Removing the semicolon at the macro use will fail checkpatch,
removing at the macro definition will not.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/linuxkernel/MacroTrailingSemiCheck.cpp

31 ↗

(On Diff #306381)

Please don't use auto when type is not spelled explicitly in same statement or iterator.

99 ↗

(On Diff #306381)

Please don't use auto when type is not spelled explicitly in same statement or iterator.

113 ↗

(On Diff #306381)

Please don't use auto when type is not spelled explicitly in same statement or iterator.

[njames93]

clang-tools-extra/clang-tidy/linuxkernel/MacroTrailingSemiCheck.cpp

15 ↗

(On Diff #306381)

Any reason for including the c standard header?

102–105 ↗

(On Diff #306381)

Could this be a range for loop.

106–111 ↗

(On Diff #306381)

Could this be refactored using llvm::find_if

116 ↗

(On Diff #306381)

No need for this to be a temporary

clang-tools-extra/test/clang-tidy/checkers/linuxkernel-macro-trailing-semi.c

5

Please specify the location where this warning is emitted, you can check other test files but the typical format is
// CHECK-MESSAGES: :[[@LINE-<Offset>]]:<Column>: warning: unneeded semicolon.

6

Don't need to check the FIX-IT note, however it is preferred if you check the fix-it was applied correctly.
// CHECK-FIXES will run FileCheck over the input file after clang-tidy has gone in and made changes then check for lines in the output.
I haven't read enough into where the fix is but an example would be:
// CHECK-FIXES: #define M(a) a++{{$}}
the {{$}} says regex for end of line

[njames93]

clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp

23–24

This check hasn't landed, can you please make sure your patches are against a commit on trunk, otherwise it can't be applied.

[aaron.ballman]

The request in D90180 was to not add a check for each kind of problematic semicolon situation but to instead make a single check that has configuration options for which semi colon situations to care about. The proposed changes here should be rolled into that unified check.

[trixirt]

Addresses issues before refactoring.

[trixirt]

Refactor to combine switch and trailing macro into one fixer

[trixirt]

address precheckin issues

[nickdesaulniers]

Hi @trixirt , thanks for the follow up. I think the lint feedback about qualifying auto with pointers where applicable would be nice to have. Please make those suggested changes.

Should https://reviews.llvm.org/D90180 be abandoned in favor of this patch?

Would you mind editing the description/commit message of this patch to describe to kernel developers how they're expected to run this clang tidy check such that the fixits are applied?

clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h

21

Can this enum be declared within the class scope of ExtraSemiCheck rather than outside of it?

[trixirt]

move enum

[trixirt]

How this is run in the kernel is a wip so adding it to the commit log is not very helpful.
Here is the lkml rfc
https://lkml.org/lkml/2020/11/21/190
This calling in the kernel needs needs to change because of the refactor.

The auto suggestions fail to build, so they are not helpful.

Yes eventually the first review should be dropped, i'll do that when this one lands.

[aaron.ballman]

clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.cpp

49

Something that's not for you to solve, but for us to think about...

The AST matchers have ways to inspect parent and child relationships between AST nodes but no way to inspect sibling relationships. It's really unfortunate just how much effort you have to go through after the AST matcher fires to figure this out. It would be much cleaner if there was a way for you to do something like: compoundStmt(hasSiblings(switchStmt(), nullStmt().bind("null"))) to test for a SwitchStmt node immediately followed by a NullStmt node that appear inside a CompoundStmt. I feel like that would make these checks a lot easier for you to write.

clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h

22

I'm not familiar with the way your CI plans to use this check, but: would it make sense to treat these as a bitmask rather than mutually exclusive options? e.g., the CI can enable both "switch" and "trailing macro" in a single pass of a file rather than run the check twice with different options on the same file? The configuration could then take a delimited list of pass options to enable to set up this bitmask, and the == tests for the FixerKind would then become & tests.

37

You can drop the enum elaboration here.

[nickdesaulniers]

clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h

37

right, this is a difference between C and C++; for C the enum keyword would be required always; for C++ it's not, and generally is omitted. (In C++ was also don't generally use struct or class for function parameters, or local variables, though you could.)

clang-tools-extra/clang-tidy/linuxkernel/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   FrontendOpenMP
   Support
   )
 
 add_clang_library(clangTidyLinuxKernelModule
+  ExtraSemiCheck.cpp
   LinuxKernelTidyModule.cpp
   MustCheckErrsCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyUtils
 
   DEPENDS

clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.h

+//===--- ExtraSemiCheck.h - clang-tidy --------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_EXTRASEMICHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_EXTRASEMICHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/Lex/MacroInfo.h"
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace linuxkernel {
+
+class ExtraSemiCheck : public ClangTidyCheck {
+
+  enum ExtraSemiFixerKind { ESFK_None, ESFK_Switch, ESFK_TrailingMacro };
+
+public:
+  ExtraSemiCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+                           Preprocessor *ModuleExpanderPP) override final;
+
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void checkMacro(SourceManager &SM, const Token &MacroNameTok,
+                  const MacroInfo *MI);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  std::vector<const MacroInfo *> SuspectMacros;
+  enum ExtraSemiFixerKind FixerKind;
+  const std::string ExtraSemiFixerKindName;
+};
+
+} // namespace linuxkernel
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LINUXKERNEL_EXTRASEMICHECK_H

clang-tools-extra/clang-tidy/linuxkernel/ExtraSemiCheck.cpp

+//===--- ExtraSemiCheck.cpp - clang-tidy ----------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ExtraSemiCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace linuxkernel {
+
+class ExtraSemiCheckPPCallbacks : public PPCallbacks {
+public:
+  ExtraSemiCheckPPCallbacks(Preprocessor *PP, ExtraSemiCheck *Check)
+      : PP(PP), Check(Check) {}
+
+  void MacroDefined(const Token &MacroNameTok,
+                    const MacroDirective *MD) override {
+    const MacroInfo *MI = MD->getMacroInfo();
+
+    if (MI->isBuiltinMacro() || MI->isObjectLike())
+      return;
+    Check->checkMacro(PP->getSourceManager(), MacroNameTok, MI);
+  }
+
+private:
+  Preprocessor *PP;
+  ExtraSemiCheck *Check;
+};
+
+ExtraSemiCheck::ExtraSemiCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context), FixerKind(ESFK_None),
+      ExtraSemiFixerKindName(Options.get("Fixer", ExtraSemiFixerKindName)) {
+  if (ExtraSemiFixerKindName == "Switch")
+    FixerKind = ESFK_Switch;
+  else if (ExtraSemiFixerKindName == "TrailingMacro")
+    FixerKind = ESFK_TrailingMacro;
+}
+
+void ExtraSemiCheck::registerMatchers(MatchFinder *Finder) {
+  if (FixerKind == ESFK_Switch) {
+    // From the reproducer
+    // void foo (int a) {
+    //   switch (a) {};
+    // }
+    // The AST
+    // `-FunctionDecl
+    //   |-ParmVarDecl
+    //   `-CompoundStmt <--- "comp", 'C'
+    //    |-SwitchStmt <-- "switch", 'S'
+    //    | |-ImplicitCastExpr
+    //    | | `-DeclRefExpr
+    //    | `-CompoundStmt
+    //    `-NullStmt <-------------- 'N'
+    Finder->addMatcher(
+        compoundStmt(has(switchStmt().bind("switch"))).bind("comp"), this);
+  } else if (FixerKind == ESFK_TrailingMacro) {
+    // From the reproducer
+    // #define M(a) a++;
+    // int f() {
+    //   int v = 0;
+    //   M(v);
+    //   return v;
+    // }
+    // The AST
+    // `-FunctionDecl
+    //   `-CompoundStmt <--- "comp", 'C'
+    //     ...
+    //     |-UnaryOperator
+    //     | `-DeclRefExpr <-------- 'E'
+    //     |-NullStmt <------ "null" 'N'
+    //     ...
+    Finder->addMatcher(compoundStmt(has(nullStmt().bind("null"))).bind("comp"),
+                       this);
+  }
+}
+
+void ExtraSemiCheck::registerPPCallbacks(const SourceManager &SM,
+                                         Preprocessor *PP,
+                                         Preprocessor *ModuleExpanderPP) {
+  if (FixerKind == ESFK_TrailingMacro) {
+    ModuleExpanderPP->addPPCallbacks(
+        std::make_unique<ExtraSemiCheckPPCallbacks>(ModuleExpanderPP, this));
+  }
+}
+
+void ExtraSemiCheck::check(const MatchFinder::MatchResult &Result) {
+  if (FixerKind == ESFK_Switch) {
+    const auto *C = Result.Nodes.getNodeAs<CompoundStmt>("comp");
+    const auto *S = Result.Nodes.getNodeAs<SwitchStmt>("switch");
+
+    auto Current = C->body_begin();
+    auto Next = Current;
+    Next++;
+    while (Next != C->body_end()) {
+      if (*Current == S) {
+        if (const auto *N = dyn_cast<NullStmt>(*Next)) {
+          // This code has the same AST as the reproducer
+          //
+          // #define EMPTY()
+          // void foo (int a) {
+          // switch (a) {} EMPTY();
+          // }
+          //
+          // But we do not want to remove the ; because the
+          // macro may only be conditionally empty.
+          // ex/ the release version of a debug macro
+          //
+          // So check that the NullStmt does not have a
+          // leading empty macro.
+          if (!N->hasLeadingEmptyMacro() &&
+              S->getBody()->getEndLoc().isValid() &&
+              N->getSemiLoc().isValid()) {
+            auto H = FixItHint::CreateReplacement(
+                SourceRange(S->getBody()->getEndLoc(), N->getSemiLoc()), "}");
+            diag(N->getSemiLoc(), "unneeded semicolon") << H;
+            break;
+          }
+        }
+      }
+      Current = Next;
+      Next++;
+    }
+  } else if (FixerKind == ESFK_TrailingMacro) {
+    const auto *C = Result.Nodes.getNodeAs<CompoundStmt>("comp");
+    const auto *N = Result.Nodes.getNodeAs<NullStmt>("null");
+
+    auto Current = C->body_begin();
+    auto Next = Current;
+    Next++;
+    while (Next != C->body_end()) {
+
+      if (*Next == N) {
+        // This code has the same AST as the reproducer
+        //
+        // #define NOT_EMPTY(a) a++;
+        // #define EMPTY()
+        // void foo (int a) {
+        //   NOT_EMPTY(a);
+        //   EMPTY();
+        // }
+        //
+        // But we do not want to remove the ; because the
+        // macro may only be conditionally empty.
+        // ex/ the release version of a debug macro
+        //
+        // So check that the NullStmt does not have a
+        // leading empty macro.
+        if (!N->hasLeadingEmptyMacro() && N->getEndLoc().isValid()) {
+          if (const auto *E = dyn_cast<Expr>(*Current)) {
+            SourceLocation Loc = E->getEndLoc();
+            if (Loc.isMacroID()) {
+              SourceManager &SM = Result.Context->getSourceManager();
+              FullSourceLoc SpellingLoc =
+                  FullSourceLoc(Loc, SM).getSpellingLoc();
+
+              for (const MacroInfo *MI : SuspectMacros) {
+                auto TI = MI->tokens_begin();
+                for (; TI != MI->tokens_end(); TI++) {
+                  SourceLocation L = TI->getLocation();
+                  if (SpellingLoc == L)
+                    break;
+                }
+                if (TI != MI->tokens_end()) {
+                  const Token &Tok =
+                      MI->getReplacementToken(MI->getNumTokens() - 1);
+                  SourceLocation FixLoc = Tok.getLocation();
+                  SourceLocation EndLoc = Tok.getEndLoc();
+                  diag(FixLoc, "unneeded semicolon")
+                      << FixItHint::CreateRemoval(SourceRange(FixLoc, EndLoc));
+                  break;
+                }
+              }
+            }
+          }
+        }
+      }
+      Current = Next;
+      Next++;
+    }
+  }
+}
+void ExtraSemiCheck::checkMacro(SourceManager &SM, const Token &MacroNameTok,
+                                const MacroInfo *MI) {
+  unsigned Num = MI->getNumTokens();
+  if (Num && MI->getReplacementToken(Num - 1).is(tok::semi))
+    SuspectMacros.push_back(MI);
+}
+
+void ExtraSemiCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "Fixer", "");
+}
+
+} // namespace linuxkernel
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/linuxkernel/LinuxKernelTidyModule.cpp

 //===--- LinuxKernelTidyModule.cpp - clang-tidy----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "ExtraSemiCheck.h"
 #include "MustCheckErrsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace linuxkernel {
 
 /// This module is for checks specific to the Linux kernel.
 class LinuxKernelModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<ExtraSemiCheck>("linuxkernel-extra-semi");
     CheckFactories.registerCheck<MustCheckErrsCheck>(
         "linuxkernel-must-check-errs");
   }
 };
 // Register the LinuxKernelTidyModule using this statically initialized
 // variable.
 static ClangTidyModuleRegistry::Add<LinuxKernelModule>
     X("linux-module", "Adds checks specific to the Linux kernel.");
 } // namespace linuxkernel
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the LinuxKernelModule.
 volatile int LinuxKernelModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/linuxkernel-macro-trailing-semi.c

+// RUN: %check_clang_tidy %s linuxkernel-extra-semi %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN:     [{key: linuxkernel-extra-semi.Fixer, \
+// RUN:       value: 'TrailingMacro'}, \
+// RUN:     ]}"
+
+#define M(a) a++;
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: unneeded semicolon
+// CHECK-FIXES: #define M(a) a++{{$}}
+
+int f() {
+  int v = 0;
+  M(v);
+  return v;
+}
+
+// A corner case
+// An empty macro could conditionally contain another path so
+// do not warn or fix.
+#ifdef UNSET_CONDITION
+#define E() ;
+#else
+#define E()
+#endif
+#define N(a) a++;
+
+int g() {
+  int v = 0;
+  N(v)
+  E();
+  return v;
+}

clang-tools-extra/test/clang-tidy/checkers/linuxkernel-switch-semi.c

+// RUN: %check_clang_tidy %s linuxkernel-extra-semi %t -- \
+// RUN:   -config="{CheckOptions: \
+// RUN:     [{key: linuxkernel-extra-semi.Fixer, \
+// RUN:       value: 'Switch'}, \
+// RUN:     ]}"
+
+int f(int a) {
+  switch (a) {
+  case 1:
+    return 0;
+  default:
+    break;
+  };
+  // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: unneeded semicolon
+  // CHECK-FIXES:   }{{$}}
+  return 0;
+}
+
+// A normal switch, should not produce a warning
+int p1(int a) {
+  switch (a) {
+  case 1:
+    return 0;
+  default:
+    break;
+  }
+  return 0;
+}
+
+#define EMPTY_MACRO()
+// A corner case, do not fix an empty macro
+int p2(int a) {
+  switch (a) {
+  case 1:
+    return 0;
+  default:
+    break;
+  }
+  EMPTY_MACRO();
+  return 0;
+}