search

rust-lang / rls

Repository for the Rust Language Server (aka RLS)
Other
3.51k stars 258 forks source link

Cleanup spans with references to non-existing files #1351

Open Xanewok opened 5 years ago

Xanewok commented 5 years ago

Originally created by @jonasbb (https://github.com/rust-dev-tools/rls-analysis/issues/49):

The issue comes up due to the newly implemented find implementations feature. Also see this pull request for background information: https://github.com/rust-lang-nursery/rls/pull/181

The problem is that finding all implementations for traits of the standard library (like PartialEq) return spans with references to files located under the path /checkout/src/libstd or /checkout/src/libcore/ (under linux, may be different for windows and osx). These are non-existent and thus lead to poor user experience.

This is copied from the pull request:

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.

There is nothing which makes a path starting with /checkout/src/ invalid. A user could develop all her rust projects under this path. There is also no other indicator in the serialized data that these are invalid paths. This makes it harder to find a good solution for this.

A possible solution might be to rewrite all invalid paths to the rustup download of libstd. This would require to first check all paths if they are present on the file system and would fail if someone would create a file like /checkout/src/libcore/ops.rs with non-matching source code. Non-matching could either be a file unrelated to libcore (a simple name clash) or libcore in the wrong version, in which case the spans would point to the wrong place.

Xanewok commented 5 years ago

Originally replied by @nrc (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-283831484):

There is also no other indicator in the serialized data that these are invalid paths

Defs from std libs is serialised a bit differently and is marked as api_crate (https://github.com/nrc/rls-analysis/blob/master/src/lib.rs#L525). If necessary we can cross-reference spans with defs to get this info (or duplicate the info for spans or whatever). That seems more reliable than using the filename.

We already do something like this for 'goto def' - if the def is in the std libs, we don't offer jump to def, but do offer a URL for docs (or something like that, I don't recall exactly how this works). We should do pretty much the same for find all impls (or perhaps implement a generic mechanism if possible).

Xanewok commented 5 years ago

Originally replied by @jonasbb (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-304535179):

@nrc Hi, after quite some delay I wanted to pick up this work. However, I am basically still stuck at this issue without any good idea how to solve it.

In your last message you reference the api_crate field for Defs. The flag is true if the file containing the definition is a JsonAPI file instead of a Json file. I don't have any JsonAPI files on my system. Neither the rustup installed files nor the files in target/rls/debug/deps/save-analysis/ are JsonAPI.

I have found a couple of issues related to this problem: https://github.com/rust-lang-nursery/rls/issues/227 and https://github.com/editor-rs/vscode-rust/issues/157

nrc, do you know another way to determine if the span stems from a library crate or the current project? For my pull request I think it could be enough to check all file paths against root_uri or root_path (from LSP), provided that they are set.

Xanewok commented 5 years ago

Originally replied by @nrc (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-304541852):

In your last message you reference the api_crate field for Defs. The flag is true if the file containing the definition is a JsonAPI file instead of a Json file. I don't have any JsonAPI files on my system. Neither the rustup installed files nor the files in target/rls/debug/deps/save-analysis/ are JsonAPI.

Hmm, this might be a bug - all the files from Rustup should be JsonApi, not Json.

do you know another way to determine if the span stems from a library crate or the current project?

What exactly do we need to know for 'library crate'? That it is distributed by Rustup rather than generated locally? I don't think there is anything to check for that, but we could add a flag to the compiler to set a field that communicates that. The kind of the data (Json vs JsonApi) should indicate that at the moment - we should fix if not - but in the future we might use Api crates locally too, so that is not a great solution.

Xanewok commented 5 years ago

Originally replied by @jonasbb (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-304543335):

I just need to know if the source file is a library crate or the current project. For example, if I want to see all implementations for Eq I don't want any references to files outside of the current project, because this only clutters the UI (and all the spans are invalid anyway).

Xanewok commented 5 years ago

Originally replied by @nrc (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-304543638):

I just need to know if the source file is a library crate or the current project

So, depend crates should count as current project right? And if the user builds the std libs from source themselves, rather than installing via Rustup, should that be considered a library crate or current project (I assume the filenames would not be a problem in this case).

Xanewok commented 5 years ago

Originally replied by @jonasbb (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-304544508):

Personally, I would not show any results in "find implementations" for dependent crates. If the user builds std libs from source, its still a library, so I still wouldn't show it for the "find implementations" part.

A general solution to the paths might need a different solution. From some comments, e.g. https://github.com/rust-lang-nursery/rls/issues/227#issuecomment-297220808, it seems that goto_def into the std lib works more by accident (in some cases) then it being supported by RLS. Since its the std libs something like open the documentation in the browser can work here. External crates have no easy place for documentation, since https://docs.rs only works for published crates.

Currently, I see three cases:

Xanewok commented 5 years ago

Originally replied by @nrc (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-304727408):

Personally, I would not show any results in "find implementations" for dependent crates.

I don't agree with this. While it might make sense for some utility crate from crates.io, most large projects are multi-crate ones, and you probably want to see implementations in those crates. E.g., the RLS has the rls-analysis crate, if I implement a trait in that for a type in RLS, then I want to see that trait implementation.

More generally, I think it is better to show more impls, rather than fewer - it's easy to skip impls you're not interesting in, but finding impls you are interested in that aren't on the list would be frustrating.

For goto def, jumping into the std libs shouldn't work. I don't think we can or should make it work, but we should offer the URLs for source and docs. I think this is just debugging, hopefully there aren't serious issues.

For external crates, goto def mostly works by jumping to the source that Cargo downloaded. I think this basically works. However, it can be annoying because you shouldn't actually edit that file, and where there are multiple crates in one repo/project, you might have better versions of the source code locally. I'm not sure if it is easy/possible to make this work.

Xanewok commented 5 years ago

Originally replied by @dodheim (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-304868645):

For goto def, jumping into the std libs shouldn't work. I don't think we can or should make it work, but we should offer the URLs for source and docs.

Why? As a user, I do want to do this, regularly. It's part of my development workflow. Disallowing goto-def on stdlib source seems absolutely arbitrary; if there's no serious technical hurdle, please reconsider this...

Making one jump through hoops to get tooling to work for the stdlib that already works for everything else strikes me as completely contrary to the ergonomics initiative if nothing else. ;-]

Xanewok commented 5 years ago

Originally replied by @jonasbb (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-304875897):

My logic is basically the other way around. The stdlibs are not that powerful, so even minor project probably rely on many external crates. I don't want to see search results for crates which I just use as a dependency.

I understand your point of splitting projects into multiple subcrates though. Maybe this needs even more fine grained results? Like if a crate is pulled from crates.io don't show it (because it is just a dependency and you are not developing on that remote crate) and if the crate is pulled from a file path then you are likely working on it, so show results? Although this might be confusing, why suddenly some results are not shown anymore, just because of slight changes and in the Cargo.toml and I don't know what to do with crates pulled directly from git. They could hit both use cases.

Personally, I do like the jump to stdlib behaviour. And it is fact that it currently works (through racer). Also keep in mind: jumping to documentation might not work in all cases (e.g. console text editor), which showing rust source code should work always.

EDIT: The simplest solution might just be to provide a flag for the stdlib in the analysis files, this allows the spans to be corrected. And then just send all results back to the client and do the filtering there. This way everybody can configure it however they want.

Xanewok commented 5 years ago

Originally replied by @nrc (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-305011558):

@dodheim this depends on whether the user has source code for the std libs in the first place. We kind of accidentally got users to install it as its part of the Racer installation steps, but I'm not sure it should be required. Also opening the std libs in the editor is not necessarily optimal - it is read-only for one thing, and we might be able to offer a better experience in a web browser than an IDE. Of course there is something to be said for having it all in one place.

Xanewok commented 5 years ago

Originally replied by @dodheim (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-305054587):

That seems like a good basis for a policy – do proper goto-def if the source is present, or show docs otherwise. But showing docs even if I've gone out of my way to have the source locally is antithetical to:

As to what is or isn't 'optimal', that's certainly subjective. For my workflow, VSCode's Peek Definition is hands-down the best way to see trait methods (and copy/paste their signatures) that I need to implement in impl ... for ..., member fields and their types when I'm initializing a large struct, cases for a large enum when I'm matching, etc. This is my workflow in multiple languages; especially as someone relatively new to this language, making Rust the odd one out just makes me less likely to use it until "tooling matures" (i.e. I can use it the way I want to). Again, I already have the docs, but that's not nearly as convenient; and again, half the time, I really, really do want to see the implementation, whether for debugging purposes or simple curiousity.

Apologies if I come across as aggressive, I just happen to feel quite.. strongly about this. ;-D

Xanewok commented 5 years ago

Originally replied by @jonasbb (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-305111077):

[...] it is read-only for one thing, and we might be able to offer a better experience in a web browser than an IDE. The same is true for all external crates which are loaded not from a file path (either crates.io or git). Yet you don't mind goto def for crates and explicitly want to see external crates in the find implementations implementation.

@nrc Could you maybe elaborate a bit more on why you think there is such a big difference between crates and stdlib?

I don't want to force users to install stdlib, if they don't want to, but we could offer something like Eclipse does for Java and @dodheim already mentioned. Jump to source if it is present, otherwise advice the client where the documentation is.

Xanewok commented 5 years ago

Originally replied by @nrc (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-305300541):

Could you maybe elaborate a bit more on why you think there is such a big difference between crates and stdlib?

For most users: for std, source code is not present, and if it is it should be considered read-only. For crates, the source code is always present (for now, at least, in a future with closed source crates, I expect to treat them like std) and sometimes is editable. Furthermore, it is extremely rare that I want to see the source code for std functions - generally they are well-documented and the fucntionality is obvious, for a crate from crates.io, it is fairly common (for me) that the source code is useful.

Xanewok commented 5 years ago

Originally replied by @jonasbb (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-305604481):

This thread got a bit sidetracked. Back to the original question. How can I find out if it is the std libs or not. How to symbolize it? Just use the JsonApi value or have a special flag and where is the correct place to fix it?

Xanewok commented 5 years ago

Originally replied by @nrc (https://github.com/rust-dev-tools/rls-analysis/issues/49#issuecomment-306361671):

@jonasbb I would just check for JsonApi kind. I've got a PR that will fix the bug with it not being set, will land that once the tests have run.