This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Use identifier range as the definition range.
ClosedPublic

Authored by hokein on Mar 8 2018, 1:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.Mar 8 2018, 1:59 AM

I was going to change the symbol index to do the opposite :) The range of definitions including the bodies of functions, etc is used in a "peek definition" feature by several LSP clients. So for example in VSCode, you can hold Ctrl and hover on a function call and see its definition in a popup. There was some discussion about this in https://reviews.llvm.org/D35894

sammccall added inline comments.Mar 8 2018, 7:28 AM
clangd/SourceCode.cpp
55 ↗(On Diff #137549)

As discussed offline, this heuristic is... limited.

#define X Y
class X{}

and now we say the definition of the class Y is on the first line.

I think we should really be basing the heuristics on the whole definition range, even if we're then going to return just the position of the name.

This patch just moves logic around so we don't need to do it now, but it could use a FIXME

clangd/SourceCode.h
34 ↗(On Diff #137549)

This is a useful function, but it doesn't belong in SourceCode with its current scope. See the file header :)

We could add an "AST.h" or similar file, or expand the scope of SourceCode, but we haven't needed to yet because clang/AST normally has what we need.

Can this be a method on NamedDecl or a function alongside?
But since (later comment) the logic seems wrong, I'm not sure it makes sense to move it at the moment. Maybe create a separate header?

36 ↗(On Diff #137549)

Not sure it's neccesary to spell out the details of what's happening inside macros, but it would be useful to describe the normal case :-)
e.g. This is usually the spelling location, where the name of the decl occurs in source code.

39 ↗(On Diff #137549)

This name seems opaque to me.

findName?

39 ↗(On Diff #137549)

would it be more useful to return the range, and just pick out the start if you're not interested in both?

clangd/XRefs.cpp
136 ↗(On Diff #137549)

The name of this function is *really* confusing, so it took me a long time to understand the callsite.
Consider rename to makeLocaction

194 ↗(On Diff #137549)

I think this comment relates to the previous line, not the next one.
But consider just removing it - if getDeclSourceLoc had a clearer name, it'd be self-documenting.

I was going to change the symbol index to do the opposite :) The range of definitions including the bodies of functions, etc is used in a "peek definition" feature by several LSP clients. So for example in VSCode, you can hold Ctrl and hover on a function call and see its definition in a popup. There was some discussion about this in https://reviews.llvm.org/D35894

Interesting - that seemed like a more natural interpretation of LSP to me.
Others talked me down, motivated (I think) by nicer behavior of jump-to-definition... will bring it up again :)

I was going to change the symbol index to do the opposite :) The range of definitions including the bodies of functions, etc is used in a "peek definition" feature by several LSP clients. So for example in VSCode, you can hold Ctrl and hover on a function call and see its definition in a popup. There was some discussion about this in https://reviews.llvm.org/D35894

Interesting - that seemed like a more natural interpretation of LSP to me.
Others talked me down, motivated (I think) by nicer behavior of jump-to-definition... will bring it up again :)

Man, this seemed compelling to me, but there's a little bit of wiggle room in the spec (what's the "definition location"), so we looked at the MS language servers...
... and both their TS and C++ implementations return the range of the name only, despite that (IMO) being a weird interpretation of the spec.
As for VSCode:

  • the "ctrl-to-hover" behavior starts at beginning of the line containing the range, and has a heuristic for when to stop (even if you return the whole definition range, I think). So whole-range is a bit better here (particularly when type/template is on a separate line) but actually still not great.
  • "peek definition" shows the selected range in the middle of a block, with the range highlighted. Having the whole code highlighted actually looks kinda bad :/
  • "go to definition" puts your cursor at the start of the range, and the identifier seems much better here.

So I *want* to agree, but we'll be fighting the other language servers and editors (and @ioeric, @ilya-biryukov who think we'd be breaking more important workflows)... I think we're actually better off just returning the name.

I was going to change the symbol index to do the opposite :) The range of definitions including the bodies of functions, etc is used in a "peek definition" feature by several LSP clients. So for example in VSCode, you can hold Ctrl and hover on a function call and see its definition in a popup. There was some discussion about this in https://reviews.llvm.org/D35894

Interesting - that seemed like a more natural interpretation of LSP to me.
Others talked me down, motivated (I think) by nicer behavior of jump-to-definition... will bring it up again :)

Man, this seemed compelling to me, but there's a little bit of wiggle room in the spec (what's the "definition location"), so we looked at the MS language servers...
... and both their TS and C++ implementations return the range of the name only, despite that (IMO) being a weird interpretation of the spec.
As for VSCode:

  • the "ctrl-to-hover" behavior starts at beginning of the line containing the range, and has a heuristic for when to stop (even if you return the whole definition range, I think). So whole-range is a bit better here (particularly when type/template is on a separate line) but actually still not great.
  • "peek definition" shows the selected range in the middle of a block, with the range highlighted. Having the whole code highlighted actually looks kinda bad :/
  • "go to definition" puts your cursor at the start of the range, and the identifier seems much better here.

So I *want* to agree, but we'll be fighting the other language servers and editors (and @ioeric, @ilya-biryukov who think we'd be breaking more important workflows)... I think we're actually better off just returning the name.

Good points, thanks for investigating this. I agree that it should return the range of the name only so the direction of this patch looks good to me.

hokein updated this revision to Diff 137720.Mar 9 2018, 3:37 AM
hokein marked 7 inline comments as done.

Address review comments.

hokein added a comment.Mar 9 2018, 3:38 AM

Thanks for the comments!

clangd/SourceCode.cpp
55 ↗(On Diff #137549)

Thanks for pointing it out! Added a FIXME.

clangd/SourceCode.h
34 ↗(On Diff #137549)

Created a separate file.

39 ↗(On Diff #137549)

Since this function only returns name location, a source location is enough IMO.

sammccall accepted this revision.Mar 9 2018, 5:18 AM
This revision is now accepted and ready to land.Mar 9 2018, 5:18 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.