This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by sammccall on Nov 19 2021, 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.Nov 19 2021, 8:21 AM
sammccall requested review of this revision.Nov 19 2021, 8:21 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 19 2021, 8:21 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
sammccall edited the summary of this revision. (Show Details)Nov 19 2021, 11:26 AM
sammccall added inline comments.Nov 19 2021, 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.Nov 19 2021, 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.Nov 26 2021, 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)Nov 26 2021, 11:15 AM
sammccall added reviewers: davrec, rsmith, mizvekov.
davrec accepted this revision.Nov 29 2021, 3:55 AM

Looks great, thanks for identifying the need and banging this out so quickly.
Hope you had some time to enjoy the holiday with your family!
Dave

clang/include/clang/AST/RecursiveASTVisitor.h
2106

So the idea is, the UsingDecl which introduced the shadows will have already been traversed within its DeclStmt via the body traversal, but not the UsingShadowDecls it introduced? Nicely spotted.

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

I'm persuaded by your reasoning, particularly that UsingShadowDecl is invisible and thus returning one already introduces some confusion -- i.e. a user might reasonably expect getDecl() to return a UsingDecl, so better to call this something with other parallels.

This revision is now accepted and ready to land.Nov 29 2021, 3:55 AM
sammccall marked an inline comment as done.Dec 8 2021, 5:40 AM

Thanks Dave!

A last ping in case @rsmith has opinions here as clang owner. Otherwise I'd probably land this in a week or so.

clang/include/clang/AST/RecursiveASTVisitor.h
2106

That's right, they're not children of the body because CompoundStmt is not DeclContext.

This bug seems to have existed for a long time, but this change caused it to be uncovered in existing clang-tidy tests.

sammccall updated this revision to Diff 394553.Dec 15 2021, 6:48 AM
sammccall marked an inline comment as done.

update lldb

Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2021, 6:48 AM
sammccall updated this revision to Diff 394648.Dec 15 2021, 1:19 PM

Fix QualTypeNames bug that showed up in external testing.
(Now covered by a unittest added separately)

sammccall updated this revision to Diff 394666.Dec 15 2021, 2:49 PM

TypePrinter prints UsingTypes with their underlying qualifiers by default.
Fix qualifier bug in QualTypeNames
Add a bit more detail to AST dump (UsingShadow)

Having done some out-of-tree testing, it seems this silently breaks enough ASTMatchers that it'll be hard to get it to stick.

(We don't strictly need to block on out-of-tree failures, but I'm pretty sure a lot of stuff in-tree is broken too, and the out-of-tree users just have better test coverage).

It would be great if things like expr(hasType(hasDeclaration(cxxRecordDecl(...)))), loc(qualType(hasDeclaration(cxxRecordDecl(...)))) still matched the cases they used to. Even with this change at front of mind, it's really surprising and inconvenient to have to explicitly unwrap this level of sugar.
ElaboratedType gets implicitly unwrapped by hasDeclaration, I suspect UsingType is more usefully treated like this than like TypedefType.
Then some other special-purpose matcher would be used to traverse the UsingType->UsingShadowDecl edge instead of hasDeclaration.

Make hasDeclaration look through UsingType, and throughUsingDecl work on
UsingType too. This is more ergonomic, more similar to existing behavior
that matchers rely on, and similar in spirit to DeclRefExpr.

sammccall updated this revision to Diff 394806.Dec 16 2021, 3:08 AM

Fix serialization (found with msan)

davrec added a comment.EditedDec 16 2021, 7:20 AM

throughUsingDecl seems like a good solution, though I defer to regular ASTMatchers users.

One other thought: can we add diagnostic notes using this new information, e.g.

struct A1 {
protected:
  using IntType = char;
};

struct A2 {
protected:
  using IntType = unsigned long;
};

struct B : A1, A2 {
  using A1::IntType;
};

struct C : B {
  IntType foo;
  void setFoo(IntType V) {
    foo = V;
  }
};


int main() {
  C c;
  c.setFoo((unsigned long)-1); // warning: implicit conversion loses info 
}

It would be nice to add a note pointing to using A1::IntType saying "introduced here".

sammccall updated this revision to Diff 395434.Dec 20 2021, 6:47 AM

Fix ASTImporter, add test.

This revision was landed with ongoing or failed builds.Dec 20 2021, 8:16 AM
This revision was automatically updated to reflect the committed changes.

One other thought: can we add diagnostic notes using this new information, e.g.

This sounds plausible, though I didn't want to extend the scope of this patch further.
I'm also not sure how often this will be signal rather than noise - I would expect in a large majority of cases that understanding exactly how the name was brought into scope isn't going to be important to understanding or fixing the error. I may be missing something, but the example seems a bit contrived.