This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add semanticTokens modifiers for function/class/file/global scope
ClosedPublic

Authored by sammccall on Jan 29 2021, 2:41 PM.

Details

Summary

These allow (function-) local variables to be distinguished, but also a
bunch more cases.
It's not quite independent with existing information (e.g. the
field/variable distinction is redundant if you have class-scope + static
attributes) but I don't think this is terribly important.

Depends on D77811

Diff Detail

Event Timeline

sammccall created this revision.Jan 29 2021, 2:41 PM
sammccall requested review of this revision.Jan 29 2021, 2:41 PM
nridge accepted this revision.Feb 2 2021, 10:29 PM

Looks good to me!

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

code completion? I'm a bit lost :)

469

The testcase which relies on this is this one:

// Dependent template name
R"cpp(
template <template <typename> class> struct $Class[[A]] {};
template <typename $TemplateParameter[[T]]>
using $Typedef[[W]] = $Class[[A]]<
  $TemplateParameter[[T]]::template $DependentType[[Waldo]]
>;

However, it does appear that we get into here even for non-dependent template template arguments (but then also get a non-dependent highlighting kind via findExplicitReferences(), and end up discarding the DependentType via resolveConflict()).

785

Maybe add a // nonstandard comment for these as well

clang-tools-extra/clangd/SemanticHighlighting.h
82

Would it make sense to call this NamespaceScope instead?

I understand it's "global" in the sense that it's visible to other translation units, but it's not "global" in the sense that it's not necessarily in the global namespace.

(On the other hand, I can see how FileScope symbols are also "namespace scope", so... could go either way.)

This revision is now accepted and ready to land.Feb 2 2021, 10:29 PM
nridge added inline comments.Feb 2 2021, 10:56 PM
clang-tools-extra/clangd/SemanticHighlighting.cpp
469

I see you've already figured this out in D95706 :)

nridge added inline comments.Feb 2 2021, 11:27 PM
clang-tools-extra/clangd/test/initialize-params.test
91

nit: missing commas

This revision was landed with ongoing or failed builds.Feb 9 2021, 8:57 AM
This revision was automatically updated to reflect the committed changes.
sammccall marked 5 inline comments as done.
sammccall added inline comments.Feb 9 2021, 9:10 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
383

Whoops, fixed.

(This was copied from a utility function that's used in code completion ranking - I planned to share it, but it turns out we want slightly different semantics. Code completion happens inside callbacks that happen mid-parse, and if you call getLinkageInternal() at just the wrong time, you end up caching an intermediate state and crash. But I removed the check as it's not relevant here)

469

Oops, yes. I've adjusted the comment here so the intermediate state makes sense :-)
(I'd rather not emit dubious tokens and then rely on conflicts to discard them, it seems hard to maintain)

clang-tools-extra/clangd/SemanticHighlighting.h
82

Yeah, the file-scope thing. (Maybe it's not an important distinction, but it seems pretty interesting for functions at least)

The other consideration is that I harbor a little bit of hope we could get some convergence across implementations, so avoiding lang-specific terms is nice. In any case, client implementers are probably not C++ people!
(of course we can use different names internally than we use in the protocol, but it seems less confusing to align them)

nridge added inline comments.Feb 9 2021, 11:36 AM
clang-tools-extra/clangd/SemanticHighlighting.h
82

+1 for a distinct FileScope being useful