[clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[shuaiwang]

Summary

• If a function is unresolved, assume it mutates its arguments
• Follow unresolved member expressions for nested mutations

[JonasToth]

Please add a test for the same usecase as in Sema.

template <typename T>
struct SizeIndicator {
  constexpr int value = 8;
};
template <>
struct SizeIndicator<int> {
  constexpr int value = 4;
};
template <typename Foo>
void fooFunction() {
  char Characters[SizeIndicator<Foo>::value];
  void ArrayToPointerDecay(Characters);
}

template <>
void fooFunction<int>() {
  char Character[SizeIndicator<int>::value];
  void ArrayToPointerDecay(Characters);
}

In both cases the mutation must be detected. I assume the function specialization is diagnosed correctly, because everything is resolved.

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

454

Is there already a test for a method from a templated type?

E.g.

template <typename T>
struct Foo {
  T x;
  Foo() { int local_int; x(local_int); }
};

One can not say that local_int could be const or not.

454

x.method(local_int); would be interesting, just for security, given that the first case is an operator overload and the second one a classical method call.

[shuaiwang]

• Fix a few cases overlooked previously
• More test cases

[JonasToth]

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

117

I think you could add another test with X<T> x (if you don't have it already and I overlooked them)

309

I think we are missing tests for non-type template paramters (template <size_t N>). They should behave the same. But the following case would not be a mutation:

void non_mutating(const char* Array, int size) { /* Foo */ }
template <int N>
struct ArrayLike {
  char* data[N]; // Initialized to something
  void SomeFunction() {
    non_mutating(data, N);
  }
};

The difference between the 'normal' and non-type templates would be, that N is not mutatable at all and the semantics is clear (builtin integer-like type).

If the current implementation would not figure that out, you can just add a test for it and assume a mutation. Handling non-type templates later is absolutly ok.

394

Out of curiosity: Why is the result with y different from the result for x? Both time x is mutated and g() mutates them.

[shuaiwang]

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

394

This is ultimately caused by not handling pointers yet.
As soon as the address of an object is taken we assume the object is mutated.
e.g.

void f(const int*);
void g() {
  int x;
  f(&x); // <-- address of x taken, assume mutation
  int y[10];
  f(y); // <-- address of y taken, assume mutation
}

And in all such cases the "mutated by" expression is the expression that takes the address.

So back in this case, g(x) mutates x because we're assuming g mutates its argument through non-const reference. Note that the declared g might not be the one actually being called because of overload resolution, there could be another void g(char(&)[8])
While for g(y) we know it's calling the void g(char*) so there's an array to pointer decay, and the decay is the point we assumed mutation not the function call.

[shuaiwang]

More test cases

[shuaiwang]

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

309

We have to assume data is mutated here as well. I'll add a test case for this.

void g(const char*, int); // <-- doesn't mutate
void g(char(&)[8], int); // <-- do mutate

template <int N>
void f() {
    char data[N];
    g(data, N); // <-- we don't know which `g` will be called yet
}

void h() {
    f<8>(); // <-- f calls g(char(&)[8], int) internally
    f<9>(); // <-- f calls g(const char*, int) internally
}

[shuaiwang]

Test case with non-type template

[JonasToth]

I see, thank you for the clarification :)

Am 15.08.2018 um 19:25 schrieb Shuai Wang via Phabricator:

shuaiwang added inline comments.

Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:410
+ match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+ EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y"));

+}

JonasToth wrote:

Out of curiosity: Why is the result with y different from the result for x? Both time x is mutated and g() mutates them.

This is ultimately caused by not handling pointers yet.
As soon as the address of an object is taken we assume the object is mutated.
e.g.

void f(const int*);
void g() {
  int x;
  f(&x); // <-- address of x taken, assume mutation
  int y[10];
  f(y); // <-- address of y taken, assume mutation
}

And in all such cases the "mutated by" expression is the expression that takes the address.

So back in this case, g(x) mutates x because we're assuming g mutates its argument through non-const reference. Note that the declared g might not be the one actually being called because of overload resolution, there could be another void g(char(&)[8])
While for g(y) we know it's calling the void g(char*) so there's an array to pointer decay, and the decay is the point we assumed mutation not the function call.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

[JonasToth]

You are totally right.

Am 16.08.2018 um 02:41 schrieb Shuai Wang via Phabricator:

shuaiwang added inline comments.

Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:309

+TEST(ExprMutationAnalyzerTest, CallUnresolved) {

+ auto AST =

JonasToth wrote:

I think we are missing tests for non-type template paramters (template <size_t N>). They should behave the same. But the following case would not be a mutation:

void non_mutating(const char* Array, int size) { /* Foo */ }
template <int N>
struct ArrayLike {
  char* data[N]; // Initialized to something
  void SomeFunction() {
    non_mutating(data, N);
  }
};

The difference between the 'normal' and non-type templates would be, that N is not mutatable at all and the semantics is clear (builtin integer-like type).

If the current implementation would not figure that out, you can just add a test for it and assume a mutation. Handling non-type templates later is absolutly ok.

We have to assume data is mutated here as well. I'll add a test case for this.

void g(const char*, int); // <-- doesn't mutate
void g(char(&)[8], int); // <-- do mutate

template <int N>
void f() {
    char data[N];
    g(data, N); // <-- we don't know which `g` will be called yet
}

void h() {
    f<8>(); // <-- f calls g(char(&)[8], int) internally
    f<9>(); // <-- f calls g(const char*, int) internally
}

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

[JonasToth]

@shuaiwang i tried to apply this and check the clang-tidy part again, but it does not compile (log attached).
I update clang to master, did you add a matcher or something like this?

error.log13 KBDownload

[shuaiwang]

In D50619#1202135, @JonasToth wrote:

@shuaiwang i tried to apply this and check the clang-tidy part again, but it does not compile (log attached).
I update clang to master, did you add a matcher or something like this?

error.log13 KBDownload

Oops, my bad, this depends on D50617

[JonasToth]

@shuaiwang Unfortunatly the analysis does not pass and fails on an assertion

→ ~/opt/llvm/build/bin/clang-tidy -checks=-*,cppcoreguidelines-const-correctness ItaniumDemangle.cpp --
clang-tidy: ../tools/clang/include/clang/AST/ExprCXX.h:3581: clang::Expr* clang::CXXDependentScopeMemberExpr::getBase() const: Assertion `!isImplicitAccess()' failed.
Abgebrochen (Speicherabzug geschrieben)

I did not investigate further, sorry for the long time to try it out.

[shuaiwang]

In D50619#1207785, @JonasToth wrote:

@shuaiwang Unfortunatly the analysis does not pass and fails on an assertion

→ ~/opt/llvm/build/bin/clang-tidy -checks=-*,cppcoreguidelines-const-correctness ItaniumDemangle.cpp --
clang-tidy: ../tools/clang/include/clang/AST/ExprCXX.h:3581: clang::Expr* clang::CXXDependentScopeMemberExpr::getBase() const: Assertion `!isImplicitAccess()' failed.
Abgebrochen (Speicherabzug geschrieben)

I did not investigate further, sorry for the long time to try it out.

D50617 updated, could you help try again? Thanks!

[JonasToth]

Sure, thanks for the fast fix :)

Am 22.08.2018 um 00:04 schrieb Shuai Wang via Phabricator:

shuaiwang added a comment.

In https://reviews.llvm.org/D50619#1207785, @JonasToth wrote:

@shuaiwang Unfortunatly the analysis does not pass and fails on an assertion

→ ~/opt/llvm/build/bin/clang-tidy -checks=-*,cppcoreguidelines-const-correctness ItaniumDemangle.cpp --
clang-tidy: ../tools/clang/include/clang/AST/ExprCXX.h:3581: clang::Expr* clang::CXXDependentScopeMemberExpr::getBase() const: Assertion `!isImplicitAccess()' failed.
Abgebrochen (Speicherabzug geschrieben)

I did not investigate further, sorry for the long time to try it out.

https://reviews.llvm.org/D50617 updated, could you help try again? Thanks!

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

[shuaiwang]

Ping :)

[JonasToth]

Ups, sorry i overlooked.

I applied your changes to the current version of the const check, and everything seems fine. The false negative is gone.

clang-tidy/utils/ExprMutationAnalyzer.cpp

const auto AsAssignmentLhs =
       binaryOperator(isAssignmentOperator(), hasLHS(equalsNode(Exp)));
 
   // Operand of increment/decrement operators.
   const auto AsIncDecOperand =
       unaryOperator(anyOf(hasOperatorName("++"), hasOperatorName("--")),
                     hasUnaryOperand(equalsNode(Exp)));
 
   // Invoking non-const member function.
+  // A member function is assumed to be non-const when it is unresolved.
   const auto NonConstMethod = cxxMethodDecl(unless(isConst()));
   const auto AsNonConstThis =
       expr(anyOf(cxxMemberCallExpr(callee(NonConstMethod), on(equalsNode(Exp))),
                  cxxOperatorCallExpr(callee(NonConstMethod),
-                                     hasArgument(0, equalsNode(Exp)))));
+                                     hasArgument(0, equalsNode(Exp))),
+                 callExpr(callee(expr(anyOf(
+                     unresolvedMemberExpr(hasObjectExpression(equalsNode(Exp))),
+                     cxxDependentScopeMemberExpr(
+                         hasObjectExpression(equalsNode(Exp)))))))));
 
   // Taking address of 'Exp'.
   // We're assuming 'Exp' is mutated as soon as its address is taken, though in
   // theory we can follow the pointer and see whether it escaped `Stm` or is
   // dereferenced and then mutated. This is left for future improvements.
   const auto AsAmpersandOperand =
       unaryOperator(hasOperatorName("&"),
                     // A NoOp implicit cast is adding const.
                     unless(hasParent(implicitCastExpr(hasCastKind(CK_NoOp)))),
                     hasUnaryOperand(equalsNode(Exp)));
   const auto AsPointerFromArrayDecay =
       castExpr(hasCastKind(CK_ArrayToPointerDecay),
                unless(hasParent(arraySubscriptExpr())), has(equalsNode(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.
   const auto NonConstRefParam = forEachArgumentWithParam(
       equalsNode(Exp), parmVarDecl(hasType(nonConstReferenceType())));
-  const auto AsNonConstRefArg =
-      anyOf(callExpr(NonConstRefParam), cxxConstructExpr(NonConstRefParam));
+  const auto AsNonConstRefArg = anyOf(
+      callExpr(NonConstRefParam), cxxConstructExpr(NonConstRefParam),
+      callExpr(callee(unresolvedLookupExpr()), hasAnyArgument(equalsNode(Exp))),
+      cxxUnresolvedConstructExpr(hasAnyArgument(equalsNode(Exp))));
 
   // Captured by a lambda by reference.
   // If we're initializing a capture with 'Exp' directly then we're initializing
   // a reference capture.
   // For value captures there will be an ImplicitCastExpr <LValueToRValue>.
   const auto AsLambdaRefCaptureInit = lambdaExpr(hasCaptureInit(Exp));
 
   // Returned as non-const-ref.
const auto Matches =
                                AsNonConstRefReturn))
                         .bind("stmt")),
             *Stm, *Context);
   return selectFirst<Stmt>("stmt", Matches);
 }
 
 const Stmt *ExprMutationAnalyzer::findMemberMutation(const Expr *Exp) {
   // Check whether any member of 'Exp' is mutated.
-  const auto MemberExprs = match(
-      findAll(memberExpr(hasObjectExpression(equalsNode(Exp))).bind("expr")),
+  const auto MemberExprs =
+      match(findAll(expr(anyOf(memberExpr(hasObjectExpression(equalsNode(Exp))),
+                               cxxDependentScopeMemberExpr(
+                                   hasObjectExpression(equalsNode(Exp)))))
+                        .bind("expr")),
             *Stm, *Context);
   return findExprMutation(MemberExprs);
 }
 
 const Stmt *ExprMutationAnalyzer::findArrayElementMutation(const Expr *Exp) {
   // Check whether any element of an array is mutated.
   const auto SubscriptExprs = match(
       findAll(arraySubscriptExpr(hasBase(ignoringImpCasts(equalsNode(Exp))))
                   .bind("expr")),

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

 TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
   const auto AST = tooling::buildASTFromCode(
       "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
 }
 
+TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
+  auto AST = tooling::buildASTFromCode(
+      "struct X { template <class T> void mf(); };"
+      "template <class T> void f() { X x; x.mf<T>(); }");
+  auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf<T>()"));
+
+  AST =
+      tooling::buildASTFromCode("template <class T> void f() { T x; x.mf(); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+}
+
 TEST(ExprMutationAnalyzerTest, ConstMemberFunc) {
   const auto AST = tooling::buildASTFromCode(
       "void f() { struct Foo { void mf() const; }; Foo x; x.mf(); }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
const auto AST = tooling::buildASTFromCode(
       "forward(typename std::remove_reference<T>::type&&) noexcept;"
       "void f() { struct A {}; A x; std::forward<A &>(x); }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()),
               ElementsAre("std::forward<A &>(x)"));
 }
 
+TEST(ExprMutationAnalyzerTest, CallUnresolved) {
+  auto AST =
+      tooling::buildASTFromCode("template <class T> void f() { T x; g(x); }");
+  auto Results =
+      match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(x)"));
+
+  AST = tooling::buildASTFromCode(
+      "template <class T> void f() { int x; g(T(), x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(T(), x)"));
+
+  AST = tooling::buildASTFromCode(
+      "template <class T> void f() { int x; (void)T(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("T(x)"));
+}
+
 TEST(ExprMutationAnalyzerTest, ReturnAsValue) {
   const auto AST = tooling::buildASTFromCode("int f() { int x; return x; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, ReturnAsNonConstRef) {
TEST(ExprMutationAnalyzerTest, FollowRefModified) {
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()),
               ElementsAre("r0", "r1", "r2", "r3", "r3 = 10"));
 }
 
 TEST(ExprMutationAnalyzerTest, FollowRefNotModified) {
   const auto AST = tooling::buildASTFromCode(
       "void f() { int x; int& r0 = x; int& r1 = r0; int& r2 = r1; "
       "int& r3 = r2; int& r4 = r3; int& r5 = r4;}");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, FollowConditionalRefModified) {
   const auto AST = tooling::buildASTFromCode(

 TEST(ExprMutationAnalyzerTest, ArrayElementNotModified) {
   const auto AST = tooling::buildASTFromCode("void f() { int x[2]; x[0]; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, NestedMemberModified) {
-  const auto AST = tooling::buildASTFromCode(
+  auto AST = tooling::buildASTFromCode(
       "void f() { struct A { int vi; }; struct B { A va; }; "
       "struct C { B vb; }; C x; x.vb.va.vi = 10; }");
-  const auto Results =
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.vb.va.vi = 10"));
+
+  AST = tooling::buildASTFromCode(
+      "template <class T> void f() { T x; x.y.z = 10; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.y.z = 10"));
 }
 
 TEST(ExprMutationAnalyzerTest, NestedMemberNotModified) {
-  const auto AST = tooling::buildASTFromCode(
+  auto AST = tooling::buildASTFromCode(
       "void f() { struct A { int vi; }; struct B { A va; }; "
       "struct C { B vb; }; C x; x.vb.va.vi; }");
-  const auto Results =
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST =
+      tooling::buildASTFromCode("template <class T> void f() { T x; x.y.z; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, CastToValue) {
   const auto AST =
       tooling::buildASTFromCode("void f() { int x; static_cast<double>(x); }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));