Page MenuHomePhabricator

[clangd] Implement semantic selections.
ClosedPublic

Authored by usaxena95 on Sep 9 2019, 8:48 AM.

Details

Summary

For a given cursor position, it returns ranges that are interesting to the user.
Currently the semantic ranges correspond to the nodes of the syntax trees.

Diff Detail

Repository
rL LLVM

Event Timeline

usaxena95 created this revision.Sep 9 2019, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2019, 8:48 AM
usaxena95 updated this revision to Diff 219381.Sep 9 2019, 9:53 AM

Removed logs for debugging.

lebedev.ri retitled this revision from Implement semantic selections. to [clangd] Implement semantic selections..Sep 9 2019, 10:08 AM
usaxena95 updated this revision to Diff 219386.Sep 9 2019, 10:13 AM

Create range only if it represents a valid file range.

usaxena95 edited subscribers, added: hokein, sammccall; removed: MaskRay, mgorny, jkorous, arphaman.
sammccall marked 3 inline comments as done.Sep 9 2019, 11:02 AM

Nice! Particularly: great tests.

Only big thing is toHalfOpenFileRange should get you substantially better macro handling.

clang-tools-extra/clangd/SemanticSelection.cpp
18 ↗(On Diff #219386)

nit: "unique" suggests this function checks against all the ranges in *Result (which it doesn't, and indeed doesn't need to)
I'd suggest renaming addIfDistinct and adding a comment that only consecutive ranges should be able to coincide

33 ↗(On Diff #219386)

we use log() or elog() for this, make sure you include Offset.takeError() for the original message

Alternatively, you might consider returning Expected<SemanticSelectionResult> so the error gets propagated to the client (as this really is a client error)

39 ↗(On Diff #219386)

I think you probably want to bail out once you hit TranslationUnitDecl.

And maybe at namespace decls? Though I have often in my editor wondered "what's the enclosing namespace"...

45 ↗(On Diff #219386)

Ah, this will work if the start and the end of the AST node are written directly in the file, and no macros are involved. But try toHalfOpenFileRange in SourceCode.h, which should work in a lot more cases.

In particular, this should work:

#define INC(X) X + 1
int a, b;
int c = INC(a * b);
            ^
            ~
            ~~~~~
        ~~~~~~~~~~
~~~~~~~~~~~~~~~~~~
clang-tools-extra/clangd/SemanticSelection.h
21 ↗(On Diff #219386)

I'm not sure this struct pulls its weight, I'd suggest just returning vector<Range> here.
This isn't a stable API, we can change it if we need to add things.
Or is there something you have in mind?

Often we mirror the LSP here, but LSP is weird enough here (a linked list, seriously?) that I don't particularly think we should.

27 ↗(On Diff #219386)

incomplete sentence. It'd be nice to spell out on this line what an "interesting" range is (just one that corresponds to an AST node, right?)

28 ↗(On Diff #219386)

somewhere you should mention that front() is the inner most range, and back() the outermost.

clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
53 ↗(On Diff #219386)

this is the case where toHalfOpenFileName should help

59 ↗(On Diff #219386)

interesting, this *might* be worth special handling, to select the whole macro expansion. Wonder if people will complain. Let's leave it for now for simplicity

82 ↗(On Diff #219386)

If you're interested, I think the fix here is in pointBounds() in selection.cpp.
Since selection-tree now treats semicolons as if they don't exist, we should probably handle them the same way we do whitespace, and look left instead of right.

(definitely a different patch though)

88 ↗(On Diff #219386)

oh, this is interesting. it's a side-effect of how selectiontree now ignores whitespace (and semicolons, and comments).

We convert a cursor into a size-one selection, and that used to always select something, but no longer does.
This is unfortunate but hopefully pretty rare. If it's common to e.g. want to select a functiondecl while on a blank line inside it, we may want to revisit this.

115 ↗(On Diff #219386)

why commented out?

usaxena95 updated this revision to Diff 220071.Sep 13 2019, 3:57 AM
usaxena95 marked 13 inline comments as done.

Addressed comments.

usaxena95 updated this revision to Diff 220074.Sep 13 2019, 4:01 AM

Minor changes.

usaxena95 added inline comments.Sep 13 2019, 4:04 AM
clang-tools-extra/clangd/SemanticSelection.cpp
33 ↗(On Diff #219386)

Returning Expected<vector<Ranges>> now to propagate the error.

39 ↗(On Diff #219386)

Bailing out once we hit the TUDecl. I am not sure about exiting on namespace decl. I am keeping this for now. We can remove this later if the need be.

45 ↗(On Diff #219386)

Interesting. This works fine in such cases. Thanks.
Though it works strangely with these ugly macros. I guess this is because it tries to follow the macro expansion.

#define MUL ) * (
int var = (4 + 15 MUL 6 + 10);
                ^
               ~~
           ~~~~~~
          ~~~~~~~~~~~
clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp
115 ↗(On Diff #219386)

Since the statement contains many [[ and ]], I have added the actual statement in comments to make it more readable.

usaxena95 updated this revision to Diff 220075.Sep 13 2019, 4:06 AM

Remove extraneous namespace comment.

sammccall accepted this revision.Sep 13 2019, 7:42 AM

Nice!

Please make sure you run clang-format (I think it'll add missing newlines at EOF)

clang-tools-extra/clangd/SemanticSelection.cpp
22 ↗(On Diff #220075)

nit: it's idiomatic in LLVM to pass mutable references rather than pointers where possible

52 ↗(On Diff #220075)

|| SM.getFileID(SR->getBegin()) != SM.getMainFileID()

(begin and end are guaranteed to be the same file, but it may not be the main file)

clang-tools-extra/clangd/SemanticSelection.h
22 ↗(On Diff #220075)

nit: if you intend to use doxygen comments (with \p syntax) you probably want /// so doxygen will render them

This revision is now accepted and ready to land.Sep 13 2019, 7:42 AM
nridge added a subscriber: nridge.Sep 15 2019, 7:59 PM

Which LSP feature is this related to?

Which LSP feature is this related to?

selectionRange:

It hasn't been published/released yet, but AIUI it will be soon. (Thus no mention of it being an extension everywhere).
I think @usaxena95 is working on a followup to add the LSP bindings.

usaxena95 updated this revision to Diff 220303.Sep 16 2019, 3:20 AM
usaxena95 marked 3 inline comments as done.

Resolved comments.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2019, 4:28 AM