[clang-tidy]: Google: new check 'google-upgrade-googletest-case'

[astrelni]

Summary

Introduce a new check to upgrade user code based on API changes in Googletest.

The check finds uses of old Googletest APIs with "case" in their name and replaces them with the new APIs named with "suite".

[Eugene.Zelenko]

I think will be good idea to replace upgrade with modernize to be consistent with similar checks in other module.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp

170

Function should be static instead on placed in anonymous namespace. See LLVM Coding Guidelines. Same for other places.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h

13

Please remove unnecessary empty line.

clang-tools-extra/docs/clang-tidy/checks/google-upgrade-googletest-case.rst

13 ↗

(On Diff #203424)

Please remove space before colon.

[astrelni]

Style updates.

[astrelni]

Fix space.

[lebedev.ri]

You need to upload the entire patch, not as compared to the previous patch.
Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.

[astrelni]

In D62977#1533384, @Eugene.Zelenko wrote:

I think will be good idea to replace upgrade with modernize to be consistent with similar checks in other module.

I will work on changing this in the next diff update, if your opinion holds. The context for currently using "upgrade" is:

a) The modernize module seems to contain checks that are geared at using more modern language features as opposed to API breaking changes. With some exceptions, they seem to be about changing from old to new practice while both ways remain valid code. The goal of this check is to take code that will break with future googletest API changes and turn it into working code, allowing users to upgrade the version of googletest they use. Maybe upgrade is not the best term, but I don't think modernize is a better fit.
b) The check is meant to be consistent with abseil-upgrade-duration-conversions, which is aimed to upgrade code before API breaking changes. So if we change this check, I would like to change the Abseil one as well. Maybe this would be better done together in a follow-up patch?

I don't feel very strongly, just thought it would be good to provide the context. Please let me know how you'd like me to proceed.

[astrelni]

Fix mistake not uploading full diff

[astrelni]

Fix diff issues again

[EricWF]

Generally this LGTM.
I'll take another pass after the comments are addressed.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp

19

Could you rename this to better represent what the diagnostic text is saying, rather than just being diagnostic text?
I think it'll help readability below.

Why not put this inside the unnamed namespace instead?

Same with the function below.

201

Instead of walking the tree, can't you ask if the Node is dependent in some way?

294

Maybe add an assertion for this comment?

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h

23

https all the things!

clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp

9 ↗

(On Diff #204318)

You probably don't have to test for the full message every time. but I don't know it matters either way.

[lebedev.ri]

In D62977#1540184, @lebedev.ri wrote:

Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.

I don't believe this question was answered.

[EricWF]

clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp

30 ↗

(On Diff #204318)

Just tried building this. I think you're missing an include for UpgradeGoogletestCaseCheck.

[aaron.ballman]

clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp

41–42 ↗

(On Diff #204318)

Please keep this sorted alphabetically.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp

20

Should this be spelled Google Test instead?

107–108

No need to register this unless in C++ mode.

[astrelni]

In D62977#1556346, @lebedev.ri wrote:

In D62977#1540184, @lebedev.ri wrote:

Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.

I don't believe this question was answered.

Sorry, the original comment got lost between me updating two patches as I noticed I did it wrong without seeing your comment.

Unfortunately there are no versioning macros in googletest, so I'm not sure how to keep this check from providing warnings if it is run while depending on a version that is too early. The new names are in master and will be part of an upcoming version 1.9 release. I have tried to update the documentation to clarify which version of googletest this intended for. Please let me know how you think we should proceed.

clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp

30 ↗

(On Diff #204318)

Thanks for catching this, was having issues in the diff.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp

19

I had a previous comment that llvm coding guidelines prefer static and like to keep anonymous namespaces as small as possible and only around type declarations.

20

Thanks for spotting this. There is some inconsistency within the project. The top of the github repository is named "Googletest" with directories being "googletest" instead of "google_test", but documentation does use "Google Test".

For now I have changed the message to use "Google Test", but have kept "googletest" in the check name. Let me know if you would prefer "GoogleTest" and "google-test" elsewhere as well.

201

I think it is a trade-off. Given where this gets called, what we are really looking to gather are all places that are inside a template that are not instantiation dependent. If we only use a check against instantiation dependent, then the hash set gets a lot bigger because it holds every matched location and is not filtered down to the ones inside templates. Let me know if you think I should explore that route instead.

[lebedev.ri]

In D62977#1559334, @astrelni wrote:

In D62977#1556346, @lebedev.ri wrote:

In D62977#1540184, @lebedev.ri wrote:

Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.

I don't believe this question was answered.

Sorry, the original comment got lost between me updating two patches as I noticed I did it wrong without seeing your comment.

Unfortunately there are no versioning macros in googletest, so I'm not sure how to keep this check from providing warnings if it is run while depending on a version that is too early. The new names are in master and will be part of an upcoming version 1.9 release. I have tried to update the documentation to clarify which version of googletest this intended for. Please let me know how you think we should proceed.

I'm not fully current on what can and can't be done in clang-tidy, but i suppose if the
check has matched something, then it must mean that the googletest headers were parsed.
Can you perhaps look in the AST that the new names are present?
E.g. the new macros, specified in getNewMacroName().

[astrelni]

In D62977#1559344, @lebedev.ri wrote:

In D62977#1559334, @astrelni wrote:

In D62977#1556346, @lebedev.ri wrote:

In D62977#1540184, @lebedev.ri wrote:

Without seeing the tests - what version checks does this have?
It shouldn't fire if the googletest version is the one before that rename.

I don't believe this question was answered.

Sorry, the original comment got lost between me updating two patches as I noticed I did it wrong without seeing your comment.

Unfortunately there are no versioning macros in googletest, so I'm not sure how to keep this check from providing warnings if it is run while depending on a version that is too early. The new names are in master and will be part of an upcoming version 1.9 release. I have tried to update the documentation to clarify which version of googletest this intended for. Please let me know how you think we should proceed.

I'm not fully current on what can and can't be done in clang-tidy, but i suppose if the
check has matched something, then it must mean that the googletest headers were parsed.
Can you perhaps look in the AST that the new names are present?
E.g. the new macros, specified in getNewMacroName().

Yes, makes sense. Let me know what you think of the approach I've added in the latest diff. I think it is sufficient to check for the existence of one of the new macros in the right file and one new method in each matched class.

[lebedev.ri]

It sounds correct, but i don't see accompanying test changes, so i can't be sure.

[astrelni]

In D62977#1559628, @lebedev.ri wrote:

It sounds correct, but i don't see accompanying test changes, so i can't be sure.

The tests as they are cover the positive case in that they would not show warning or fixes if we didn't find the replacements. The only way to test the negative is to make a second test with a second set of mock googletest headers that declare things in the v1.8 way. Is this what you would prefer?

[lebedev.ri]

In D62977#1559637, @astrelni wrote:

In D62977#1559628, @lebedev.ri wrote:

It sounds correct, but i don't see accompanying test changes, so i can't be sure.

The tests as they are cover the positive case in that they would not show warning or fixes if we didn't find the replacements.

Yep

In D62977#1559637, @astrelni wrote:

The only way to test the negative is to make a second test with a second set of mock googletest headers that declare things in the v1.8 way. Is this what you would prefer?

If that is what it takes to get the test coverage, i suppose so.

[astrelni]

In D62977#1559649, @lebedev.ri wrote:

In D62977#1559637, @astrelni wrote:

In D62977#1559628, @lebedev.ri wrote:

It sounds correct, but i don't see accompanying test changes, so i can't be sure.

The tests as they are cover the positive case in that they would not show warning or fixes if we didn't find the replacements.

Yep

In D62977#1559637, @astrelni wrote:

The only way to test the negative is to make a second test with a second set of mock googletest headers that declare things in the v1.8 way. Is this what you would prefer?

If that is what it takes to get the test coverage, i suppose so.

What do you think of this?

[lebedev.ri]

Looks reasonable. I did not review the check itself though.
Are test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp and test/clang-tidy/google-upgrade-googletest-case.cpp identical other than the included header and expected output?
I'd recommend to condense it into a single file, and just have two RUN lines each one checking different message prefixes

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h

34

Have you tried llvm::DenseSet instead?
This may not matter *here*, but std::unordered_set usually results in horrible perf.

clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp

9 ↗

(On Diff #206855)

Hm?

[astrelni]

In D62977#1560879, @lebedev.ri wrote:

Looks reasonable. I did not review the check itself though.
Are test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp and test/clang-tidy/google-upgrade-googletest-case.cpp identical other than the included header and expected output?
I'd recommend to condense it into a single file, and just have two RUN lines each one checking different message prefixes

The nosuite test was a strict subset. I've combined them with a few lines that needed to be turned via preprocessor branches. Thank you, I actually wasn't aware that these tests can have multiple RUN lines.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h

34

Thanks, I wasn't aware of what is available.

clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case-nosuite.cpp

9 ↗

(On Diff #206855)

I added a comment to better explain this in the combined test. check_clang_tidy.py asserts that there is at least one message, fix or note comment present in the file. If there isn't one, the script returns without even running the test. I couldn't see an option to pass that would turn of this check, please let me know if you are aware of one.

[EricWF]

I don't have any other comments.

LGTM.

[EricWF]

Committed in r367263. Thanks for the change, sorry about the delay.

I'll go watch the bots now.

[tsatsulin]

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp

83

The body of this method is the same as Ifdef. Please check.

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h

 //===--- UpgradeGoogletestCaseCheck.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_GOOGLE_UPGRADEGOOGLETESTCASECHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_UPGRADEGOOGLETESTCASECHECK_H
 
 #include "../ClangTidyCheck.h"
-
 #include <unordered_set>
 
 namespace clang {
 namespace tidy {
 namespace google {
 
 /// Finds uses of deprecated Googletest APIs with names containing "case" and
 /// replaces them with equivalent names containing "suite".
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/google-upgrade-googletest-case.html
 class UpgradeGoogletestCaseCheck : public ClangTidyCheck {
 public:
   UpgradeGoogletestCaseCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                            Preprocessor *ModuleExpanderPP) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   std::unordered_set<unsigned> MatchedTemplateLocations;
 };
 
 } // namespace google
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_GOOGLE_UPGRADEGOOGLETESTCASECHECK_H

clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp

 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace google {
-namespace {
 
-const llvm::StringRef CheckMessage =
+static const llvm::StringRef CheckMessage =
     "Googletest APIs named with 'case' are deprecated; use equivalent APIs "
     "named with 'suite'";
 
-llvm::Optional<llvm::StringRef> getNewMacroName(llvm::StringRef MacroName) {
+static llvm::Optional<llvm::StringRef>
+getNewMacroName(llvm::StringRef MacroName) {
   std::pair<llvm::StringRef, llvm::StringRef> ReplacementMap[] = {
       {"TYPED_TEST_CASE", "TYPED_TEST_SUITE"},
       {"TYPED_TEST_CASE_P", "TYPED_TEST_SUITE_P"},
       {"REGISTER_TYPED_TEST_CASE_P", "REGISTER_TYPED_TEST_SUITE_P"},
       {"INSTANTIATE_TYPED_TEST_CASE_P", "INSTANTIATE_TYPED_TEST_SUITE_P"},
       {"INSTANTIATE_TEST_CASE_P", "INSTANTIATE_TEST_SUITE_P"},
   };
 
   for (auto &Mapping : ReplacementMap) {
     if (MacroName == Mapping.first)
       return Mapping.second;
   }
 
   return llvm::None;
 }
 
+namespace {
+
 class UpgradeGoogletestCasePPCallback : public PPCallbacks {
 public:
   UpgradeGoogletestCasePPCallback(UpgradeGoogletestCaseCheck *Check,
                                   Preprocessor *PP)
       : Check(Check), PP(PP) {}
 
   void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
                     SourceRange Range, const MacroArgs *) override {

 private:
   enum class CheckAction { Warn, Rename };
 
   void MacroUsed(const clang::Token &MacroNameTok, const MacroDefinition &MD,
                  SourceLocation Loc, CheckAction Action) {
     std::string Name = PP->getSpelling(MacroNameTok);
 
     llvm::Optional<llvm::StringRef> Replacement = getNewMacroName(Name);
     if (!Replacement)
       return;
 
-    StringRef FileName = PP->getSourceManager().getFilename(
+    llvm::StringRef FileName = PP->getSourceManager().getFilename(
         MD.getMacroInfo()->getDefinitionLoc());
     if (!FileName.endswith("gtest/gtest-typed-test.h"))
       return;
 
     DiagnosticBuilder Diag = Check->diag(Loc, CheckMessage);
 
     if (Action == CheckAction::Rename)
       Diag << FixItHint::CreateReplacement(
           CharSourceRange::getTokenRange(Loc, Loc), *Replacement);
   }
 
   UpgradeGoogletestCaseCheck *Check;
   Preprocessor *PP;
 };
 
 } // namespace
 
 void UpgradeGoogletestCaseCheck::registerPPCallbacks(const SourceManager &,
                                                      Preprocessor *PP,
                                                      Preprocessor *) {
   PP->addPPCallbacks(
       llvm::make_unique<UpgradeGoogletestCasePPCallback>(this, PP));
 }
 
 void UpgradeGoogletestCaseCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
   auto LocationFilter =
       unless(isExpansionInFileMatching("gtest/gtest(-typed-test)?\\.h$"));
Finder->addMatcher(
           .bind("typeloc"),
       this);
   Finder->addMatcher(
       usingDecl(hasAnyUsingShadowDecl(hasTargetDecl(TestCaseTypeAlias)))
           .bind("using"),
       this);
 }
 
-namespace {
-
-llvm::StringRef getNewMethodName(llvm::StringRef CurrentName) {
+static llvm::StringRef getNewMethodName(llvm::StringRef CurrentName) {
   std::pair<llvm::StringRef, llvm::StringRef> ReplacementMap[] = {
       {"SetUpTestCase", "SetUpTestSuite"},
       {"TearDownTestCase", "TearDownTestSuite"},
       {"test_case_name", "test_suite_name"},
       {"OnTestCaseStart", "OnTestSuiteStart"},
       {"OnTestCaseEnd", "OnTestSuiteEnd"},
       {"current_test_case", "current_test_suite"},
       {"successful_test_case_count", "successful_test_suite_count"},
       {"failed_test_case_count", "failed_test_suite_count"},
       {"total_test_case_count", "total_test_suite_count"},
       {"test_case_to_run_count", "test_suite_to_run_count"},
       {"GetTestCase", "GetTestSuite"}};
 
   for (auto &Mapping : ReplacementMap) {
     if (CurrentName == Mapping.first)
       return Mapping.second;
   }
 
   llvm_unreachable("Unexpected function name");
 }
 
 template <typename NodeType>
-bool isInInstantiation(const NodeType &Node,
+static bool isInInstantiation(const NodeType &Node,
                               const MatchFinder::MatchResult &Result) {
   return !match(isInTemplateInstantiation(), Node, *Result.Context).empty();
 }
 
 template <typename NodeType>
-bool isInTemplate(const NodeType &Node,
+static bool isInTemplate(const NodeType &Node,
                          const MatchFinder::MatchResult &Result) {
   internal::Matcher<NodeType> IsInsideTemplate =
       hasAncestor(decl(anyOf(classTemplateDecl(), functionTemplateDecl())));
   return !match(IsInsideTemplate, Node, *Result.Context).empty();
 }
 
-bool derivedTypeHasReplacementMethod(const MatchFinder::MatchResult &Result,
-                                     llvm::StringRef ReplacementMethod) {
+static bool
+derivedTypeHasReplacementMethod(const MatchFinder::MatchResult &Result,
   const auto *Class = Result.Nodes.getNodeAs<CXXRecordDecl>("class");
   return !match(cxxRecordDecl(
                     unless(isExpansionInFileMatching(
                         "gtest/gtest(-typed-test)?\\.h$")),
                     hasMethod(cxxMethodDecl(hasName(ReplacementMethod)))),
                 *Class, *Result.Context)
               .empty();
 }
 
-CharSourceRange getAliasNameRange(const MatchFinder::MatchResult &Result) {
+static CharSourceRange
+getAliasNameRange(const MatchFinder::MatchResult &Result) {
   if (const auto *Using = Result.Nodes.getNodeAs<UsingDecl>("using")) {
     return CharSourceRange::getTokenRange(
         Using->getNameInfo().getSourceRange());
   }
   return CharSourceRange::getTokenRange(
       Result.Nodes.getNodeAs<TypeLoc>("typeloc")->getSourceRange());
 }
 
-} // namespace
-
 void UpgradeGoogletestCaseCheck::check(const MatchFinder::MatchResult &Result) {
   llvm::StringRef ReplacementText;
   CharSourceRange ReplacementRange;
   if (const auto *Method = Result.Nodes.getNodeAs<CXXMethodDecl>("method")) {
     ReplacementText = getNewMethodName(Method->getName());
 
     bool IsInInstantiation;
     bool IsInTemplate;
if (IsInTemplate) {
           ReplacementRange.getBegin().getRawEncoding());
     }
 
     if (!AddFix) {
       diag(ReplacementRange.getBegin(), CheckMessage);
       return;
     }
   } else {
     // This is a match for `TestCase` to `TestSuite` refactoring.
     ReplacementText = "TestSuite";
     ReplacementRange = getAliasNameRange(Result);
 
     // We do not need to keep track of template instantiations for this branch,
     // because we are matching a `TypeLoc` for the alias declaration. Templates
     // will only be instantiated with the true type name, `TestSuite`.
   }