RLS always fails textDocument/definition, ctrl-hover and ctrl-click no longer working

[norru]

rls-preview 0.130.5 (1c755ef 2018-10-20) rustc nightly-2018-10-28

Ctrl-click and ctrl-hover no longer work, at all. "result" is always []

LSP4E to org.eclipse.corrosion.rls:{"jsonrpc":"2.0","id":"1006","method":"textDocument/definition","params":{"textDocument":{"uri":"***/src/render.rs"},"uri":"***/src/render.rs","position":{"line":92,"character":52}}}
org.eclipse.corrosion.rls to LSP4E:{"jsonrpc":"2.0","id":1006,"result":[]}

UPDATE:

• Toolchain nightly-2018-09-20 is unaffected
• Toolchain nightly-2018-10-17 is unaffected
• Toolchain nightly-2018-10-20 is affected
• Toolchain nightly-2018-10-28 is affected
• Toolchain nightly-2018-11-14 is affected
• Toolchain nightly-2018-11-20 is in some cases affected

[norru]

Checking if this was the cause: 2aa398b

[norru]

In fact

• Toolchain nightly-2018-10-17 is not affected so the Racer upgrade might have broken textDocument/definition (at least in Eclipse)

[norru]

@mickaelistria

[dvdplm]

Is this the equivalent of "Go to definition" in vsCode? Seems completely broken on recent nightlies.

[norru]

Yes, and yes. Pretty much.

[Xanewok]

Might be fixed by rust-lang/rust#55521.

[norru]

Seems fixed in

nightly-x86_64-unknown-linux-gnu rustc 1.32.0-nightly (8b096314a 2018-11-02)

EDIT: my first test failed but after a kick of the IDE it does seem fine.
EDIT: I believe my success was spurious. Failing still in 2018-11-14

[norru]

Unfortunately due to #1107 I cannot upgrade my Nightly.

[norru]

Spoke too soon, last toolchains still having broken rls-preview 0.130.5 (1c755ef 2018-10-20)

[norru]

@Xanewok, do you need to release the fix?

$HOME/.rustup/toolchains/nightly-2018-11-14-x86_64-unknown-linux-gnu/bin/rls --version

rls-preview 0.130.5 (1c755ef 2018-10-20)

[norru]

I am seeing this problem intermittently (problem does not seem to be 100%) - cargo clean and a restart of the IDE seems to fix it for a while.

[diaevd]

with beta now working, with nightly partialy.
I noticed coincedence, if the "textDocument/documentHighlight" works, then the "textDocument/defintion" works too.
"textDocument/hover" - works in the same place.
code like in the same file:

use crate::api::{
    auth::signup,
    home::index,
};

sighup - all just fine.

.resource("/signup", |r| {
    r.method(Method::GET).with(signup);
}

signup:
"textDocument/hover" - works
"textDocument/documentHighlight" - fail
"textDocument/defintion" - fail

in the beta all works.

[Xanewok]

@norru relevant fixes were on librustc_save_analysis, which resides inside the Rust repo. The fixes were made regarding how the underlying index data is collected but this shouldn't affect RLS front.

I tried testing RLS with Rust master with rust-lang/rust#55936 applied and it seems to be fixed now, but today's nightlies don't contain the fix yet, hopefully the new ones will.

Could you try using the next nightly version to see if the problem still persists?

[norru]

@Xanewok yes, I'm waiting for the next drop of rls-preview in nightly. As soon as it drops I'll test again. Problem doesn't seem to be 100%. When it starts happening once, it sticks (so becomes 100% from a certain point on), but it can be worked around by doing a cargo clean (which deletes the rls, and cargo's binaries cache).

In fact, now that I found a workaround I can safely update nightly as usual.

[Xanewok]

Uh, I think we still might need a new version of rls-analysis, which means we still need to update the RLS in-tree (so just updating nightly probably won't fix this).

However, I think this still might not work in the general case, while setting "rust.all_targets": false seems to work around it for now.

[norru]

How do you set "rust.all_targets": false?

[norru]

Can no longer repro after 2011-11-18.

[diaevd]

"textDocument/defintion" - still not working with nightly

$ rustc -V
rustc 1.32.0-nightly (f1e2fa8f0 2018-11-20)
$ rls -V
rls-preview 1.31.6 (daa138c 2018-11-20)

beta chain works just fine

$ rustc -V
rustc 1.31.0-beta.15 (4b3a1d911 2018-11-20)
$ rls -V
rls-preview 1.31.6 (daa138c 2018-11-20)

before the test always did cargo clean

[norru]

Can no longer reproduce as of 2018-11-23

[diaevd]

No, in nightly rls return nothing. Like i wrote above. Why are you closed?

Request/response Output from rls with nightly toolchain:

{
  "jsonrpc": "2.0",
  "method": "textDocument/definition",
  "params": {
    "textDocument": {
      "uri": "file:///home/diabolo/src/rust/bbs/db-service3/src/router.rs"
    },
    "position": {
      "line": 54,
      "character": 52
    }
  },
  "id": 121
}
Output from language server: {
  "jsonrpc": "2.0",
  "id": 121,
  "result": []
}

$ rustc -V
rustc 1.32.0-nightly (1f57e4841 2018-11-23)
$ rls -V
rls-preview 1.31.6 (daa138c 2018-11-20)

Request/response Output from rls with beta toolchain:

{
  "jsonrpc": "2.0",
  "method": "textDocument/definition",
  "params": {
    "textDocument": {
      "uri": "file:///home/diabolo/src/rust/bbs/db-service3/src/router.rs"
    },
    "position": {
      "line": 54,
      "character": 52
    }
  },
  "id": 35
}
Output from language server: {
  "jsonrpc": "2.0",
  "id": 35,
  "result": [
    {
      "uri": "file:///home/diabolo/src/rust/bbs/db-service3/src/api/auth.rs",
      "range": {
        "start": {
          "line": 11,
          "character": 7
        },
        "end": {
          "line": 11,
          "character": 13
        }
      }
    }
  ]
}

$ rustc -V
rustc 1.31.0-beta.16 (1c16fa472 2018-11-23)
$ rls -V
rls-preview 1.31.6 (daa138c 2018-11-20)

[diaevd]

Temporary solution is enable "goto_def_racer_fallback" which is false by default. Racer fallback is enabled by default for hover/highlight. Internal resolve definition not works.

[norru]

definition appears to be basically broken.

There is no way to set "goto_def_racer_fallback" option to True in Corrosion.

@nrc @mickaelistria #1139

@alexheretic why is the default setting to fallback to Racer on for hover but off for definition? They should be both on, or both off, by default. Having inconsistent results between the two doesn't have any good rationale I can think of. "Performance reasons" doesn't seem to be a good argument unless there is a benchmark result where latency has been measured, and proven bad.

This bug happens randomly to me - couldn't figure out what triggers it. To me, restarting RLS temporarily fixes it for a little while.

[diaevd]

@norru for me it persistent. Only enabling fallback to racer gives me opportunity to work properly with nightly toolchains for now.

P.S. Performance reasons - insignificant
P.P.S. Thanks for link to #1139 issue, didn't read it early

[alexheretic]

why is the default setting to fallback to Racer on for hover but off for definition? They should be both on, or both off, by default. Having inconsistent results between the two doesn't have any good rationale I can think of. "Performance reasons" doesn't seem to be a good argument unless there is a benchmark result where latency has been measured, and proven bad.

Having developed neither, my analysis would be that they were developed at different times in an evolving project. definition used to be a race between racer & rustc-analysis to minimise latency, later I suppose it was decided the latter solved the problem well enough to disable the former.

Generally rls was moving away from relying on racer at all but it has no completion alternative.

The current hover implementation came later, I guess the dev took the view that rustc-analysis mostly works but we may as well try racer after a failure.

I think I agree that we should lazy fallback in both situations or neither.

Enable lazy fallbacks

• + helps functionality work in more situations.
• - 2 implementations to debug when stuff doesn't work.

Remove fallbacks

• + Simpler.
• - doesn't help functionality work in more situations.

I'd lean towards enabling myself. @nrc @Xanewok ?

[norru]

the latter solved the problem well enough to disable the former.

Unfortunately this no longer seems to be the case, if it ever was - or perhaps the functionality has regressed badly since then?

Remove fallbacks

I don't think the "main" implementation is robust enough that we can afford removing the fallbacks without severely affecting overall user experience and functionality.

Now, if the "main" option (compiler-based analysis) were just a bit more robust, that would be a completely different kettle of fish.

[norru]

Just found this: https://github.com/jwilm/racerd

[Xanewok]

I imagine we can bring old back behaviour and lazily fallback to Racer - previously both implementations raced and Racer was a best-guess, so we disabled it.

However, the definition has regressed in the last 2-3 weeks due to work on linking data to each path component foo::bar::baz - previously only requests on baz worked. We could enable the fallback but in general we need to fix the definition in the first place, preferably before the Rust 2018 release.

As per .cargo it's a tough nut to crack... In these cases I think the Racer fallback can really help, since atm we don't do any major effort to create a project layout and infer the compilation options unless given explicitly by a top-level Cargo.toml file (then we run Cargo and interpret the project layout ourselves).

[norru]

@Xanewok With the current state of affairs, lazily falling back to Racer by default for both definition and hover seems the obvious solution.

Once the dust has settled on the symbol definition you may want to revisit again. If the "main" solution is good, the failover should be triggered once in a blue moon.

Have you thought of adding telemetry to RLS (for users to opt-in)? RLS could gather information locally in the form of counters and timers and then call home on demand, or every day, or even every hour so you can actively monitor which functionality is actually used on the field. Once you have monitoring in place it's very easy to track any sort of regression (performance, stability, functionality) in the wild with very little effort on the users' side.

[Xanewok]

Apart from the fallback in the PR, the proposed goto-def fix is at rust-dev-tools/rls-analysis#159.

Re telemetry, I think it'd be hard to get any meaningful data - the latency can wildly differ depending on the project, definition, we'd have to identify those for it to be more useful but then the users might feel weird when a project can continuously analyze their code (some of it it's closed source - I know it's opt-in, but sometimes you can forget yadda yadda) so I'm not sold on that.

We could analyze time to complete a request, its kind and success rate but that'll require supporting telemetry infrastructure for unclear benefits.

[norru]

I understand it requires substantial effort and you have bigger fish to fry.

However, if you ever get to implement it, bear in mind that benefits will be also substantial (I have used some form of telemetry in large projects and I now don't ever understand how people can do without!).

Setting up the infrastructure is costly, but once it's there it's easy to iteratively improve it to get incrementally better monitoring and significant metrics.

Metrics will be in the form of counters and timers so there will be no possibility of code leaking.

The telemetry doesn't have to be enabled by default - an env var such as RLS_TELEMETRY=1 is unambiguous enough that I'm pretty sure it can't be activated by mistake :)

[norru]

We are getting closer to have custom startup settings for RLS in Corrosion: eclipse-corrosion/corrosion#182

[Xanewok]

With rust-lang/rust#56406 this should be fixed on today’s nightlies (beta also has the fix backported). Could you please see if the newest rls has the problem fixed?

[norru]

Just got nightly, and nothing works. All i get is the following

@mickaelistria @Xanewok

java.util.concurrent.ExecutionException: org.eclipse.lsp4j.jsonrpc.ResponseErrorException: missing field `codeActionKind`
	at java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:357)
	at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1915)
	at org.eclipse.lsp4e.LanguageServerWrapper.getServerCapabilities(LanguageServerWrapper.java:573)
	at org.eclipse.lsp4e.LanguageServiceAccessor.getLSWrappers(LanguageServiceAccessor.java:301)
	at org.eclipse.lsp4e.LanguageServiceAccessor.getLSPDocumentInfosFor(LanguageServiceAccessor.java:444)
	at org.eclipse.lsp4e.ConnectDocumentToLanguageServerSetupParticipant$1.run(ConnectDocumentToLanguageServerSetupParticipant.java:75)
	at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63)
Caused by: org.eclipse.lsp4j.jsonrpc.ResponseErrorException: missing field `codeActionKind`
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.handleResponse(RemoteEndpoint.java:209)
	at org.eclipse.lsp4j.jsonrpc.RemoteEndpoint.consume(RemoteEndpoint.java:193)
	at org.eclipse.lsp4e.LanguageServerWrapper.lambda$1(LanguageServerWrapper.java:219)
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.handleMessage(StreamMessageProducer.java:192)
	at org.eclipse.lsp4j.jsonrpc.json.StreamMessageProducer.listen(StreamMessageProducer.java:94)
	at org.eclipse.lsp4j.jsonrpc.json.ConcurrentMessageProcessor.run(ConcurrentMessageProcessor.java:99)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

[norru]

Bug above fixed, we have also enabled Racer fallback in Corrosion. It looks good my side!

[Xanewok]

Sounds great, thank you! Feel free to reopen if the issue still persists.