Skip to content
This repository was archived by the owner on Dec 29, 2022. It is now read-only.

Dependency graph not updated after non-top-level Cargo.toml change #546

Closed
vorner opened this issue Nov 4, 2017 · 15 comments
Closed

Dependency graph not updated after non-top-level Cargo.toml change #546

vorner opened this issue Nov 4, 2017 · 15 comments
Labels
Milestone

Comments

@vorner
Copy link

vorner commented Nov 4, 2017

Hello

I'm observing this situation:

  • I'm editing some rust code, let's say main.rs.
  • I decide I want to add another dependency, so I go up the file and add extern crate whatever;.
  • The line is marked as wrong, saying it can't find the crate. „Huh, what? Ah, right, it needs to go to Cargo.toml too“.
  • I open the Cargo.toml, add it there and switch back.
  • The error about the missing crate never goes away (and prevents other errors from showing, completion from that crate doesn't want to work either).

I suspect one of this happens:

  • RLS generates the depgraph at start and never regenerates it again.
  • RLS doesn't notice the change on the file. This might be related to the fact that I use neo-vim and Cargo.toml isn't a rust file, so neo-vim doesn't bother telling RLS about opening it (in fact I use a different language server to check syntax of toml files). Or sometimes I even open the file in a different editor. Should RLS watch file changes on all the relevant files in the project (though some inotify) and fallback to that notification if it doesn't get one from the editor? Eg. synthesizing a didChange event if the file is edited externally/loaded from git/whatever?
@Xanewok
Copy link
Member

Xanewok commented Nov 4, 2017

RLS generates the depgraph at start and never regenerates it again.

This is implemented in "rust.workspace_mode": true (also needs "rust.unstable_features": true), so I'd suggest turning that on and see if it works (providing IDE features for more than one package target, e.g. bin/lib, is blocked on #547).

Currently in single package mode the RLS runs Cargo, saves rustc invocation for final compilation and only ever reruns that, regardless of the type of the changes. The workspace_mode is an attempt to teach the RLS about dependencies between package (targets), so we'll probably gradually move towards stabilizing and using that instead.

@vorner
Copy link
Author

vorner commented Nov 4, 2017

It didn't seem to have any effect whatsoever. But I'll try digging into it a bit more, I'm not 100% sure I persuaded neo-vim to send the configuration correctly (vim configuration doesn't seem to support true/false and has only 1/0, so the JSON contains that too :-|).

Anyway, to make sure we both talk about the same thing. I'm not adding a crate in a workspace, I'm adding a 3rd party dependency (like the log crate, or something). Is your answer still relevant to that?

Also, you list "rust.workspace_mode", but the readme file contains just "workspace_mode". How should the JSON actually sent to RLS look like?

Should I provide the whole communication with RLS and logs?

@vorner
Copy link
Author

vorner commented Nov 4, 2017

Another update. I managed to send the real true, and I sent both with and without the rust. prefix. It still doesn't help.

@Xanewok
Copy link
Member

Xanewok commented Nov 4, 2017

Which LSP client are you using? So the configuration currently has to be sent via workspace/didChangeConfiguration LSP message with a "rust": {...} object that can have appropriate values set (here "rust.workspace_mode": true key is using a nested key notation, so it means passing "rust": { workspace_mode: true, ...} configuration JSON object via the message.

I'm not sure if any nvim LSP client supports the configuration part of the protocol at this point.

I know that workspace_mode might imply toggling support only for multiple packages, but since it required to cache, regenerate and reason about dep graph, it seemed reasonable to leave the existing single package mode as-is not to break it, and work on the new mode under a separate flag.

@vorner
Copy link
Author

vorner commented Nov 4, 2017

I use autozimu/LanguageClient-neovim. There's no direct support, but it has a way to send an arbitrary notification to the server, so I'm just creating the message myself. This is what I currently send (just to make sure I don't miss any part where it might be needed):

{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"workspace_mode":true,"unstable_features":true,"rust.workspace_mode":true,"rust.unstable_features":true,"rust":{"workspace_mode":true,"unstable_features":true}},"rust":{"workspace_mode":true,"unstable_features":true}}}

Even after adding the "rust": { part, nothing changed.

@Xanewok
Copy link
Member

Xanewok commented Nov 4, 2017

Can you try passing every setting, like this:

`{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"rust":{"sysroot":null,"target":null,"rustflags":null,"clear_env_rust_log":true,"build_lib":null,"build_bin":null,"cfg_test":false,"unstable_features":true,"wait_to_build":500,"show_warnings":true,"goto_def_racer_fallback":false,"use_crate_blacklist":true,"build_on_save":false,"workspace_mode":true,"analyze_package":null}}}}

It's all defaults except the workspace_mode and unstable_features set to true.
It's hijacked from what the rls-vscode sends, so it should probably work.

@vorner
Copy link
Author

vorner commented Nov 5, 2017

Still no luck :-(. I managed to get RLS to some very broken state when it reported nothing at all (trying to tweak the config further), but the adding of a crate doesn't seem to work.

@Xanewok
Copy link
Member

Xanewok commented Nov 5, 2017

I tried to reproduce doing the following:

  1. Create a new project using cargo new example
  2. Open VSCode at the project directory, with workspace_mode on
  3. Add extern crate flame; at the top, the line was marked as error (can't find extern crate...)
  4. Go to Cargo.toml, add flame = "0.2" dep, save Cargo.toml
  5. RLS started working and building after Cargo.toml save, line still underlined in red while it works
  6. RLS finished working and updated analysis data, making the red error line disappear and started offering autocompletion

I know the bug can be irritating, but to first rule out possible client differences I'd try using VSCode and if it can't be reproduced with it, look towards possibly improving the support for configuration or watch LSP messages in nvim LSP client, as the dep graph update feature needs both of these to work.

However if nvim LSP client does send messages in compliance with the protocol, it might still uncover some protocol compliance issues with the RLS itself, so that's surely something worth looking at, however the core feature looks like it's working in the server.

@vorner
Copy link
Author

vorner commented Nov 5, 2017

I can confirm it works as you describe in VSCode. But even when I forced vim to consider Cargo.toml to be rust (and therefore use the same language server for it as the other rust files and send notifications about changes in that file to it), I didn't make it work in nvim. I don't know what the difference is (I didn't manage to capture rls' stdio in VSCode, log to file configuration gives me only the backtraces), but on a first glance the messages sent to it look somewhat sane. I didn't read the protocol spec, though.

I'm attaching the captured stdio from nvim, if you can spot a problem.

rls.input.txt
rls.output.txt

@nrc
Copy link
Member

nrc commented Mar 22, 2018

This seems like a client problem. If there are things we can address on the server side, then feel free to comment

@nrc nrc closed this as completed Mar 22, 2018
@vorner
Copy link
Author

vorner commented Mar 23, 2018

Can you point at a concrete problem with what the client sends? If it's a client problem, then it should be fixed in the client, but it would be great to know what the client does wrong in the first place. Looking at the rls.input.txt line 81, it does send the change in Cargo.toml.

@nrc
Copy link
Member

nrc commented Mar 26, 2018

The client should send a workspace/didChangeWatchedFiles notification. During initialisation the server registers an interest in Cargo.toml and the client should thus send these notifications.

@nrc
Copy link
Member

nrc commented Apr 5, 2018

Reports that this is broken again in VSCode

@nrc nrc reopened this Apr 5, 2018
@nrc nrc added the P-high label Apr 5, 2018
@nrc nrc added this to the 1.0 milestone Apr 5, 2018
@nrc
Copy link
Member

nrc commented May 6, 2018

Confirmed that this is working in VSCode

@nrc nrc closed this as completed May 6, 2018
@sfackler
Copy link
Member

sfackler commented May 8, 2018

At least on the latest nightly, RLS doesn't update when a Cargo.toml is updated that isn't the top level one in a workspace.

@nrc nrc reopened this May 9, 2018
@nrc nrc changed the title Dependency graph not updated after Cargo.toml change Dependency graph not updated after non-top-level Cargo.toml change May 9, 2018
@nrc nrc closed this as completed in ac50ecb May 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants