Fix `performance-unnecessary-value-param` for template specialization

[Sockke]

Summary

The checker missed a check for parameter type of primary template of specialization template and this could cause build breakages.

[flx]

Thanks for improving this check!

clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp

148–149

Could you add a comment here why we're skipping the fix here?

clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

388

Should we apply the fixes or just issue the warning? For virtual methods we suppress the fix since we can't necessarily update all overrides of the method. Are template specializations always guaranteed to be in the same translation unit which would make this safe?

[Sockke]

Thanks for your review!

clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp

148–149

Could you add a comment here why we're skipping the fix here?

Specialization template may match the primary template again by getPreviousDecl. Skipping the fix to avoid repeated fixes for the primary template.

clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

388

Should we apply the fixes or just issue the warning? For virtual methods we suppress the fix since we can't necessarily update all overrides of the method. Are template specializations always guaranteed to be in the same translation unit which would make this safe?

Do you mean that specialization templates are defined in different translation units? If fixing one by one translation unit does have the problem, the readability-const-return-type also has such a problem. clang-tidy can not analyze across translation units, but the diagnosis and fix of it are separate, we can specify the complete compile_commands.json to avoid it.
I'm not sure whether this is reasonable, we may make a choice between clang-tidy's fault tolerance and advantages. What's your suggestion?

[Sockke]

Friendly ping.

[flx]

clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp

148–149

Thanks for the explanation. Would you mind adding this as code comment?

clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

388

Should we apply the fixes or just issue the warning? For virtual methods we suppress the fix since we can't necessarily update all overrides of the method. Are template specializations always guaranteed to be in the same translation unit which would make this safe?

Do you mean that specialization templates are defined in different translation units? If fixing one by one translation unit does have the problem, the readability-const-return-type also has such a problem. clang-tidy can not analyze across translation units, but the diagnosis and fix of it are separate, we can specify the complete compile_commands.json to avoid it.

I'm not sure how I understand how the compile command json file would help. I think std::hash (https://en.cppreference.com/w/cpp/utility/hash) is an example where you would specialize a type across translation units. If std::hash itself had the issue you wouldn't be able to modify it since it is an external library.

I'm not sure whether this is reasonable, we may make a choice between clang-tidy's fault tolerance and advantages. What's your suggestion?

Yeah, this is not an easy balance to strike. ClangTidy is used in many different contexts, in some where you would like to see aggressive fixes, in others where only the safest fixes should be applied.

From previous reviews and my own experience we should by default only provide safe fixes with low false positive rate. If you think its' not enough to only notify the user of an inefficiency in template specializations you could add an option to the check for fixing these occurrences.

This was done for example here to make a check find issues (including false positives) and report them: https://github.com/llvm/llvm-project/blob/765dd8b8a44cd9689c87c0433739f421b9871061/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp#L26

In summary, I would make this a warning only, but not issue the fix here.

[Sockke]

Removed the fix for the template.

[aaron.ballman]

It looks like precommit CI is failing:

Failed Tests (1):

Clang Tools :: clang-tidy/checkers/performance-unnecessary-value-param.cpp

[Sockke]

Added a release note.

[flx]

I'm still seeing build failures. Could you resolve them?

[Sockke]

rebased.

[Sockke]

I'm not quite sure why the test case is not passing on Windows, could you please check this problem? Thanks for your assistance. @flx

[flx]

I don't have access to Windows, perhaps rebase and try again? It looks like the template specialization related tests are failing there though.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

1

This will fix your Windows testing issue, I believe.

[Sockke]

Rebased and added '-fno-delayed-template-parsing' option for the test file.

[aaron.ballman]

LGTM aside from a very minor nit that can be corrected when landing.

clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp

143

No need for the extra parens.

[steakhal]

clang-tools-extra/docs/ReleaseNotes.rst

205–208

This is not at the expected alphabetical place.

clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp

auto Matches = match(
                decl(forEachDescendant(declRefExpr(
                    equalsNode(&DeclRef),
                    unless(hasAncestor(stmt(anyOf(forStmt(), cxxForRangeStmt(),
                                                  whileStmt(), doStmt())))))))),
       Decl, Context);
   return Matches.empty();
 }
 
-bool isExplicitTemplateSpecialization(const FunctionDecl &Function) {
-  if (const auto *SpecializationInfo = Function.getTemplateSpecializationInfo())
-    if (SpecializationInfo->getTemplateSpecializationKind() ==
-        TSK_ExplicitSpecialization)
-      return true;
-  if (const auto *Method = llvm::dyn_cast<CXXMethodDecl>(&Function))
-    if (Method->getTemplatedKind() == FunctionDecl::TK_MemberSpecialization &&
-        Method->getMemberSpecializationInfo()->isExplicitSpecialization())
-      return true;
-  return false;
-}
-
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       Inserter(Options.getLocalOrGlobal("IncludeStyle",
                                         utils::IncludeSorter::IS_LLVM),
                areDiagsSelfContained()),
auto Diag =
            "invocation%select{ but only used as a const reference|}0; consider "
            "making it a %select{const |}0reference")
       << IsConstQualified << paramNameOrIndex(Param->getName(), Index);
   // Do not propose fixes when:
   // 1. the ParmVarDecl is in a macro, since we cannot place them correctly
   // 2. the function is virtual as it might break overrides
   // 3. the function is referenced outside of a call expression within the
   //    compilation unit as the signature change could introduce build errors.
-  // 4. the function is an explicit template specialization.
+  // 4. the function is a primary template or an explicit template
+  // specialization.
   const auto *Method = llvm::dyn_cast<CXXMethodDecl>(Function);
   if (Param->getBeginLoc().isMacroID() || (Method && Method->isVirtual()) ||
       isReferencedOutsideOfCallExpr(*Function, *Result.Context) ||
-      isExplicitTemplateSpecialization(*Function))
+      (Function->getTemplatedKind() != FunctionDecl::TK_NonTemplate))
     return;
   for (const auto *FunctionDecl = Function; FunctionDecl != nullptr;
        FunctionDecl = FunctionDecl->getPreviousDecl()) {
     const auto &CurrentParam = *FunctionDecl->getParamDecl(Index);
     Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
                                                    *Result.Context);
     // The parameter of each declaration needs to be checked individually as to
     // whether it is const or not as constness can differ between definition and
     // declaration.
     if (!CurrentParam.getType().getCanonicalType().isConstQualified()) {
       if (llvm::Optional<FixItHint> Fix = utils::fixit::addQualifierToVarDecl(
               CurrentParam, *Result.Context, DeclSpec::TQ::TQ_const))
         Diag << *Fix;
     }

clang-tools-extra/docs/ReleaseNotes.rst

 - Fixed a false positive in :doc:`readability-non-const-parameter
   <clang-tidy/checks/readability-non-const-parameter>` when the parameter is
   referenced by an lvalue.
 
 - Expanded :doc:`readability-simplify-boolean-expr
   <clang-tidy/checks/readability-simplify-boolean-expr>` to simplify expressions
   using DeMorgan's Theorem.
 
+- Fixed a crash in :doc:`performance-unnecessary-value-param
+  <clang-tidy/checks/readability-suspicious-call-argument>` when the specialization
+  template has an unnecessary value paramter. Removed the fix for a template.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param-delayed.cpp

 struct PositiveConstValueConstructor {
   PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {}
   // CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy'
   // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {}
 };
 
 template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
   templateWithNonTemplatizedParameter(ExpensiveToCopyType(), ExpensiveToCopyType());
   templateWithNonTemplatizedParameter(ExpensiveToCopyType(), 5);
 }
 
 template <typename T> void negativeTemplateType(const T V) {

clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp

-// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t
+// RUN: %check_clang_tidy %s performance-unnecessary-value-param %t -- -- -fno-delayed-template-parsing
 
 // CHECK-FIXES: #include <utility>
 
 struct ExpensiveToCopyType {
   const ExpensiveToCopyType & constReference() const {
     return *this;
   }
   void nonConstMethod();

 struct PositiveConstValueConstructor {
   PositiveConstValueConstructor(const ExpensiveToCopyType ConstCopy) {}
   // CHECK-MESSAGES: [[@LINE-1]]:59: warning: the const qualified parameter 'ConstCopy'
   // CHECK-FIXES: PositiveConstValueConstructor(const ExpensiveToCopyType& ConstCopy) {}
 };
 
 template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType S, T V) {
   // CHECK-MESSAGES: [[@LINE-1]]:90: warning: the const qualified parameter 'S'
-  // CHECK-FIXES: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
+  // CHECK-FIXES-NOT: template <typename T> void templateWithNonTemplatizedParameter(const ExpensiveToCopyType& S, T V) {
 }
 
 void instantiated() {
   templateWithNonTemplatizedParameter(ExpensiveToCopyType(), ExpensiveToCopyType());
   templateWithNonTemplatizedParameter(ExpensiveToCopyType(), 5);
 }
 
 template <typename T> void negativeTemplateType(const T V) {

 }
 
 template<>
 void templateFunction<ExpensiveToCopyType>(ExpensiveToCopyType E) {
   // CHECK-MESSAGES: [[@LINE-1]]:64: warning: the parameter 'E' is copied
   // CHECK-FIXES: void templateFunction<ExpensiveToCopyType>(ExpensiveToCopyType E) {
   E.constReference();
 }
+
+template <class T>
+T templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: T templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return T();
+}
+
+template <>
+bool templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:57: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: bool templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return true;
+}
+
+template <>
+int templateSpecializationFunction(ExpensiveToCopyType E) {
+  // CHECK-MESSAGES: [[@LINE-1]]:56: warning: the parameter 'E' is copied
+  // CHECK-FIXES-NOT: int templateSpecializationFunction(const ExpensiveToCopyType& E) {
+  return 0;
+}