[analyzer] Handle forwarding reference better in ExprMutationAnalyzer.

Summary:
We used to treat an `Expr` mutated whenever it's passed as non-const
reference argument to a function. This results in false positives in
cases like this:
```
int x;
std::vector<int> v;
v.emplace_back(x); // `x` is passed as non-const reference to `emplace_back`
```
In theory the false positives can be suppressed with
`v.emplace_back(std::as_const(x))` but that's considered overly verbose,
inconsistent with existing code and spammy as diags.

This diff handles such cases by following into the function definition
and see whether the argument is mutated inside.

Reviewers: lebedev.ri, JonasToth, george.karpenkov

Subscribers: xazax.hun, szepet, a.sidorin, mikhail.ramalho, Szelethus, cfe-commits

Differential Revision: https://reviews.llvm.org/D52008

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@342271 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h b/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
index 2c4704ed1cdb51e78dc2a8d50637e0c7b9fd5b2d..42d5b29aa86b5c5915c4eb09458283183d3770d1 100644 (file)
--- a/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
+++ b/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h
@@ -17,6 +17,8 @@
 
 namespace clang {
 
+class FunctionParmMutationAnalyzer;
+
 /// Analyzes whether any mutative operations are applied to an expression within
 /// a given statement.
 class ExprMutationAnalyzer {
@@ -27,13 +29,13 @@ public:
   bool isMutated(const Decl *Dec) { return findDeclMutation(Dec) != nullptr; }
   bool isMutated(const Expr *Exp) { return findMutation(Exp) != nullptr; }
   const Stmt *findMutation(const Expr *Exp);
+  const Stmt *findDeclMutation(const Decl *Dec);
 
 private:
   bool isUnevaluated(const Expr *Exp);
 
   const Stmt *findExprMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
   const Stmt *findDeclMutation(ArrayRef<ast_matchers::BoundNodes> Matches);
-  const Stmt *findDeclMutation(const Decl *Dec);
 
   const Stmt *findDirectMutation(const Expr *Exp);
   const Stmt *findMemberMutation(const Expr *Exp);
@@ -41,12 +43,32 @@ private:
   const Stmt *findCastMutation(const Expr *Exp);
   const Stmt *findRangeLoopMutation(const Expr *Exp);
   const Stmt *findReferenceMutation(const Expr *Exp);
+  const Stmt *findFunctionArgMutation(const Expr *Exp);
 
   const Stmt &Stm;
   ASTContext &Context;
+  llvm::DenseMap<const FunctionDecl *,
+                 std::unique_ptr<FunctionParmMutationAnalyzer>>
+      FuncParmAnalyzer;
   llvm::DenseMap<const Expr *, const Stmt *> Results;
 };
 
+// A convenient wrapper around ExprMutationAnalyzer for analyzing function
+// params.
+class FunctionParmMutationAnalyzer {
+public:
+  FunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context);
+
+  bool isMutated(const ParmVarDecl *Parm) {
+    return findMutation(Parm) != nullptr;
+  }
+  const Stmt *findMutation(const ParmVarDecl *Parm);
+
+private:
+  ExprMutationAnalyzer BodyAnalyzer;
+  llvm::DenseMap<const ParmVarDecl *, const Stmt *> Results;
+};
+
 } // namespace clang
 
 #endif // LLVM_CLANG_ANALYSIS_ANALYSES_EXPRMUTATIONANALYZER_H

diff --git a/lib/Analysis/ExprMutationAnalyzer.cpp b/lib/Analysis/ExprMutationAnalyzer.cpp
index f4be35b3219716519d7f6d57576cb5baa8c8634b..678eeebab2debe6b15b069698978c230421998bb 100644 (file)
--- a/lib/Analysis/ExprMutationAnalyzer.cpp
+++ b/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -79,7 +79,8 @@ const Stmt *ExprMutationAnalyzer::findMutation(const Expr *Exp) {
                              &ExprMutationAnalyzer::findArrayElementMutation,
                              &ExprMutationAnalyzer::findCastMutation,
                              &ExprMutationAnalyzer::findRangeLoopMutation,
-                             &ExprMutationAnalyzer::findReferenceMutation}) {
+                             &ExprMutationAnalyzer::findReferenceMutation,
+                             &ExprMutationAnalyzer::findFunctionArgMutation}) {
     if (const Stmt *S = (this->*Finder)(Exp))
       return Results[Exp] = S;
   }
@@ -192,10 +193,15 @@ const Stmt *ExprMutationAnalyzer::findDirectMutation(const Expr *Exp) {
 
   // Used as non-const-ref argument when calling a function.
   // An argument is assumed to be non-const-ref when the function is unresolved.
+  // Instantiated template functions are not handled here but in
+  // findFunctionArgMutation which has additional smarts for handling forwarding
+  // references.
   const auto NonConstRefParam = forEachArgumentWithParam(
       equalsNode(Exp), parmVarDecl(hasType(nonConstReferenceType())));
+  const auto NotInstantiated = unless(hasDeclaration(isInstantiated()));
   const auto AsNonConstRefArg = anyOf(
-      callExpr(NonConstRefParam), cxxConstructExpr(NonConstRefParam),
+      callExpr(NonConstRefParam, NotInstantiated),
+      cxxConstructExpr(NonConstRefParam, NotInstantiated),
       callExpr(callee(expr(anyOf(unresolvedLookupExpr(), unresolvedMemberExpr(),
                                  cxxDependentScopeMemberExpr(),
                                  hasType(templateTypeParmType())))),
@@ -305,4 +311,82 @@ const Stmt *ExprMutationAnalyzer::findReferenceMutation(const Expr *Exp) {
   return findDeclMutation(Refs);
 }
 
+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),
+                         cxxConstructExpr(NonConstRefParam, IsInstantiated,
+                                          FuncDecl)))
+                  .bind("expr")),
+      Stm, Context);
+  for (const auto &Nodes : Matches) {
+    const auto *Exp = Nodes.getNodeAs<Expr>("expr");
+    const auto *Func = Nodes.getNodeAs<FunctionDecl>("func");
+    if (!Func->getBody())
+      return Exp;
+
+    const auto *Parm = Nodes.getNodeAs<ParmVarDecl>("parm");
+    const ArrayRef<ParmVarDecl *> AllParams =
+        Func->getPrimaryTemplate()->getTemplatedDecl()->parameters();
+    QualType ParmType =
+        AllParams[std::min<size_t>(Parm->getFunctionScopeIndex(),
+                                   AllParams.size() - 1)]
+            ->getType();
+    if (const auto *T = ParmType->getAs<PackExpansionType>())
+      ParmType = T->getPattern();
+
+    // If param type is forwarding reference, follow into the function
+    // definition and see whether the param is mutated inside.
+    if (const auto *RefType = ParmType->getAs<RValueReferenceType>()) {
+      if (!RefType->getPointeeType().getQualifiers() &&
+          RefType->getPointeeType()->getAs<TemplateTypeParmType>()) {
+        std::unique_ptr<FunctionParmMutationAnalyzer> &Analyzer =
+            FuncParmAnalyzer[Func];
+        if (!Analyzer)
+          Analyzer.reset(new FunctionParmMutationAnalyzer(*Func, Context));
+        if (Analyzer->findMutation(Parm))
+          return Exp;
+        continue;
+      }
+    }
+    // Not forwarding reference.
+    return Exp;
+  }
+  return nullptr;
+}
+
+FunctionParmMutationAnalyzer::FunctionParmMutationAnalyzer(
+    const FunctionDecl &Func, ASTContext &Context)
+    : BodyAnalyzer(*Func.getBody(), Context) {
+  if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(&Func)) {
+    // CXXCtorInitializer might also mutate Param but they're not part of
+    // function body, check them eagerly here since they're typically trivial.
+    for (const CXXCtorInitializer *Init : Ctor->inits()) {
+      ExprMutationAnalyzer InitAnalyzer(*Init->getInit(), Context);
+      for (const ParmVarDecl *Parm : Ctor->parameters()) {
+        if (Results.find(Parm) != Results.end())
+          continue;
+        if (const Stmt *S = InitAnalyzer.findDeclMutation(Parm))
+          Results[Parm] = S;
+      }
+    }
+  }
+}
+
+const Stmt *
+FunctionParmMutationAnalyzer::findMutation(const ParmVarDecl *Parm) {
+  const auto Memoized = Results.find(Parm);
+  if (Memoized != Results.end())
+    return Memoized->second;
+
+  if (const Stmt *S = BodyAnalyzer.findDeclMutation(Parm))
+    return Results[Parm] = S;
+
+  return Results[Parm] = nullptr;
+}
+
 } // namespace clang

diff --git a/unittests/Analysis/ExprMutationAnalyzerTest.cpp b/unittests/Analysis/ExprMutationAnalyzerTest.cpp
index f8b70565dc79a0f329f92cbfb5a4b686d736b9c5..318fb0d82aecef47896c0fa553da73ca234fb95d 100644 (file)
--- a/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ b/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -595,6 +595,96 @@ TEST(ExprMutationAnalyzerTest, FollowConditionalRefNotModified) {
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
+TEST(ExprMutationAnalyzerTest, FollowFuncArgModified) {
+  auto AST =
+      tooling::buildASTFromCode("template <class T> void g(T&& t) { t = 10; }"
+                                "void f() { int x; g(x); }");
+  auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(x)"));
+
+  AST = tooling::buildASTFromCode(
+      "void h(int&);"
+      "template <class... Args> void g(Args&&... args) { h(args...); }"
+      "void f() { int x; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(x)"));
+
+  AST = tooling::buildASTFromCode(
+      "void h(int&, int);"
+      "template <class... Args> void g(Args&&... args) { h(args...); }"
+      "void f() { int x, y; g(x, y); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(x, y)"));
+  Results = match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void h(int, int&);"
+      "template <class... Args> void g(Args&&... args) { h(args...); }"
+      "void f() { int x, y; g(y, x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(y, x)"));
+  Results = match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "struct S { template <class T> S(T&& t) { t = 10; } };"
+      "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(
+      "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"));
+}
+
+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()));
+
+  AST = tooling::buildASTFromCode("template <class T> void g(T&& t) { t; }"
+                                  "void f() { int x; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST =
+      tooling::buildASTFromCode("template <class... Args> void g(Args&&...) {}"
+                                "void f() { int x; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST =
+      tooling::buildASTFromCode("template <class... Args> void g(Args&&...) {}"
+                                "void f() { int y, x; g(y, x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void h(int, int&);"
+      "template <class... Args> void g(Args&&... args) { h(args...); }"
+      "void f() { int x, y; g(x, y); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "struct S { template <class T> S(T&& t) { t; } };"
+      "void f() { int x; S s(x); }");
+  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()));
+}
+
 TEST(ExprMutationAnalyzerTest, ArrayElementModified) {
   const auto AST =
       tooling::buildASTFromCode("void f() { int x[2]; x[0] = 10; }");