Rls: RLS "textDocument/definition" always returns `[]` if invoked on `.cargo` cache entries

[norru]

I have imported the whole of .cargo as a project in Eclipse Corrosion - textDocument/hover seems to work for the entries but textDocument/definition doesn't (so Ctrl+click for quick navigation doesn't work)

LSP4E to org.eclipse.corrosion.rls:{"jsonrpc":"2.0","id":"229","method":"textDocument/definition","params":{"textDocument":{"uri":"file:///home/nico/.cargo/registry/src/github.com-1ecc6299db9ec823/log4rs-0.8.0/src/config.rs"},"uri":"file:///home/nico/.cargo/registry/src/github.com-1ecc6299db9ec823/log4rs-0.8.0/src/config.rs","position":{"line":286,"character":17}}}
org.eclipse.corrosion.rls to LSP4E:{"jsonrpc":"2.0","id":229,"result":[]}
LSP4E to org.eclipse.corrosion.rls:Content-Length: 327


LSP4E to org.eclipse.corrosion.rls:{"jsonrpc":"2.0","id":"230","method":"textDocument/hover","params":{"textDocument":{"uri":"file:///home/nico/.cargo/registry/src/github.com-1ecc6299db9ec823/log4rs-0.8.0/src/config.rs"},"uri":"file:///home/nico/.cargo/registry/src/github.com-1ecc6299db9ec823/log4rs-0.8.0/src/config.rs","position":{"line":286,"character":17}}}
org.eclipse.corrosion.rls to LSP4E:{"jsonrpc":"2.0","id":230,"result":{"contents":[{"language":"rust","value":"appenders: Vec<Appender>"}]}}

[alexheretic]

Most rls functionality works from compiler analyses. So in general if you can't cargo check it you can't rls it.

Racer is a notable exception not using the compiler at all, so functionality that uses racer may continue to work in directories that can't be compiled.

[norru]

So, the workaround is to switch to an IDE which doesn't rely on RLS, such as CLion?

[norru]

Or, would it be possible to add options so the RLS reads other directories? Browsing through libraries is a very common operation. IIRC RustDT (based on Racer) could do this, at least partially.

[norru]

@alexheretic Why does textDocument/hover work though? Is it not implemented the same way as textDocument/definition?

[alexheretic]

Or, would it be possible to add options so the RLS reads other directories? Browsing through libraries is a very common operation. IIRC RustDT (based on Racer) could do this, at least partially.

It would be cool for rls to handle 1 or more child projects without a workspace. Perhaps a rls-manager wrapper that starts up rls processes for each folder. But it's not a simple thing and maybe is a clientside concern.

Why does textDocument/hover work though? Is it not implemented the same way as textDocument/definition?

Looking at the source textDocument/definition does not use racer, it has a fallback to racer which is disabled by default. textDocument/hover also falls back to using racer, but this is enabled by default, more precisely it uses the racer_completion config which is enabled by default.

[norru]

How does one enable Racer for the textDocument/definition fallback?

[alexheretic]

You can enable the definition racer fallback with goto_def_racer_fallback = true, however you're client does config. See https://github.com/rust-lang/rls/blob/master/src/config.rs#L130

I'd guess this is soft-deprecated as we don't document this config at the moment.

[norru]

This configuration option is not available in Corrosion. We need a way to set this option in RLS.

@mickaelistria @alexheretic @nrc

We are going round in circles here. We need to solve the following paradox:

• Corrosion will not implement this functionality because its developers think it's a RLS specific option.

Corrosion builds on LSP4E, which has no plan to support language server-specific settings and only settings which are available in EVERY language server will be supported by LSP4E

• RLS will not implement this functionality because its developers think it's a Corrosion-specfic feature.

RLS does not provide any means of configuration outside of the client interface. This means than every client has to separately build a configuration interface and try to keep up with the configuration settings, which requires a lot of (usually scarce) time and effort.

This has been the case for many months. As a result there are many issues open in Corrosion caused by the root problem: users are still unable to change settings in Eclipse.

Would you gentlemen please agree on a solution?

[alexheretic]

My take is that the current config situation is quite reasonable, rls provides the best general purpose default setup without config but also allows configuration from the client via the LSP.

A language server client in general is a whole load of code common to all clients and a little specialised to a particular server. For example ide-rust mostly relies on atom-languageclient, but it also has some RLS specific stuff of it's own like config, toolchain management & window/progress handling. So it was possible here to have the LSP dealt with by a common library but allowing extension to deal with custom stuff too.

I personally didn't find the client config much effort at all. ide-rust just watches a root rls.toml file and sends whatever the contents is.

On the other hand if you don't implement any client config stuff, rls _still_ works. Indeed it should work well.

To be clear, I'm not the guy that makes decisions for rls. You can totally ignore me. I just send in occasional patches to make the git log statistically more handsome. But why not accept that rls wants config sent from client and work from there?

• Would #1026 have helped? If so where is the pr? This would not be too hard to develop as far as I can see.
• Why not develop the client code to send config and see how bad it is?
• Or develop a tiny binary that starts rls as a child process forwarding over it's own stdin, but also watches for a rls.toml file sending workspace/didChangeConfiguration for changes. You could use that binary instead of rls directly and share it with any other client that wants similar functionality.
• Or send in a pr for an optional rls.toml watching mode rls --watch-rls-toml.

[norru]

But why not accept that rls wants config sent from client and work from there?

I think this is the whole point - again I summon @mickaelistria :)

What happens when an unstoppable force meets an immovable object?

The clients could say "why not accept that you have to configure your language server using language server settings and move from there?", which may be not far from the truth in LSP4E which has to handle language servers in a generic way and not just RLS, and definitely the LSP4E development team doesn't appear to have time and resources to take on this task.

I would look into it myself but I have little spare time and can use it to either write Rust code or write Java code which allows me to edit Rust code. My priority is to write Rust code. Writing Java code to fix the ways Rust code is edited would not leave me with enough time to write Rust code, thus defeating the purpose.

[norru]

Or develop a tiny binary that starts rls as a child process forwarding over it's own stdin, but also watches for a rls.toml file sending workspace/didChangeConfiguration for changes. You could use that binary instead of rls directly and share it with any other client that wants similar functionality.

Looks like a "S.E.P." solution.

This will produce another fiddly moving part which has to comply not with one but with two interfaces (and keep track of changes in both), and needs to be maintained, documented, distributed as a rustup component etc.

ide-rust just watches a root rls.toml file and sends whatever the contents is.

This would be a good compromise (I think we floated this idea in the Corrosion discussion threads). Still the question remains: if this is so common an option, why does every client has to reinvent it? Wouldn't it be more practical for everybody to just have the RLS load its own config from somewhere handy?

Or send in a pr for an optional rls.toml watching mode rls --watch-rls-toml.

@mickaelistria - would Corrosion support optional command line parameters in the rls startup? Would an env var bypass the problem instead (at the cost of usability)?

@alexheretic for all my purposes, https://github.com/rust-lang/rls/issues/1047 would do perfectly. I can live with RLS only reading its config at startup even if it means I have to restart it when I need a configuration change. I have to restart it many times anyway given it often gets stuck or silently stops working (or is it LSP4E?).

[alexheretic]

if this is so common an option, why do every client has to reinvent it?

Well no-one else has asked for it, so it's not common. Even I wouldn't use it in ide-rust, as clientside config gives clients more flexibility. (I use it so I can set global config for clippy_preference & all_targets in ide-rust settings, which I think is nice to have) Afaik ide-rust is the only client using rls.toml, and I may migrate away once atom's own per-project configuration ideas stabilize.

[alexheretic]

@norru out of interest what has you wedded to eclipse anyway?

[mickaelistria]

It would be cool for rls to handle 1 or more child projects without a workspace.

The Language Server Protocol has support for the workspaceFolders for this case (or you're talking about something else?)

rls provides the best general purpose default setup without config

No, it does not provide the best general purpose default setup without config. This ticket clearly shows that many people would expect by default the RLS to be configured to support some navigation of libraries in .cargo with textDocument/definition, and it does not.
For example, this goto_def_racer_fallback = true should be the default in RLS if it's providing better results. If I understood the discussion, just changing that could be a fix for the issue that wouldn't hurt neither RLS nor Corrosion.

if this is so common an option, why does every client has to reinvent it?
...
Well no-one else has asked for it, so it's not common.

I think it's just taking the time for RLS to learn from its various clients. I can understand those cases are not obvious to consider upfront when dealing with a single client and when not realizing the cost multiplies by the number of integrations. With the clients becoming more numerous and diverse, it's natural that such requirements emerge now as a rationalization of effort.
By RLS being more and more used, such requests for less adoption effort will become common.

would Corrosion support optional command line parameters in the rls startup?

Yes, that's fine if we need some flags; as usual it'd be better if it were the vanilla execution instead of a diverged one so it would be shared with ide-rust or others and would allow better factorization of effort.

Even I wouldn't use it in ide-rust, as clientside config gives clients more flexibility.

Flexibility has a cost that's not necessarily worth it in all IDEs. For Corrosion, it's something we cannot afford at the moment and really want to avoid as much as necessary as Eclipse lessons have taught us that configuration is something the vast majority of end-users seem to want, but never do properly so just good defaults is often more important.

[alexheretic]

I imagine goto_def_racer_fallback was disabled for performance reasons, considering it shouldn't need to be used. I've not heard of people opening their .cargo as a project before, it certainly isn't normal. But I do agree that RLS could have a better strategy for projects with rust projects within that aren't formal cargo-workspaces. Though creating a root Cargo.toml workspace is a general solution that works now.

I've only just read the description of workspaceFolders but it certainly sounds related. It also seems like more work for the client. Wouldn't the client have to scan the .cargo dir and pass all the Cargo.toml containing child-dirs as workspaceFolders? But then we also have to check that these aren't already part of a cargo-workspace (in the general case). It does sound a little painful, but hopefully I'm missing something.

[mickaelistria]

>

But I do agree that RLS could have a better strategy for projects with
rust projects within that aren't formal cargo-workspaces.

Yes, that'd be great to support .rs files that are "into the wild" without
a related Cargo.toml.

Though creating a root Cargo.toml workspace is a general solution that
works now.

If you mean user or IDE creating this Cargo.toml, then it's more a
workaround than a solution. Could RLS fail-back to this strategy of using a
dummy Cargo.toml for files which aren't detected as being in a Cargo
workspace?

[alexheretic]

Yes "workaround" is definitely closer to the mark. I agree it with you on the wild rust file support.

[norru]

@alexheretic, It's not like I have to use Eclipse :). I just happen to have many years of experience on it (so if anything breaks I am able to fix it quickly) and it ticks a large amount of boxes:

• reliable editor with configurable shortcuts and easy navigation
• multiplatform (Windows + Linux, optionally macOS if available)
• efficient navigation (go to declaration/go to definition) with history back/forward. This is how I learn other people's code.
• sensible management of compiler errors
• integrated debugging with pretty printers
• direct mapping of project to filesystem (no Visual Studio-style projects)
• configurable color schemes
• reliable autocompletion
• flexible user layout with drag and drop so I can place editors, projects, inspections and project trees wherever I need them on screen at any time
• git integration with up-to-date live status and changelist and one click commitand push
• reilable check-as-you-type error detection
• multiple languages in the same workspace
• multiple projects in the same workspace
• (sometimes) multiple languages in the same project
• multiple workspaces
• multiple instance of the IDEs running concurrently with different language and toolchain setups
• global error and warning list
• in-IDE plugin download and auto-update
• quite a lot of other stuff (such as refactoring tools, autoformatting, templates etc...)

If you're wondering, I do indeed use all the stuff above pretty much routinely.

The best Rust IDE I have ever used was RustDT on Eclipse, which is alas abandoned. I tried Visual Studio Code a few months ago but I didn't manage to customize the desktop's layout so I quickly gave it up for CLion, which partially suffers from the same. I had been using CLion with IntelliJ-Rust (IntelliJ itself has no Rust debugging) for a while but I got fed of the horrible Inspections and compiler error feedback (look mom, no RLS!) so as soon as Corrosion became available and just barely usable I jumped back to Eclipse.

There are not many IDEs out there with this level of flexibility and as feature-rich. I use it a lot and every time I try to switch out there are one or two important features missing. I usually try to adapt my workflow as well but if in a few months worth of use I am not proficient enough I always end up switching back.

I also use other IDEs and editors for different applications. The best tool for the job and all.

(edit: grammar and syntax)

[alexheretic]

@norru fair enough! It is a great thing about LSP that we can contribute to better ide experience across a bunch of editors.

I think getting #1026 done would be a good first step. It should be easy to develop, and no-one seems to disagree with it. We just need to find someone to do it. I could probably bosh it out sometime, but no promises on when.

[norru]

I've not heard of people opening their .cargo as a project before, it certainly isn't normal.

Dunno. Use case really is simple. Perhaps I'm old school? I wonder how people write code these days?

• write user/app code that uses crates.io lib
• run your code, something doesn't work as it should
• write unit test if necessary
• put breakpoint in user code to find bug
• step down a few levels until you track your data in any library stored in the .cargo cache
• place another few breakpoints in the lib to repeat and track down the bug by inspecting variables (the bug may well be in the library itself!) [this requires .cargo in Eclipse]
• in order to figure out what's going, and to understand how things interact with each other, navigate around the code by means of "go-to-definition", jumping around lib code and application code [this requires .cargo in Eclipse but currently doesn't work for libs, so I have to grep/search/browse filesystem a lot]
• repeat steps above a few times until you find the bug
• if the bug is in lib submit a Github issue or a PR, if the bug is in app code just fix the bug. As a very nice side effect you have learned a little bit about the lib you were tracing into.
• repeat many times a day
• profit

[nrc]

If you're in a Cargo project which depends on a lib, then that lib should be indexed by the RLS. I.e., you can jump into it from normal code and continue to explore the lib (in the Cargo cache). Unless the crate is black-listed, then that should Just Work.

[norru]

@nrc @mickaelistria no, doing that does not work for me. 1) in order for the source code to be recognised by LSP it has to be in a project (that's why we import the whole of .cargo and 2) once I've added the library as .cargo project, navigating through the library (or its dependencies) via definition does not work, but hover does.

Unless I'm so terribly unlucky that ALL my dependencies are blacklisted...?

[norru]

Besides, I often see this behaviour (hover works but definition doesn't) even in non .cargo entries and even in local declarations, so there is definitely someting broken in the definition resolution. Restarting RLS fixes these cases though.

Currently the only way to enforce a reliable restart of RLS in Corrosion is to restart the IDE.

This is why I have asked the Corrosion team to implement a "reboot RLS" button.

[mickaelistria]

I imagine goto_def_racer_fallback was disabled for performance reasons, considering it shouldn't need to be used.

If this is a fallback, why does it affect performance? Isn't a fallback supposed to only startup lazily when main strategy fails? In such case, it would only induce a performance cost when there is no better known solution.
It still seems to me having this on by default -if implemented as I described above- would be greatly profitable without much overhead, and moreover easy to implement as far as I understand.

[alexheretic]

It's not implemented like that, presumably to minimise latency when it was more used in _"rustc analysis goto doesn't work"_ context. See https://github.com/rust-lang/rls/blob/master/src/actions/requests.rs#L197.

I'd support having it enabled by default if the execution came after the default failing. It would be trivial to change the behaviour so lets continue the discussion in your pr. Also see https://github.com/rust-lang/rls/issues/1106#issuecomment-442416174.

[norru]

eclipse/corrosion#182

[norru]

(mostly) fixed in eclipse/corrosion#183 and https://github.com/rust-lang/rls/pull/1152 by enabling Racer fallback by default