rust-lang / rust-analyzer

A Rust compiler front-end for IDEs
https://rust-analyzer.github.io/
Apache License 2.0
14.05k stars 1.56k forks source link

Collapse invocationLocation and invocationStrategy into a single setting #17848

Closed RalfJung closed 3 weeks ago

RalfJung commented 1 month ago

These options were added to explore how RA can best work with repos like rustc and Miri that need special non-cargo invocations (./x.py ..././miri ...). The consensus that generally has emerged is that these scripts should be invoked exactly once, in the root, and they will emit all the diagnostics for all parts of the project with paths relative to the root. There were ideas of having RA pass --manifest-path or so, with template magic available in overrideCommand to control where to put that flag; I don't think any more that those are promising.

If there are no other usecases being covered by these flags then most of the freedom provided by having two independent settings, invocationLocation and invocationStrategy, isn't actually needed. A possible simplification here might be to only have invocationLocation, and if it is set to "root" then that also implies running it only once (since it seems kind of pointless to run the same command in the same location multiple times). invocationStrategy could then be removed. Or conversely, invocationStrategy is preserved and setting it to once means "run it once, in the root".

Veykril commented 1 month ago

+1 for keeping invocationStrategy, I think that is the clearer name one to imply both things (though im also open to a new name for this altogether).

To add context, these settings were generally never meant to stick around, they were only an escape hatch for rustc's x tool, so simplifying this is definitely wanted. We still have the --saved-file stuff, but those also mainly want the root-once behavior to my knowledge so they aren't relevant to this.

Veykril commented 1 month ago

So the best way to go about this is to remove the one config half here https://github.com/rust-lang/rust-analyzer/blob/56f63dfd8aeebf80e3fc87894fa3d5a40f98a329/crates/rust-analyzer/src/config.rs#L83-L95 and here https://github.com/rust-lang/rust-analyzer/blob/56f63dfd8aeebf80e3fc87894fa3d5a40f98a329/crates/rust-analyzer/src/config.rs#L185-L198 and let the compiler guide you in removing whatever depends on that (such a great thing with rustc really).

In the build scripts part changes are simply, the branch here can be dropped for picking workspace.root() unconditionally https://github.com/rust-lang/rust-analyzer/blob/56f63dfd8aeebf80e3fc87894fa3d5a40f98a329/crates/project-model/src/build_dependencies.rs#L66-L69 and the branch here can be dropped, we will always use root here https://github.com/rust-lang/rust-analyzer/blob/56f63dfd8aeebf80e3fc87894fa3d5a40f98a329/crates/project-model/src/build_dependencies.rs#L92-L100

For flycheck you'll have to drop the outer match here https://github.com/veykril/rust-analyzer/blob/56f63dfd8aeebf80e3fc87894fa3d5a40f98a329/crates/rust-analyzer/src/flycheck.rs#L434-L449 and change the InvocationStrategy::Once arm to use the root, so that means we'll have to thread the root through InvocationStrategy::Once (change it to what Invocationlocation::Root was, holding a path).

Tyrubias commented 4 weeks ago

@rustbot claim