[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 6 inline comments as done.
nridge added a comment.

I'm posting some partial responses because I have some questions (really just one, about fuzzy matching).

In general the comments seem reasonable and I plan to address all of them.

(I've marked some comments as done because I've addressed them locally. I'm not uploading a revised patch yet because it wouldn't be very interesting.)



================
Comment at: clang-tools-extra/clangd/XRefs.cpp:226
+
+std::vector<LocatedSymbol> navigationFallback(ParsedAST &AST,
+                                              const SymbolIndex *Index,
----------------
sammccall wrote:
> this function should have a high-level comment describing the strategy and the limitations (e.g. idea of extending it to resolve nearby matching tokens).
> 
> A name like `locateSymbolNamedTextuallyAt` would better describe what this does, rather than what its caller does.
> 
> I would strongly consider exposing this function publicly for the detailed tests, and only smoke-testing it through `locateSymbolAt`. Having to break the AST in tests or otherwise rely on the "primary" logic not working is brittle and hard to verify.
> I would strongly consider exposing this function publicly for the detailed tests, and only smoke-testing it through locateSymbolAt. Having to break the AST in tests or otherwise rely on the "primary" logic not working is brittle and hard to verify.

I was going to push back against this, but I ended up convincing myself that your suggestion is better :)

For the record, the consideration that convinced me was:

Suppose in the future we add fancier AST-based logic that handles a case like `T().foo()` (for example, by surveying types for which `T` is actually substituted, and offering `foo()` inside those types). If all we're testing is "navigation works for this case" rather than "navigation works for this case //via the AST-based mechanism//", we could regress the AST logic but have our test still pass because the testcase is simple enough that the text-based navigation fallback (that we're adding here) works as well.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:252
+  Index->fuzzyFind(Req, [&](const Symbol &Sym) {
+    auto Loc = symbolToLocation(Sym, MainFilePath);
+    if (!Loc) {
----------------
sammccall wrote:
> I'm not sure why we're using SymbolToLocation here:
>  - Main file URI check: the `Symbol` has URIs. They need to be canonicalized to file URIs before comparison. This allows checking both decl and def location.
>  - PreferredDeclaration and Definition can be more easily set directly from the `Symbol`
Well the `Symbol` has `SymbolLocation`s and we need protocol `Location`s, so we have to use something to convert them. Other places that perform such conversion use `symbolToLocation()`, so I reused it.

But you're right that `symbolToLocation()` also has some "pick the definition or the declaration" logic which is less appropriate here. I can factor out the `SymbolLocation` --> `Location` conversion logic from `symbolToLocation()`, and just use that here.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:258
+
+    // For now, only consider exact name matches, including case.
+    // This is to avoid too many false positives.
----------------
sammccall wrote:
> I wouldn't bother qualifying this as "for now". Any code is subject to change in the future, but requiring an exact name match for index-based results seems more like a design decision than a fixme.
Do we want to rule out the possibility of handling typos in an identifier name in a comment (in cases where we have high confidence in the match, e.g. a long / unique name, small edit distance, only one potential match) in the future?

This is also relevant to whether we want to keep the `FuzzyMatcher` or not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72874/new/

https://reviews.llvm.org/D72874