[clang-tidy] Improve modernize-redundant-void-arg to recognize macro uses

Sometimes a macro invocation will look like an argument list
declaration.  Improve the check to detect this situation and not
try to modify the macro invocation.

Thanks to Nathan James for the fix.

- Ignore implicit typedefs (e.g. compiler builtins)
- Improve lexing state machine to locate void argument tokens
- Add additional return_t() macro tests
- clang-format control in the test case file
- remove braces around single statements per LLVM style guide

Fixes #43791

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

2 files changed, 140 insertions, 62 deletions

diff --git a/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp b/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
index 3214ae84b74d..4f791d404d9d 100644
--- a/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
@@ -20,15 +20,12 @@ namespace {
 
 // Determine if the given QualType is a nullary function or pointer to same.
 bool protoTypeHasNoParms(QualType QT) {
-  if (const auto *PT = QT->getAs<PointerType>()) {
+  if (const auto *PT = QT->getAs<PointerType>())
     QT = PT->getPointeeType();
-  }
-  if (auto *MPT = QT->getAs<MemberPointerType>()) {
+  if (auto *MPT = QT->getAs<MemberPointerType>())
     QT = MPT->getPointeeType();
-  }
-  if (const auto *FP = QT->getAs<FunctionProtoType>()) {
+  if (const auto *FP = QT->getAs<FunctionProtoType>())
     return FP->getNumParams() == 0;
-  }
   return false;
 }
 
@@ -48,7 +45,8 @@ void RedundantVoidArgCheck::registerMatchers(MatchFinder *Finder) {
                                   unless(isInstantiated()), unless(isExternC()))
                          .bind(FunctionId),
                      this);
-  Finder->addMatcher(typedefNameDecl().bind(TypedefId), this);
+  Finder->addMatcher(typedefNameDecl(unless(isImplicit())).bind(TypedefId),
+                     this);
   auto ParenFunctionType = parenType(innerType(functionType()));
   auto PointerToFunctionType = pointee(ParenFunctionType);
   auto FunctionOrMemberPointer =
@@ -72,34 +70,36 @@ void RedundantVoidArgCheck::registerMatchers(MatchFinder *Finder) {
 
 void RedundantVoidArgCheck::check(const MatchFinder::MatchResult &Result) {
   const BoundNodes &Nodes = Result.Nodes;
-  if (const auto *Function = Nodes.getNodeAs<FunctionDecl>(FunctionId)) {
+  if (const auto *Function = Nodes.getNodeAs<FunctionDecl>(FunctionId))
     processFunctionDecl(Result, Function);
-  } else if (const auto *TypedefName =
-                 Nodes.getNodeAs<TypedefNameDecl>(TypedefId)) {
+  else if (const auto *TypedefName =
+               Nodes.getNodeAs<TypedefNameDecl>(TypedefId))
     processTypedefNameDecl(Result, TypedefName);
-  } else if (const auto *Member = Nodes.getNodeAs<FieldDecl>(FieldId)) {
+  else if (const auto *Member = Nodes.getNodeAs<FieldDecl>(FieldId))
     processFieldDecl(Result, Member);
-  } else if (const auto *Var = Nodes.getNodeAs<VarDecl>(VarId)) {
+  else if (const auto *Var = Nodes.getNodeAs<VarDecl>(VarId))
     processVarDecl(Result, Var);
-  } else if (const auto *NamedCast =
-                 Nodes.getNodeAs<CXXNamedCastExpr>(NamedCastId)) {
+  else if (const auto *NamedCast =
+               Nodes.getNodeAs<CXXNamedCastExpr>(NamedCastId))
     processNamedCastExpr(Result, NamedCast);
-  } else if (const auto *CStyleCast =
-                 Nodes.getNodeAs<CStyleCastExpr>(CStyleCastId)) {
+  else if (const auto *CStyleCast =
+               Nodes.getNodeAs<CStyleCastExpr>(CStyleCastId))
     processExplicitCastExpr(Result, CStyleCast);
-  } else if (const auto *ExplicitCast =
-                 Nodes.getNodeAs<ExplicitCastExpr>(ExplicitCastId)) {
+  else if (const auto *ExplicitCast =
+               Nodes.getNodeAs<ExplicitCastExpr>(ExplicitCastId))
     processExplicitCastExpr(Result, ExplicitCast);
-  } else if (const auto *Lambda = Nodes.getNodeAs<LambdaExpr>(LambdaId)) {
+  else if (const auto *Lambda = Nodes.getNodeAs<LambdaExpr>(LambdaId))
     processLambdaExpr(Result, Lambda);
-  }
 }
 
 void RedundantVoidArgCheck::processFunctionDecl(
     const MatchFinder::MatchResult &Result, const FunctionDecl *Function) {
+  const auto *Method = dyn_cast<CXXMethodDecl>(Function);
+  SourceLocation Start = Method && Method->getParent()->isLambda()
+                             ? Method->getBeginLoc()
+                             : Function->getLocation();
+  SourceLocation End = Function->getEndLoc();
   if (Function->isThisDeclarationADefinition()) {
-    SourceLocation Start = Function->getBeginLoc();
-    SourceLocation End = Function->getEndLoc();
     if (const Stmt *Body = Function->getBody()) {
       End = Body->getBeginLoc();
       if (End.isMacroID() &&
@@ -109,10 +109,20 @@ void RedundantVoidArgCheck::processFunctionDecl(
     }
     removeVoidArgumentTokens(Result, SourceRange(Start, End),
                              "function definition");
-  } else {
-    removeVoidArgumentTokens(Result, Function->getSourceRange(),
+  } else
+    removeVoidArgumentTokens(Result, SourceRange(Start, End),
                              "function declaration");
-  }
+}
+
+bool isMacroIdentifier(const IdentifierTable &Idents, const Token &ProtoToken) {
+  if (!ProtoToken.is(tok::TokenKind::raw_identifier))
+    return false;
+
+  IdentifierTable::iterator It = Idents.find(ProtoToken.getRawIdentifier());
+  if (It == Idents.end())
+    return false;
+
+  return It->second->hadMacroDefinition();
 }
 
 void RedundantVoidArgCheck::removeVoidArgumentTokens(
@@ -127,49 +137,86 @@ void RedundantVoidArgCheck::removeVoidArgumentTokens(
           .str();
   Lexer PrototypeLexer(CharRange.getBegin(), getLangOpts(), DeclText.data(),
                        DeclText.data(), DeclText.data() + DeclText.size());
-  enum TokenState {
-    NothingYet,
-    SawLeftParen,
-    SawVoid,
+  enum class TokenState {
+    Start,
+    MacroId,
+    MacroLeftParen,
+    MacroArguments,
+    LeftParen,
+    Void,
   };
-  TokenState State = NothingYet;
+  TokenState State = TokenState::Start;
   Token VoidToken;
   Token ProtoToken;
+  const IdentifierTable &Idents = Result.Context->Idents;
+  int MacroLevel = 0;
   std::string Diagnostic =
       ("redundant void argument list in " + GrammarLocation).str();
 
   while (!PrototypeLexer.LexFromRawLexer(ProtoToken)) {
     switch (State) {
-    case NothingYet:
-      if (ProtoToken.is(tok::TokenKind::l_paren)) {
-        State = SawLeftParen;
-      }
+    case TokenState::Start:
+      if (ProtoToken.is(tok::TokenKind::l_paren))
+        State = TokenState::LeftParen;
+      else if (isMacroIdentifier(Idents, ProtoToken))
+        State = TokenState::MacroId;
+      break;
+    case TokenState::MacroId:
+      if (ProtoToken.is(tok::TokenKind::l_paren))
+        State = TokenState::MacroLeftParen;
+      else
+        State = TokenState::Start;
+      break;
+    case TokenState::MacroLeftParen:
+      ++MacroLevel;
+      if (ProtoToken.is(tok::TokenKind::raw_identifier)) {
+        if (isMacroIdentifier(Idents, ProtoToken))
+          State = TokenState::MacroId;
+        else
+          State = TokenState::MacroArguments;
+      } else if (ProtoToken.is(tok::TokenKind::r_paren)) {
+        --MacroLevel;
+        if (MacroLevel == 0)
+          State = TokenState::Start;
+        else
+          State = TokenState::MacroId;
+      } else
+        State = TokenState::MacroArguments;
       break;
-    case SawLeftParen:
-      if (ProtoToken.is(tok::TokenKind::raw_identifier) &&
-          ProtoToken.getRawIdentifier() == "void") {
-        State = SawVoid;
-        VoidToken = ProtoToken;
-      } else if (ProtoToken.is(tok::TokenKind::l_paren)) {
-        State = SawLeftParen;
-      } else {
-        State = NothingYet;
+    case TokenState::MacroArguments:
+      if (isMacroIdentifier(Idents, ProtoToken))
+        State = TokenState::MacroLeftParen;
+      else if (ProtoToken.is(tok::TokenKind::r_paren)) {
+        --MacroLevel;
+        if (MacroLevel == 0)
+          State = TokenState::Start;
       }
       break;
-    case SawVoid:
-      State = NothingYet;
-      if (ProtoToken.is(tok::TokenKind::r_paren)) {
+    case TokenState::LeftParen:
+      if (ProtoToken.is(tok::TokenKind::raw_identifier)) {
+        if (isMacroIdentifier(Idents, ProtoToken))
+          State = TokenState::MacroId;
+        else if (ProtoToken.getRawIdentifier() == "void") {
+          State = TokenState::Void;
+          VoidToken = ProtoToken;
+        }
+      } else if (ProtoToken.is(tok::TokenKind::l_paren))
+        State = TokenState::LeftParen;
+      else
+        State = TokenState::Start;
+      break;
+    case TokenState::Void:
+      State = TokenState::Start;
+      if (ProtoToken.is(tok::TokenKind::r_paren))
         removeVoidToken(VoidToken, Diagnostic);
-      } else if (ProtoToken.is(tok::TokenKind::l_paren)) {
-        State = SawLeftParen;
-      }
+      else if (ProtoToken.is(tok::TokenKind::l_paren))
+        State = TokenState::LeftParen;
       break;
     }
   }
 
-  if (State == SawVoid && ProtoToken.is(tok::TokenKind::r_paren)) {
+  if (State == TokenState::Void && ProtoToken.is(tok::TokenKind::r_paren))
     removeVoidToken(VoidToken, Diagnostic);
-  }
 }
 
 void RedundantVoidArgCheck::removeVoidToken(Token VoidToken,
@@ -181,19 +228,17 @@ void RedundantVoidArgCheck::removeVoidToken(Token VoidToken,
 void RedundantVoidArgCheck::processTypedefNameDecl(
     const MatchFinder::MatchResult &Result,
     const TypedefNameDecl *TypedefName) {
-  if (protoTypeHasNoParms(TypedefName->getUnderlyingType())) {
+  if (protoTypeHasNoParms(TypedefName->getUnderlyingType()))
     removeVoidArgumentTokens(Result, TypedefName->getSourceRange(),
                              isa<TypedefDecl>(TypedefName) ? "typedef"
                                                            : "type alias");
-  }
 }
 
 void RedundantVoidArgCheck::processFieldDecl(
     const MatchFinder::MatchResult &Result, const FieldDecl *Member) {
-  if (protoTypeHasNoParms(Member->getType())) {
+  if (protoTypeHasNoParms(Member->getType()))
     removeVoidArgumentTokens(Result, Member->getSourceRange(),
                              "field declaration");
-  }
 }
 
 void RedundantVoidArgCheck::processVarDecl(
@@ -206,30 +251,27 @@ void RedundantVoidArgCheck::processVarDecl(
               .getLocWithOffset(-1);
       removeVoidArgumentTokens(Result, SourceRange(Begin, InitStart),
                                "variable declaration with initializer");
-    } else {
+    } else
       removeVoidArgumentTokens(Result, Var->getSourceRange(),
                                "variable declaration");
-    }
   }
 }
 
 void RedundantVoidArgCheck::processNamedCastExpr(
     const MatchFinder::MatchResult &Result, const CXXNamedCastExpr *NamedCast) {
-  if (protoTypeHasNoParms(NamedCast->getTypeAsWritten())) {
+  if (protoTypeHasNoParms(NamedCast->getTypeAsWritten()))
     removeVoidArgumentTokens(
         Result,
         NamedCast->getTypeInfoAsWritten()->getTypeLoc().getSourceRange(),
         "named cast");
-  }
 }
 
 void RedundantVoidArgCheck::processExplicitCastExpr(
     const MatchFinder::MatchResult &Result,
     const ExplicitCastExpr *ExplicitCast) {
-  if (protoTypeHasNoParms(ExplicitCast->getTypeAsWritten())) {
+  if (protoTypeHasNoParms(ExplicitCast->getTypeAsWritten()))
     removeVoidArgumentTokens(Result, ExplicitCast->getSourceRange(),
                              "cast expression");
-  }
 }
 
 void RedundantVoidArgCheck::processLambdaExpr(
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
index 15348f4b0552..89bf7f04f557 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
@@ -199,6 +199,7 @@ typedef void (function_ptr2)
 // CHECK-FIXES-NEXT: {{^    \);$}}
 
 // intentionally not LLVM style to check preservation of whitespace
+// clang-format off
 typedef
 void
 (
@@ -240,7 +241,7 @@ void
 // CHECK-FIXES-NOT:  {{[^ ]}}
 // CHECK-FIXES:      {{^\)$}}
 // CHECK-FIXES-NEXT: {{^;$}}
-
+// clang-format on
 
 void (gronk::*p1)(void);
 // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: {{.*}} in variable declaration
@@ -556,3 +557,38 @@ void f_testTemplate() {
   S_3<int>();
   g_3<int>();
 }
+
+#define return_t(T) T
+extern return_t(void) func(void);
+// CHECK-MESSAGES: :[[@LINE-1]]:28: warning: redundant void argument list in function declaration
+// CHECK-FIXES: extern return_t(void) func();
+
+return_t(void) func(void) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: redundant void argument list in function definition
+  // CHECK-FIXES: return_t(void) func() {
+  int a;
+  (void)a;
+}
+
+extern return_t(void) func(return_t(void) (*fp)(void));
+// CHECK-MESSAGES: :[[@LINE-1]]:49: warning: redundant void argument list in variable declaration
+// CHECK-FIXES: extern return_t(void) func(return_t(void) (*fp)());
+
+return_t(void) func(return_t(void) (*fp)(void)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:42: warning: redundant void argument list in variable declaration
+  // CHECK-FIXES: return_t(void) func(return_t(void) (*fp)()) {
+  int a;
+  (void)a;
+}
+
+extern return_t(return_t(void)) func2(return_t(return_t(void)) (*fp)(void));
+// CHECK-MESSAGES: :[[@LINE-1]]:70: warning: redundant void argument list in variable declaration
+// CHECK-FIXES: extern return_t(return_t(void)) func2(return_t(return_t(void)) (*fp)());
+
+return_t(return_t(void)) func2(return_t(return_t(void)) (*fp)(void)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:63: warning: redundant void argument list in variable declaration
+  // CHECK-FIXES: return_t(return_t(void)) func2(return_t(return_t(void)) (*fp)()) {
+  int a;
+  (void)a;
+}
+#undef return_t