[clang-tidy] Don't offer partial fix-its for `modernize-pass-by-value`

[avogelsgesang]

Summary

This commit improves the fix-its of modernize-pass-by-value by
no longer proposing partial fixes. In the presence of using/typedef,
we failed to rewrite the function signature but still adjusted the
function body. This led to incorrect, partial fix-its. Instead, the
check now simply doesn't offer any fixes at all in such a situation

[avogelsgesang]

not sure whom to add as a reviewer. According to git log this check wasn't changed for a while...
Adding @alexfh based on CODE_OWNERS.TXT

[avogelsgesang]

@ymandel, @whisperity, @aaron.ballman could one of you review this/point me in the direction of a good reviewer for this change?
(Sorry for the spam - I am new to the LLVM project, and I guess I still have to learn how to navigate Phabricator/find the correct reviewers for my changes)

[aaron.ballman]

In D112648#3132157, @avogelsgesang wrote:

@ymandel, @whisperity, @aaron.ballman could one of you review this/point me in the direction of a good reviewer for this change?
(Sorry for the spam - I am new to the LLVM project, and I guess I still have to learn how to navigate Phabricator/find the correct reviewers for my changes)

No worries; this is not spam, it was super handy for raising awareness!

I've added a few more reviewers who can hopefully help. I'll try to take a look once I get the chance, but I'm pretty swamped currently with C standards meetings this week. Also, the LLVM dev conference is this week, so reviewers may be hard to come by. Thank you for your patience though!

[aaron.ballman]

Thanks for your patience!

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

198

Please spell out this type (per our usual coding conventions). The use on the next line is fine because the type name is spelled out in the initialization.

211–221

I don't think we should take these changes, unless they're strictly necessary for program correctness. The rule of thumb in clang-tidy for fix-its is that we do not make much attempt to have pretty formatting from the fix (the fix needs to be correct, but doesn't need to be formatted); the user can run clang-format after applying fixes. Someday, we'd like to consume clang-format as a library and automatically reformat the code after applying fixes instead of trying to catch every formatting concern in each individual check.

212–216

Please spell out the types.

218–220

(Style guideline nit)

[avogelsgesang]

Address review comments

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

198

Thanks for the hint. I wasn't aware of that convention. I fixed it here.

Should I also fix the other violations in this file?

E.g., a couple of lines above, we have

auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move");

which I guess should be

clang::DiagnosticBuilder Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move");

211–221

sounds good. Removed the whitespace related parts from this patch

212–216

no longer applicable after removing the whitespace related part of this patch

218–220

no longer applicable after removing the whitespace related part of this patch

[aaron.ballman]

LGTM aside from a small commenting nit.

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

194

198

Thanks for the hint. I wasn't aware of that convention.

No problem! In case you haven't seen it, we document our coding style here: https://llvm.org/docs/CodingStandards.html

Should I also fix the other violations in this file?

If you want to, you can do it as an NFC change separate from these changes, but I certainly don't insist.

[avogelsgesang]

Add missing dot to comment

[avogelsgesang]

@aaron.ballman can you please commit this on my behalf? I don't have commit access

[aaron.ballman]

In D112648#3165302, @avogelsgesang wrote:

@aaron.ballman can you please commit this on my behalf? I don't have commit access

OMG, I'm sorry for missing this note! Yes, I'm happy to commit on your behalf. What name and email address would you like me to use for patch attribution?

[avogelsgesang]

What name and email address would you like me to use for patch attribution?

Adrian Vogelsgesang, [e-mail]

[aaron.ballman]

Thanks for the fix and your patience with getting it landed! I've commit on your behalf in 5d3b8956e83484ff7234dda5e939abbdc9bd2c88

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

void PassByValueCheck::check(const MatchFinder::MatchResult &Result) {
   // If the parameter is trivial to copy, don't move it. Moving a trivially
   // copyable type will cause a problem with performance-move-const-arg
   if (ParamDecl->getType().getNonReferenceType().isTriviallyCopyableType(
           *Result.Context))
     return;
 
   auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move");
 
-  // Iterate over all declarations of the constructor.
+  // If we received a `const&` type, we need to rewrite the function
+  // declarations.
+  if (ParamDecl->getType()->isLValueReferenceType()) {
+    // Check if we can succesfully rewrite all declarations of the constructor.
     for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
-    auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
-    auto RefTL = ParamTL.getAs<ReferenceTypeLoc>();
-
-    // Do not replace if it is already a value, skip.
-    if (RefTL.isNull())
-      continue;
+      TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+      ReferenceTypeLoc RefTL = ParamTL.getAs<ReferenceTypeLoc>();
+      if (RefTL.isNull()) {
+        // We cannot rewrite this instance. The type is probably hidden behind
+        // some `typedef`. Do not offer a fix-it in this case.
+        return;
+      }
+    }
+    // Rewrite all declarations.
+    for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+      TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+      ReferenceTypeLoc RefTL = ParamTL.getAs<ReferenceTypeLoc>();
 
       TypeLoc ValueTL = RefTL.getPointeeLoc();
-    auto TypeRange = CharSourceRange::getTokenRange(ParmDecl->getBeginLoc(),
-                                                    ParamTL.getEndLoc());
-    std::string ValueStr = Lexer::getSourceText(CharSourceRange::getTokenRange(
-                                                    ValueTL.getSourceRange()),
-                                                SM, getLangOpts())
-                               .str();
+      CharSourceRange TypeRange = CharSourceRange::getTokenRange(
+          ParmDecl->getBeginLoc(), ParamTL.getEndLoc());
+      std::string ValueStr =
+          Lexer::getSourceText(
+              CharSourceRange::getTokenRange(ValueTL.getSourceRange()), SM,
+              getLangOpts())
       ValueStr += ' ';
       Diag << FixItHint::CreateReplacement(TypeRange, ValueStr);
     }
+  }
 
   // Use std::move in the initialization list.
   Diag << FixItHint::CreateInsertion(Initializer->getRParenLoc(), ")")
        << FixItHint::CreateInsertion(
               Initializer->getLParenLoc().getLocWithOffset(1), "std::move(")
        << Inserter.createIncludeInsertion(
               Result.SourceManager->getFileID(Initializer->getSourceLocation()),
               "<utility>");
 }
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang

clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp

 struct T {
   T(array<int, 10> a) : a_(a) {}
   // CHECK-FIXES: T(array<int, 10> a) : a_(a) {}
   array<int, 10> a_;
 };
 
 struct U {
   U(const POD &M) : M(M) {}
+  // CHECK-FIXES: U(const POD &M) : M(M) {}
   POD M;
 };
+
+// The rewrite can't look through `typedefs` and `using`.
+// Test that we don't partially rewrite one decl without rewriting the other.
+using MovableConstRef = const Movable &;
+struct V {
+  V(MovableConstRef M);
+  // CHECK-FIXES: V(MovableConstRef M);
+  Movable M;
+};
+V::V(const Movable &M) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: V::V(const Movable &M) : M(M) {}