This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add decl/def support to SymbolDetails
ClosedPublic

Authored by dgoldman on Jul 18 2022, 1:52 PM.

Details

Summary

Add an optional declarationRange and definitionRange to SymbolDetails.

This will allow SourceKit-LSP to implement toggling between goto
definition/declaration based on whether the symbol at the cursor
is a definition or declaration.

In addition, make some minor fixes for ObjCMethodDecl:

  • XRefs.cpp getDefinition now sees ObjC method defs but I've left a TODO to implement proper searching for impls from a decl
  • nameLocation for ObjC methods should point to foo in - (void)foo, not the -

Diff Detail

Event Timeline

dgoldman created this revision.Jul 18 2022, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 1:52 PM
dgoldman requested review of this revision.Jul 18 2022, 1:52 PM
dgoldman updated this revision to Diff 445618.Jul 18 2022, 1:55 PM

Revert unintended change in symbol-info.test

For more context see the discussion on b/187405187

thanks, this LG per symboldetails change but as discussed offline i think we actually need a new request or a capability to indicate this (and having a new request is just easier for both clients and clangd) otherwise it can't be reliably used by clients.

in addition to that let's move the objcmethod related changes to its own patch since it's addressing a different issue.

clang-tools-extra/clangd/AST.cpp
175

can we do this in a separate change? as it has wider implications

clang-tools-extra/clangd/Protocol.h
1097

ah getting absolute paths .. alright i guess we need to make this one optional, sorry for the noise on this one :/

clang-tools-extra/clangd/XRefs.cpp
86

again let's move this into a separate change

1484

let's just pass the TUPath from ClangdServer into the request. (we should do the same for locateSymbolAt, but on a separate change)

1514

let's do this cannonicalization in the beginning of the loop, i.e:

for (const NamedDecl *D : getDeclAtPosition(AST, *CurLoc, Relations)) {
  D = getPreferredDecl(D);
  ....
}
clang-tools-extra/clangd/test/symbol-info.test
4

can you also introduce the definition?

clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp
24

nit: drop the empty lines in between these fields

29

since you're always calling these decl or def, you can instead check whether the annotation has any range named def/decl and expect those accordingly rather than mentioning them one extra time inside the testcases. i.e:

case:

void $def[[foo]]() {}
          int bar() {
            fo^o();
          }

check:

Annotations Test(Case);
SymbolDetails Expected;
auto Def = Test.ranges("def");
auto Decl = Test.ranges("decl");
if (!Def.empty()) {
  Expected.decl = Expected.def = makelocation ...
} else if (!Decl.empty()) {
  Expected.decl = makelocation ...
}
dgoldman added inline comments.Jul 19 2022, 2:37 PM
clang-tools-extra/clangd/AST.cpp
175

Sent out https://reviews.llvm.org/D130095, will rebase this on top once that's submitted.

clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp
29

Thanks, although that won't work for the // Multiple symbols returned - using overloaded function name test case. Should I move that one out to to be handled separately?

sammccall added inline comments.Jul 20 2022, 12:41 AM
clang-tools-extra/clangd/XRefs.cpp
1484

driveby: @kadircet any reason not to store the TUPath on ParsedAST? We already pass it to ParsedAST::build()

nridge added a subscriber: nridge.Jul 23 2022, 11:50 PM
dgoldman updated this revision to Diff 447771.Jul 26 2022, 11:15 AM
dgoldman marked 5 inline comments as done.

Minor fixes for review (some still pending based on discussion)

clang-tools-extra/clangd/XRefs.cpp
1484

I think this would simplify things quite a bit - otherwise I also need to update the DumpSymbol tweak to store/compute the TUPath (or just provide an empty one). LMK what I should do.

dgoldman updated this revision to Diff 447813.Jul 26 2022, 1:25 PM

Run clang-format

dgoldman added inline comments.Jul 26 2022, 2:26 PM
clang-tools-extra/clangd/XRefs.cpp
1484

Also, not sure how this would affect the Windows failure, which currently fails since the decl/definition locations reference "file:///C:/clangd-test/simple.cpp" instead of "test:///simple.cpp"... could add a regex or limit the test to non-Windows as a work around though. "textDocument/publishDiagnostics" also seems to reference that absolute path as well instead of the test:// URI

kadircet added inline comments.Jul 28 2022, 2:22 AM
clang-tools-extra/clangd/XRefs.cpp
1484

sent out D130690 for preserving TUPath in ParsedAST.

as for matching output in lit tests, clangd always outputs file uris. so you should rather match with a regex (unless testing something platform-specific).
you can use # CHECK-NEXT: "uri": "file://{{.*}}/clangd-test/simple.cpp" instead to match output.

clang-tools-extra/clangd/unittests/SymbolInfoTests.cpp
29

ah sorry i missed those, nvm this one. no need for splitting those out.

dgoldman updated this revision to Diff 449033.Aug 1 2022, 8:42 AM

Fix broken test and swap to AST.tuPath()

dgoldman updated this revision to Diff 449050.Aug 1 2022, 9:46 AM

Run clang-format

This revision was not accepted when it landed; it landed in state Needs Review.Aug 1 2022, 11:42 AM
This revision was automatically updated to reflect the committed changes.

Ah my bad didn't realize you hadn't accepted this, was there anything else you wanted me to change?

no worries, as discussed offline this LG