Page MenuHomePhabricator

[AST] Preserve more structure in UsingEnumDecl node.
ClosedPublic

Authored by sammccall on Sep 20 2022, 12:45 PM.

Details

Summary
  • store NestedNameSpecifier & Loc for the qualifiers This information was entirely missing from the AST.
  • expose the location information for enum/qualifier/identifier as typeloc (ElaboratedTypeLoc+EnumTypeLoc for now). This allows many traversals/astmatchers etc to handle these generically along with other references. The decl vs type split can help preserve typedef sugar when https://github.com/llvm/llvm-project/issues/57659 is resolved.
  • fix the SourceRange of UsingEnumDecl to include 'using'.

Fixes https://github.com/clangd/clangd/issues/1283

Diff Detail

Event Timeline

sammccall created this revision.Sep 20 2022, 12:45 PM
Herald added a project: Restricted Project. · View Herald Transcript
sammccall requested review of this revision.Sep 20 2022, 12:45 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 20 2022, 12:45 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I just saw this touches the same code as @urnathan's D134283, specifically Sema::ActOnUsingEnumDeclaration.

I'm happy to wait for that to land and then merge, I think these will play nicely together. The changes to the actual UsingEnumDecl class are the main thing I'd like some feedback on, and those won't be affected.

nridge added a subscriber: nridge.Sep 21 2022, 12:50 AM

AFAICT the UsingDecl doesn't capture the NestedNameSpecifier location, is UsingEnumDecl special in some way, or would this information be better placed in BaseUsingDecl? Thanks for spotting the possible mid-air collistion with D134283.

AFAICT the UsingDecl doesn't capture the NestedNameSpecifier location

We have NestedNameSpecifierLoc UsingDecl::QualifierLoc already, maybe I'm misunderstanding.

is UsingEnumDecl special in some way, or would this information be better placed in BaseUsingDecl?

It would be possible to unify the two by moving UsingDecl's QualifierLoc into BaseUsingDecl..

I think this comes at the cost of a worse API for UsingEnumDecl: making the qualifier as part of a Typeloc/TypeSourceInfo means that AST consumers with some generic handling for TypeLocs can reuse it. (e.g. the clangd changes in this patch are mostly just testing various things that now work for free).

UsingDecl already has that comparatively awkward API today - it's one of the AST nodes that very often needs to be handled explicitly - unavoidably as "reference to a decl" is not modeled in the AST, and because it may point to an overload set etc anyway.

martong added inline comments.Sep 22 2022, 1:57 AM
clang/lib/AST/ASTImporter.cpp
4872

This name would be more consistent with getEnumType.

Does this still apply with the fix for dr2621 landed?

Does this still apply with the fix for dr2621 landed?

Not cleanly, as getType already creates the ElaboratedTypeLoc for the qualifier but without the enum keyword. Wrapping it with a second ElaboratedType isn't consistent with how these types are modeled elsewhere in the AST. Plumbing through parameters to make getType() add the elaborated-type-keyword seems disproportionately invasive.
I think probably it's best just to drop the idea that "enum" is part of a tag name (especially since now the name can be a typedef) and keep treating it as an extra SourceLocation instead.

sammccall updated this revision to Diff 464118.Sep 29 2022, 5:54 PM

Rebase on top of using enum parsing changes, store 'enum' location separately.

ping, +additional potential reviewer

ilya-biryukov accepted this revision.Oct 10 2022, 2:11 AM

Modelling this as a TypeLoc for code reuse reasons gives me a pause too.
However, I think we still accept this as keeping the clients simpler seems like the right trade-off to me.

Putting information about both typedefs and qualifiers explicitly will make the representation of UsingEnumDecl way too complicated for a seemingly simple and niche feature.

clang-tools-extra/clangd/unittests/SelectionTests.cpp
554

Could we also test that checks what happens in presence of typedefs?

namespace enums {
  enum class X { a, b};
  using Y = X;
}

namespace uses {
    using enum enums::Y;
}
This revision is now accepted and ready to land.Oct 10 2022, 2:11 AM
hokein accepted this revision.Oct 10 2022, 2:29 AM

it looks good to me, just a few optional nits.

clang/include/clang/AST/DeclCXX.h
3619

nit: we only rename EnumLocation but not the UsingLocation, it is a little wired, I think either we rename both or keep both unchanged.

3619–3620

I think we can probably get rid of the Enum field, since we are storing the EnumType and we can get the EnumDecl by dyn_cast<EnumDecl>(EnumType->getType()->getAsTagDecl())

clang/lib/Sema/SemaDeclCXX.cpp
11874–11881

nit: adding = nullptr initializer

This revision was landed with ongoing or failed builds.Oct 12 2022, 10:55 AM
This revision was automatically updated to reflect the committed changes.
sammccall marked 5 inline comments as done.
sammccall added inline comments.Oct 12 2022, 10:57 AM
clang/include/clang/AST/DeclCXX.h
3619

oops, at some point this was matching the use of a TypeLoc vs SourceLocation, but no longer does. Fixed

3619–3620

Good point. Did this an added an assert in create().