Improve cargo backend + refactor to separate Bacon and Cargo backends - #113
Conversation
|
I will have time to review this over the weekend! In the meantime, there is a conflict caused by 2 MRs I just merged that are fixing some issues caused by an old nix lockfile and Cargo.lock |
|
Thanks so much for this! There's a lot of really valuable work here. The dependency pruning, the cargo backend rewrite, the macro-expansion handling, and the channel-based diagnostic streaming are all clear wins, and the test coverage you added with real cargo JSON fixtures is great. Before merging I'd like to get a few things sorted. The blockers are mostly small fixes, plus a doc cleanup:
Other minor things I would like to address if possible:
The overall direction is good! Let's get the bootstrap path solid, fix the command name, and tighten up the docs. |
| execute_command_provider: Some(ExecuteCommandOptions { | ||
| commands: vec!["bacon_ls.run".to_string()], | ||
| ..Default::default() | ||
| }), |
There was a problem hiding this comment.
This registers bacon_ls.run, but execute_command below only handles bacon_ls.check, so the manually-triggered check is dead code. Pick one name and use it both here and at line 282.
| } | ||
|
|
||
| async fn initialized(&self, _: InitializedParams) { | ||
| // self.pull_configuration().await; |
There was a problem hiding this comment.
This commented-out line is the heart of the bootstrap problem. Right now:
initialize()no longer parsesparams.initialization_options(used to in main)pull_configuration()is disabled heredid_change_configuration()is the only path that materializes a backend.
So a client that doesn't proactively push workspace/didChangeConfiguration ends up with state.backend == None, this function logs "No backend initialized", and no diagnostics are ever produced. AFAIK Neovim happens to work because nvim-lspconfig pushes settings. vscode/helix/coc are not guaranteed to.
Two options we can use here I think:
- restore parsing of
initialization_optionsininitialize()as a startup fallback - figure out what was breaking the bacon backend when
pull_configurationwas called early and fix that root cause.
Either way, please remove the dead comment when you settle on a solution.
There was a problem hiding this comment.
I commented it out because I had a problem with it. The short version is that in did_change_configuration the client sends
settings : {
"bacon_ls: {
//...
}
}
But i was expection directly the object that is in bacon_ls :
But in a commit that is part of this history, I fixed, but did not restore this pull
There's still one thing, if the sends an empty config (or if we pull and empty config), we are still in a state where no backend is initialized. So atm, the minimal config is
settings : {
"bacon_ls: {
// either "cargo" :{}, or "bacon": {}, or "backend": "cargo",
}
}
So what we should probably do is that if after the pull there is still no backend, we init the cargo backend as a default
There was a problem hiding this comment.
Defaulting to the cargo backend seems to me like a good idea!
| } | ||
|
|
||
| async fn execute_command(&self, params: ExecuteCommandParams) -> jsonrpc::Result<Option<LSPAny>> { | ||
| if params.command == "bacon_ls.check" { |
There was a problem hiding this comment.
String mismatch with the registered command at line 83, see the comment there.
| } | ||
|
|
||
| impl BaconOptions { | ||
| pub(crate) fn update_from_json_obj(&mut self, bacon_obj: &Map<String, Value>) -> jsonrpc::Result<()> { |
There was a problem hiding this comment.
CargoOptions has a reset() that's called before re-parsing on config update, so deleting a key from the user's config takes effect on the next update. BaconOptions doesn't have an equivalent and isn't reset before re-parsing at line 694, so once a bacon key is set, removing it from the config silently keeps the prior value.
Could you add a matching BaconOptions::reset() and call it from the same code path?
|
|
||
| [dev-dependencies] | ||
| pretty_assertions = "1.4.1" | ||
| tempfile = "3.26.0" |
There was a problem hiding this comment.
| tempfile = "3.26.0" | |
| tempfile = "3.27.0" |
| * `runBaconInBackground`: Run `bacon` in background for the `bacon-ls` job (default: true) | ||
| * `runBaconInBackgroundCommand`: Path to the command used to run `bacon` in the background (defaults to find in `$PATH`). | ||
| * `runBaconInBackgroundCommandArguments`: Command line arguments to pass to `bacon` running in background (default "--headless -j bacon-ls") | ||
| * `synchronizeAllOpenFilesWaitMillis`: How many milliseconds to wait between background diagnostics check to synchronize all open files (default: 2000). |
There was a problem hiding this comment.
This whole bullet list documents the old flat keys (useBaconBackend, updateOnSave, runBaconInBackground, validateBaconPreferences, …) which no longer exist after this PR. Please delete this block as the new schema is already documented in the JSON example at the top of the Configuration section.
| if config.name == "bacon_ls" then | ||
| local settings = codesettings.local_settings()["_settings"]["bacon_ls"] | ||
| if settings ~= nil then | ||
| config["settings"]["bacon_ls"] = sett6ings |
There was a problem hiding this comment.
| config["settings"]["bacon_ls"] = sett6ings | |
| config["settings"]["bacon_ls"] = settings |
| } | ||
| ``` | ||
|
|
||
| ### Configuration - Native Cargo Backend |
There was a problem hiding this comment.
"Configuration - Native Cargo Backend" / "Configuration - Bacon Backend" section headers are out of date.
These two subsection headers (and the "This works only with bacon-ls is configured with initOptions = { useBaconBackend = true }" line) reference the pre-PR config model. Either drop the headers entirely or rewrite them around the new backend: "cargo"|"bacon" key.
| ```lua | ||
| vim.lsp.config('bacon-ls', { | ||
| init_options = { | ||
| updateOnSave = true | ||
| updateOnSaveWaitMillis = 1000 | ||
| ... | ||
| } | ||
| }) |
There was a problem hiding this comment.
After this PR, init_options (== initializationOptions) is no longer parsed and those keys no longer exist. Please update this to use the new settings = { bacon_ls = { cargo = { ... } } } shape.
I have the save-trigger easy, leading to bacon-ls spawning a bunch of cargo commands. This adds 2 mode, 1 where if a cargo command is already running it gets killed to let the new one run. The second mode waits for the command to finish but 'queues' another one to be launched once the former is done.
And parse these from the more common settings
This ensure deleted keys from user config are not 'cached'
{
"env1": "value1",
"env2: 2,
}
is less confusing
it made neovim log an error about it
Push errors when the command failed to the client Parse `Blocking waiting on lock` from cargo's output as its a better message than just progress being 0
cargo wasn't even used removing them allows to go from 458 transitive deps to 121 a good win in a world of supply chain attacks
If it is ever needed again it could be re-added
in the cargo backend, add a set of files for which we added diagnostics. This allows to properly clears diagnostics for files which previously had. e.g foo.rs and bar.rs had diagnostics you fixed bar.rs hit saved, bacon-ls runs cargo which says that there is no error. We now cleany clear both foo.rs and bar.rs whereas previously, user had to trigged a save of foo.rs. Another change is now that we publish diagnostics for all files even non-opened ones.
It was never None
This commit create two struct one per backend to better isolate what are the things each backend needs. On top of this an enum that contains the currently active backend is used to make the code easier to write and follow. The backend is now chosen on startup using the user's settings, the settings for a backend can change while the server is running, but to change backend, a restart must happen.
The progress for the cargo backend was not as good as it should have been. This was because we used read_line to iterate on the lines however, the output of cargo uses \r (something we split against after the read_line). Cargo outputs a newline when it prints `Checking xxx`, for `Building xxx` it ends with a `\r`. This meant we only updated the progress when a `Checking xxx` was emitted because we caught the line `Building xxx\r Building yyy\r Checking zzzz\n`. This was mostly problematic when the cargo check was ran on a non clean build where the output was `Checking xxx\n Building xxx [98/100]\r`. We were waiting for a newline which is not there, and thus only showed progress as 0 when it was actually 98 and only when cargo was actually done it would output lines with `\n`. The fix is to simply read until we get either an `\r` or an `\n`.
since only the bacon bacon tracks opened files we can do the match only once and not per file also we now hold the lock for longer but that is fine I would say
the cargo backend pushes diagnostics for all files even not opened ones so this publish diagnostic thingis not needed
This adds a `refreshIntervalSeconds` option that if Some(..) will make bacon publish/refresh the diagnostics it has parsed so far to the client approximately every refreshInterval seconds. This allows to not have to wait for the cargo command to complete to have the first feedbacks, as on some larger projects, the last steps can be quite long. default to 5 seconds, to disable the `refreshIntervalSeconds` option can be set to null or a negative number
it was not used
The code did not follow the expansion field of of the span which meant that errors originating from macros from a different crate would not show up be cause the span used pointed to a dependency source in cargo's registry
This refactor fixes a few problems the previous code had:
- Children were put as diagnostics, leading to having more
things like `if this is intentional prefix with _` for unused variable
(for example) as diagnostics
- Handle code actions more fully. The previous code
put 1 action per replacement, with the generic 'Replace with bacon-ls
suggestion'.
The new code groups the replacement actions into one.
This is because for example in `use std::io::{Cursor, Read}`,
if Cursor is an unused import, there are 3 replacements to apply:
`{`, `Cursor,`, `}`.
Also the new code action tries to have a less generic name to be clearer for the users
it prevents bacon backend from being usable
We changed the child diagnostics from cargo (the ones like "help a method name xx exists) to be sent as related information with the hope that the client would display them better (i.e less pollution of the editor) + these are already part of the rendered message However it seems that neovim does not support them out of the box (a plugin called tiny-inline-diagnostics does) so we change the behavior to: 1) first detect if the client says it supports them 2) add a bacon_ls.cargo.separateChildDiagnostics (that overrides the client advertised capabilities) in case the user still wants these as their own diagnostics
This clears any diagnostics that we publish when a new check if published
We where setting the state to Running before calling publish_cargo_diagnostics which meant that the function would just re-queue indefinitely
|
Comments regarding the code should be fixed, now i have to update the readme |
I had a look at the changes and I think you can leave the README to me if that's ok. This looks great. I'll merge, do some tests, update the README and release a new version. Thanks a million for the contribution! |
* Fix CI/CD * Update VsCode extension Signed-off-by: Matteo Bigoi <bigo@crisidev.org>
Updates to complete changes from #113
|
|
2 participants
This chain of commit refactor bacon_ls
A BackendRuntime enum is introduced to have a clear split between what state is meant for the cargo backend, and what sate is used for the bacon backend, as well as enforcing the correct parts of the backend is used
The backend is initialized from what the user config gives, if the config changes what backend to use a restart must be done. There is one thing though, it sort of relies on the client to send its config after init, which seems to be the case for neovim but would need to be tested with other clients like vscode
The cargo backend has been refactored to allow more easily to configure the command that is run (check vs clippy, which args, what env var)
An async closure is "spawned" to parse the stderr from cargo to get progress and report to lsp client
The parsing of json diagnostics groups refactorings that are meant to be done in one go
The parsing of json diagnostics follows expansion (given in the json diagnostics) until it reaches a file that is in the project to properly report diagnostics tied to macros
The childrens of diagnostics are either sent as "relatedInformation" of the parent (children are often things like "help: a function name xxx exists) OR sent as their own parent depending on the capabilities advertised by the client. This can be overriden by a config flag
The function responsible for parsing now no longer collects diagnostics, but send them via a channel to a receiver that then collects and is able to periodically push to the client new diagnostics (every 5 secs by default, configurable and can be disabled). This was done because in one of my project, cargo check can print some diagnostics, but take few more seconds to finish, and so not having to wait for the command to be fully finished is great
the cargo backend is more "workspace/project" oriented this means:
Fixes #48
Should fix #64