This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix one testcase in XRefsTests.
ClosedPublic

Authored by hokein on Aug 16 2019, 7:35 AM.

Details

Summary

The test didn't test anything actually -- it used "[]" as annotation which should be
"[[]]".

This patch also fixes a bug in XRef where we may return duplicated refs.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein created this revision.Aug 16 2019, 7:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2019, 7:35 AM
ilya-biryukov added inline comments.Aug 16 2019, 8:37 AM
clang-tools-extra/clangd/XRefs.cpp
389 ↗(On Diff #215601)

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.

clang-tools-extra/clangd/unittests/XRefsTests.cpp
2079 ↗(On Diff #215601)

NIT: use ASSERT_THAT(ExpectedLocations, Not(IsEmpty())) for more detailed output in case of assertion failures.

hokein added inline comments.Aug 19 2019, 1:27 AM
clang-tools-extra/clangd/XRefs.cpp
389 ↗(On Diff #215601)

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?

ilya-biryukov added inline comments.Aug 19 2019, 1:43 AM
clang-tools-extra/clangd/XRefs.cpp
389 ↗(On Diff #215601)

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?

hokein marked an inline comment as done.Aug 19 2019, 1:48 AM
hokein added inline comments.
clang-tools-extra/clangd/XRefs.cpp
389 ↗(On Diff #215601)

Yes, that is correct.

ilya-biryukov added inline comments.Aug 19 2019, 8:07 AM
clang-tools-extra/clangd/XRefs.cpp
389 ↗(On Diff #215601)

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)

hokein marked an inline comment as done.Aug 20 2019, 1:22 AM
hokein added inline comments.
clang-tools-extra/clangd/XRefs.cpp
389 ↗(On Diff #215601)

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.

ilya-biryukov added inline comments.Aug 20 2019, 1:46 AM
clang-tools-extra/clangd/XRefs.cpp
389 ↗(On Diff #215601)

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.

hokein marked an inline comment as done.Aug 20 2019, 3:36 AM
hokein added inline comments.
clang-tools-extra/clangd/XRefs.cpp
389 ↗(On Diff #215601)

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 Locs with different HighlightingKinds.

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 HighlightingKinds, I think most of clients just choose one to render, so making clangd return only one result seems reasonable.

ilya-biryukov added inline comments.Aug 20 2019, 4:21 AM
clang-tools-extra/clangd/XRefs.cpp
389 ↗(On Diff #215601)

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.

hokein updated this revision to Diff 216136.Aug 20 2019, 6:43 AM
hokein marked 2 inline comments as done.

Address review comments.

This revision is now accepted and ready to land.Aug 20 2019, 6:49 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 7:06 AM