[clang-tidy] Handle sugared reference types in ExprMutationAnalyzer

[shuaiwang]

Summary

This handles cases like this:

typedef int& IntRef;
void mutate(IntRef);
void f() {
  int x;
  mutate(x);
}

where the param type is a sugared type (TypedefType) instead of a
reference type directly.

Note that another category of similar but different cases are already
handled properly before:

typedef int Int;
void mutate(Int&);
void f() {
  int x;
  mutate(x);
}

[JonasToth]

What happens to pointers in a typedef (in the sense of * instead of &)?

clang-tidy/utils/ExprMutationAnalyzer.cpp

51

Not directly related, but this matcher matches universal references, even though they might result in a value. (one of the bugs lebedevri reported).

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

171

i think the following tests should be added as well

• typedef const int& IntRef;
• template <typename T> using TemplateRef = T&

It would be better to have at least one positive and negative test (with const and without const in the typedef) for each of theses cases.

[shuaiwang]

more test cases.

[shuaiwang]

In D50953#1229287, @JonasToth wrote:

What happens to pointers in a typedef (in the sense of * instead of &)?

I checked around and I believe reference type is the only type we're explicitly matching right now. We'll need to handle carefully when handling pointer types in the future.

clang-tidy/utils/ExprMutationAnalyzer.cpp

51

Noted, I'll take a look at the bug.

[JonasToth]

In D50953#1230003, @shuaiwang wrote:

In D50953#1229287, @JonasToth wrote:

What happens to pointers in a typedef (in the sense of * instead of &)?

I checked around and I believe reference type is the only type we're explicitly matching right now. We'll need to handle carefully when handling pointer types in the future.

We match on pointers as values. So figure this out int * >const< ptr = nullptr.
And the constness transformation is especially intersting for pointer typedefs, because in typedef int * IntPtr; const IntPtr p1; IntPtr const p2; p1 and p2 are different things. It would be nice if you could check, that the value semantic of the pointer is detected through the typedef as well.

[shuaiwang]

more test cases

[JonasToth]

Could you please add a mutating test for the pointers as well?

[shuaiwang]

In D50953#1230138, @JonasToth wrote:

In D50953#1230003, @shuaiwang wrote:

In D50953#1229287, @JonasToth wrote:

What happens to pointers in a typedef (in the sense of * instead of &)?

I checked around and I believe reference type is the only type we're explicitly matching right now. We'll need to handle carefully when handling pointer types in the future.

We match on pointers as values. So figure this out int * >const< ptr = nullptr.
And the constness transformation is especially intersting for pointer typedefs, because in typedef int * IntPtr; const IntPtr p1; IntPtr const p2; p1 and p2 are different things. It would be nice if you could check, that the value semantic of the pointer is detected through the typedef as well.

Added test cases verifying we're treating pointers as values.
I feel constness doesn't matter much since we're treating them as values and both const values & non-const values are just values.

[JonasToth]

I feel constness doesn't matter much since we're treating them as values and both const values & non-const values are just values.

marking the pointer itself const is not very common as well, thats why i feel we should be especially exhaustive with our tests as real projects probably won't show problems.

[shuaiwang]

More test cases:

• Mutating pointers
• Const values

[JonasToth]

LGTM

clang-tidy/utils/ExprMutationAnalyzer.cpp

const ast_matchers::internal::VariadicDynCastAllOfMatcher<Stmt,
     genericSelectionExpr;
 
 AST_MATCHER_P(GenericSelectionExpr, hasControllingExpr,
               ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
   return InnerMatcher.matches(*Node.getControllingExpr(), Finder, Builder);
 }
 
 const auto nonConstReferenceType = [] {
-  return referenceType(pointee(unless(isConstQualified())));
+  return hasUnqualifiedDesugaredType(
+      referenceType(pointee(unless(isConstQualified()))));
 };
 
 } // namespace
 
 const Stmt *ExprMutationAnalyzer::findMutation(const Expr *Exp) {
   const auto Memoized = Results.find(Exp);
   if (Memoized != Results.end())
     return Memoized->second;

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

 
 TEST(ExprMutationAnalyzerTest, ByNonConstRefArgument) {
   auto AST =
       tooling::buildASTFromCode("void g(int&); 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("typedef int& IntRef;"
+                                  "void g(IntRef); 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(
+      "template <class T> struct identity { using type = T; };"
+      "template <class T, class U = T&> void g(typename identity<U>::type);"
+      "void f() { int x; g<int>(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("g<int>(x)"));
+
   AST = tooling::buildASTFromCode(
       "void f() { struct A {}; A operator+(A&, int); A x; x + 1; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x + 1"));
 
   AST = tooling::buildASTFromCode(
       "void f() { struct A { A(int&); }; int x; A y(x); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
TEST(ExprMutationAnalyzerTest, ArrayToPointerDecay) {
   const auto AST =
       tooling::buildASTFromCode("void g(int*); void f() { int x[2]; g(x); }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
 }
 
 TEST(ExprMutationAnalyzerTest, FollowRefModified) {
-  const auto AST = tooling::buildASTFromCode(
+  auto AST = tooling::buildASTFromCode(
       "void f() { int x; int& r0 = x; int& r1 = r0; int& r2 = r1; "
       "int& r3 = r2; r3 = 10; }");
-  const auto Results =
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()),
               ElementsAre("r0", "r1", "r2", "r3", "r3 = 10"));
+
+  AST = tooling::buildASTFromCode(
+      "typedef int& IntRefX;"
+      "using IntRefY = int&;"
+      "void f() { int x; IntRefX r0 = x; IntRefY r1 = r0;"
+      "decltype((x)) r2 = r1; r2 = 10; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre("r0", "r1", "r2", "r2 = 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());
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()));
 }
 
 TEST(ExprMutationAnalyzerTest, CastToRefModified) {
-  const auto AST = tooling::buildASTFromCode(
+  auto AST = tooling::buildASTFromCode(
       "void f() { int x; static_cast<int &>(x) = 10; }");
-  const auto Results =
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()),
               ElementsAre("static_cast<int &>(x) = 10"));
+
+  AST = tooling::buildASTFromCode(
+      "typedef int& IntRef;"
+      "void f() { int x; static_cast<IntRef>(x) = 10; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()),
+              ElementsAre("static_cast<IntRef>(x) = 10"));
 }
 
 TEST(ExprMutationAnalyzerTest, CastToRefNotModified) {
   const auto AST =
       tooling::buildASTFromCode("void f() { int x; static_cast<int&>(x); }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
const auto AST =
       tooling::buildASTFromCode("void f() { int x; [&x]() { x = 10; }; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()),
               ElementsAre(ResultOf(removeSpace, "[&x](){x=10;}")));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByRefModified) {
-  const auto AST = tooling::buildASTFromCode(
+  auto AST = tooling::buildASTFromCode(
       "void f() { int x[2]; for (int& e : x) e = 10; }");
-  const auto Results =
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("e", "e = 10"));
+
+  AST = tooling::buildASTFromCode(
+      "typedef int& IntRef;"
+      "void f() { int x[2]; for (IntRef e : x) e = 10; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("e", "e = 10"));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByRefNotModified) {
   const auto AST =
       tooling::buildASTFromCode("void f() { int x[2]; for (int& e : x) e; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));