This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Implement semantic token modifier "definition"
ClosedPublic

Authored by ckandeler on Jun 9 2022, 7:38 AM.

Diff Detail

Event Timeline

ckandeler created this revision.Jun 9 2022, 7:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2022, 7:38 AM
ckandeler requested review of this revision.Jun 9 2022, 7:38 AM
ckandeler updated this revision to Diff 435547.Jun 9 2022, 7:40 AM

Indentation.

ckandeler updated this revision to Diff 435561.Jun 9 2022, 8:24 AM

Addressed clang-format complaints.

Trass3r added a subscriber: Trass3r.Jun 9 2022, 9:26 AM
dgoldman added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
847–849

Do we need similar code for ObjCProtocolDecl? Also should ObjCImplDecl be considered definitions here?

nridge added a subscriber: nridge.Jun 12 2022, 12:06 AM
ckandeler added inline comments.Jun 12 2022, 11:53 PM
clang-tools-extra/clangd/SemanticHighlighting.cpp
847–849

Possibly. I know next to nothing about Objective C, so I'll just do as I'm told here. On a related note, the code above regarding ObjCMethodDecl does not seem to do anything, i.e. none of the constructs that to my eye appear to be Objective-C methods get the "def" modifier.

dgoldman added inline comments.Jun 13 2022, 8:20 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
847–849

Gotcha, yeah I think it makes sense to do the same for ObjCImplDecl, ObjCProtocolDecl, and ObjCCategoryDecl.

re: methods, ah yeah, that's because canHighlightName will return false since we need to special case handling for ObjC methods since their names can be split across non contiguous tokens (selectors). Instead, would need to update VisitObjCMethodDecl and highlightObjCSelector.

Objective-C improvements.

ckandeler updated this revision to Diff 448655.Jul 29 2022, 9:11 AM

Addressed remaining Objective-C issues

dgoldman added inline comments.Sep 8 2022, 11:58 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
837

Instead of doing it like this, I wonder if would make more sense to call getDefinition from https://cs.github.com/llvm/llvm-project/blob/ae071a59bc6cc09e0e0043e0ef1d9725853f1681/clang-tools-extra/clangd/XRefs.cpp#L76 and if it matches Decl, add the Definition modifier?

ckandeler added inline comments.Sep 12 2022, 2:14 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
837

Won't that incur an additional look-up or some other type of work? I'm not deeply familiar with the implementation, but a cursory glance seems to suggests that isThisDeclarationADefinition() is just an accessor, while getDefinition() "does things".

sammccall accepted this revision.Sep 28 2022, 10:20 PM

This is fantastic, I'm really not sure how I missed it, sorry :-(

clang-tools-extra/clangd/SemanticHighlighting.cpp
831

nit: move this comment to above the inner if?

837

Yeah, getDefinition may need to walk over all the redecl chain to find the actual def, which we don't care about.
Probably not a big deal, but it's not quite the right abstraction, and that function isn't exposed publicly currently anyway.

837

can you pull these checks out into a function bool isUniqueDefinition or so?

("unique" because I think we're intentionally returning false for things like NamespaceDecls which are definitions but may be multiply-defined)

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
54

I wonder if we want to special case when we're printing _def to not print _decl and instead assert that it is present?

It's a bit irregular but as you've seen this gets everywhere in the tests, and the noise level seems concerning.

This revision is now accepted and ready to land.Sep 28 2022, 10:20 PM
ckandeler marked 3 inline comments as done.

Adapted according to latest comments.

ckandeler added inline comments.Sep 30 2022, 12:56 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
77

I'm not 100% sure about this one, by the way. I've just never heard anyone talk about a "parameter definition".

So, is this okay to merge now?

sammccall accepted this revision.Oct 10 2022, 7:20 AM

Thanks! Sorry for letting this drop.

clang-tools-extra/clangd/SemanticHighlighting.cpp
77

ParmVarDecls should be marked as definition. They definitely are in the formal C++ sense, and they're so similar to local variables that I think we have to be consistent.
(There are other formally-definitions that we're not marking here like NamespaceDecl, but they seem unlikely to cause confusion).

I've just never heard anyone talk about a "parameter definition".

Yeah, I suspect this is mostly because:

  • there's no definition/declaration distinction - you can't forward-declare a param.
  • imprecise mental models of params as aliases to the written args rather than copies of them (at least I sometimes think this way)

But I think it's important we treat these consistently with e.g. local variables since they're so similar.

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
58

can you add a brief comment here: "// _decl_def is common and redundant, just print _def instead."

ckandeler updated this revision to Diff 466510.Oct 10 2022, 8:15 AM
ckandeler marked 2 inline comments as done.

Use modifier also for parameters,

ckandeler requested review of this revision.Oct 10 2022, 8:17 AM
sammccall accepted this revision.Oct 10 2022, 10:20 AM

Just realized ParmVarDecls (and template args) are maybe more complicated than I thought: I suppose they're only really definitions if the function/template has a body (i.e. is itself a definition).
Anyway don't worry about that, let's get this landed and maybe refine later.

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
804–805

nit: T should be a def here too.
it's not critical so if this case is hard at all feel free to leave it.

This revision is now accepted and ready to land.Oct 10 2022, 10:20 AM
ckandeler updated this revision to Diff 466731.Oct 11 2022, 1:48 AM

Template parameters are definitions

ckandeler marked an inline comment as done.Oct 11 2022, 1:48 AM

Everything good now?

Everything good now?

Yes :-)
(I also marked it as accepted above, so feel free to land!)
Thank you!

Could you please land it for me? I don't have the privileges. (How does one attain those, by the way?)

This revision was automatically updated to reflect the committed changes.

I went ahead and landed this for you

ormris added a subscriber: ormris.Oct 17 2022, 3:43 PM

Looks like there are some test failures associated with this commit: https://lab.llvm.org/buildbot/#/builders/123/builds/13698

The change list is a bit confusing, as it's only showing LLVM commits. But the change list includes the commit before and the commit after this one.

The tests in this patch are also failing in our internal CI. @ckandeler, could you take a look?

Looks like an error introduced when rebasing the patch: a recently added test case (for https://github.com/clangd/clangd/issues/1222) needs to be updated to reflect the _decl to _def change that this patch makes to all semantic highlighting tests. I'll land a fix.

thakis added a subscriber: thakis.Oct 17 2022, 4:47 PM

Looks like this breaks check-clangd everywhere, eg http://45.33.8.238/linux/89237/step_9.txt

Landing the fix now

Still failing with that: http://45.33.8.238/linux/89240/step_9.txt

Time to revert to green? Things have been broken for a few hours now. Maybe time to regroup and analyze async?

Fixed in https://reviews.llvm.org/rGd5a99bf5e134, and confirmed via a local build and test run that the tests are passing. Apologies for the breakage.

Fixed in https://reviews.llvm.org/rGd5a99bf5e134, and confirmed via a local build and test run that the tests are passing. Apologies for the breakage.

Thanks.