[cfe-commits] [PATCH] RecursiveASTVisitor: fix type traversal for EnumDecl

[Philip Craig]

On Tue, Sep 18, 2012 at 5:41 AM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Fri, Sep 7, 2012 at 2:32 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>> +richard & james, who might both have things to say about the
>> semantics involved...
>
>
> We definitely do want to traverse the underlying type as written for an
> enum.
>
> For the other half of the patch (not visiting the type declared by enums and
> template type parameters), I'd like to understand what RAV's policy is here
> -- the original design [0] doesn't even talk about visiting Types, just
> TypeLocs. I think this patch is the right direction, but I think we need to
> know what the end goal is, too.

My expectation, as a new clang user that wants to use RAV for
refactoring tools, is that by default RAV should visit exactly once
each node that corresponds to preprocessed source code. Visiting other
nodes should be optional behaviour that is enabled using things like
shouldVisitImplicitCode(), or by overriding the traverse functions.

> From a quick skim of RAV, the cases where we currently switch from
> traversing a node with locations to traversing a node without locations
> (Type, NestedNameSpecifier, TemplateArgument) are:
>   1) Cases where we don't preserve source location information (for
> ComplexTypeLoc and VectorTypeLoc, the exception types in a dynamic exception
> specification for a FunctionProtoTypeLoc, and the NestedNameSpecifier in a
> TemplateName). These are bugs.
>   2) For the class type in a MemberPointerTypeLoc. This is a bug -- we have
> the TypeLoc but don't traverse it!
>   3) For the type declared in an EnumDecl or a TemplateTypeParmDecl. We
> probably shouldn't be visiting these at all (this is fixed by Philip's
> patch).
>   4) When the AST is missing the type source information (for a
> DeclaratorDecl or for a TemplateArgumentLoc for a type). I'm not sure
> whether there are any cases where this is reasonable for a valid AST.

Ideally there shouldn't be any cases of missing source information. So
if there currently are any cases where this is reasonable, should we
classify these are bugs? Or are there reasons why it's not possible to
have source information?

>   5) For AutoType, we visit the deduced Type (and there is no TypeLoc). This
> is probably convenient for RAV users, but it's not clear whether it's
> correct (especially since we don't have a definition of 'correct'). We don't
> do the same for SubstTemplateTypeParmTypes.

Based on my definition above, I would expect traversal of AutoType to
be disabled by default. But I can see how it would be useful. Maybe
this requires special handling for AutoType in the AST matchers. Do
users of the AST matchers need to control whether they match AutoType
or not?

It's unclear to me whether SubstTemplateTypeParmType should traverse
its type. But I don't think RAV will traverse any
SubstTemplateTypeParmTypes by default anyway, since I believe they
only occur within template instantiations, and the part of the AST
containing the SubstTemplateTypeParmType isn't traversed unless
shouldVisitTemplateInstantiations() is true, even for explicit
instantiations. So this decision should be based on the use cases for
enabling visiting of template instantiations.

> Perhaps the goal should be that traversing an entity with location
> information should never lead to traversing an entity without? What do RAV
> clients actually want here?

I think the basic idea of that goal is what we want. However, as an
example, template instantiations have location information, but they
shouldn't be traversed by default since that source code was already
traversed as part of the template declaration.

>  [0]: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2010-June/009273.html
>
>>
>> On Tue, Sep 4, 2012 at 11:43 AM, Philip Craig <philipjcraig at gmail.com>
>> wrote:
>> > On Mon, Sep 3, 2012 at 9:30 PM, Manuel Klimek <klimek at google.com> wrote:
>> >> On Mon, Sep 3, 2012 at 12:53 PM, Philip Craig <philipjcraig at gmail.com>
>> >> wrote:
>> >>> On Mon, Sep 3, 2012 at 8:14 PM, Manuel Klimek <klimek at google.com>
>> >>> wrote:
>> >>>> On Sat, Sep 1, 2012 at 7:41 AM, Philip Craig <philipjcraig at gmail.com>
>> >>>> wrote:
>> >>>>> RecursiveASTVisitor was traversing D->getTypeForDecl() for EnumDecl,
>> >>>>> but shouldn't (same as for other TagDecl). It also wasn't traversing
>> >>>>> the C++11 integer type.
>> >>>>
>> >>>> Don't we still visit the type as part of the typeloc traversal after
>> >>>> your patch?
>> >>>
>> >>> We still visit a type, but it's a different type. Previously it
>> >>> visited an EnumType (which is the type that is the result of this
>> >>> declaration, not part of it), but now it visits whatever type has been
>> >>> specified, if any (such as BuiltinType for int). Maybe I should have
>> >>> split this into separate patches?
>> >>>
>> >>>> Perhaps add a negative test for what we don't want to visit any more?
>> >>>
>> >>> I can if you think it's a good idea. I didn't for two reasons.
>> >>> - There's no end of things you could test for the absence of, is this
>> >>> important enough?
>> >>
>> >> Yes, the potential for negative tests is unlimited. On the other hand,
>> >> having regression tests is in my opinion very useful and valuable - if
>> >> one person has made the error once, chances are, somebody else will
>> >> introduce the same error again (I've seen that happen many times).
>> >>
>> >>> - Most of the other TypeDecl are already skipping this type, and have
>> >>> comments to that effect, but no tests. Should they have tests too?
>> >>
>> >> Well, the RAV is definitely undertested. But I don't think the
>> >> asymmetry is too bad here - suddenly having to write tests for
>> >> everything costs a lot of effort, but introducing tests when fixing
>> >> bugs / implementing new features will already give lots of pay-out at
>> >> comparatively little effort.
>> >>
>> >>> The way I'd prefer to test this is to visit everything, compare that
>> >>> against a whitelist of things we expected to visit, and fail the test
>> >>> if anything unexpected was visited, rather than having a blacklist of
>> >>> things not to visit. That seems like it will need a lot of test
>> >>> framework changes though.
>> >>
>> >> I like tests that test very specific things. In my experience those
>> >> tend to be easier to maintain over the long run.
>> >
>> > I'm going to split this patch into two. Here's the first patch, which
>> > removes getTypeForDecl() traversal. I've removed it from
>> > TemplateTypeParmDecl as well, and added tests for them. Once that is
>> > okay I'll send the second patch, which adds EnumDecl integer type
>> > traversal.
>> >
>> > Patch description:
>> > TypeDecl::getTypeForDecl() is always the type being defined by the
>> > declaration, so it isn't written in the source and shouldn't be
>> > traversed by RecursiveASTVisitor.
>
>