[PATCH] D66349: [clangd] Fix one testcase in XRefsTests.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:389
     llvm::sort(References, [](const Reference &L, const Reference &R) {
-      return std::tie(L.Loc, L.CanonicalTarget, L.Role) <
-             std::tie(R.Loc, R.CanonicalTarget, R.Role);
+      return L.Loc < R.Loc;
     });
----------------
hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > hokein wrote:
> > > > > ilya-biryukov wrote:
> > > > > > hokein wrote:
> > > > > > > ilya-biryukov wrote:
> > > > > > > > What are the elements `References` for the problematic case?
> > > > > > > > 
> > > > > > > > If we have duplicate elements, then `sort()` would now give us one of the items. Which exact `Decl` we're gonna end up seeing is not well-defined, i.e. it's non-deterministic.
> > > > > > > > What are the elements References for the problematic case?
> > > > > > > 
> > > > > > > The testcase is using declarations (see the existing test) -- we will get 3 refs on `using ::fo^o`, each ref has a different decl.  
> > > > > > > 
> > > > > > > ```
> > > > > > > void [[foo]](int);
> > > > > > > void [[foo]](double);
> > > > > > > 
> > > > > > > namespace ns {
> > > > > > > using ::[[fo^o]];
> > > > > > > }
> > > > > > > ```
> > > > > > > 
> > > > > > > > If we have duplicate elements, then sort() would now give us one of the items. Which exact Decl we're gonna end up seeing is not well-defined, i.e. it's non-deterministic.
> > > > > > > 
> > > > > > > I think we could make it deterministic, but only return one refs, WDYT?
> > > > > > > 
> > > > > > > 
> > > > > > To make sure I understand the problem: we used to get 4 references in total:
> > > > > > - 2 references to the functions (`foo(int)` and `foo(double)`)
> > > > > > - 2 references pointing to the using declaration, e.g. the `using ::[foo]]`
> > > > > > 
> > > > > > After this patch we would remove the duplicate `using ::[[foo]]` from the results and get only 3 references as a result.
> > > > > > 
> > > > > > Is that correct?
> > > > > Yes, that is correct.
> > > > Interestingly enough, we always ignore the `CanonicalTarget` in the returned `Reference`.
> > > > 
> > > > Maybe we could remove the `CanonicalTarget` from the `Reference` struct instead?
> > > > That keeps the interface more consistent: clients will always get deterministic results (as there is no `CanonicalDecl`, which is different now)
> > > > Maybe we could remove the CanonicalTarget from the Reference struct instead?
> > > 
> > > unfortunately, this may not work either because of the `Role` -- we still fail on the above sample, the references pointing to the using decl have different roles.
> > We only look at the role in `DocumentHighlights` to determine the `DocumentHighlightKind` (`Read`, `Write` or `Text`).
> > I suggest we do this earlier and replace the `Reference::Role`  field with `Reference::DocumentHighlightKind`.
> > Then we can de-duplicate here and keep deterministic interface.
> > 
> > If we feel that's the wrong layering (we are putting LSP-specific things into Reference, which only has clang-specific types stuff now), we could move de-duplication downstream to `findDocumentHighlights` and `findRefs`. They return `Loc` and `DocumentHighlighting` and can safely de-duplicate on those without changing observable results.
> > 
> > Looks like replacing `Role` with `HighlightingKind` is the simplest option, though. And I don't think this breaks layering that much, it's an implementation detail of cross-references anyway.
> > Looks like replacing Role with HighlightingKind is the simplest option, though. And I don't think this breaks layering that much, it's an implementation detail of cross-references anyway.
> 
> However, we may still encounter cases where we have duplicated `Loc`s with different `HighlightingKind`s.
> 
> This leaves us two choices:
> 1) de-duplicate the refs only by loc (as the original implemenation of the patch)
> 2) keep Role in refs, and do deduplication on both `findDocumentHighlights` and `findRefs`
> 
> I personally prefer 1). For document highlights,  I'm not sure whether we should return multiple highlights on the same location with different `HighlightingKind`s, I think most of clients just choose one to render, so  making clangd return only one result seems reasonable.
I would vouch for (2):
- It's easy to explain why we de-duplicate same locations in `findRefs` as this is what it is exactly the return value.
- The decision about picking the kind for a duplicated location falls onto `findDocumentHighlights`, which is, again, the right place to make this decision (even if the decision is random, e.g. preferring `Write` over `Read`).

But if we really want to keep the code shared for both versions, let's define how we pick the winner in the case where location is the same.

The important bit is avoid non-deterministic in the observable results of this function. We should avoid `Decl*` or `Role` being part of the result and being picked randomly from a few options that we have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66349