This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add support for type hierarchy (super types only for now)
ClosedPublic

Authored by nridge on Jan 6 2019, 4:32 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nridge added a comment.Jan 6 2019, 4:42 PM
This comment was removed by nridge.
nridge planned changes to this revision.Jan 6 2019, 4:42 PM
nridge retitled this revision from Add support for the textDocument/superTypes request to [clangd] Add support for the textDocument/superTypes request.Jan 6 2019, 4:42 PM
simark added a subscriber: simark.Jan 7 2019, 2:47 PM
nridge updated this revision to Diff 182686.Jan 19 2019, 2:08 PM

Cleaned up patch, ready for review

nridge edited the summary of this revision. (Show Details)Jan 19 2019, 2:09 PM
nridge added a comment.EditedJan 19 2019, 2:13 PM

Please see also the clangd-dev discussion about this feature.

A particular question that I could use some feedback on from reviewers: the patch takes several functions that were previously file-local, such as declToSym() in FindSymbols.cpp and toURI() in SymbolCollector.cpp, and promotes them to functions declared in the corresponding header, so that they can be used from elsewhere (e.g. declToSym() from XRefs.cpp). Should these functions be moved to a dedicated file for utilities used for converting from internal representations (e.g. AST nodes) to protocol structures?

Hi Nate, thanks for the patch!

I am just wondering which data structure fits the best here. Do you have any thoughts on support for virtual inheritance? In other words - should we use a tree or a DAG?

clang-tools-extra/clangd/XRefs.cpp
836 ↗(On Diff #182686)

This looks a bit confusing naming-wise. Maybe renaming the property to something like graphChildren would be better? WDYT?

nridge planned changes to this revision.Jan 25 2019, 6:20 PM
nridge marked an inline comment as done.

I am just wondering which data structure fits the best here. Do you have any thoughts on support for virtual inheritance? In other words - should we use a tree or a DAG?

All the editors I know of that support a type hierarchy view, use a tree view of some sort to display it, so I think having a tree data structure at the protocol level makes sense. We could potentially annotate virtual bases with a flag (perhaps visualized as an icon / decoration) to draw attention to them.

clang-tools-extra/clangd/XRefs.cpp
836 ↗(On Diff #182686)

The property name here comes from the fact that the formulation of the protocol that I implemented is reusing the existing DocumentSymbol type.

However, the proposal has been updated just a few days ago to use a dedicated new type instead of reusing DocumentSymbol. The new type, TypeHierarchyItem, has separate fields named parents and children.

It probably makes sense for me to update this implementation to reflect the updated proposal, before it is reviewed further, so I will mark it as "Changes Planned" for now.

nridge updated this revision to Diff 183792.Jan 27 2019, 8:03 PM

Implement revised specification (WIP)

nridge planned changes to this revision.Jan 27 2019, 8:03 PM
nridge updated this revision to Diff 183793.Jan 27 2019, 8:04 PM

Remove unrelated file

nridge planned changes to this revision.Jan 27 2019, 8:04 PM
nridge retitled this revision from [clangd] Add support for the textDocument/superTypes request to [clangd] Add support for type hierarchy (super types only for now).Jan 27 2019, 8:04 PM
nridge edited the summary of this revision. (Show Details)

This is a partial implementation of the revised spec for type hierarchy. Things that still need to be done:

  • Handle the resolve and direction parameters in TypeHierarchyRequest
  • Implement typeHierarchy/resolve
  • (Later, in a separate patch: subtypes. Will requre index changes.)
nridge updated this revision to Diff 184984.Feb 3 2019, 4:03 PM
nridge edited the summary of this revision. (Show Details)

This completes the implementation of the revised spec (for supertypes only)

Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2019, 4:03 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nridge updated this revision to Diff 184985.Feb 3 2019, 4:04 PM

remove unrelated file

nridge marked 3 inline comments as done.Feb 3 2019, 4:06 PM
nridge added inline comments.
clang-tools-extra/clangd/XRefs.cpp
820 ↗(On Diff #184985)

I am open to feedback on whether we want to reduce the duplication between these functions, and if so, suggestions for how.

901 ↗(On Diff #184985)

I am deliberately leaving this part for a follow-up patch, as it will require index changes.

clang-tools-extra/unittests/clangd/Matchers.h
130 ↗(On Diff #184985)

Should I split this out into a separate patch? It's used by the tests being added for this functionality.

Implementation LG, but I am not sure about adding a functionality on a proposal that might change. WDYT @sammccall ?

clang-tools-extra/clangd/FindSymbols.cpp
16 ↗(On Diff #184985)

it seems like there is no change in file requiring this

218 ↗(On Diff #184985)

can you revert this one as well ?

clang-tools-extra/clangd/Protocol.cpp
817 ↗(On Diff #184985)

nit: get rid of nesting by

auto T = E.getas..;
if(!T)
  return false;
if(*T not in rage)
  return false;
Out = cast(*T);
return true;
clang-tools-extra/clangd/Protocol.h
1026 ↗(On Diff #184985)

why not just use an int then ?

1046 ↗(On Diff #184985)

again we can just store a bool, and only serialize if it is true

clang-tools-extra/clangd/XRefs.cpp
876 ↗(On Diff #184985)

again reduce nesting

891 ↗(On Diff #184985)

A problem that might occur when you add children traversal:
do we really want to include current decl in children of parent when we have BOTH as direction? If so, maybe we should rather cache the results? Maybe leave a todo/fixme ?

917 ↗(On Diff #184985)

llvm::None

925 ↗(On Diff #184985)

what about member fields ?

932 ↗(On Diff #184985)

reduce nesting

935 ↗(On Diff #184985)

Is this mentioned in proposal?

939 ↗(On Diff #184985)

llvm::None

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

can you format the code?

1387 ↗(On Diff #184985)

You can use Source.points() and use Pt directly instead of doing Source.point(pt). Same for other tests.

Implementation LG, but I am not sure about adding a functionality on a proposal that might change. WDYT @sammccall ?

We discussed this a bit on the thread, I think we should follow the proposed spec, but with some useful hedges:

  • only implement enough bits to be useful
  • I'm not sure the internal APIs/implementation should follow the "shape" of the spec so deeply.

So on a concrete but still high-level:

  • I don't think we should implement the resolve operation, or resolution bounds, at this point - this patch doesn't do anything slow. Returning complete ancestors and never returning any children seems simplest.
  • in 'XRefs.h', I think the API to introduce is findTypeAt(Position) -> Decl* + typeAncestors(Decl*) and later typeDescendants(Decl*), rather than a single more complex typeHierarchy call. These two operations have little in common implementation-wise, and it's easy to imagine editors preferring to expose them separately. In clangdserver of course we need to expose a single operation because of transactionality. The stitching together could go in clangdserver, or a higher-level function exposed by xrefs - but I think the separate functions are what we should be unit-testing.
clang-tools-extra/unittests/clangd/Matchers.h
130 ↗(On Diff #184985)

This is nice! I think it should probably go in llvm/Testing/Support/..., but it's OK here.

Updated reviewers line to reflect (I think) current reality so this doesn't get lost, but feel free to reassign as you'd prefer.

nridge updated this revision to Diff 186136.Feb 9 2019, 3:19 PM

Add tests for scenarios involving class template specializations

Also improve support for dependent bases

nridge planned changes to this revision.Feb 9 2019, 3:19 PM
nridge added a comment.Feb 9 2019, 3:21 PM

Add tests for scenarios involving class template specializations

Also improve support for dependent bases

(This update is unrelated to the review comments, it's just improvements I had in the queue already. Another update to address review comments will come next.)

nridge marked 19 inline comments as done.Feb 9 2019, 4:21 PM

Thank you for the reviews!

clang-tools-extra/clangd/Protocol.h
1026 ↗(On Diff #184985)

I was making the data structure correspond closely to the protocol, where the field is optional. But we can handle this in the serialization logic, yes.

clang-tools-extra/clangd/XRefs.cpp
891 ↗(On Diff #184985)

Actually, this is a bug: if the top-level call uses Both, the recursive calls should still just use Parents and Children (i.e. we never want children of parents or parents of children). Thanks for spotting that!

925 ↗(On Diff #184985)

It's not clear what the desired semantics would be for a member field: get the type hierarchy of the enclosing class type, or the type hierarchy of the field's type?

935 ↗(On Diff #184985)

It's not specified; I left a comment on the proposal asking about it.

clang-tools-extra/unittests/clangd/Matchers.h
130 ↗(On Diff #184985)

I'll leave it here for now. We can move it in a follow-up patch.

nridge updated this revision to Diff 186137.Feb 9 2019, 4:21 PM
nridge marked 4 inline comments as done.

Address Kadir's review comments

nridge planned changes to this revision.Feb 9 2019, 4:21 PM
nridge added a comment.Feb 9 2019, 4:25 PM

So on a concrete but still high-level:

  • I don't think we should implement the resolve operation, or resolution bounds, at this point - this patch doesn't do anything slow. Returning complete ancestors and never returning any children seems simplest.

A hypothetical client could always ask for one level at a time, and ignore any levels it didn't ask for; such a client would break if we did this.

I think the implementation of resolve is straightforward enough that we might as well keep it.

  • in 'XRefs.h', I think the API to introduce is findTypeAt(Position) -> Decl* + typeAncestors(Decl*) and later typeDescendants(Decl*), rather than a single more complex typeHierarchy call. These two operations have little in common implementation-wise, and it's easy to imagine editors preferring to expose them separately. In clangdserver of course we need to expose a single operation because of transactionality. The stitching together could go in clangdserver, or a higher-level function exposed by xrefs - but I think the separate functions are what we should be unit-testing.

This sounds nice and clean, thanks for the suggestion! I will give it a try.

nridge updated this revision to Diff 186139.Feb 9 2019, 5:15 PM

Introduce and use findRecordTypeAt() and typeAncestors() helpers

nridge added a comment.Feb 9 2019, 5:29 PM
  • in 'XRefs.h', I think the API to introduce is findTypeAt(Position) -> Decl* + typeAncestors(Decl*) and later typeDescendants(Decl*), rather than a single more complex typeHierarchy call. These two operations have little in common implementation-wise, and it's easy to imagine editors preferring to expose them separately. In clangdserver of course we need to expose a single operation because of transactionality. The stitching together could go in clangdserver, or a higher-level function exposed by xrefs - but I think the separate functions are what we should be unit-testing.

In the latest update I introduced these helpers. I made the first one findRecordTypeAt() instead of findTypeAt(), since findTypeAt() suggests it also works for built-in types like int, but I don't think we can get a Decl* for int.

As far as reworking the tests to use these functions, I've thought about that a bit:

  • These functions return AST nodes. It's not clear to me how I would come up with "expected" AST nodes to test the return values against.
  • Even if we find a way to get "expected" AST nodes, we would be losing test coverage of functions like declToTypeHierarchyItem() (though I suppose we could write separate tests for that).
  • We could instead test against the properties of the AST nodes, such as their names and source ranges, but then the test code to query those properties would basically be duplicating code in declToTypeHierarchyItem(). It would seem to make more sense to just test the resulting TypeHierarchyItem instead (as the tests are doing now).
sammccall added a comment.EditedFeb 11 2019, 1:32 AM

As far as reworking the tests to use these functions, I've thought about that a bit:

  • These functions return AST nodes. It's not clear to me how I would come up with "expected" AST nodes to test the return values against.

See findDecl in ASTUnit.h

  • Even if we find a way to get "expected" AST nodes, we would be losing test coverage of functions like declToTypeHierarchyItem() (though I suppose we could write separate tests for that).

Yes, please do.

clang-tools-extra/clangd/ClangdServer.cpp
543 ↗(On Diff #186139)

relying on the item range to resolve the actual symbol isn't reliable:

  • the source code may have changed
  • the range may not be within the TU, and might be e.g. in an indexed header we don't have a compile command for.
kadircet added inline comments.Feb 11 2019, 1:58 AM
clang-tools-extra/clangd/XRefs.cpp
925 ↗(On Diff #184985)

I think it is sensible to go for enclosing type. But up to you, in any case could you add a comment stating how FieldDecl are handled?

nridge marked an inline comment as done.Feb 12 2019, 6:22 PM
nridge added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
543 ↗(On Diff #186139)

Good point. I appreciate a bit better now why you suggested trying to avoid resolve for now :)

What do you think of the following implementation approach:

  • Use the data field of TypeHierarchyItem (which the client will send back to the server for resolve queries) to send the SymbolID of the item to the client.
  • When the client sends back the SymbolID in the resolve request, look up the symbol in the index, and use the source range from the symbol.

(Later, when we start storing base <--> derived relationships in the index for subtype support, we could just answer the entire resolve query using the index.)

sammccall added inline comments.Feb 12 2019, 6:55 PM
clang-tools-extra/clangd/ClangdServer.cpp
543 ↗(On Diff #186139)

Thanks for exploring all the options here!

Generally we've tried to avoid relying on the index unless it's needed, using the AST where possible. There are failure modes here:

  • the base type is in the current file, which is actively edited. The ranges in the index may be off due to staleness.
  • the base type is not indexed, because it is e.g. a member class inside a class template
  • there's no index (-index=0, though I'm not sure why we still support this) or the index is stale and the type is missing (we're working on making index updates more async)
  • the base type is not encodable.

There are just more moving pieces here, I think. Is there a clear reason to support resolve for parents?

nridge marked 2 inline comments as done.Feb 12 2019, 8:36 PM
nridge added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
543 ↗(On Diff #186139)

Is there a clear reason to support resolve for parents?

Just what I said earlier about a hypothetical client that relies on it.

However, given the complications involved in implementing it, I'm happy with only being concerned with actual clients, not hypothetical ones. The only client implementation I currently know of is Theia, and I checked that it works fine without resolve, so I'm happy with deferring work on resolve for now.

If another client comes along that relies on resolve, we can revisit this.

sammccall added inline comments.Feb 13 2019, 1:09 AM
clang-tools-extra/clangd/ClangdServer.cpp
543 ↗(On Diff #186139)

Just what I said earlier about a hypothetical client that relies on it.

I'll try to get the spec clarified such that eager resolution is always fine.
But if we ever do need to deal with such clients, @ioeric had a neat idea: we can just stash the rest of the response in the data field, and then "resolution" can just pull it out. This should be easy to bolt on.

clang-tools-extra/clangd/XRefs.cpp
842 ↗(On Diff #186139)

This is more conversions than necessary.
I think we just need:

auto FilePath = getCanonicalPath(SM.getFileEntryForID(SM.getFileID(BeginLoc)), SM);
auto TUPath = getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()));
if (!FilePath || !TUPath)
  return;
THI.uri = URIForFile::Canonicalize(*FilePath, *TUPath);

(This is still more lines than I'd like, maybe we should have some helper for going from (File ID, SourceManager) --> URI)

sammccall added inline comments.Feb 13 2019, 2:05 AM
clang-tools-extra/clangd/XRefs.cpp
842 ↗(On Diff #186139)

(I looked at adding such a helper, and I don't think it's a good idea - the few existing callsites that could use it currently benefit significantly from hoisting the getCanonicalPath from the TUPath outside of a loop. It may or may not be worth doing that here)

nridge updated this revision to Diff 186944.Feb 14 2019, 4:29 PM
  • Rework tests to test the lower-level functions like typeAncestors()
  • Remove support for typeHierarchy/resolve
nridge marked an inline comment as done.Feb 14 2019, 4:30 PM
nridge added inline comments.
clang-tools-extra/unittests/clangd/XRefsTests.cpp
1604 ↗(On Diff #186944)

Any suggestions for how we can make findDecl() support template-ids?

nridge updated this revision to Diff 186947.Feb 14 2019, 4:49 PM

Fix a test failure

I think this is ready to continue to be reviewed :)

nridge updated this revision to Diff 189065.Mar 2 2019, 5:17 PM

Rebased.

Hi Nathan,
Really sorry about the delay here.
I'm actually out on leave at the moment, and didn't get things wrapped up before I went.
(I'm 1 week into 4 week parental leave, and got sick the week I was supposed to be handing things off).

@kadircet, can you finish the review here so we can get this landed?
My main concern is I think this can be further simplified by always fetching all ancestors, and treating "fill descendants" as basically a separate operation.
If it's not clear how to proceed there, we can land this and I can work on simplifying when I get back.
Cheers, Sam

clang-tools-extra/clangd/ClangdServer.cpp
530 ↗(On Diff #186947)

nit: decltype(CB) CB is I think a little more readable (once you're used to the idiom)

clang-tools-extra/clangd/Protocol.cpp
830 ↗(On Diff #189065)

use ObjectMapper here? It's more idiomatic and avoids the issues below.

831 ↗(On Diff #189065)

this seems confused - if there is a "resolve" field, but it's not a valid integer, then we should return false (fail to decode)

832 ↗(On Diff #189065)

prefer to initialize it inline in the struct, rather than manually initializing in failure cases.
As written, if the "resolve" field isn't present at all, the field will be uninitialized.

860 ↗(On Diff #189065)

nit: this happens in lots of places, and we don't usually add a comment in each.
return std::move(Result); should be enough.

(technically I think this *isn't* legal in C++11, as DR1579 was only addressed in 14. But most compilers backport it...)

clang-tools-extra/clangd/Protocol.h
1025 ↗(On Diff #189065)

= 0

clang-tools-extra/clangd/XRefs.cpp
990 ↗(On Diff #186947)

this kind of interpreting of request messages should be a few layers up: probably TypeHierarchyRequest should just have a TypeHierarchyDirection direction = Parents and replace it if present while parsing.

885 ↗(On Diff #189065)

The scope and complexity of this function seems unneccesarily large:

  • it's (in future?) capable of resolving in both directions
  • it calls itself recursively, modifying TypeHierarchyDirection to control the search
  • it handles levels, even though this optimization is only important for derived types

Can we restrict this to retrieving (all) recursive base types?
i.e. Optional<TypeHierarchyItem> getTypeAncestors(const CXXRecordDecl &CXXRD, ASTContext &Ctx)
Then later, subtype resolution can be the job of another function: resolveTypeDescendants(TypeHierarchyItem& Item, int depth)

That way the recursion of getTypeAncestors is simpler to understand, as it has much smaller scope. And code sharing between the two LSP calls is clearer: fetching type hierarchy is a call to getTypeAncestors and a call to resolveTypeDescendants (if direction is children or both, and resolve is > 0), and resolving a hierarchy is a call to resolveTypeDescendants.

clang-tools-extra/clangd/XRefs.h
65 ↗(On Diff #189065)

the name says "ancestors" (i.e. recursive) but the implementation is parents (non-recursive). I guess the name should change?

72 ↗(On Diff #189065)

XRefs.h isn't a suitable place to expose such a function.
It seems to be called only in one place in XRefs.cpp now, which doesn't need to be changed in this patch, can we revert the change?

If you do need to keep the function, I think it can be private.

nridge marked 19 inline comments as done.Mar 3 2019, 11:27 AM
nridge added inline comments.
clang-tools-extra/clangd/Protocol.cpp
830 ↗(On Diff #189065)

The reason I didn't use ObjectMapper is that I didn't know how to use it while also reusing the fromJSON() method for the base class, TextDocumentPositionParams.

I suppose I could just not reuse it (i.e. mention the fields of TextDocumentPositionParams in this function directly).

clang-tools-extra/clangd/XRefs.cpp
885 ↗(On Diff #189065)

If we remove "levels" here, should we introduce some kind of guard for infinite recursion, in case the user writes something like:

template <int N>
struct S : S<N + 1> {};

S<0> s;
nridge updated this revision to Diff 189092.Mar 3 2019, 11:30 AM
nridge marked 2 inline comments as done.

Address latest review comments

nridge updated this revision to Diff 189114.Mar 3 2019, 8:55 PM

Address a couple of outstanding TODOs

kadircet added inline comments.Mar 4 2019, 2:02 AM
clang-tools-extra/clangd/ClangdServer.cpp
365 ↗(On Diff #189114)

Can you revert this change?

clang-tools-extra/clangd/FindSymbols.h
16 ↗(On Diff #189114)

this should rather be clang/Index/IndexSymbol.h. index::SymbolKind resides in that header.

21 ↗(On Diff #189114)

I don't think these two are necessary inside header file.

clang-tools-extra/clangd/Protocol.cpp
871 ↗(On Diff #189114)

Shouldn't we fail if some of the above fields(like name) fails to decode ?

clang-tools-extra/clangd/XRefs.cpp
885 ↗(On Diff #189065)

clang should be limiting recursive template instantiations. Also since we are just traversing the AST produced by clang, it should never be infinite, but still a nice test case can you add one?

918 ↗(On Diff #189114)

Nit: Get rid of CXXRD and return inside if/else branches.

924 ↗(On Diff #189114)

why not use a for-range loop ? (CXXRD->bases())

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

Can you put a TODO?

1809 ↗(On Diff #189114)

I suppose parent resolving should not depend on level, so what about setting it to 0 instead of 10 so that test is consistent even after "child resolution" implementation?

Thanks for picking this up... just wanted to leave one last comment as I'd been thinking about the recursive template case too.

clang-tools-extra/clangd/XRefs.cpp
885 ↗(On Diff #189065)

I think there is a point here...

Consider our S template with the direction reversed and a base case specialized:

template <int N>
struct S : S<N - 1> {};
template<>
struct S<0>{};

S<2> instance;

Now the hierarchy of S<2> is well defined and finite: S<2> : S<1> : S<0>.
However IIUC the CXXRecordDecl for S<2> is the instantiation of the primary template, whose base is S<N - 1>, which is dependent[1], so the base's CXXRecordDecl is the primary template, whose base is S<N - 1>... and we never reach the base case.

Actually I'm not sure whether this happens if the base is dependent merely where it's spelled, or still dependent after instantiation? Even in the latter case one can construct examples where we'll infloop:

template <int N>
struct Foo {
  S<N> instance;
}

Trying to get the hierarchy on S<N> could infloop. (I agree these should both be test cases)

What's the Right Thing?

  • not sure about a recursion limit, as showing 10 S<N-1>s in the hierarchy is silly.
  • not sure about bailing out on dependent types either, as knowing that e.g. my SmallSet<T> inherits from SmallMap<T, bool> is meaningful or useful.
  • maybe we should bail out once we see instantiations of the same template decl twice in a parent chain. I.e. keep the seen non-null CXXRecordDecl->getTemplateInstantiationPattern()s in a set and stop recursing if insertion fails.

However for this patch I'd suggest just bailing out on dependent types with a TODO as the simplest, this is an edge case that can be fixed later.

nridge updated this revision to Diff 189444.Mar 5 2019, 7:55 PM
nridge marked 10 inline comments as done.

Address most of the latest review comments.

The infinite recursion issue remains to be fixed.

nridge added inline comments.Mar 5 2019, 7:55 PM
clang-tools-extra/clangd/ClangdServer.cpp
365 ↗(On Diff #189114)

I tried, but clang-format automatically makes the change again.

Is there some particular clang-format configuration I should apply, so it produces the same results? I don't currently have any configuration, i.e. I think it's just reading the .clang-format file in the monorepo root.

clang-tools-extra/clangd/XRefs.cpp
885 ↗(On Diff #189065)

The current patch does actually recurse infinitely on this test case, likely for the reasons Sam outlined (our handling of dependent types), and eventually runs into the segfault.

I will update the patch to address this.

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

I added a comment to clarify that a field does not unambiguously specify a record type. i don't think there's anything further "to do" here.

nridge marked an inline comment as done.Mar 5 2019, 7:56 PM
nridge added inline comments.
clang-tools-extra/clangd/XRefs.cpp
885 ↗(On Diff #189065)

I meant to say "eventually runs into a stack overflow".

kadircet added inline comments.Mar 6 2019, 12:48 AM
clang-tools-extra/clangd/ClangdServer.cpp
365 ↗(On Diff #189114)

That's what we use as well(and your formatted version is correct), but unfortunately sometimes people send out changes with wrong formatting and they are hard to notice during review. Generally we try not to touch those lines in irrelevant patches to keep blame clean.

If you are running clang-format directly you can instead try clang-format-diff to format only changed lines. But not that crucial.

nridge updated this revision to Diff 190040.Mar 10 2019, 7:42 PM

Address the infinite recursion issue

nridge marked an inline comment as done.Mar 10 2019, 7:47 PM

The updated patch addresses the infinite recursion issue by bailing on dependent bases for now, as Sam suggested. I will implement the more comprehensive suggested fix ("bail out once we see instantiations of the same template decl twice in a parent chain") in a follow-up patch. I did add all three testcases that have come up during discussion.

I believe all review comments to date are addressed now.

clang-tools-extra/clangd/ClangdServer.cpp
365 ↗(On Diff #189114)

I'm not running clang-format directly, VSCode's clang-format extension is doing so automatically upon every file-save. I have not found an option to configure it to format only changed lines.

Would it help if I moved the formatting corrections to their own patch (and made this patch depend on that)?

nridge updated this revision to Diff 190041.Mar 10 2019, 8:20 PM

Fix a (somewhat amusing) typo where I wrote '0' instead of 'O' in a fromJSON() implementation

(Does the fact that this didn't cause any test failures suggest that the fromJSON() functions aren't exercised by any tests?)

nridge added a comment.EditedMar 10 2019, 8:24 PM

Unfortunately, there is a further problem: the Theia client-side implementation of type hierarchy has recently merged, and their code has changed so that they do require typeHierarchy/resolve to be supported. They ask for 1 level in the initial request, ignore any extra levels the server might send, and ask for extra levels using typeHierarchy/resolve instead.

What should we do here -- seek to change Theia, or implement typeHierachy/resolve for supertypes after all?

kadircet marked an inline comment as done.EditedMar 12 2019, 8:31 AM

Fix a (somewhat amusing) typo where I wrote '0' instead of 'O' in a fromJSON() implementation

(Does the fact that this didn't cause any test failures suggest that the fromJSON() functions aren't exercised by any tests?)

Unfortunately we usually test LSP layer with lit tests, and since we don't have any lit tests for this use-case it wasn't caught. Could you add a lit test for overall use case? You can see examples inside clang-tools-extra/test/clangd/ folder.

Unfortunately, there is a further problem: the Theia client-side implementation of type hierarchy has recently merged, and their code has changed so that they do require typeHierarchy/resolve to be supported. They ask for 1 level in the initial request, ignore any extra levels the server might send, and ask for extra levels using typeHierarchy/resolve instead.

What should we do here -- seek to change Theia, or implement typeHierachy/resolve for supertypes after all?

Looking at https://github.com/theia-ide/theia/pull/3802#issuecomment-455992523 inside theia's implementation of this feature, I believe these are all subject to change.
So let's leave it as it is, even if it would mean you will be able to get only one level of parents knowledge in theia for now. I believe it should be addressed in theia's implementation and proposal itself(which is acutally addressed by sam, https://github.com/Microsoft/vscode-languageserver-node/pull/426/files#r255416980)
And if the conclusion happens to be in favor of theia's current implementation we can always change current implementation to respond to those queries as well.

clang-tools-extra/clangd/ClangdServer.cpp
365 ↗(On Diff #189114)

it is not that important, nvm.

kadircet added inline comments.Mar 12 2019, 8:50 AM
clang-tools-extra/unittests/clangd/XRefsTests.cpp
1496 ↗(On Diff #190041)

Sorry for not pointing this out before, but it would be great if you could move related tests into a new file, XRefsTests is getting out of hand.

Feel free to ignore if you don't feel like it.

1834 ↗(On Diff #190041)

Could you also check for the response, since parent traversal for these cases are also disabled currently(due to dependent types issue).
So that we won't forget to fix any issues regarding these cases.
I suppose, a check with only current symbol and empty parents and children should be enough. Same for other two cases.

Unfortunately, there is a further problem: the Theia client-side implementation of type hierarchy has recently merged, and their code has changed so that they do require typeHierarchy/resolve to be supported. They ask for 1 level in the initial request, ignore any extra levels the server might send, and ask for extra levels using typeHierarchy/resolve instead.

What should we do here -- seek to change Theia, or implement typeHierachy/resolve for supertypes after all?

Looking at https://github.com/theia-ide/theia/pull/3802#issuecomment-455992523 inside theia's implementation of this feature, I believe these are all subject to change.

Note, that is a fairly old comment, and as mentioned the PR in question has recently merged.

So let's leave it as it is, even if it would mean you will be able to get only one level of parents knowledge in theia for now. I believe it should be addressed in theia's implementation and proposal itself(which is acutally addressed by sam, https://github.com/Microsoft/vscode-languageserver-node/pull/426/files#r255416980)
And if the conclusion happens to be in favor of theia's current implementation we can always change current implementation to respond to those queries as well.

Ok, I filed a Theia issue about it for now.

Unfortunately we usually test LSP layer with lit tests, and since we don't have any lit tests for this use-case it wasn't caught. Could you add a lit test for overall use case? You can see examples inside clang-tools-extra/test/clangd/ folder.

Are these tests written manually, or with the help or some kind of tool?

nridge updated this revision to Diff 190375.Mar 12 2019, 10:23 PM

Address remaining review comments

(I figure out how to write a lit test)

nridge marked 2 inline comments as done.Mar 12 2019, 10:24 PM

Ok, I filed a Theia issue about it for now.

Thanks!

Are these tests written manually, or with the help or some kind of tool?

Yes unfortunately :(

Thanks for moving the tests!

clang-tools-extra/test/clangd/type-hierarchy.test
1 ↗(On Diff #190375)

why strict-whitespace?

just a few drive-by comments ;)

clang-tools-extra/clangd/FindSymbols.h
28 ↗(On Diff #190375)

This could probably live in Protocol.h.

clang-tools-extra/clangd/XRefs.cpp
957 ↗(On Diff #190375)

nit: s/TODO/FIXME(owner)/

same elsewhere.

clang-tools-extra/clangd/index/SymbolCollector.h
154 ↗(On Diff #190375)

why are we pulling this into the header? This still seems to be only used in SymbolCollector.cpp.

A few small NITs

clang-tools-extra/clangd/ClangdServer.h
188 ↗(On Diff #190375)

Could you rename it to typeHierarchy?
We try to keep these method names aligned with the LSP methods.

clang-tools-extra/clangd/XRefs.h
62 ↗(On Diff #190375)

This method looks like an implementation detail and does not align with other methods in XRefs.h which are high-level methods that implement LSP functionality.

It would be more appropriate to move it to AST.h or directly into the XRefs.cpp. WDYT?

nridge marked 7 inline comments as done.Mar 14 2019, 4:55 PM
nridge added inline comments.
clang-tools-extra/clangd/XRefs.h
62 ↗(On Diff #190375)

@sammccall asked for this method to be exposed in the header and some of the tests written in terms of it.

clang-tools-extra/clangd/index/SymbolCollector.h
154 ↗(On Diff #190375)

You're right. This is a leftover from an earlier version of the patch.

clang-tools-extra/test/clangd/type-hierarchy.test
1 ↗(On Diff #190375)

That's what the other lit tests seem to do, I just copied it :)

nridge updated this revision to Diff 190755.Mar 14 2019, 4:57 PM
nridge marked an inline comment as done.

Address latest review comments

ilya-biryukov added inline comments.Mar 15 2019, 2:47 AM
clang-tools-extra/clangd/XRefs.h
62 ↗(On Diff #190375)

LG, thanks for the clarification. I haven't been following the review closely.
Having methods that return pointers in XRefs.h seems like the wrong layering to me, but in any case it's a minor detail.

kadircet accepted this revision.Mar 15 2019, 2:53 AM

LGTM, thanks Nathan!

This revision is now accepted and ready to land.Mar 15 2019, 2:53 AM

Great, thanks for the reviews!

Could you commit the patch as well?

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