Page MenuHomePhabricator

[AST] Add a sugar type for types found via UsingDecl
Needs ReviewPublic

Authored by sammccall on Fri, Nov 19, 8:21 AM.

Details

Summary

Currently there's no way to find the UsingDecl that a typeloc found its
underlying type through. Compare to DeclRefExpr::getFoundDecl().

This is critical for some tooling use cases, e.g. finding unused #include directives.
(AST-based approaches to this problem are imperfect, but nevertheless useful).
Moreover in general clang does retain such sugar in the AST.

Design decisions:

  • a sugar type, as there are many contexts this type of use may appear in
  • UsingType is a leaf like TypedefType, the underlying type has no TypeLoc
  • not unified with UnresolvedUsingType: a single name is appealing, but being sometimes-sugar is often fiddly.
  • not unified with TypedefType: the UsingShadowDecl is not a TypedefNameDecl or even a TypeDecl, and users think of these differently.
  • does not cover other rarer aliases like objc @compatibility_alias, in order to be have a concrete API that's easy to understand.

Scope:

  • This does not cover types associated with template names introduced by using declarations. A future patch should introduce a sugar TemplateName variant for this. (CTAD deduced types fall under this)
  • There are enough AST matchers to fix the in-tree clang-tidy tests and probably any other matchers, though more may be useful later.

Downsides:

  • This changes a common pattern in the AST people likely depend on matching. Previously, typeLoc(loc(recordType())) matched whether a struct was referred to by its original scope or introduced via using-decl. Now, the using-decl case is not matched, and needs a separate matcher. This is similar to the case of typedefs but nevertheless both adds complexity and breaks existing code.

Diff Detail

Event Timeline

sammccall created this revision.Fri, Nov 19, 8:21 AM
sammccall requested review of this revision.Fri, Nov 19, 8:21 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFri, Nov 19, 8:21 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sammccall edited the summary of this revision. (Show Details)Fri, Nov 19, 11:26 AM
sammccall added inline comments.Fri, Nov 19, 11:34 AM
clang/include/clang/AST/Type.h
4373

Hmm, maybe I should be getting this from Found, rather than storing it.
We'd have to do more work than e.g. TypedefType though, as we have to check the possible types of Found.

davrec added a subscriber: davrec.Fri, Nov 19, 5:26 PM

Looks good, a few notes.

clang/include/clang/AST/ASTContext.h
1559

I believe you mentioned ObjC considerations might require expanding beyond UsingShadowDecl as the type of Found -- is that why this is a generic NamedDecl? I.e. can Found indeed be other things than a UsingShadowDecl?

clang/include/clang/AST/RecursiveASTVisitor.h
984

This should just be DEF_TRAVERSE_TYPE(UsingType, {}), i.e. same as for TypedefType -- RecursiveASTVisitor only visits the child nodes, to avoid retraversal of any nodes; by traversing the desugared node this would jump back to a node it probably already traversed.

1256

DEF_TRAVERSE_TYPELOC(UsingType, {})

clang/include/clang/AST/Type.h
4382

I would rename this to getDecl(), to match the interface for other types that just wrap a decl. E.g. if something is a RecordType I know I can call getDecl() to get the RecordDecl; likewise a TypedefType::getDecl() returns a TypedefNameDecl; I think it would follow this pattern for UsingType to have a getDecl() method that returns a UsingShadowDecl (or whatever else it can be, per other question).

clang/include/clang/AST/TypeLoc.h
671

Should this be analogous to TypedefTypeLoc, with base as

InheritingConcreteTypeLoc<TypeSpecTypeLoc,
                                                 UsingTypeLoc,
                                                 UsingType>

?

clang/include/clang/AST/TypeProperties.td
366

Rename to "declaration" iff renaming UsingType::getFoundDecl to UsingType::getDecl

clang/lib/AST/ASTContext.cpp
4612

UsingType *NewType = new (*this, TypeAlignment) UsingType(Found, Underlying, Canon)

clang/lib/AST/ASTStructuralEquivalence.cpp
952

Is there a good reason this checks the underlying type in addition to getFoundDecl()? I'm looking at the Typedef case below and it only checks the decl, does this need to be different from that?

clang/lib/AST/Type.cpp
1819

Do we need this here? I ask because I mainly because don't see a VisitTypedefType, not 100% sure what this class does though.

sammccall marked 7 inline comments as done.Fri, Nov 26, 11:07 AM

Thanks a lot for the comments, very helpful.
The next revision is much narrower: it'll only sugar exactly the type associated with a UsingShadowDecl. It no longer wraps the underlying type.
I've added fixes for all the clang-tools-extra breakages, and enough AST-matchers to support that.

(Slow turnaround: I am interested in this but quarantined with small kids right now...)

clang/include/clang/AST/ASTContext.h
1559

That was the reason. NamedDecl is what lookup returns, and the code around DeclRefExpr suggests it can be an objc compatibility alias.

However I'm not sure trying to generalize across cases I don't understand well is a great idea. I've narrowed this to cover UsingShadowDecl only.

clang/include/clang/AST/RecursiveASTVisitor.h
984

You're right, changed the underlying type to not be a child.

(Before I'd fully thought through the template case, I thought we wanted to UsingType to wrap TemplateSpecializationType, and so it had to contain the underlying type to capture the args. But that model is silly, and templates are best handled in TemplateName separately)

clang/include/clang/AST/Type.h
4382

I do prefer getFoundDecl() for a few reasons:

  • the parallel with NamedDecl::getFoundDecl() is closer/more important than with TypedefDecl I think
  • there are always two decls here: the invisible UsingShadowDecl and the underlying one. Saying "decl" without a hint seems error-prone to me. (Compare with TypedefType where in general there's no underlying decl).
  • I do find TypedefType::getDecl() confusing, because wherever I see it called I have to verify that it's TypedefType::getDecl() rather than some Type::getDecl() to be sure I understand the semantics.

Would be happy to hear a third opinion here though.

clang/include/clang/AST/TypeLoc.h
671

Yup, after switching to not wrapping the underlying type this is much cleaner, thanks!

clang/lib/AST/ASTContext.cpp
4612

Wow, well spotted, thank you!

clang/lib/AST/ASTStructuralEquivalence.cpp
952

I figured if I was storing both, not checking both would invite trouble.
And in fact I eventually found such a case: where the underlying was a CTAD type and the decl was the template name.

But now I've made sure we don't handle that case, store only the decl, and assert that the underlying type is what we expect.

clang/lib/AST/Type.cpp
1819

No longer needed now the underlying cannot be a CTAD type.

sammccall marked 6 inline comments as done.
sammccall edited the summary of this revision. (Show Details)
  • Address review comments, make the type class much more concrete
  • upgrade clang-tools-extra, add matchers
  • add tests
  • release notes
sammccall edited the summary of this revision. (Show Details)Fri, Nov 26, 11:15 AM
sammccall added reviewers: davrec, rsmith, mizvekov.