This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Improve semantic highlighting in dependent contexts (fixes #154)
ClosedPublic

Authored by nridge on Sep 22 2019, 9:29 PM.

Diff Detail

Event Timeline

nridge created this revision.Sep 22 2019, 9:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2019, 9:29 PM

The heuristic of guessing a concrete type seems fragile, and may not work for all cases. Instead of trying out test to guess the underlying type, I think we could introduce a new highlighting kind (e.g. entity.name.type.dependent) for this.

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

This could be member functions, a case is like

template<class T>
class Foo {
public:
  void foo() {
    this->foo();
  }
};
nridge marked an inline comment as done.Sep 24 2019, 1:22 PM
nridge added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
199

Thanks for the example.

Do you have a suggestion for how to discriminate this case? To me, it would seem logical to do it based on syntax (highlight as a member function if the expression forms the function name of a function call expression). That would require navigating from the expression to its parent node. Is there a way to do that?

nridge updated this revision to Diff 221583.Sep 24 2019, 1:31 PM

Updated to use a new "dependent type" highlighting

ilya-biryukov added inline comments.Sep 25 2019, 1:46 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
199

There is no way to do this in C++.
Even if the name is followed by a pair of parenthese, this could either be a field with overloaded operator() (e.g. a std::function<void()> field) or a function with the same name.

It's much better to pick a separate highlighting kind for dependent names, this follows the actual semantics of C++.

hokein added inline comments.Sep 25 2019, 2:56 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
176

I'm doubting we will encounter other exceptions.

struct X {
template<class T>
static T t;
};

template<typename T>
void f() {
  X::t<T>; // this is a `UnresolvedLookupExpr`.
}
199

+1, I think we should just highlight them as a dependent type.

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

thanks for catching this!

nridge marked 2 inline comments as done.Sep 25 2019, 9:17 AM
nridge added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
199

Of course, any attempt to disambiguate between a member function and a field would be heuristic only. I figured that would be better than nothing. But if you prefer using a separate highlighting for dependent names that resolve to a function or a variable, we could do that.

(Hokein, I assume you don't actually mean using the dependent *type* highlighting. Using a type highlighting for something we know is not a type, but rather a function or variable, would be rather confusing.)

hokein added inline comments.Sep 27 2019, 5:19 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
199

But if you prefer using a separate highlighting for dependent names that resolve to a function or a variable, we could do that.

I'd suggest we'd better follow this. The heuristic could fail in some cases. For these cases, the wrong highlighting'd confuse users too.

(Hokein, I assume you don't actually mean using the dependent *type* highlighting. Using a type highlighting for something we know is not a type, but rather a function or variable, would be rather confusing.)

sorry for the confusion, I meant for anything that could not resolve to a specific declaration (e.g. CXXDependentScopeMemberExpr, UnresolvedLookupExpr), we use the new dependent highlighting kind.

nridge updated this revision to Diff 222333.Sep 29 2019, 11:21 AM

Address review comments

I do feel strongly that types and non-types should be highlighted differently, so the updated patch adds two new dependent highlightings, one for types and one for variables/functions.

hokein added a comment.Oct 1 2019, 7:59 AM

I do feel strongly that types and non-types should be highlighted differently, so the updated patch adds two new dependent highlightings, one for types and one for variables/functions.

I see your point here, no objection.

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

nit: remove the {} and elsewhere, LLVM prefers not adding {} if the body only contains a single statement.

181

I think we should use E->getName() since we are highlighting the NameLoc below.

229

nit: we have kindForType for hanlding all types, so I'd move the logic of detecting the dependent type there.

nridge updated this revision to Diff 222725.Oct 1 2019, 4:48 PM
nridge marked 9 inline comments as done.

Address review comments

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

I did try this, but it doesn't quite work, because VisitTypeLoc adds the highlighting to the TypeLoc's getBeginLoc(). For something like typename T::type, that highlights the typename token rather than the type token. By contrast, here I add the highlighting to the DependentNameTypeLoc's getNameLoc() which will correctly highlight the type token.

ilya-biryukov added inline comments.Oct 2 2019, 1:51 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
229

You'd want to implement WalkUpFromDependentNameTypeLoc instead of Visit* to avoid adding extra highlightings in VisitTypeLoc.

In fact, I'm surprised we're not seeing them now.

ilya-biryukov added inline comments.Oct 2 2019, 3:09 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
519

Maybe have a separate category for all dependent entities instead, i.e. use
entity.name.dependent.type.cpp and entity.name.dependent.other.cpp?

This would allow to specify a single highlighting for both names by stopping at dependent subcategory.
I'm not sure how this plays into the default colors provided in the editors that support semantic highlighting...

hokein added inline comments.Oct 2 2019, 6:31 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
519

Having a dedicate dependent entity doesn't align with the existing textmate patterns (the dependent type should be under the entity.name.type umbrella) -- most highlighters don't have a specific pattern for dependent, so we'll fallback to the entity.name.type color. If we use entity.name.dependent.type.cpp, we'll fall back to entity.name color.

nridge marked 2 inline comments as done.Oct 3 2019, 2:19 PM
nridge added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
229

We're not seeing extra highlightings with the current patch, because VisitTypeLoc does not add any highlightings for dependent types (kindForType() returns None for them).

So, I don't think there's a problem with using VisitDependentNameTypeLoc?

519

+1, I think it's more important that dependent types are highlighted as types out-of-the-box in cases where the theme contains a highlighting for types but not one specifically for dependent types.

hokein added inline comments.Oct 9 2019, 6:05 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
229

I'm second on Ilya's suggestion. It follows the existing pattern (see WalkupFrom* methods above) and make the code clearer, and we can move the DependentType to kindForType.

nridge updated this revision to Diff 224241.Oct 9 2019, 8:34 PM

Updated to use WalkUpFromDependentNameTypeLoc

nridge added a comment.Oct 9 2019, 8:36 PM

Is this what you had in mind?

I'm not seeing where the kindForType part comes in. In particular, it seems like it would be silly to call kindForType in WalkUpFromDependentNameTypeLoc(), because we know the answer will be HighlightingKind::DependentType.

ilya-biryukov added inline comments.Oct 10 2019, 12:53 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
175

Could we highlighting based on the kinds of E->decls()? If they're all the same, you could just use the corresponding highlighting kind.
Most of the time ADL at template instantiation time does not affect the resulting highlighting, so we could just assume it's going to stay the same.

If there are no decls() or some of them have different highlighting kinds (e.g. static and non-static member functions), you could fallback to DependentName.

180

Same here. Could we try guessing the highlighting type based on E->decls()?

nridge updated this revision to Diff 224453.Oct 10 2019, 12:49 PM

Updated to use heuristics based on OverloadExpr::decl()

Also folded VisitUnresolvedLookupExpr and VisitUnresolvedMemberExpr together
into a single VisitOverloadExpr

nridge marked 3 inline comments as done.Oct 10 2019, 12:51 PM

I like how we went from using heuristics, to not using heuristics, to using heuristics again :)

But admittedly, the new heuristics are more accurate because they're based on phase 1 lookup results rather than just syntax, so I'm happy with the outcome!

I like how we went from using heuristics, to not using heuristics, to using heuristics again :)

But admittedly, the new heuristics are more accurate because they're based on phase 1 lookup results rather than just syntax, so I'm happy with the outcome!

I also liked the irony of it :-)

Mostly LG, just NITs from my side. Happy to LGTM when they're addressed.

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

Could we do this in kindForDecl instead?
I suspect we want to be consistent in other cases this might potentially called in.

112

Maybe simplify the rest of the loop body to the following code?

auto Kind = ...;
if (!Kind || Result && Result != Kind)
  return llvm::None;
Result = Kind;

If you want to have fewer assignments, we could also do:

if (!Result) Result = Kind;

at the end. But I'd just keep it a tad simpler instead.

nridge updated this revision to Diff 224767.Oct 12 2019, 7:11 PM

Address final comments

nridge marked 5 inline comments as done.Oct 12 2019, 7:12 PM

LGTM from my side, but not marking as accepted yet to make sure @hokein has a chance to take a final look.

hokein accepted this revision.Oct 14 2019, 1:28 AM

thanks, looks great now!

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

nit: remove the enclosing {}.

106

nit: the parameter type seems a bit tangled, may use llvm::iterator_range<UnresolvedSetIterator>, but it is not a big deal, up to you.

This revision is now accepted and ready to land.Oct 14 2019, 1:28 AM
nridge updated this revision to Diff 224880.Oct 14 2019, 11:18 AM
nridge marked 2 inline comments as done.

Address last review comments

This revision was automatically updated to reflect the committed changes.