Page MenuHomePhabricator

[clangd] Handle members of anon structs in SelectionTree
ClosedPublic

Authored by kadircet on Sep 30 2021, 6:27 AM.

Details

Summary

References to fields inside anon structs contain an implicit children
for the container, which has the same SourceLocation with the field.
This was resulting in SelectionTree always picking the anon-struct rather than
the field as the selection.

This patch prevents that by claiming the range for the field early.

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

Diff Detail

Event Timeline

kadircet created this revision.Sep 30 2021, 6:27 AM
kadircet requested review of this revision.Sep 30 2021, 6:27 AM
kadircet added inline comments.Sep 30 2021, 6:28 AM
clang-tools-extra/clangd/Selection.cpp
754

in theory this check should be redundant, as each field should own its tokens. but given the complexity of C++ just wanted to be conservative.

kadircet edited the summary of this revision. (Show Details)Sep 30 2021, 6:29 AM

Thanks for fixing!

Early-claim is a blunt hammer and I'm always dreading side-effects.
This also feels a *lot* like implicit-this which is handled in another way.

So that seems worth a shot, but this approach is fine if that one doesn't work.

clang-tools-extra/clangd/Selection.cpp
443

seems like it'd be more consistent to handle it here?

If it's a MemberExpr and the member is in an anonymous struct, then it's implicit if isImplicit(ME.getBase()).

clang-tools-extra/clangd/unittests/XRefsTests.cpp
368

This example is quite complicated to the point where it's hard to see what it's testing... The example you have in the comment is much clearer.

If we also want to test named fields with inline struct types, maybe that should be a second test...

kadircet added inline comments.Sep 30 2021, 7:14 AM
clang-tools-extra/clangd/Selection.cpp
443

the test case was actually to demonstrate why we can't do it here. we want to still keep traversing the AST after hitting a field inside an anon struct. handling here terminates the traversal for that subtree completely, and the real node we are interested might be down the tree (y.[[x]] in the test case).

clang-tools-extra/clangd/unittests/XRefsTests.cpp
368

it is actually important to demonstrate we are not claiming the whole range at the first encounter (and to make sure traversal is not stopped for any other reason).

sammccall added inline comments.Sep 30 2021, 8:04 AM
clang-tools-extra/clangd/Selection.cpp
443

That's what I meant by isImplicit(ME.getBase())

The following passes tests (added to isImplicit)...

if (auto *ME = llvm::dyn_cast<MemberExpr>(S)) 
  if (auto *FD = llvm::dyn_cast<FieldDecl>(ME->getMemberDecl()))
    if (FD->isAnonymousStructOrUnion())
      return isImplicit(ME->getBase());

and the member is in an anonymous struct

Oops, I meant "and the member *is* an anonymous struct".
It seems more natural to recognize and special-case the field-access to the anon struct itself (which is intuitively implicit!), than the one to a normal field within it.

sammccall added inline comments.Sep 30 2021, 8:08 AM
clang-tools-extra/clangd/unittests/XRefsTests.cpp
368

Makes sense, but without comment it's not clear.
Can we make the latter a separate test case, and simplify it too? This seems to be enough:

struct [[Foo]] {
  struct { int x; }
};
int a = ^Foo{}.x;

Here the Foo{} is the non-empty base of the anon member access that must be traversed.

kadircet updated this revision to Diff 376238.Sep 30 2021, 8:47 AM
kadircet marked 4 inline comments as done.
  • Use isImplicit rather than earlyClaim
kadircet added inline comments.Sep 30 2021, 8:47 AM
clang-tools-extra/clangd/Selection.cpp
443

right, it makes sense. i was missing the isImplicit(base) bit + got stunned by the beautiful shape of the AST. thanks for bearing with me :)

sammccall accepted this revision.Sep 30 2021, 9:41 AM

Thanks!

clang-tools-extra/clangd/Selection.cpp
446

hmm, "implicit accesses of anonymous structs"?

450

The double negatives and references to a "traversal" here seem confusing.
Maybe something more concrete?

// If Base is an implicit CXXThis, then the whole MemberExpr has no tokens.
// If it's a normal e.g. DeclRef, we treat the MemberExpr like an implicit cast.
This revision is now accepted and ready to land.Sep 30 2021, 9:41 AM
kadircet updated this revision to Diff 376445.Oct 1 2021, 1:18 AM
kadircet marked 2 inline comments as done.
  • update comments
This revision was automatically updated to reflect the committed changes.