Implement find implementations extension #181

jonasbb · 2017-02-22T23:34:50Z

I tried using the recent addition to the save-analysis data to tackle two items from the wishlist.

Find all implementations of trait.
Listed under wishlist tracking issue: wishlist #142

Fails for traits which are implemented using derive
Find supertrait of trait
Listed under wishlist tracking issue: wishlist #142

Only proof of concept, not yet transitive.
To Check: Which span makes more sense. The span of the supertrait (as
currently) or the span of where the subtrait defines the dependency?

Requires the find-impls branch of https://github.com/jonasbb/rls_vscode
Also requires the find-impls branch of rls-analysis.

I have a few specific questions which I am not sure how to tackle them:

Find all implementations on a library provided trait like PartialEq only produces spans with a file path like /checkout/src/libstd/. lower_span in rls_analysis is not able to convert them correctly. Should this be fixed in rustc or rls_analysis and how? The spans do not have any information in them to indicate the invalid file paths.
Find all implementations does not work with derive. Is this considered a problem?
Find supertrait currently shows the definition span of the supertrait. I find this more useful than the line were the relationship is specified (here 5).
I currently open the references popup at the location where the users performed the request. Should the popup be moved to the def location of the type?

nrc · 2017-02-23T00:08:18Z

It would be good to communicate that you're working on this stuff before you get into it - I also have substantial changes to rls-analysis mostly implemented to support the save-analysis changes.

To answer your questions:

probably in rls-analysis
no, it is something we should fix, but shouldn't block landing
I think find subtraits is more useful than finding super traits, since the latter basically works just by 'jump to def'. Find super-traits should jump to the super trait, not the relation site
I'm not sure, we should follow what VSCode does in other cases.

jonasbb · 2017-02-23T19:46:07Z

@nrc If you already have the changes to rls-analysis that's good. Just use them and ignore my stuff. I do this more for fun and self-education anyway. But I try to communicate more, to avoid duplicating work.

nrc · 2017-02-23T20:00:37Z

@jonasbb to be clear, I only have the rls-analysis parts implemented (and they are on master now) - I haven't done anything in VSCode or the RLS, so I think your work here will be very useful

jonasbb · 2017-03-02T21:11:59Z

@nrc I changed it to use the upstream rls-analysis version. I dropped the find supertrait stuff for the moment. This can be added easily with some copy and paste.
I also added documentation in contributing.md.

Lastly, I decided to filter out all paths starting with /checkout/src/. I am sorry if somebody is developing their rust code in there. However, I don't know what the equivalent path would be under windows and OSX. It might be worthwhile to also filter out those.

nrc

Thanks, this is looking good. The only bit that gives me pause is the checkout filtering - more inline.

nrc · 2017-03-02T21:25:53Z

src/actions/mod.rs

+            .into_iter()
+            // FIXME: these are part of libstd and libcore but the path probably does not
+            // exists and thus should be removed. Replace with correct path later.
+            .filter(|x| x.file.starts_with(Path::new("/checkout/src/")))


I'm curious about this - why is it needed here and not for other queries? I think ideally any filtering like this should happen in rls-analysis, not here (and probably in a more principled way). I also think, but not sure, that this is probably incorrect, since this will cover all upstream crates, not just core/std, but it is only the std/core ones that do not exist?

I assume this is an artifact how the save-analysis data is generated. I assume this is the path used in the buildbots.

In general this can only become a problem for the values ref_spans and impls in PerCreateAnalysis, because here we map to a span. ref_spans is only filled by the "refs": array in the serialized json. globs and def_id_for_span map a span to something else. If they contain such a broken span this only leads to a failed lookup but not to wrong filenames being presented to the user.

In the local (target/rls) analysis data I can only find references to the path /checkout/src/libcore/macros.rs. They appear here only in the macro_ref section, which is currently unused by rls-analysis, thus they could not have been a problem. No span in "refs" contains such a path.

In the global ('rustlib/.../analysis') analysis data I see references to a lot of different files of the libraries. But none of those json files have any values in "refs", which is the only existing possibly problematic case.

This mean, neither the local nor global data could have contributed until this change with broken spans.

nrc · 2017-03-02T21:26:23Z

src/server.rs

@@ -351,6 +354,12 @@ impl LsService {
                            trace!("command(reformat range): {:?}", params);
                            this.handler.reformat(id, params.text_document, &*this.output);
                        }
+
+                        // custom extensions


comment is unnecessary

jonasbb · 2017-03-02T22:40:02Z

@nrc I addressed both comments by you. I just thought that a temporary solution for the checkout stuff would have been better than none. I opened a bug for this rust-dev-tools/rls-analysis#49

nrc

Thanks for the change, I thought of something else we need to do - sorry I should have included this in the last review, but didn't think of it.

nrc · 2017-03-03T00:55:01Z

src/actions/mod.rs

+            .unwrap_or(vec![])
+            .into_iter()
+            .map(|x| ls_util::rls_to_location(&x))
+            .collect();


Could we spawn a thread with timeout to do this please? We do that for the other queries (e.g., find all refs) and I think we should do it here too).

nrc · 2017-03-03T00:56:12Z

It would also be great to add a test for this.

Xanewok · 2017-05-08T01:47:15Z

Hey @jonasbb, are you still working on the issue by any chance?

jonasbb · 2017-06-13T22:06:46Z

I updated the implementation and implemented a test case for it.
This PR depends somewhat on rust-dev-tools/rls-analysis#70, to make it better usable for traits defined in std lib, however, it also compiles with the current version of rls-analysis.

This generates two new warnings for unused items, once in src/actions/lsp_extensions.rs:16 and in src/server.rs:17, but I am not sure if I should just put a couple of allow annotations on them. The import is needed for the code generated by the macro, but somehow rust still annotates it as unused_import.

The updated client changes are in https://github.com/jonasbb/rls_vscode/tree/find-impls

nrc

Looks good. Could we put this behind the unstable features config flag please? (At least to start with)

nrc · 2017-06-13T23:51:18Z

src/server.rs

@@ -247,6 +247,7 @@ messages! {
        "textDocument/rename" => Rename(RenameParams);
        "textDocument/formatting" => Formatting(DocumentFormattingParams);
        "textDocument/rangeFormatting" => RangeFormatting(DocumentRangeFormattingParams);
+        "rustDocument/implementations" => FindImpls(TextDocumentPositionParams);


Can we use the const here?

nrc · 2017-06-13T23:54:38Z

src/actions/mod.rs

+        let analysis = self.analysis.clone();
+
+        let handle = thread::spawn(move || {
+            // FIXME send failure


Could you expand this comment please

nrc · 2017-06-14T00:01:17Z

src/actions/mod.rs

+            }
+            Err(_) => {
+                out.failure(id, "Find Implementations failed to complete successfully");
+            }


style nit: I'd get rid of the {} here

nrc · 2017-06-14T00:03:45Z

This generates two new warnings for unused items

That is kind of disturbing - I guess this happens because the identifier is put together with concat_idents - this is presumably a bug in the Rust compiler. Could you add the allow annotations please?

jonasbb · 2017-08-15T19:35:40Z

Lets give this one more try.
@nrc

nrc · 2017-08-16T05:33:48Z

The new test seems to be failing, hard to tell why.

List all implementation block for traits, structs, and enums. Fails for traits which are implemented using derive Listed under wishlist #142 Requires the find-impls branch of https://github.com/jonasbb/rls_vscode

jonasbb · 2017-08-16T22:05:14Z

This test contains code for testing implementations of Eq. However, rust-analysis is not installed on Travis making rls-analysis fail why retrieving the typeid. Installing rust-analysis is also not an option, because this makes other test timeout. As such I commented the failing code.

nrc · 2017-08-17T06:24:20Z

I don't really understand the test issue - what is rust-analysis and why is it needed for the test? What do type ids have to do with it? The URL in the comment links to a passing Travis test, so I can't see any error messages. The Cargo.toml doesn't mention rust-analysis, so I don't know why Cargo needs to install it.

Could you just test for the literal impls in the test code (including Eq) and skip the derive completely? If that still doesn't work, remove all the Eq tests and just test for Sub and Super impls.

jonasbb · 2017-08-17T06:35:23Z

@nrc sorry for being so brief. It was late last night.
Summary: the test required the rustup rust-analysis component (for type definitions in the stdlib) which isn't and can't easily be installed on Travis.

I wrote tests to find implementation for three different spans in the test. The last span is Eq. The find_impls implementation asks rls-analysis for the typeid of the Eq type (trait). Eq is defined in the stdlib, however Travis does not install the rustup rust-analysis component which would be needed. This means rls-analysis fails to find the typeid for Eq.

The url in the comment is passing because for debugging I installed the rust-analysis component once on Travis. If I do this the test passes.

Now the code tests Sub and Super. The Eq is commented out with a reasoning why it does not work on Travis. Derives where never tested in the first place.

nrc · 2017-08-17T06:45:27Z

Ah, OK, I understand thanks! And thanks for working on this for so long!

jonasbb · 2017-08-17T16:24:51Z

I'm just glad that finally everything is in order :)

Xanewok · 2017-09-16T08:29:25Z

@jonasbb I tried to use the Find Implementations, but got this error:
Analysis: Getting typeid from span: Unclassified.
Got any idea what's causing it? Can I help debug it somehow? I tried trying it out in member package inside a workspace, can that affect it somehow?

jonasbb · 2017-09-16T12:31:21Z

The error message means, that rls-analysis is not able to get a typeid from a span. This either means the span (selected characters) does not match any known type or somehow the analysis data is missing a Def definition for this type. It would help if you could mention what was under your cursor when invoking the command.

I don't know if a workspace has any influence on the analysis data.

Xanewok · 2017-09-16T13:39:51Z

@jonasbb
Given:

struct MyStruct;
trait MyTrait {}
impl MyTrait for MyStruct {}

I had crashes for MyStructs and MyTraits, either with cursor inside or when selecting a whole symbol. Am I doing it wrong?

Meta:
Using rls from current master branch.

$ rustc --version
rustc 1.22.0-nightly (539f2083d 2017-09-13)
$ rustup --version
rustup 1.6.0 (a11c01e8c 2017-08-30)
$ rustup toolchain list
nightly-x86_64-unknown-linux-gnu (default)
$ rustup component list
cargo-x86_64-unknown-linux-gnu (default)
rls-x86_64-unknown-linux-gnu (installed)
rust-analysis-x86_64-unknown-linux-gnu (installed)
rust-docs-x86_64-unknown-linux-gnu (default)
rust-src (installed)

jonasbb · 2017-09-16T14:00:19Z

@Xanewok For a simple library project (only a lib.rs with your content above) it works for me. I try to look at a workspace later. Although I have never used them before, so it might take a while.

Xanewok · 2017-09-17T10:30:43Z

I tried doing the same in bare lib or bare bin project, but to no avail. I'll try fixing my setup.

nrc mentioned this pull request Mar 2, 2017

Tracking issue: new features #194

Closed

3 tasks

jonasbb changed the title ~~Implement two custom extensions~~ Implement find implementations extension Mar 2, 2017

nrc reviewed Mar 2, 2017

View reviewed changes

jonasbb mentioned this pull request Mar 2, 2017

Cleanup spans with references to non-existing files rust-dev-tools/rls-analysis#49

Open

nrc reviewed Mar 3, 2017

View reviewed changes

nrc reviewed Jun 14, 2017

View reviewed changes

jonasbb added 3 commits August 17, 2017 00:00

Implement custom find implementations extension

d417558

List all implementation block for traits, structs, and enums. Fails for traits which are implemented using derive Listed under wishlist #142 Requires the find-impls branch of https://github.com/jonasbb/rls_vscode

Implement testcase for find impls

b8655b6

Fix test for Travis since rust-analysis is not installed

67fb3a4

nrc merged commit 85724ef into rust-lang:master Aug 17, 2017

jonasbb deleted the find-impls branch September 16, 2017 11:46

Xanewok mentioned this pull request Sep 16, 2017

The call can be inside the new thread to avoid crashing RLS if it fails #481

Merged

kraai mentioned this pull request Sep 26, 2017

Find Implementation of traits rust-lang/rust-mode#241

Closed

Xanewok mentioned this pull request Feb 25, 2019

Cleanup spans with references to non-existing files #1351

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement find implementations extension #181

Implement find implementations extension #181

jonasbb commented Feb 22, 2017

nrc commented Feb 23, 2017

jonasbb commented Feb 23, 2017

nrc commented Feb 23, 2017

jonasbb commented Mar 2, 2017

nrc left a comment

nrc Mar 2, 2017

jonasbb Mar 2, 2017

nrc Mar 2, 2017

jonasbb commented Mar 2, 2017

nrc left a comment

nrc Mar 3, 2017

nrc commented Mar 3, 2017

Xanewok commented May 8, 2017

jonasbb commented Jun 13, 2017

nrc left a comment

nrc Jun 13, 2017

nrc Jun 13, 2017

nrc Jun 14, 2017

nrc commented Jun 14, 2017

jonasbb commented Aug 15, 2017

nrc commented Aug 16, 2017

jonasbb commented Aug 16, 2017

nrc commented Aug 17, 2017

jonasbb commented Aug 17, 2017 •

edited

Loading

nrc commented Aug 17, 2017

jonasbb commented Aug 17, 2017

Xanewok commented Sep 16, 2017

jonasbb commented Sep 16, 2017

Xanewok commented Sep 16, 2017

jonasbb commented Sep 16, 2017

Xanewok commented Sep 17, 2017

Implement find implementations extension #181

Implement find implementations extension #181

Conversation

jonasbb commented Feb 22, 2017

nrc commented Feb 23, 2017

jonasbb commented Feb 23, 2017

nrc commented Feb 23, 2017

jonasbb commented Mar 2, 2017

nrc left a comment

Choose a reason for hiding this comment

nrc Mar 2, 2017

Choose a reason for hiding this comment

jonasbb Mar 2, 2017

Choose a reason for hiding this comment

nrc Mar 2, 2017

Choose a reason for hiding this comment

jonasbb commented Mar 2, 2017

nrc left a comment

Choose a reason for hiding this comment

nrc Mar 3, 2017

Choose a reason for hiding this comment

nrc commented Mar 3, 2017

Xanewok commented May 8, 2017

jonasbb commented Jun 13, 2017

nrc left a comment

Choose a reason for hiding this comment

nrc Jun 13, 2017

Choose a reason for hiding this comment

nrc Jun 13, 2017

Choose a reason for hiding this comment

nrc Jun 14, 2017

Choose a reason for hiding this comment

nrc commented Jun 14, 2017

jonasbb commented Aug 15, 2017

nrc commented Aug 16, 2017

jonasbb commented Aug 16, 2017

nrc commented Aug 17, 2017

jonasbb commented Aug 17, 2017 • edited Loading

nrc commented Aug 17, 2017

jonasbb commented Aug 17, 2017

Xanewok commented Sep 16, 2017

jonasbb commented Sep 16, 2017

Xanewok commented Sep 16, 2017

jonasbb commented Sep 16, 2017

Xanewok commented Sep 17, 2017

jonasbb commented Aug 17, 2017 •

edited

Loading