Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass --bin to cargo check when a binary is changed #17829

Closed
mo8it opened this issue Aug 8, 2024 · 14 comments · Fixed by #17912
Closed

Pass --bin to cargo check when a binary is changed #17829

mo8it opened this issue Aug 8, 2024 · 14 comments · Fixed by #17912
Assignees
Labels
A-flycheck issues with flycheck a.k.a. "check on save" C-bug Category: bug

Comments

@mo8it
Copy link
Contributor

mo8it commented Aug 8, 2024

I marked this as important because it affects Rust beginners and I don't want them to get a bad expression while learning with Rustlings. It causes Rustlings to freeze for a long time because Rust-Analyzer blocks Cargo.

The issue is described here with a video: rust-lang/rustlings#2071

I was able to reproduce it in VS-Code in a Windows 10 VM (probably not a Windows problem, didn't test on Linux).

You don't even need to have Rustlings running in the background to see how Rust-Analyzer rebuilds everything on every file save:

  • Initialize Rustlings with rustlings inti
  • Open VS-Code in the generated rustlings/ directory
  • Wait for Rust-Analyzer to be done
  • Open a file and trigger a save
  • See that Rust-Analyzer rebuilds all 188 binaries!

It looks like an issue in the Rust-Analyzer VS-Code plugin because it doesn't happen in Helix in the same VM.

The effect probably became more dramatic after I added --keep-going in #17561

@Veykril
Copy link
Member

Veykril commented Aug 8, 2024

Which rust-analyzer version are you using with your helix setup? (wanna make sure whether its vscode specific or your helix r-a is just too old)

@Veykril
Copy link
Member

Veykril commented Aug 8, 2024

Also running CARGO_LOG=cargo::core::compiler::fingerprint=info with the appropriate cargo check command after r-a presumably invalidated the build would be good, that will tell what the cause of the invalidations is

@mo8it
Copy link
Contributor Author

mo8it commented Aug 8, 2024

I used Rust-Analyzer as the component installed by rustup for the stable channel. But your comment made me check that version and it is indeed an older one from May. So the change in #17561 isn't applied yet. VS-Code has a recent version of Rust-Analyzer. So I made the beta channel the default which has a recent version of Rust-Analyzer. Then I also saw this issue in Helix. So it is not a VS-Code problem.


I disabled Rust-Analyzer and ran CARGO_LOG=cargo::core::compiler::fingerprint=info cargo check after saving a file. It showed the following for all exercises but the one changed:

0.146508600s  INFO prepare_target{force=false package_id=exercises v0.0.0 (C:\Users\vm\rustlings) target="intro2"}: cargo::core::compiler::fingerprint: fingerprint dirty for exercises v0.0.0 (C:\Users\vm\rustlings)/Check { test: false }/TargetInner { name: "intro2", doc: true, ..: with_path("C:\\Users\\vm\\rustlings\\exercises/00_intro/intro2.rs", Edition2021) }
   0.146615600s  INFO prepare_target{force=false package_id=exercises v0.0.0 (C:\Users\vm\rustlings) target="intro2"}: cargo::core::compiler::fingerprint:     dirty: FsStatusOutdated(Stale)

The one changed showed a changed timestamp which is expected.

I don't see why Rust-Analyzer thinks that everything is invalid.

@mo8it
Copy link
Contributor Author

mo8it commented Aug 8, 2024

In case this is helpful, I copied the logs of Helix after saving a file:

2024-08-08T15:18:18.364 helix_lsp::transport [INFO] rust-analyzer -> {"jsonrpc":"2.0","method":"textDocument/formatting","params":{"options":{"insertSpaces":true,"tabSize":4},"textDocument":{"uri":"file:///C:/Users/vm/rustlings/exercises/03_if/if1.rs"}},"id":1}
2024-08-08T15:18:18.504 helix_lsp::transport [INFO] rust-analyzer <- {"jsonrpc":"2.0","id":1,"result":null}
2024-08-08T15:18:18.504 helix_lsp::transport [INFO] rust-analyzer <- null
2024-08-08T15:18:18.522 helix_lsp::transport [INFO] rust-analyzer -> {"jsonrpc":"2.0","method":"textDocument/didSave","params":{"textDocument":{"uri":"file:///C:/Users/vm/rustlings/exercises/03_if/if1.rs"}}}
2024-08-08T15:18:18.523 helix_lsp::transport [INFO] rust-analyzer -> {"jsonrpc":"2.0","method":"workspace/didChangeWatchedFiles","params":{"changes":[{"type":2,"uri":"file:///C:/Users/vm/rustlings/exercises/03_if/if1.rs"}]}}
2024-08-08T15:18:18.592 helix_lsp::transport [INFO] rust-analyzer <- {"jsonrpc":"2.0","id":21,"method":"window/workDoneProgress/create","params":{"token":"rust-analyzer/flycheck/0"}}
2024-08-08T15:18:18.592 helix_lsp::transport [INFO] rust-analyzer <- {"jsonrpc":"2.0","method":"$/progress","params":{"token":"rust-analyzer/flycheck/0","value":{"kind":"begin","title":"cargo check","cancellable":true}}}
2024-08-08T15:18:18.597 helix_lsp::transport [INFO] rust-analyzer -> {"jsonrpc":"2.0","result":null,"id":21}
2024-08-08T15:18:18.925 helix_lsp::transport [INFO] rust-analyzer <- {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///c:/Users/vm/rustlings/exercises/15_traits/traits2.rs","diagnostics":[]}}
2024-08-08T15:18:18.925 helix_lsp::transport [INFO] rust-analyzer <- {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///c:/Users/vm/rustlings/exercises/15_traits/traits3.rs","diagnostics":[]}}

… (A message for every diagnostic in every binary)

2024-08-08T15:18:28.681 helix_lsp::transport [INFO] rust-analyzer <- {"jsonrpc":"2.0","method":"$/progress","params":{"token":"rust-analyzer/flycheck/0","value":{"kind":"end"}}}

As you can see, it takes 10 seconds in the Windowns VM with 2 CPUs until the checks are done although nothing was changed, only a file save was triggered.

In case you try to reproduce it: The effect isn't dramatic if Rust-Analyzer is running on bare metal with 16 threads. It is bad if someone has an older PC.

@mo8it
Copy link
Contributor Author

mo8it commented Aug 8, 2024

Maybe there is no invalidation problem here. When I run cargo check in the VM, it takes about 10 seconds too because of the 188 binaries.

@Veykril Does Rust-Analyzer run cargo check without specifying a binary every time a binary changes? If yes, then that is the problem. It should only run cargo check --bin BINARY_NAME.

@Veykril
Copy link
Member

Veykril commented Aug 8, 2024

Ye I think you are right, there is no invalidation here. By default we run check with --workspace, try setting rust-analyzer.check.workspace to false, that should fix this I then I think

@mo8it
Copy link
Contributor Author

mo8it commented Aug 8, 2024

I just tried setting rust-analyzer.check.workspace to false in Helix and VS-Code but it didn't help. Its documentation says that it passes -p instead of --workspace. But Rustlings is one package with 188 binaries. We need some option to pass --bin when a binary is changed, not -p or --workspace. That should be enabled by default. I would hate to tell beginners to change settings of their language server.

@mo8it mo8it changed the title IMPORTANT: Rebuilding all binaries on file save makes Rustlings unusable Pass --bin to cargo check when a binary is changed Aug 8, 2024
@Veykril
Copy link
Member

Veykril commented Aug 8, 2024

That should be enabled by default. I would hate to tell beginners to change settings of their language server.

We won't change the default for this, as imo that is a worse default than what we have there right now. We do have a rust-analyzer.toml which should allow (if not yet, then that needs to change) setting that relevant setting for the generated rustlings workspace.

Though given all of these are binaries you are raising an interesting point, --workspace config enabled or not, when only a binary source file changes/gets saved, we should always only check that binary as it can't have other crates depend on it anyways (likewise for examples and integration tests I think?)!

@Veykril Veykril added the A-flycheck issues with flycheck a.k.a. "check on save" label Aug 8, 2024
@Veykril
Copy link
Member

Veykril commented Aug 8, 2024

(Though that will require some bigger changes to the flychecking infra unfortunately)

@alibektas alibektas self-assigned this Aug 8, 2024
@mo8it
Copy link
Contributor Author

mo8it commented Aug 12, 2024

@alibektas Since you assigned yourself: Did you already start working on it? I wanted to try implementing it myself and start tomorrow if you don't mind. Otherwise, I would be happy to review (if my review counts).

@alibektas
Copy link
Member

@alibektas Since you assigned yourself: Did you already start working on it? I wanted to try implementing it myself and start tomorrow if you don't mind. Otherwise, I would be happy to review (if my review counts).

@mo8it I tried one or two things myself but I don't really have anything concrete atm. If you want to implement it yourself I have nothing against it. But let me also offer that I can try getting it done today and if I can't you can take over. How does that sound?

@mo8it
Copy link
Contributor Author

mo8it commented Aug 12, 2024

@alibektas Sounds good! I won't be able to start before tomorrow night anyway ;)

@alibektas
Copy link
Member

@mo8it 👋 I happened to solve this issue while working on a similar one. I want to honor my promise and let you submit the PR, but if you don't have anything ready yet, I could make the PR instead. Let me know what you think.

@mo8it
Copy link
Contributor Author

mo8it commented Aug 16, 2024

Nice! No, go ahead. I didn't find time for it 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-flycheck issues with flycheck a.k.a. "check on save" C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants