[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–52

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

203

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–52

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()))));
 };
 
 const auto nonConstPointerType = [] {
-  return pointerType(pointee(unless(isConstQualified())));
+  return hasUnqualifiedDesugaredType(
+      pointerType(pointee(unless(isConstQualified()))));
 };
 
 const auto isMoveOnly = [] {
   return cxxRecordDecl(
       hasMethod(cxxConstructorDecl(isMoveConstructor(), unless(isDeleted()))),
       hasMethod(cxxMethodDecl(isMoveAssignmentOperator(), unless(isDeleted()))),
       unless(anyOf(hasMethod(cxxConstructorDecl(isCopyConstructor(),
                                                 unless(isDeleted()))),
const auto AsAmpersandOperand =
                     unless(hasParent(implicitCastExpr(hasCastKind(CK_NoOp)))),
                     hasUnaryOperand(equalsNode(Exp)));
   const auto AsPointerFromArrayDecay =
       castExpr(hasCastKind(CK_ArrayToPointerDecay),
                unless(hasParent(arraySubscriptExpr())), has(equalsNode(Exp)));
   // Treat calling `operator->()` of move-only classes as taking address.
   // These are typically smart pointers with unique ownership so we treat
   // mutation of pointee as mutation of the smart pointer itself.
-  const auto AsOperatorArrowThis = cxxOperatorCallExpr(
-      hasOverloadedOperatorName("->"),
-      callee(cxxMethodDecl(
-          ofClass(isMoveOnly()),
-          returns(hasUnqualifiedDesugaredType(nonConstPointerType())))),
+  const auto AsOperatorArrowThis =
+      cxxOperatorCallExpr(hasOverloadedOperatorName("->"),
+                          callee(cxxMethodDecl(ofClass(isMoveOnly()),
+                                               returns(nonConstPointerType()))),
                           argumentCountIs(1), hasArgument(0, 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),
       callExpr(callee(expr(anyOf(unresolvedLookupExpr(), unresolvedMemberExpr(),

unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

 
 TEST(ExprMutationAnalyzerTest, ByValueArgument) {
   auto AST =
       tooling::buildASTFromCode("void g(int); void f() { int x; g(x); }");
   auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 
+  AST = tooling::buildASTFromCode("void g(int*); void f() { int* x; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode("typedef int* IntPtr;"
+                                  "void g(IntPtr); void f() { int* x; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
   AST = tooling::buildASTFromCode(
       "void f() { struct A {}; A operator+(A, int); A x; x + 1; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 
   AST = tooling::buildASTFromCode(
       "void f() { struct A { A(int); }; int x; A y(x); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 
   AST = tooling::buildASTFromCode(
       "void f() { struct A { A(); A(A); }; A x; A y(x); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 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> using TRef = T&;"
+                                "void g(TRef<int>); 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());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
 
   AST = tooling::buildASTFromCode(
       "void f() { struct A { A(); A(A&); }; A x; A y(x); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("x"));
 }
 
 TEST(ExprMutationAnalyzerTest, ByConstRefArgument) {
   auto AST = tooling::buildASTFromCode(
       "void g(const int&); void f() { int x; g(x); }");
   auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 
+  AST = tooling::buildASTFromCode("typedef const int& CIntRef;"
+                                  "void g(CIntRef); void f() { int x; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "template <class T> using CTRef = const T&;"
+      "void g(CTRef<int>); void f() { int x; g(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "template <class T> struct identity { using type = T; };"
+      "template <class T, class U = const T&>"
+      "void g(typename identity<U>::type);"
+      "void f() { int x; g<int>(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
   AST = tooling::buildASTFromCode(
       "void f() { struct A {}; A operator+(const A&, int); A x; x + 1; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
 
   AST = tooling::buildASTFromCode(
       "void f() { struct A { A(const int&); }; int x; A y(x); }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
TEST(ExprMutationAnalyzerTest, CallUnresolved) {
   AST = tooling::buildASTFromCodeWithArgs(
       "template <class T> void f() { int x; (void)T(x); }",
       {"-fno-delayed-template-parsing"});
   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 =
+  auto AST = tooling::buildASTFromCode("int f() { int x; return x; }");
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode("int* f() { int* x; return x; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode("typedef int* IntPtr;"
+                                  "IntPtr f() { int* x; return x; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, ReturnAsNonConstRef) {
   const auto AST = tooling::buildASTFromCode("int& f() { int x; return x; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("return x;"));
 }
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(
+  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(
+  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 =
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { int x; int& r0 = x; const int& r1 = r0;}");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "typedef const int& CIntRefX;"
+      "using CIntRefY = const int&;"
+      "void f() { int x; int& r0 = x; CIntRefX r1 = r0;"
+      "CIntRefY r2 = r1; decltype((r1)) r3 = r2;}");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, FollowConditionalRefModified) {
   const auto AST = tooling::buildASTFromCode(
       "void f() { int x, y; bool b; int &r = b ? x : y; r = 10; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre("r", "r = 10"));
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()));
 }
 
 TEST(ExprMutationAnalyzerTest, CastToConstRef) {
-  const auto AST = tooling::buildASTFromCode(
+  auto AST = tooling::buildASTFromCode(
       "void f() { int x; static_cast<const int&>(x); }");
-  const auto Results =
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST =
+      tooling::buildASTFromCode("typedef const int& CIntRef;"
+                                "void f() { int x; static_cast<CIntRef>(x); }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, LambdaDefaultCaptureByValue) {
   const auto AST =
       tooling::buildASTFromCode("void f() { int x; [=]() { x = 10; }; }");
   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()));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByValue) {
-  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_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "void f() { int* x[2]; for (int* e : x) e = nullptr; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "typedef int* IntPtr;"
+      "void f() { int* x[2]; for (IntPtr e : x) e = nullptr; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByConstRef) {
-  const auto AST = tooling::buildASTFromCode(
+  auto AST = tooling::buildASTFromCode(
       "void f() { int x[2]; for (const int& e : x) e; }");
-  const auto Results =
+  auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
   EXPECT_FALSE(isMutated(Results, AST.get()));
+
+  AST = tooling::buildASTFromCode(
+      "typedef const int& CIntRef;"
+      "void f() { int x[2]; for (CIntRef e : x) e; }");
+  Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForNonArrayByRefModified) {
   const auto AST =
       tooling::buildASTFromCode("struct V { int* begin(); int* end(); };"
                                 "void f() { V x; for (int& e : x) e = 10; }");
   const auto Results =
       match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());