[clang-tidy] misc-argument-comment support for gmock

Now for real. The use case supported previously is used by approximately nobody.
What's needed is support for matching argument comments in EXPECT_xxx calls to
the names of parameters of the mocked methods.

llvm-svn: 294193

1 files changed, 124 insertions, 45 deletions

diff --git a/clang-tools-extra/clang-tidy/misc/ArgumentCommentCheck.cpp b/clang-tools-extra/clang-tidy/misc/ArgumentCommentCheck.cpp
index 1861afcc6c7..cad112bfb67 100644
--- a/clang-tools-extra/clang-tidy/misc/ArgumentCommentCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/ArgumentCommentCheck.cpp
@@ -12,6 +12,7 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Token.h"
+#include "../utils/LexerUtils.h"
 
 using namespace clang::ast_matchers;
 
@@ -86,8 +87,26 @@ getCommentsInRange(ASTContext *Ctx, CharSourceRange Range) {
   return Comments;
 }
 
-bool ArgumentCommentCheck::isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params,
-                                        StringRef ArgName, unsigned ArgIndex) {
+static std::vector<std::pair<SourceLocation, StringRef>>
+getCommentsBeforeLoc(ASTContext *Ctx, SourceLocation Loc) {
+  std::vector<std::pair<SourceLocation, StringRef>> Comments;
+  while (Loc.isValid()) {
+    clang::Token Tok =
+        utils::lexer::getPreviousToken(*Ctx, Loc, /*SkipComments=*/false);
+    if (Tok.isNot(tok::comment))
+      break;
+    Loc = Tok.getLocation();
+    Comments.emplace_back(
+        Loc,
+        Lexer::getSourceText(CharSourceRange::getCharRange(
+                                 Loc, Loc.getLocWithOffset(Tok.getLength())),
+                             Ctx->getSourceManager(), Ctx->getLangOpts()));
+  }
+  return Comments;
+}
+
+static bool isLikelyTypo(llvm::ArrayRef<ParmVarDecl *> Params,
+                         StringRef ArgName, unsigned ArgIndex) {
   std::string ArgNameLowerStr = ArgName.lower();
   StringRef ArgNameLower = ArgNameLowerStr;
   // The threshold is arbitrary.
@@ -128,72 +147,129 @@ static bool sameName(StringRef InComment, StringRef InDecl, bool StrictMode) {
   return InComment.compare_lower(InDecl) == 0;
 }
 
+// This uses implementation details of MOCK_METHODx_ macros: for each mocked
+// method M it defines M() with appropriate signature and a method used to set
+// up expectations - gmock_M() - with each argument's type changed the
+// corresponding matcher. This function finds M by gmock_M.
+static const CXXMethodDecl *
+findMockedMethod(const CXXMethodDecl *ExpectMethod) {
+  if (!ExpectMethod->getNameInfo().getName().isIdentifier())
+    return nullptr;
+  StringRef Name = ExpectMethod->getName();
+  if (!Name.startswith("gmock_"))
+    return nullptr;
+  Name = Name.substr(strlen("gmock_"));
+
+  const DeclContext *Ctx = ExpectMethod->getDeclContext();
+  if (Ctx == nullptr || !Ctx->isRecord())
+    return nullptr;
+  for (const auto *D : Ctx->decls()) {
+    if (const auto *Method = dyn_cast<CXXMethodDecl>(D)) {
+      if (Method->getName() != Name)
+        continue;
+      // Sanity check the mocked method.
+      if (Method->getNextDeclInContext() == ExpectMethod &&
+          Method->getLocation().isMacroID() &&
+          Method->getNumParams() == ExpectMethod->getNumParams()) {
+        return Method;
+      }
+    }
+  }
+  return nullptr;
+}
+
+// For gmock expectation builder method (the target of the call generated by
+// `EXPECT_CALL(obj, Method(...))`) tries to find the real method being mocked
+// (returns nullptr, if the mock method doesn't override anything). For other
+// functions returns the function itself.
+static const FunctionDecl *resolveMocks(const FunctionDecl *Func) {
+  if (const auto *Method = dyn_cast<CXXMethodDecl>(Func)) {
+    if (const auto *MockedMethod = findMockedMethod(Method)) {
+      // If mocked method overrides the real one, we can use its parameter
+      // names, otherwise we're out of luck.
+      if (MockedMethod->size_overridden_methods() > 0) {
+        return *MockedMethod->begin_overridden_methods();
+      }
+      return nullptr;
+    }
+  }
+  return Func;
+}
+
 void ArgumentCommentCheck::checkCallArgs(ASTContext *Ctx,
-                                         const FunctionDecl *Callee,
+                                         const FunctionDecl *OriginalCallee,
                                          SourceLocation ArgBeginLoc,
                                          llvm::ArrayRef<const Expr *> Args) {
+  const FunctionDecl *Callee = resolveMocks(OriginalCallee);
+  if (!Callee)
+    return;
+
   Callee = Callee->getFirstDecl();
-  for (unsigned I = 0,
-                E = std::min<unsigned>(Args.size(), Callee->getNumParams());
-       I != E; ++I) {
+  unsigned NumArgs = std::min<unsigned>(Args.size(), Callee->getNumParams());
+  if (NumArgs == 0)
+    return;
+
+  auto makeFileCharRange = [Ctx](SourceLocation Begin, SourceLocation End) {
+    return Lexer::makeFileCharRange(CharSourceRange::getCharRange(Begin, End),
+                                    Ctx->getSourceManager(),
+                                    Ctx->getLangOpts());
+  };
+
+  for (unsigned I = 0; I < NumArgs; ++I) {
     const ParmVarDecl *PVD = Callee->getParamDecl(I);
     IdentifierInfo *II = PVD->getIdentifier();
     if (!II)
       continue;
     if (auto Template = Callee->getTemplateInstantiationPattern()) {
       // Don't warn on arguments for parameters instantiated from template
-      // parameter packs. If we find more arguments than the template definition
-      // has, it also means that they correspond to a parameter pack.
+      // parameter packs. If we find more arguments than the template
+      // definition has, it also means that they correspond to a parameter
+      // pack.
       if (Template->getNumParams() <= I ||
           Template->getParamDecl(I)->isParameterPack()) {
         continue;
       }
     }
 
-    CharSourceRange BeforeArgument = CharSourceRange::getCharRange(
-        I == 0 ? ArgBeginLoc : Args[I - 1]->getLocEnd(),
-        Args[I]->getLocStart());
-    BeforeArgument = Lexer::makeFileCharRange(
-        BeforeArgument, Ctx->getSourceManager(), Ctx->getLangOpts());
+    CharSourceRange BeforeArgument =
+        makeFileCharRange(ArgBeginLoc, Args[I]->getLocStart());
+    ArgBeginLoc = Args[I]->getLocEnd();
 
-    for (auto Comment : getCommentsInRange(Ctx, BeforeArgument)) {
+    std::vector<std::pair<SourceLocation, StringRef>> Comments;
+    if (BeforeArgument.isValid()) {
+      Comments = getCommentsInRange(Ctx, BeforeArgument);
+    } else {
+      // Fall back to parsing back from the start of the argument.
+      CharSourceRange ArgsRange = makeFileCharRange(
+          Args[I]->getLocStart(), Args[NumArgs - 1]->getLocEnd());
+      Comments = getCommentsBeforeLoc(Ctx, ArgsRange.getBegin());
+    }
+
+    for (auto Comment : Comments) {
       llvm::SmallVector<StringRef, 2> Matches;
-      if (IdentRE.match(Comment.second, &Matches)) {
-        if (!sameName(Matches[2], II->getName(), StrictMode)) {
-          {
-            DiagnosticBuilder Diag =
-                diag(Comment.first, "argument name '%0' in comment does not "
-                                    "match parameter name %1")
-                << Matches[2] << II;
-            if (isLikelyTypo(Callee->parameters(), Matches[2], I)) {
-              Diag << FixItHint::CreateReplacement(
-                  Comment.first,
-                  (Matches[1] + II->getName() + Matches[3]).str());
-            }
+      if (IdentRE.match(Comment.second, &Matches) &&
+          !sameName(Matches[2], II->getName(), StrictMode)) {
+        {
+          DiagnosticBuilder Diag =
+              diag(Comment.first, "argument name '%0' in comment does not "
+                                  "match parameter name %1")
+              << Matches[2] << II;
+          if (isLikelyTypo(Callee->parameters(), Matches[2], I)) {
+            Diag << FixItHint::CreateReplacement(
+                Comment.first, (Matches[1] + II->getName() + Matches[3]).str());
           }
-          diag(PVD->getLocation(), "%0 declared here", DiagnosticIDs::Note)
-              << II;
+        }
+        diag(PVD->getLocation(), "%0 declared here", DiagnosticIDs::Note) << II;
+        if (OriginalCallee != Callee) {
+          diag(OriginalCallee->getLocation(),
+               "actual callee (%0) is declared here", DiagnosticIDs::Note)
+              << OriginalCallee;
         }
       }
     }
   }
 }
 
-static const FunctionDecl *resolveMocks(const MatchFinder::MatchResult &Result,
-                                        const FunctionDecl *Func) {
-  if (auto *Method = dyn_cast<CXXMethodDecl>(Func)) {
-    if (Method->getLocation().isMacroID() &&
-        Lexer::getImmediateMacroName(Method->getLocation(),
-                                     *Result.SourceManager,
-                                     Result.Context->getLangOpts())
-            .contains("MOCK_METHOD") &&
-        Method->size_overridden_methods() != 0) {
-      Func = *Method->begin_overridden_methods();
-    }
-  }
-  return Func;
-}
-
 void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *E = Result.Nodes.getNodeAs<Expr>("expr");
   if (const auto *Call = dyn_cast<CallExpr>(E)) {
@@ -201,12 +277,15 @@ void ArgumentCommentCheck::check(const MatchFinder::MatchResult &Result) {
     if (!Callee)
       return;
 
-    Callee = resolveMocks(Result, Callee);
-
     checkCallArgs(Result.Context, Callee, Call->getCallee()->getLocEnd(),
                   llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()));
   } else {
     const auto *Construct = cast<CXXConstructExpr>(E);
+    if (Construct->getNumArgs() == 1 &&
+        Construct->getArg(0)->getSourceRange() == Construct->getSourceRange()) {
+      // Ignore implicit construction.
+      return;
+    }
     checkCallArgs(
         Result.Context, Construct->getConstructor(),
         Construct->getParenOrBraceRange().getBegin(),