[analyzer] Treat std::{move,forward} as casts in ExprMutationAnalyzer.

[shuaiwang]

Summary

This is a follow up of D52008 and should make the analyzer being able to handle perfect forwardings in real world cases where forwardings are done through multiple layers of function calls with std::forward.

Fixes PR38891

[shuaiwang]

@lebedev.ri could you help test whether this fully resolves PR38891? Thanks!

[lebedev.ri]

In D52120#1235479, @shuaiwang wrote:

@lebedev.ri could you help test whether this fully resolves PR38891? Thanks!

That is what i'm trying to do here :)
Still waiting for the build to finish...

[lebedev.ri]

Thank you for working on this!

In D52120#1235480, @lebedev.ri wrote:

In D52120#1235479, @shuaiwang wrote:

@lebedev.ri could you help test whether this fully resolves PR38891? Thanks!

That is what i'm trying to do here :)
Still waiting for the build to finish...

Ok, got the results.
Without that change, my check fires ~1k times.
With this change it only fires 572 times.
So this clearly changes things.
The entirety of the new build log:

buildlog.txt151 KBDownload

I see no mentions of emplace_back/push_back.
Though some of that is still false-positives (pointers, va_arg, callback lambdas passed as templated function param), but i'll file further bugs i guess.

As far i'm concerned, this is good.
But please wait for the actual code review.

Once again, thank you for working on this!

[lebedev.ri]

In D52120#1235515, @lebedev.ri wrote:

Though some of that is still false-positives (pointers, va_arg, callback lambdas passed as templated function param), but i'll file further bugs i guess.

Filed the ones that i can pinpoint, marked them as blockers of PR38890

[JonasToth]

Thank you very much for the good work from my side as well!

unittests/Analysis/ExprMutationAnalyzerTest.cpp

387 ↗

(On Diff #165578)

I am thinking about the correctness of the change.

If we have this case:

std::string s1 = "Hello World!";
std::string s2 = std::move(s1);

s1 gets mutated, but it looks like it would not be considered as mutation?

On the other hand

int i1 = 42;
int i2 = std::move(i1);

should resolve in a copy i1 and therefor not be a mutation.

Could you please add tests demonstrating this difference and the correct diagnostic detection for that.

Potentially interesting:

• Builtin types should be Copy-Only?, builtin arrays as expensive to move types for the completeness please as well
• Types with deleted move operations, should resolve in copy too
• Move-Only types like unique_ptr
• Types with both move and copy constructor (vector-like)
• and something with everything defaulted

These thoughts are just thinking and writing, I just wondered that moving a variable with std::move is not considered as mutation here, even though there are cases were it is actually a mutation.

[JonasToth]

unittests/Analysis/ExprMutationAnalyzerTest.cpp

387 ↗

(On Diff #165578)

Related: https://clang.llvm.org/extra/clang-tidy/checks/performance-move-const-arg.html

[shuaiwang]

unittests/Analysis/ExprMutationAnalyzerTest.cpp

387 ↗

(On Diff #165578)

Added these tests:

• std::move copy only type doesn't mutate
• std::move move only type does mutate
• std::move copiable & movable type does mutate
• std::move const copiable & movable type doesn't mutate

Didn't test array because array can't be assigned (so copy/move isn't relevant) and passing array to function can only be done via reference (passing by value results in array to pointer decay) so no copy/move there as well.

[shuaiwang]

Added more test cases around std::move

[JonasToth]

The last testcase i mentioned, after that LGTM

unittests/Analysis/ExprMutationAnalyzerTest.cpp

441 ↗

(On Diff #165684)

You could add a final test for

struct S {
  S();
  S(S s);
  S& operator(S s);
};

Note that the copy and assignment take by value, so should not resolve in a mutation by move.

Then i think the testsuite covers all potential variants

[shuaiwang]

Added test case with copy-ctor & assignment operator taking value as param.

cfe/trunk/lib/Analysis/ExprMutationAnalyzer.cpp

const Stmt *ExprMutationAnalyzer::findCastMutation(const Expr *Exp) {
   const auto Casts =
       match(findAll(castExpr(hasSourceExpression(equalsNode(Exp)),
                              anyOf(explicitCastExpr(hasDestinationType(
                                        nonConstReferenceType())),
                                    implicitCastExpr(hasImplicitDestinationType(
                                        nonConstReferenceType()))))
                         .bind(NodeID<Expr>::value)),
             Stm, Context);
-  return findExprMutation(Casts);
+  if (const Stmt *S = findExprMutation(Casts))
+    return S;
+  // Treat std::{move,forward} as cast.
+  const auto Calls =
+      match(findAll(callExpr(callee(namedDecl(
+                                 hasAnyName("::std::move", "::std::forward"))),
+                             hasArgument(0, equalsNode(Exp)))
+                        .bind("expr")),
+            Stm, Context);
+  return findExprMutation(Calls);
 }
 
 const Stmt *ExprMutationAnalyzer::findRangeLoopMutation(const Expr *Exp) {
   // If range for looping over 'Exp' with a non-const reference loop variable,
   // check all declRefExpr of the loop variable.
   const auto LoopVars =
       match(findAll(cxxForRangeStmt(
                 hasLoopVariable(varDecl(hasType(nonConstReferenceType()))

 
 const Stmt *ExprMutationAnalyzer::findFunctionArgMutation(const Expr *Exp) {
   const auto NonConstRefParam = forEachArgumentWithParam(
       equalsNode(Exp),
       parmVarDecl(hasType(nonConstReferenceType())).bind("parm"));
   const auto IsInstantiated = hasDeclaration(isInstantiated());
   const auto FuncDecl = hasDeclaration(functionDecl().bind("func"));
   const auto Matches = match(
-      findAll(expr(anyOf(callExpr(NonConstRefParam, IsInstantiated, FuncDecl),
+      findAll(expr(anyOf(callExpr(NonConstRefParam, IsInstantiated, FuncDecl,
+                                  unless(callee(namedDecl(hasAnyName(
+                                      "::std::move", "::std::forward"))))),
                          cxxConstructExpr(NonConstRefParam, IsInstantiated,
                                           FuncDecl)))
                   .bind(NodeID<Expr>::value)),
       Stm, Context);
   for (const auto &Nodes : Matches) {
     const auto *Exp = Nodes.getNodeAs<Expr>(NodeID<Expr>::value);
     const auto *Func = Nodes.getNodeAs<FunctionDecl>("func");
     if (!Func->getBody())

cfe/trunk/unittests/Analysis/ExprMutationAnalyzerTest.cpp

 
 std::string removeSpace(std::string s) {
   s.erase(std::remove_if(s.begin(), s.end(),
                          [](char c) { return std::isspace(c); }),
           s.end());
   return s;
 }
 
+const std::string StdRemoveReference =
+    "namespace std {"
+    "template<class T> struct remove_reference { typedef T type; };"
+    "template<class T> struct remove_reference<T&> { typedef T type; };"
+    "template<class T> struct remove_reference<T&&> { typedef T type; }; }";
+
+const std::string StdMove =
+    "namespace std {"
+    "template<class T> typename remove_reference<T>::type&& "
+    "move(T&& t) noexcept {"
+    "return static_cast<typename remove_reference<T>::type&&>(t); } }";
+
+const std::string StdForward =
+    "namespace std {"
+    "template<class T> T&& "
+    "forward(typename remove_reference<T>::type& t) noexcept { return t; }"
+    "template<class T> T&& "
+    "forward(typename remove_reference<T>::type&&) noexcept { return t; } }";
+
 } // namespace
 
 TEST(ExprMutationAnalyzerTest, Trivial) {
   const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
TEST(ExprMutationAnalyzerTest, ByConstRRefArgument) {
 
   AST = tooling::buildASTFromCode("void f() { struct A { A(); A(const A&&); }; "
                                   "A x; A y(static_cast<A&&>(x)); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, Move) {
-  // Technically almost the same as ByNonConstRRefArgument, just double checking
-  const auto AST = tooling::buildASTFromCode(
-      "namespace std {"
-      "template<class T> struct remove_reference { typedef T type; };"
-      "template<class T> struct remove_reference<T&> { typedef T type; };"
-      "template<class T> struct remove_reference<T&&> { typedef T type; };"
-      "template<class T> typename std::remove_reference<T>::type&& "
-      "move(T&& t) noexcept; }"
+  auto AST =
+      tooling::buildASTFromCode(StdRemoveReference + StdMove +
                                 "void f() { struct A {}; A x; std::move(x); }");
-  const auto Results =
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("std::move(x)"));
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      StdRemoveReference + StdMove +
+      "void f() { struct A {}; A x, y; std::move(x) = y; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("std::move(x) = y"));
+
+  AST = tooling::buildASTFromCode(StdRemoveReference + StdMove +
+                                  "void f() { int x, y; y = std::move(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      StdRemoveReference + StdMove +
+      "struct S { S(); S(const S&); S& operator=(const S&); };"
+      "void f() { S x, y; y = std::move(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST =
+      tooling::buildASTFromCode(StdRemoveReference + StdMove +
+                                "struct S { S(); S(S&&); S& operator=(S&&); };"
+                                "void f() { S x, y; y = std::move(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("y = std::move(x)"));
+
+  AST =
+      tooling::buildASTFromCode(StdRemoveReference + StdMove +
+                                "struct S { S(); S(const S&); S(S&&);"
+                                "S& operator=(const S&); S& operator=(S&&); };"
+                                "void f() { S x, y; y = std::move(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("y = std::move(x)"));
+
+  AST = tooling::buildASTFromCode(
+      StdRemoveReference + StdMove +
+      "struct S { S(); S(const S&); S(S&&);"
+      "S& operator=(const S&); S& operator=(S&&); };"
+      "void f() { const S x; S y; y = std::move(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(StdRemoveReference + StdMove +
+                                  "struct S { S(); S(S); S& operator=(S); };"
+                                  "void f() { S x, y; y = std::move(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      StdRemoveReference + StdMove +
+      "struct S{}; void f() { S x, y; y = std::move(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("y = std::move(x)"));
+
+  AST = tooling::buildASTFromCode(
+      StdRemoveReference + StdMove +
+      "struct S{}; void f() { const S x; S y; y = std::move(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, Forward) {
-  // Technically almost the same as ByNonConstRefArgument, just double checking
-  const auto AST = tooling::buildASTFromCode(
-      "namespace std {"
-      "template<class T> struct remove_reference { typedef T type; };"
-      "template<class T> struct remove_reference<T&> { typedef T type; };"
-      "template<class T> struct remove_reference<T&&> { typedef T type; };"
-      "template<class T> T&& "
-      "forward(typename std::remove_reference<T>::type&) noexcept;"
-      "template<class T> T&& "
-      "forward(typename std::remove_reference<T>::type&&) noexcept;"
+  auto AST = tooling::buildASTFromCode(
+      StdRemoveReference + StdForward +
       "void f() { struct A {}; A x; std::forward<A &>(x); }");
-  const auto Results =
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      StdRemoveReference + StdForward +
+      "void f() { struct A {}; A x, y; std::forward<A &>(x) = y; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()),
-              ElementsAre("std::forward<A &>(x)"));
+              ElementsAre("std::forward<A &>(x) = y"));
 }
 
 TEST(ExprMutationAnalyzerTest, CallUnresolved) {
   auto AST = tooling::buildASTFromCodeWithArgs(
       "template <class T> void f() { T x; g(x); }",
       {"-fno-delayed-template-parsing"});
   auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
TEST(ExprMutationAnalyzerTest, FollowFuncArgModified) {
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
 
   AST = tooling::buildASTFromCode(
       "struct S { template <class T> S(T&& t) : m(++t) { } int m; };"
       "void f() { int x; S s(x); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
+
+  AST = tooling::buildASTFromCode(
+      StdRemoveReference + StdForward +
+      "template <class... Args> void u(Args&...);"
+      "template <class... Args> void h(Args&&... args)"
+      "{ u(std::forward<Args>(args)...); }"
+      "template <class... Args> void g(Args&&... args)"
+      "{ h(std::forward<Args>(args)...); }"
+      "void f() { int x; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(x)"));
 }
 
 TEST(ExprMutationAnalyzerTest, FollowFuncArgNotModified) {
   auto AST = tooling::buildASTFromCode("template <class T> void g(T&&) {}"
                                        "void f() { int x; g(x); }");
   auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
TEST(ExprMutationAnalyzerTest, FollowFuncArgNotModified) {
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 
   AST = tooling::buildASTFromCode(
       "struct S { template <class T> S(T&& t) : m(t) { } int m; };"
       "void f() { int x; S s(x); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      StdRemoveReference + StdForward +
+      "template <class... Args> void u(Args...);"
+      "template <class... Args> void h(Args&&... args)"
+      "{ u(std::forward<Args>(args)...); }"
+      "template <class... Args> void g(Args&&... args)"
+      "{ h(std::forward<Args>(args)...); }"
+      "void f() { int x; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, ArrayElementModified) {
   const auto AST =
       tooling::buildASTFromCode("void f() { int x[2]; x[0] = 10; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x[0] = 10"));