This is an archive of the discontinued LLVM Phabricator instance.

Extend hasType narrowing matcher for TypedefDecls, add functionProtoType matcher for FunctionProtoType nodes, extend parameterCountIs to FunctionProtoType nodes
ClosedPublic

Authored by LegalizeAdulthood on Mar 8 2015, 8:29 AM.

Details

Summary

Add the following matchers and unit tests:

  • hasUnderlyingType() as a narrowing matcher for TypedefDecls()
  • functionProtoType() matches FunctionProtoType nodes
  • extend parameterCountIs() to FunctionProtoType nodes

Previously you could match a functionType() node, but such a node is not guaranteed to have a full function signature (it could be a C function that is forward declared with no prototype). Matching a FunctionProtoType() node allowed you to discriminate among the different kinds of functions based on their argument count.

Diff Detail

Event Timeline

LegalizeAdulthood retitled this revision from to Add hasUnderlyingType narrowing matcher for TypedefDecls.
LegalizeAdulthood updated this object.
LegalizeAdulthood edited the test plan for this revision. (Show Details)
LegalizeAdulthood added a subscriber: Unknown Object (MLST).
LegalizeAdulthood edited the test plan for this revision. (Show Details)Mar 8 2015, 8:29 AM

Can I get a review please? This has been waiting for over a month...

klimek edited edge metadata.Apr 16 2015, 9:24 AM

Argh, sorry for missing it - please always feel free to ping earlier (especially my reviews you can ping 2x per week).
Can you please add a unit test? Apart from that it looks good.

Argh, sorry for missing it - please always feel free to ping earlier (especially my reviews you can ping 2x per week).
Can you please add a unit test? Apart from that it looks good.

I added a FileCheck test in the tools-extra repository (see http://reviews.llvm.org/D8150). Is that sufficient, or would you also like to see a unit test?

I found the ClangUnitTests target for building the unit tests, but it doesn't run them automatically. Is there a target that does: build implementation, build tests and run tests?

LegalizeAdulthood retitled this revision from Add hasUnderlyingType narrowing matcher for TypedefDecls to Add hasUnderlyingType narrowing matcher for TypedefDecls, functionProtoType matcher for FunctionProtoType nodes, extend parameterCountIs to FunctionProtoType nodes.
LegalizeAdulthood updated this object.
LegalizeAdulthood edited the test plan for this revision. (Show Details)
LegalizeAdulthood edited edge metadata.
klimek accepted this revision.May 18 2015, 7:47 AM
klimek edited edge metadata.

LG after:

  • fixing nits above
  • running:

$ cd docs/tools
$ ./dump_ast_matchers.py

include/clang/ASTMatchers/ASTMatchers.h
2545–2546

\c FunctionProtoTypes

unittests/ASTMatchers/ASTMatchersTest.cpp
4677–4680 ↗(On Diff #25920)

Can you add a negative test with hasUnderlyingType(asString("float")) or something?

This revision is now accepted and ready to land.May 18 2015, 7:47 AM
klimek added a comment.Jul 3 2015, 7:00 AM

What's the state here?

alexfh edited edge metadata.Nov 5 2015, 1:23 PM

Richard, what's the state of this patch?

Regenerate documentation from dump_ast_matchers.py

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
3468

Will this match g() in C mode?

unittests/ASTMatchers/ASTMatchersTest.cpp
4282 ↗(On Diff #43761)

I would like to see tests using matchesC() for void f() (for both functionProtoType() and parameterCountIs()).

4988 ↗(On Diff #43761)

Can we have a test for:

EXPECT_TRUE(matches("typedef int foo; typedef foo bar;", typedefDecl(hasUnderlyingType(asString("int")), hasName("bar"))));

(I assume that this should work?)

LegalizeAdulthood marked an inline comment as done.Dec 30 2015, 12:25 PM
LegalizeAdulthood added inline comments.
include/clang/ASTMatchers/ASTMatchers.h
3468

No, because it doesn't declare g as a prototype. To declare it as a prototype, it needs to be written

void g(void);

unittests/ASTMatchers/ASTMatchersTest.cpp
4988 ↗(On Diff #43761)

This passes:

EXPECT_TRUE(
    matches("typedef int foo; typedef foo bar;",
            typedefDecl(hasUnderlyingType(asString("foo")), hasName("bar"))));

but this does not:

EXPECT_TRUE(
    matches("typedef int foo; typedef foo bar;",
            typedefDecl(hasUnderlyingType(asString("int")), hasName("bar"))));

I'm not really sure why; I expected that Node.getUnderlyingType() would return int, not foo.

Add more unit tests.

aaron.ballman added inline comments.Dec 30 2015, 1:12 PM
include/clang/ASTMatchers/ASTMatchers.h
2546

Can you document the expected parameter count for:

void f(int i, ...);

(I would expect it to report 1.)

3468

That's what I was expecting; can you update the comment to match reality?

unittests/ASTMatchers/ASTMatchersTest.cpp
4282 ↗(On Diff #43800)

This isn't quite what I had in mind. The test you added is a good one to add, but I wanted a test that functionProtoType() does *not* match f() (because it has no prototype), and another test for parameterCountIs() that it does't explode on a function without a prototype in C.

4990 ↗(On Diff #43800)

I would expect hasUnderlyingType to look through all of the type sugar, not just the top layer of it (since existing matchers can implement the current behavior by using hasType, I believe). I think the correct approach is to get the underlying type, then loop to see whether that type matches, and if not, strip off a layer of sugar and try again. Terminate the loop when there's no more sugar to strip. The following should all match:

EXPECT_TRUE(
      matches("typedef int foo; typedef foo bar; typedef bar baz;",
              typedefDecl(hasUnderlyingType(asString("int")), hasName("bar"))));
EXPECT_TRUE(
      matches("typedef int foo; typedef foo bar; typedef bar baz;",
              typedefDecl(hasUnderlyingType(asString("foo")), hasName("baz"))));
EXPECT_TRUE(
      matches("typedef int foo; typedef foo bar; typedef bar baz;",
              typedefDecl(hasUnderlyingType(asString("int")), hasName("baz"))));

The other question I have is about qualifiers. Should this match?

EXPECT_TRUE(
      matches("typedef const int foo;",
              typedefDecl(hasUnderlyingType(asString("int")), hasName("foo"))));

Or should it require asString("const int")?
(Regardless of the answer, we should document the behavior and add a test with qualifiers.)

unittests/ASTMatchers/ASTMatchersTest.cpp
4990 ↗(On Diff #43800)

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.

Add more unit tests from comments

unittests/ASTMatchers/ASTMatchersTest.cpp
4282 ↗(On Diff #43800)

I don't understand what would be tested by adding a test for parameterCountIs() on a function that doesn't have a prototype since it is a nested matcher passed to functionProtoType() and since the function doesn't have a prototype, that outer matcher won't match.

aaron.ballman added inline comments.Jan 6 2016, 5:23 AM
unittests/ASTMatchers/ASTMatchersTest.cpp
4283 ↗(On Diff #43965)

I don't understand what would be tested by adding a test for parameterCountIs() on a function that doesn't have a prototype since it is a nested matcher passed to functionProtoType() and since the function doesn't have a prototype, that outer matcher won't match.

Not according to the AST matcher that was implemented -- it supports FunctionProtoType and FunctionDecl. I want to make sure that if you get to parameterCountIs() through functionDecl() that it doesn't fail.

4992 ↗(On Diff #43965)

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().

unittests/ASTMatchers/ASTMatchersTest.cpp
4283 ↗(On Diff #43965)

Ah, OK, you were asking for a test on functionDecl, not functionProtoType. I can do that. I guess what threw me off is that the comment was attached to the prototype tests.

4992 ↗(On Diff #43965)

I just named the matcher after the accessor 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.

aaron.ballman added inline comments.Jan 6 2016, 11:53 AM
unittests/ASTMatchers/ASTMatchersTest.cpp
4283 ↗(On Diff #43965)

Ah, yeah, one of the downsides to Phab sometimes is finding a good place to put comments for things that are missing. :-P Sorry for the confusion!

4992 ↗(On Diff #43965)

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;
unittests/ASTMatchers/ASTMatchersTest.cpp
4992 ↗(On Diff #43965)

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?

unittests/ASTMatchers/ASTMatchersTest.cpp
4992 ↗(On Diff #43965)

Oops, that code snippet should have been:

typedef void (fnReturningVoid)(void);
typedef fnReturningVoid foo;

In other words, foo was just anohter synonym for fnReturningVoid.

unittests/ASTMatchers/ASTMatchersTest.cpp
4992 ↗(On Diff #43965)

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.
unittests/ASTMatchers/ASTMatchersTest.cpp
4992 ↗(On Diff #43965)

Looks like that last one could be simplified slightly:

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

Match #1:

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

Add more unit tests based on comments.

LegalizeAdulthood marked 4 inline comments as done.

Update documentation from review comments.

aaron.ballman added inline comments.Jan 7 2016, 7:29 AM
unittests/ASTMatchers/ASTMatchersTest.cpp
1567 ↗(On Diff #44192)

What should be the outcome of this test (matches or not matches)?

matchesC("void f();", functionDecl(parameterCountIs(0)));

(Note, it's using C where this is variadic.) My guess is that it should match, but I'd like to make sure (and have a test for it).

4994 ↗(On Diff #44192)

Thank you for those examples! Given the following code:

typedef int foo;
typedef foo bar;

bar i;

clang-query> match varDecl(hasType(asString("int")))
0 matches.
clang-query> match varDecl(hasType(asString("foo")))
0 matches.
clang-query> match varDecl(hasType(asString("bar")))

Match #1:

E:\Desktop\t.cpp:4:1: note: "root" binds here
bar i;
^~~~~
1 match.

So hasType() looks at what the immediate type is for the declaration (which we document, yay us!). Based on that, I don't think hasUnderlyingType() makes sense -- you should modify hasType() to work on a TypedefNameDecl (not just a TypedefDecl!) so that it looks at the immediate type of the type definition. I would expect your tests then to result in:

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(hasType(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(hasType(typedefType()))

Match #1:

/tmp/a.cpp:2:1: note: "root" binds here
typedef fn foo;
^~~~~~~~~~~~~~
1 match.
clang-query> match typedefDecl(hasType(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(hasType(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.

The end results are the same, so this is just changing the way the information is surfaced to the user that is logically consistent. ValueDecls have an immediate type, and so do TypedefDecls. By using TypedefNameDecl instead of TypedefDecl, this also covers the case where hasType() is useful for an alias-declaration. (We don't expose the matcher for that yet, but it seems quite reasonable to add in the future, and it would be nice for hasType to automatically work with that.)

You can implement this with a helper function to handle abstracting away the call to getType() vs getUnderlyingType(), then updating the hasType() matchers to use it. Something like:

template <typename Ty>
struct UnderlyingTypeGetter {
  static QualType get(const Ty &Node) {
    return Node.getType();
  }
};

template <>
QualType UnderlyingTypeGetter<TypedefNameDecl>::get(const TypedefNameDecl &Node) {
  return Node.getUnderlyingType();
}

(Somewhere in ASTMatchersInternal.h most likely.)

LegalizeAdulthood marked an inline comment as done.Jan 11 2016, 9:44 PM
LegalizeAdulthood added inline comments.
unittests/ASTMatchers/ASTMatchersTest.cpp
4994 ↗(On Diff #44192)

When I try to extend hasType to work on TypedefDecl, I get this error:

error: static assertion failed: right polymorphic conversion
     static_assert(TypeListContainsSuperOf<ReturnTypes, T>::value,

...because TypedefDecl is derived from NamedDecl and the existing definition for hasType looks like this:

AST_POLYMORPHIC_MATCHER_P_OVERLOAD(hasType,
                                   AST_POLYMORPHIC_SUPPORTED_TYPES(Expr,
                                                                   ValueDecl),
                                   internal::Matcher<Decl>, InnerMatcher, 1) {
  return qualType(hasDeclaration(InnerMatcher))
      .matches(Node.getType(), Finder, Builder);
}

So I'll need some guidance on how to extend hasType to work for TypedefNamedDecl nodes. I don't understand exactly what all these nasty macros do. So far, I've simply made changes by imitation, but my approach didn't work this time.

aaron.ballman added inline comments.Jan 12 2016, 6:45 AM
unittests/ASTMatchers/ASTMatchersTest.cpp
4994 ↗(On Diff #44192)

This (

) does all of what you need (sans documentation, testing, etc).

(File should be attached, but if you need me to send it to you via email, I can do so -- I've never tried this with Phab before.)

unittests/ASTMatchers/ASTMatchersTest.cpp
4994 ↗(On Diff #44192)

What you had was very similar to what I attempted. You wrote:

AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, TypdefNameDecl, ValueDecl)

and I wrote

AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, ValueDecl, TypedefNameDecl)

so apparently the derivation relations between the arguments to this macro are order dependent?

aaron.ballman added inline comments.
unittests/ASTMatchers/ASTMatchersTest.cpp
4994 ↗(On Diff #44192)

I was being OCD and alphabetizing, but I think you may be right that order matters. @sbenza, may know more (if it's purposeful, we should document it).

sbenza added inline comments.Jan 12 2016, 1:14 PM
unittests/ASTMatchers/ASTMatchersTest.cpp
4994 ↗(On Diff #44192)

Order should not matter.
We just iterate over the list to see if there is a matching type.
I could not reproduce the compiler error.

unittests/ASTMatchers/ASTMatchersTest.cpp
4994 ↗(On Diff #44192)

The compile error occurs in the tests, not in the implementation. See the attached shell output. What am I doing wrong? I don't know what to do about the error.

Is there any reason we can't proceed with the patch as-is and then see if it can be refactored into an overload of hasType?

aaron.ballman edited edge metadata.Jan 19 2016, 7:00 AM

Is there any reason we can't proceed with the patch as-is and then see if it can be refactored into an overload of hasType?

The patch currently includes hasUnderlyingType() which we don't want because hasType() is the logically correct place to expose that functionality (it would be providing duplicate functionality with no benefit to expose hasUnderlyingType()). If you removed hasUnderlyingType() and split the hasType() changes into its own patch, we can easily proceed with the function prototype changes (AFAICT, there's only one outstanding test to add for that and it otherwise looks fine to me).

unittests/ASTMatchers/ASTMatchersTest.cpp
1567 ↗(On Diff #44192)

I still don't see a matchesC() test case for functionDecl(parameterCountIs() on a varargs function, so I don't believe this comment is Done yet. (I do see one for getting to it through functionProtoType()).

sbenza added inline comments.Jan 19 2016, 8:31 AM
unittests/ASTMatchers/ASTMatchersTest.cpp
4994 ↗(On Diff #44192)

There are two overloads of hasType(). You only fixed one of them in that patch and the test happens to use the other one.

LegalizeAdulthood retitled this revision from Add hasUnderlyingType narrowing matcher for TypedefDecls, functionProtoType matcher for FunctionProtoType nodes, extend parameterCountIs to FunctionProtoType nodes to Extend hasType narrowing matcher for TypedefDecls, add functionProtoType matcher for FunctionProtoType nodes, extend parameterCountIs to FunctionProtoType nodes.
LegalizeAdulthood edited edge metadata.

Update from review comments.
Replace hasUnderlyingType with extension to hasType

aaron.ballman accepted this revision.Jan 25 2016, 8:12 AM
aaron.ballman edited edge metadata.

LGTM with one addition, I still would like to see this test added and passing:

EXPECT_TRUE(matchesC("void f();", functionDecl(parameterCountIs(0))));

Thank you for working on these!

LegalizeAdulthood edited edge metadata.

Update from review comments, add test case

LGTM! Thank you for working on this!

If someone could commit this for me, that would be great. I do not have commit access.

Patch by Richard Thomson

aaron.ballman closed this revision.Jan 29 2016, 9:07 AM

Thanks! I've commit as r259210.

Fix RegistryTest unit test
make check-all passes

It seems that phabricator hasn't been configured to allow me to reopen the revision, but I've updated the diff.

I haven't quite figured out the proper phab workflow for reverts yet either, but I took the updated patch, applied it, and commit in r259359. Thank you for the fix!