[PATCH] D8149: Add hasUnderlyingType narrowing matcher for TypedefDecls, functionProtoType matcher for FunctionProtoType nodes, extend parameterCountIs to FunctionProtoType nodes

[Richard via cfe-commits]

LegalizeAdulthood added inline comments.

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:4992
@@ +4991,3 @@
+  EXPECT_TRUE(matches("typedef int hasUnderlyingTypeTest;",
+                      typedefDecl(hasUnderlyingType(asString("int")))));
+  EXPECT_TRUE(matches("typedef const int T;",
----------------
LegalizeAdulthood wrote:
> LegalizeAdulthood wrote:
> > aaron.ballman wrote:
> > > LegalizeAdulthood wrote:
> > > > aaron.ballman wrote:
> > > > > > I'm happy to make the improvement, but I don't know how. I simply call the Node.underlyingType(), just like hasType calls Node.getType(). I don't know why they are different.
> > > > > 
> > > > > hasType() and hasUnderlyingType() have different operational semantic implications, at least to me. hasType() asks, "ultimately, what is the type of this thing?", and Node.underlyingType() answers that. To me, hasUnderlyingType() asks, "in the chain of types that this typedef refers to, does this thing match any of them?", and Node.underlyingType() does not answer that -- it only looks at the final desugared type. The difference we want is that hasType() continues to look at the final type, but hasUnderlyingType() needs to do more work to look at intermediary types and terminate that loop when the answer is "yes, it has this type" or "no, we can't desugar any further."
> > > > > 
> > > > > If we don't care about the intermediate types for your needs, then I don't see the point to hasUnderlyingType() being an AST matcher at all. hasType() does exactly what is needed, and you can instead modify that to accept a TypedefDecl in addition to Expr and ValueDecl. However, I still see value in hasUnderlyingType() because typedef chains can be long and complex (like in the Win32 APIs), and being able to query for intermediary types would be useful. e.g., I want to know about all types that have an intermediary type of DWORD (which itself is a typedef for an unsigned integer type). hasType() will always go to the final type, making such a matcher impossible without something like hasUnderlyingType().
> > > > I just named the matcher after the [accessor](http://clang.llvm.org/doxygen/classclang_1_1TypedefNameDecl.html#a5fccedff6d3854db365a540145029158) on a typedefDecl node.  To me it's just a narrowing matcher on that node that gives you access to whatever the accessor returns.
> > > > 
> > > > Things may have changed in the meantime, but when I originally created this patch you couldn't do `typedefDecl(hasType(asString("int")))`.  It doesn't compile.  Since `hasType` just matched against the value of whatever `Node.getType()` returned it seemed natural to add a matcher called `hasUnderlyingType` that just matched against the value of whatever `Node.getUnderlyingType()` returned.
> > > > 
> > > > As I said, I'm happy to make the suggested improvement, but beyond simple immitation of existing matchers, I have no idea how to traverse the encodings of types within the guts of clang.  This is what I mean when I say "I don't know how".  I understand conceptually what you are saying but I have no idea how to express that in clang's code base.
> > > That's why I am trying to understand what your goal is. As best I can tell, there are two distinct things someone may want to do with a typedef's type: figure out what it ultimately boils down to, or traverse the typedef chain. I was uncertain which one fit your needs better, but it sounds like "get the ultimate type" is what you want. From there we can discuss what the proper design of the matcher is. I think I was thrown off by you naming it *has*UnderlyingType instead of *get*UnderlyingType because *has* implies a different relationship than *get* and I couldn't see why you wouldn't just update hasType() instead.
> > > 
> > > Another thing to think about: the typedef matcher is a bit controversial in terms of the semantics, but the other matchers are not. Perhaps it makes sense to split the patch into two parts?
> > > 
> > > As for how to actually implement the traversal, I think it looks something like (totally untested):
> > > ```
> > > QualType QT = Context.getTypedefType(&Node);
> > > for (QualType NextQT; QT != NextQT; NextQT = QT.getSingleStepDesugaredType(Context)) {
> > >   // If matches, return Type
> > > }
> > > return QT;
> > > ```
> > This all came up because of the work I was doing on clang-tidy to implement the modernize-redundant-void-arg check.  There wasn't a way to narrow typedefDecl nodes based on the actual type for which a synonym was being introduced.  In that situation, suppose we have something like this:
> > 
> > ```
> > typedef void (fnReturningVoid)(void);
> > typedef foo fnReturningVoid;
> > ```
> > 
> > I would want to match the first typedef because that is the one my check needs to modify.  I don't want to match the second typedef because even though it has the same ultimate type as the first, it doesn't contain the redundant void argument tokens that need to be fixed up.
> > 
> > So for my needs in this scenario, the existing matcher does **exactly** what I need, namely match the first typedef, but not the second.  It turns out that on a typedefDecl node you call the member function `getUnderlyingType()` in order to match the first typedef, so that is what I have called the matcher.
> > 
> > If the phrase "underlying type" in common clang parlance means the ultimate desugaring of a type down to it's most basic form with all intervening typedefs removed, then it seems that not only is my matcher misnamed, but also this member function is misnamed.  Because that's not what this member function is returning for the typedef-of-a-typedef situation (as the earlier failed test fragment discussed).
> > 
> > Am I making sense?
> Oops, that code snippet should have been:
> 
> ```
> typedef void (fnReturningVoid)(void);
> typedef fnReturningVoid foo;
> ```
> 
> In other words, `foo` was just anohter synonym for `fnReturningVoid`.
OK, so I experimented with clang-query a little bit and here's where this changeset gets us:

```
1: typedef void (fn)(void);
2: typedef fn foo;
3: typedef int bar;
4: typedef int (f);
5: typedef int (fn2)(int);
```

```
clang-query> match typedefDecl(hasUnderlyingType(asString("int")))

Match #1:

/tmp/a.cpp:3:1: note: "root" binds here
typedef int bar;
^~~~~~~~~~~~~~~

Match #2:

/tmp/a.cpp:4:1: note: "root" binds here
typedef int (f);
^~~~~~~~~~~~~~~
2 matches.
```

```
clang-query> match typedefDecl(hasUnderlyingType(typedefType()))

Match #1:

/tmp/a.cpp:2:1: note: "root" binds here
typedef fn foo;
^~~~~~~~~~~~~~
1 match.
```

```
clang-query> match typedefDecl(hasUnderlyingType(parenType()))

Match #1:

/tmp/a.cpp:1:1: note: "root" binds here
typedef void (fn)(void);
^~~~~~~~~~~~~~~~~~~~~~~

Match #2:

/tmp/a.cpp:4:1: note: "root" binds here
typedef int (f);
^~~~~~~~~~~~~~~

Match #3:

/tmp/a.cpp:5:1: note: "root" binds here
typedef int (fn2)(int);
^~~~~~~~~~~~~~~~~~~~~~
3 matches.
```

```
clang-query> match typedefDecl(hasUnderlyingType(parenType(innerType(functionType()))))

Match #1:

/tmp/a.cpp:1:1: note: "root" binds here
typedef void (fn)(void);
^~~~~~~~~~~~~~~~~~~~~~~

Match #2:

/tmp/a.cpp:5:1: note: "root" binds here
typedef int (fn2)(int);
^~~~~~~~~~~~~~~~~~~~~~
2 matches.
```

To me this means that the matcher is working as desired.  If `hasUnderlyingType` squished out all the stuff in the middle, then you wouldn't be able to tell the difference between line 1 or line 2.  To me it seems that the matcher should do just **one** level of narrowing on a typedefDecl.  If you want to throw all the intervening sugaring away so that line 2 and line 1 match `functionType()`, then I think that is a distinct matcher, along the lines of existing matchers like `hasDescendent` that traverse multiple relationships to find a match.  There may even be a way to use existing matchers like `has` in order to do this arbitrarily deep traversal in order to find a matching type no matter how many layers of typedef aliases exist between an identifier and the real type.

Going back to my original motivation, it allows me to match typedef aliases for functions taking zero arguments:

```
clang-query> match typedefDecl(hasUnderlyingType(parenType( innerType(functionType( functionProtoType(parameterCountIs(0)) )) )))

Match #1:

/tmp/a.cpp:1:1: note: "root" binds here
typedef void (fn)(void);
^~~~~~~~~~~~~~~~~~~~~~~
1 match.
```


http://reviews.llvm.org/D8149