[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE341848: [clang-tidy] Handle unresolved expressions in 
ExprMutationAnalyzer (authored by shuaiwang, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50619?vs=160960&id=164704#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

Files:
  clang-tidy/utils/ExprMutationAnalyzer.cpp
  unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -114,6 +114,26 @@
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
 }
 
+TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
+  auto AST = tooling::buildASTFromCode(
+  "struct X { template  void mf(); };"
+  "template  void f() { X x; x.mf(); }");
+  auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+  AST =
+  tooling::buildASTFromCode("template  void f() { T x; x.mf(); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+  AST = tooling::buildASTFromCode(
+  "template  struct X;"
+  "template  void f() { X 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(); }");
@@ -292,6 +312,51 @@
   ElementsAre("std::forward(x)"));
 }
 
+TEST(ExprMutationAnalyzerTest, CallUnresolved) {
+  auto AST =
+  tooling::buildASTFromCode("template  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  void f() { char x[N]; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "template  void f(T t) { 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  void f(T t) { int x; t.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("t.mf(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "template  struct S;"
+  "template  void f() { S s; int x; s.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("s.mf(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "struct S { template  void mf(); };"
+  "template  void f(S s) { int x; s.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("s.mf(x)"));
+
+  AST = tooling::buildASTFromCode("template "
+  "void g(F f) { int x; f(x); } ");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("f(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "template  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 =
@@ -347,6 +412,21 @@
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
 }
 
+TEST(ExprMutationAnalyzerTest, TemplateWithArrayToPointerDecay) {
+  const auto AST = tooling::buildASTFromCode(
+  "template  struct S { static constexpr int v = 8; };"
+  "template <> struct S { static constexpr int v = 4; };"
+  "void g(char*);"
+  "template  void f() { char x[S::v]; g(x); }"
+  "template <> void f() { char y[S::v]; g(y); }");
+  const auto ResultsX =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(ResultsX, AST.get()), ElementsAre("g(x)"));
+  const auto ResultsY =
+  match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y"));
+}
+
 TEST(ExprMutationAnalyzerTest, FollowRefModifie

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

Ping :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

In https://reviews.llvm.org/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?
>
> F6950472: error.log 


Oops, my bad, this depends on https://reviews.llvm.org/D50617


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@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?

F6950472: error.log 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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 
>> `). They should behave the same. But the following case would not 
>> be a mutation:
>> 
>>   void non_mutating(const char* Array, int size) { /* Foo */ }
>>   template 
>>   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 
>   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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang updated this revision to Diff 160960.
shuaiwang added a comment.

Test case with non-type template


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

Files:
  clang-tidy/utils/ExprMutationAnalyzer.cpp
  unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -114,6 +114,26 @@
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
 }
 
+TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
+  auto AST = tooling::buildASTFromCode(
+  "struct X { template  void mf(); };"
+  "template  void f() { X x; x.mf(); }");
+  auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+  AST =
+  tooling::buildASTFromCode("template  void f() { T x; x.mf(); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+  AST = tooling::buildASTFromCode(
+  "template  struct X;"
+  "template  void f() { X 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(); }");
@@ -292,6 +312,51 @@
   ElementsAre("std::forward(x)"));
 }
 
+TEST(ExprMutationAnalyzerTest, CallUnresolved) {
+  auto AST =
+  tooling::buildASTFromCode("template  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  void f() { char x[N]; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "template  void f(T t) { 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  void f(T t) { int x; t.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("t.mf(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "template  struct S;"
+  "template  void f() { S s; int x; s.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("s.mf(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "struct S { template  void mf(); };"
+  "template  void f(S s) { int x; s.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("s.mf(x)"));
+
+  AST = tooling::buildASTFromCode("template "
+  "void g(F f) { int x; f(x); } ");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("f(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "template  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 =
@@ -347,6 +412,21 @@
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
 }
 
+TEST(ExprMutationAnalyzerTest, TemplateWithArrayToPointerDecay) {
+  const auto AST = tooling::buildASTFromCode(
+  "template  struct S { static constexpr int v = 8; };"
+  "template <> struct S { static constexpr int v = 4; };"
+  "void g(char*);"
+  "template  void f() { char x[S::v]; g(x); }"
+  "template <> void f() { char y[S::v]; g(y); }");
+  const auto ResultsX =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(ResultsX, AST.get()), ElementsAre("g(x)"));
+  const auto ResultsY =
+  match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y"));
+}
+
 TEST(ExprMutationAnalyzerTest, FollowRefModified) {
   const auto AST = tooling::buildASTFromCode(
   "void f() { int x; int& r0 = x; int& r1 = r0; int& r2 = r1; "
@@ -398,21 +478,43 @@
 }
 
 TEST(ExprMutationAnalyzerTest, Neste

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

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 
> `). They should behave the same. But the following case would not 
> be a mutation:
> 
> ```
> void non_mutating(const char* Array, int size) { /* Foo */ }
> template 
> 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 
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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang updated this revision to Diff 160952.
shuaiwang marked 3 inline comments as done.
shuaiwang added a comment.

More test cases


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

Files:
  clang-tidy/utils/ExprMutationAnalyzer.cpp
  unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -114,6 +114,26 @@
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
 }
 
+TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
+  auto AST = tooling::buildASTFromCode(
+  "struct X { template  void mf(); };"
+  "template  void f() { X x; x.mf(); }");
+  auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+  AST =
+  tooling::buildASTFromCode("template  void f() { T x; x.mf(); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+  AST = tooling::buildASTFromCode(
+  "template  struct X;"
+  "template  void f() { X 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(); }");
@@ -292,6 +312,46 @@
   ElementsAre("std::forward(x)"));
 }
 
+TEST(ExprMutationAnalyzerTest, CallUnresolved) {
+  auto AST =
+  tooling::buildASTFromCode("template  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  void f(T t) { 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  void f(T t) { int x; t.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("t.mf(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "template  struct S;"
+  "template  void f() { S s; int x; s.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("s.mf(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "struct S { template  void mf(); };"
+  "template  void f(S s) { int x; s.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("s.mf(x)"));
+
+  AST = tooling::buildASTFromCode("template "
+  "void g(F f) { int x; f(x); } ");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("f(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "template  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 =
@@ -347,6 +407,21 @@
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
 }
 
+TEST(ExprMutationAnalyzerTest, TemplateWithArrayToPointerDecay) {
+  const auto AST = tooling::buildASTFromCode(
+  "template  struct S { static constexpr int v = 8; };"
+  "template <> struct S { static constexpr int v = 4; };"
+  "void g(char*);"
+  "template  void f() { char x[S::v]; g(x); }"
+  "template <> void f() { char y[S::v]; g(y); }");
+  const auto ResultsX =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(ResultsX, AST.get()), ElementsAre("g(x)"));
+  const auto ResultsY =
+  match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y"));
+}
+
 TEST(ExprMutationAnalyzerTest, FollowRefModified) {
   const auto AST = tooling::buildASTFromCode(
   "void f() { int x; int& r0 = x; int& r1 = r0; int& r2 = r1; "
@@ -398,21 +473,43 @@
 }
 
 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.

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:117
 
+TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
+  auto AST = tooling::buildASTFromCode(

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



Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:309
 
+TEST(ExprMutationAnalyzerTest, CallUnresolved) {
+  auto AST =

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

```
void non_mutating(const char* Array, int size) { /* Foo */ }
template 
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.



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

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang updated this revision to Diff 160694.
shuaiwang marked 2 inline comments as done.
shuaiwang added a comment.
Herald added a subscriber: Szelethus.

- Fix a few cases overlooked previously
- More test cases


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

Files:
  clang-tidy/utils/ExprMutationAnalyzer.cpp
  unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -114,6 +114,20 @@
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
 }
 
+TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
+  auto AST = tooling::buildASTFromCode(
+  "struct X { template  void mf(); };"
+  "template  void f() { X x; x.mf(); }");
+  auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+  AST =
+  tooling::buildASTFromCode("template  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(); }");
@@ -292,6 +306,40 @@
   ElementsAre("std::forward(x)"));
 }
 
+TEST(ExprMutationAnalyzerTest, CallUnresolved) {
+  auto AST =
+  tooling::buildASTFromCode("template  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  void f(T t) { 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  void g(T t) { int x; t.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("t.mf(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "struct S { template  void mf(); };"
+  "template  void f(S s) { int x; s.mf(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("s.mf(x)"));
+
+  AST = tooling::buildASTFromCode("template "
+  "void g(F f) { int x; f(x); } ");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("f(x)"));
+
+  AST = tooling::buildASTFromCode(
+  "template  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 =
@@ -347,6 +395,21 @@
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
 }
 
+TEST(ExprMutationAnalyzerTest, TemplateWithArrayToPointerDecay) {
+  const auto AST = tooling::buildASTFromCode(
+  "template  struct S { static constexpr int v = 8; };"
+  "template <> struct S { static constexpr int v = 4; };"
+  "void g(char*);"
+  "template  void f() { char x[S::v]; g(x); }"
+  "template <> void f() { char y[S::v]; g(y); }");
+  const auto ResultsX =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(ResultsX, AST.get()), ElementsAre("g(x)"));
+  const auto ResultsY =
+  match(withEnclosingCompound(declRefTo("y")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(ResultsY, AST.get()), ElementsAre("y"));
+}
+
 TEST(ExprMutationAnalyzerTest, FollowRefModified) {
   const auto AST = tooling::buildASTFromCode(
   "void f() { int x; int& r0 = x; int& r1 = r0; int& r2 = r1; "
@@ -398,21 +461,31 @@
 }
 
 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  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"));
 }
 

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:454
+
+  AST =
+  tooling::buildASTFromCode("template  void f() { T x; x.y.z; }");

JonasToth wrote:
> Is there already a test for a method from a templated type?
> 
> E.g.
> ```
> template 
> struct Foo {
>   T x;
>   Foo() { int local_int; x(local_int); }
> };
> ```
> One can not say that `local_int` could be const or not.
`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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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

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

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




Comment at: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp:454
+
+  AST =
+  tooling::buildASTFromCode("template  void f() { T x; x.y.z; }");

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

E.g.
```
template 
struct Foo {
  T x;
  Foo() { int local_int; x(local_int); }
};
```
One can not say that `local_int` could be const or not.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50619: [clang-tidy] Handle unresolved expressions in ExprMutationAnalyzer

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang created this revision.
shuaiwang added reviewers: aaron.ballman, JonasToth.
Herald added subscribers: cfe-commits, a.sidorin, xazax.hun.
Herald added a reviewer: george.karpenkov.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50619

Files:
  clang-tidy/utils/ExprMutationAnalyzer.cpp
  unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -114,6 +114,20 @@
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
 }
 
+TEST(ExprMutationAnalyzerTest, AssumedNonConstMemberFunc) {
+  auto AST = tooling::buildASTFromCode(
+  "struct X { template  void mf(); };"
+  "template  void f() { X x; x.mf(); }");
+  auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x.mf()"));
+
+  AST =
+  tooling::buildASTFromCode("template  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(); }");
@@ -292,6 +306,24 @@
   ElementsAre("std::forward(x)"));
 }
 
+TEST(ExprMutationAnalyzerTest, CallUnresolved) {
+  auto AST =
+  tooling::buildASTFromCode("template  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  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  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 =
@@ -398,21 +430,31 @@
 }
 
 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  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  void f() { T x; x.y.z; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, CastToValue) {
Index: clang-tidy/utils/ExprMutationAnalyzer.cpp
===
--- clang-tidy/utils/ExprMutationAnalyzer.cpp
+++ clang-tidy/utils/ExprMutationAnalyzer.cpp
@@ -145,11 +145,16 @@
 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(
+