[cfe-dev] Type matcher madness

[Manuel Klimek]

On Thu, Aug 23, 2012 at 2:10 PM, Michael Diamond <mdiamond at google.com> wrote:
> We discussed this offline a bit, and we basically came to the conclusion
> that we should support both Type and QualType with auto-conversions from
> Matcher<Type> to Matcher<QualType>. We will only support binding QualTypes,
> but this shouldn't be a big deal since every Type is wrapped by a QualType.
> Are there any objections to this approach?

So, Chandler got curious which led to a longish design argument :)

We noticed a problem with the automatic conversion, specifically given
for example:
hasType(pointerType(pointee(intType())))

... and given that type node matchers will in general have similar
signatures as other matchers, so we'd have something like:
Matcher<Stmt> hasType(Matcher<QualType>)
Matcher<Type> pointerType(Matcher<PointerType>)
Matcher<PointerType> pointee(Matcher<QualType>)
Matcher<QualType> intType()

Now Chandler pointed out that the interesting part is that there's a
difference how pointer qualifications work on the different levels of
the stack. More specifically, clang's type hierarchy is designed so
that type equality is given by Type-pointer-equality, which leads to
interesting attributes for composite types.

The gist of the argument was, that the example expression above should
match an int * const, but shouldn't match a const int *, as the inner
qualification difference would make the outer types differ.

My main concern was that users will not expect this, as this runs
counter to how matcher work normally - if you do *not* specify
something, by default you'll get what matches *any* value.

Now we'll only see what problems users have when we actually start
giving them something to play around with. So I think the argument
that erring on the side of a stronger match is the safer bet is
actually a good one.

One thing that I'm not sure we clarified in the end, is that I still
see two options (given that we don't want the above example to match a
const int *):
Do we want it to match a int * const, or not. I'd rather keep the
outermost layer symmetric to the inside, but I understand that there
is the argument that they are not symmetric in the language, and thus
shouldn't be in the matchers.

The way to have the outermost layer behave differently, would be to
say something like:
Matcher<Stmt> hasType(RelaxedQualTypeMatcher)
where an implicit conversion would convert a Matcher<Type> into a
RelaxedQualTypeMatcher, which would then be equivalent to writing
hasType(qualified(...)) where qualified has the signature:
Matcher<QualType> qualified(Matcher<Type>)

If we could live with the default behavior on the outside being strict
matching we wouldn't need that and could simply use Matcher<Type> ->
Matcher<QualType> conversion to mean "only matches the type without
any qualifiers.

Thoughts?
/Manuel



>
>
> On Wed, Aug 22, 2012 at 6:25 PM, Manuel Klimek <klimek at google.com> wrote:
>>
>> On Wed, Aug 22, 2012 at 3:43 PM, Michael Diamond <mdiamond at google.com>
>> wrote:
>> > It seems to me that the distinction between QualType and Type is an
>> > implementation detail that we shouldn't bother users with. If we make
>> > everything a QualType, the users will have the same functionality in a
>> > more
>> > easily readable format. I don't really see any disadvantages to this
>> > approach. You mentioned that this would "kill the type safety for the
>> > matchers"--what did you mean exactly?
>>
>> The question is specifically: how do we intend to do the binding, and
>> what types will the matchers have.
>>
>> One possibility:
>>
>> BindableMatcher<QualType> type(Matcher<QualType>...)
>>
>> Then we implement all things that match on a specific type to go
>> through QualType. But, to get type safety, we'll want to have matchers
>> that actually break compilation if you use them in a wrong context,
>> like:
>>
>> Matcher<RecordType> hasConstFields();
>>
>> If we only use QualType, as far as I can tell we cannot do that.
>>
>> Thoughts?
>> /Manuel
>>
>> >
>> >
>> > On Wed, Aug 22, 2012 at 10:16 AM, Manuel Klimek <klimek at google.com>
>> > wrote:
>> >>
>> >> +mdiamond, who is interested in contributing here
>> >>
>> >> On Wed, Aug 1, 2012 at 12:21 PM, Richard Smith <richard at metafoo.co.uk>
>> >> wrote:
>> >> > On Wed, Aug 1, 2012 at 7:01 AM, Manuel Klimek <klimek at google.com>
>> >> > wrote:
>> >> >>
>> >> >> Given Sam's open review, I've been pondering how to implement a type
>> >> >> matcher (so far we've punted on that).
>> >> >> The obvious idea would be to have a type() function that returns a
>> >> >> Matcher<QualType>. Apart from some technical hoops to jump through I
>> >> >> have a patch ready for that.
>> >> >>
>> >> >> This leads to a different design decision, though. We'll want type()
>> >> >> to return a BindableMatcher, and for that we need to implement type
>> >> >> binding. This is not quite straight forward, as types are split into
>> >> >> QualType and Type nodes in the AST, and only the Type nodes model
>> >> >> the
>> >> >> inheritance hierarchy. My understanding is that this is for
>> >> >> performance reasons, so that every Type* exists only once, and
>> >> >> equality on types can be implemented by a pointer comparison.
>> >> >>
>> >> >> The question is: how much of that do we want to express in the
>> >> >> matcher
>> >> >> language.
>> >> >> I see two main options:
>> >> >> 1. Fully model that relationship. When somebody writes matchers for
>> >> >> types, they will look like this:
>> >> >> qualType(arrayType(hasElementType(qualType(asString("int"))))
>> >> >>
>> >> >> 2. Model QualTypes in the matchers, and allow going "through" to the
>> >> >> underlying type; this will kill the type safety for the matchers,
>> >> >> but
>> >> >> will make the matcher expressions more concise:
>> >> >> arrayType(hasElementType(asString("int")))
>> >> >
>> >> >
>> >> > Is there a fundamental reason why we can't have the best of both?
>> >> > That
>> >> > is, a
>> >> > matcher which matches Type *or* QualType, and constrains its inner
>> >> > matchers
>> >> > to match ArrayType.
>> >
>> >
>
>