[clang-tidy] Fix false positive issue in performance-unnecessary-value-param for arguments being moved in the function body.

[sukraat91]

Summary

This patch solves a bug (https://bugs.llvm.org/show_bug.cgi?id=44598) that was filed for a false positive in performance-unnecessary-value-param. This was in the case of template functions, where the argument was an expensive to copy type argument and the check would convert the argument to a const T&, even if it was being moved in the function body.

For example, this sort of code -

#include <string>
template<class T>
static T* Create(std::string s) {

return new T(std::move(s));

}

Leads to the check converting the argument to a const std::string&, even when it is being moved in the body. We saw the same behavior in my company codebase, where there were many false positives being reported.

We ran the modified check based on this submission, on tens of thousands of files to see those false positives not being reported any more. The modifications to the checker are -

1. For an expensive to copy type, before checking it is const qualified, check to see if it is being moved in the function body.

2. The argument and the AST context are passed on to a helper function. The function uses a matcher to check whether the argument is part of a std::move() call in the function body.

3. If true then ignore.

I am submitting the patch for review, along with new regression tests to verify that the check is ok if it sees an expensive to copy argument being moved, and does not recommend to change it to const T&. I have most recently applied the patch to LLVM master as of 09/10/2020, with no issues in build and all tests passing (using make -j10 check-clang-tools).

[sukraat91]

Changed the case for the first character in variable paramName from lowercase to uppercase, to pass pre-merge checks.

[sukraat91]

Revert to older commit.

[sukraat91]

Updated patch to contain changes to variable case from paramName to ParamName.

[aaron.ballman]

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

66

Can Context be const ASTContext &?

74

This may be a bigger issue with hasName() as this strikes me as possibly being a bug. I would expect hasName("") to return false for any AST node which has a nonempty name, and true for any AST node without a name.

77

We don't typically use top-level const in the project, so you can drop that, and you should only use auto when the type is spelled out in the initialization (or is impossible to spell, or strikingly obvious from context like iterators), so you should spell that type out in this case.

[riccibruno]

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

74

When I was looking at the hasName matcher I was surprised that this is not the case (see getNodeName in ASTMatchers/ASTMatchersInternal.cpp). For example an unnamed enumeration can be matched with (anonymous enum) (which should be unnamed instead). For an other example a constructor can be matched with the name of the class despite the fact that the constructor is formally unnamed (because DeclarationName::getDeclName and NamedDecl::printName are used).

I think that the hasName matcher is mixing two different concepts: the formal name of the AST node and the name for diagnostic purposes. One possible fix would be to add a matcher hasFormalName which would match the name as per the specification, and then modify the hasName matcher to use the name for diagnostic purposes (without the extra location information).

Not hard-coding the logic in getNodeName would have the additional benefit of being more consistent with the use of anonymous/unnamed terminology.

[sukraat91]

Address comments to remove top level const auto in the helper function to check if the argument is being moved, and replace it with its type spelled out.

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

66

Unfortunately, no. This is because match() does not have an overload for const ASTContext&.

[aaron.ballman]

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

66

I kind of wondered if that was the case, oh well. Thank you for checking!

74

+1, I think that's the better fix to make here. I don't have the impression that matching names for diagnostic purposes was really intended for this API (I don't recall discussions about it). Based on that, I think hasName() should probably be the one that handles the identifier used by the AST node and we can add hasDiagnosticName() to handle matching things like anonymous enums by name when we find there's a need for it.

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

   return false;
 }
 
+bool isPassedToStdMove(const ParmVarDecl &Param, ASTContext &Context) {
+  // Check if the parameter has a name, in case of functions like -
+  // void func(const ExpensiveToCopyType)
+  // {
+  //
+  // }
+  // The function having an empty body will still have a FunctionDecl and its
+  // parmVarDecl picked up by this checker. It will be an empty string and will
+  // lead to an assertion failure when using hasName(std::string) being used
+  // in the matcher below. If empty then exit indicating no move calls present
+  // for the function argument being examined.
+  llvm::StringRef ParamName = Param.getName();
+
+  if (ParamName.empty()) {
+    return false;
+  }
+  auto Matches = match(
+      callExpr(
+          callee(functionDecl(hasName("::std::move"))), argumentCountIs(1),
+          hasArgument(0, declRefExpr(to(parmVarDecl(hasName(ParamName)))))),
+      Context);
+
+  return !Matches.empty();
+}
+
 } // namespace
 
 UnnecessaryValueParamCheck::UnnecessaryValueParamCheck(

   if (Analyzer.isMutated(Param))
     return;
 
+  if (isPassedToStdMove(*Param, *Result.Context))
+    return;
+
   const bool IsConstQualified =
       Param->getType().getCanonicalType().isConstQualified();
 

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

   const_iterator end() const;
 };
 
+// Mocking std::move() and std::remove_reference<T> (since move() relies on that)
+// Since the regression tests run with -nostdinc++, standard library utilities have
+// to be mocked. Over here the mocking is required for tests that have function arguments
+// being moved.
+namespace std {
+template <typename>
+struct remove_reference;
+
+template <typename _Tp>
+struct remove_reference {
+  typedef _Tp type;
+};
+
+template <typename _Tp>
+struct remove_reference<_Tp &> {
+  typedef _Tp type;
+};
+
+template <typename _Tp>
+struct remove_reference<_Tp &&> {
+  typedef _Tp type;
+};
+
+template <typename _Tp>
+constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) {
+  return static_cast<typename std::remove_reference<_Tp>::type &&>(__t);
+}
+
+} // namespace std
+
 // This class simulates std::pair<>. It is trivially copy constructible
 // and trivially destructible, but not trivially copy assignable.
 class SomewhatTrivial {

   ~ExpensiveMovableType();
 };
 
+template <typename Arg>
+struct UsesExpensiveToCopyType {
+  Arg arg;
+  ExpensiveToCopyType expensiveType;
+
+  UsesExpensiveToCopyType() = default;
+  UsesExpensiveToCopyType(ExpensiveToCopyType eType) : expensiveType{std::move(eType)} {}
+  UsesExpensiveToCopyType(ExpensiveToCopyType eType, Arg t) : expensiveType{std::move(eType)}, arg{std::move(t)} {}
+};
+
 void positiveExpensiveConstValue(const ExpensiveToCopyType Obj);
 // CHECK-FIXES: void positiveExpensiveConstValue(const ExpensiveToCopyType& Obj);
 void positiveExpensiveConstValue(const ExpensiveToCopyType Obj) {

   Obj.nonConstMethod();
 }
 
+void negativeNoConstRefSinceMoved(ExpensiveToCopyType arg) {
+  auto F = std::move(arg);
+}
+
+template <typename T>
+T *negativeNoConstRefSinceTypeMoved(ExpensiveToCopyType t) {
+  return new T(std::move(t));
+}
+
+template <typename T>
+UsesExpensiveToCopyType<T> negativeCreate(ExpensiveToCopyType eType) {
+  return UsesExpensiveToCopyType<T>(std::move(eType));
+}
+
 struct PositiveValueUnusedConstructor {
   PositiveValueUnusedConstructor(ExpensiveToCopyType Copy) {}
   // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'Copy'