Extend cargo_unsetTest to support "all" like cargo_features

[ia0]

The UnsetTestCrates enum has 3 variants:

• None: Never used but equivalent to Only(vec![])
• Only: Always used with the content of unsetTest
• All: Never used and impossible to simulate

This PR addresses the fact that All is essentially impossible to reach.

The use-case is that None and All are the most obvious values to produce for LSP clients since they don't know the name of the crate. This permits to have shortcuts to simply toggle tests from an LSP client without asking the user for the name of the crate.

[ia0]

This probably addresses #7225.

[Veykril]

Instead of having this be a seperate config flag, we should incorporate this with the already existing unsetTest setting, similar to how my config revamp PR does it with the features setting here https://github.com/rust-lang/rust-analyzer/pull/12010/files#diff-779cb31c0defa0631d570725eb4ca124c0860d6ec07bb5b9deddc2dd34966315R1385-R1391

So with that said, I think this PR will have to wait until #12010 is being merged, then we can do this here as I don't see why we shouldn't allow disabling test for all crates if the user wishes to do so.

[ia0]

Yes, it makes sense to use the same approach as in #12010 (i.e. use a single config that takes an enum instead of multiple exclusive flags, like CargoFeatures). I'm fine to wait for #12010 and update this PR.

Note however, that if we really want to revamp the config, we should probably fix how cargo arguments are handled. Right now:

• --no-default-features has its own flag and that flag applies to all crates
• --features has its own flag (will be merged with --all-features in Config revamp #12010) and applies to all crates
• --all-features has its own flag (will be merged with --features in Config revamp #12010) and applies to all crates
• cargo test vs cargo build has its own flag, namely unsetTest, and either applies to all crates or takes a mapping from crate to config

I believe all cargo config should optionally take a mapping from crate to config. There's probably a way to design this nicely. There are multiple solutions and it's not clear which one is obviously better:

• A single config that describe the cargo invocation for each crate. The type would be something like mapping from crate name to cargo config.
• One config per cargo flag (or command). Each config would either be directly the config (applies to all crates) or a mapping from crate name to config.
• One config per cargo flag (or command). Each config would provide a default for all crates, and a mapping for exceptional configs per crate.
• Any other variant combining those ideas.

An important point in my opinion is that configs should always provide the way to apply to all crates (which makes it much easier to use when the cargo project is not a workspace, since it's silly to have to specify the crate name in that case). This is actually why I'm creating this PR. All cargo invocation config I need support applying to all crates (--features, --target, and --no-default-features) except for cargo build vs cargo test.

[Veykril]

thanks for raising those points, will check them out tomorrow (feel free to raise anything else regarding the config in the PR, this is the one point where we should allow ourselves to change things)

[flodiebold]

One thing to note is that unsetTest is different from the features configs in that it's a workaround for a problem in rust-analyzer, not really a configuration that we fundamentally want the user to be able to change. Ideally, things would just work with dependencies being analyzed without test and code you're editing having test enabled as necessary. Being able to toggle test for local crates because you're editing something with cfg(not(test)) or something like that is a slightly different usecase than what unsetTest was originally intended for.

[ia0]

I see (indeed unsetTest sounds like a work-around name rather than an actual feature name). I'm fine to introduce a new cargo_test config in that case. Ideally the semantics would be a cargo test vs cargo build invocation at the root of the project, but I'm not sure that's how rust-analyzer works (since it also needs to support projects without cargo). So it might just be a way to simulate cargo test, i.e. add --cfg=test and possibly other stuff.

Where does rust-analyzer call into rustc/cargo to get diagnostics? I couldn't find it.

[flodiebold]

The cargo.* configs are unrelated to calling cargo for diagnostics. The only occasion where we call cargo for diagnostics is the check on save, which is governed by the checkOnSave configs. The cargo. configs are used when calling cargo metadata to get the project structure for rust-analyzer's own crate graph. Cfgs are not explicitly set in the metadata, rust-analyzer has to decide them.

[ia0]

I see, but then does rust-analyzer call into rustc to get diagnostics/completion/etc? Or does it not even use the compiler?

[flodiebold]

It doesn't use the compiler.

[ia0]

Oh wow, I didn't really expect that. It's a huge amount of work to duplicate parts of the compiler behavior (as well as the resulting problems like semantics divergence over time). That's good to know, thanks!

[bors]

☔ The latest upstream changes (presumably #12010) made this pull request unmergeable. Please resolve the merge conflicts.

[ia0]

I tested on Emacs.

I can now control rust-analyzer's behavior by editing some virtual Emacs variable with the following grammar:

cargo {build|test} [--target=<target>] [--no-default-features] [--features=<features>]

(Arguments can be reordered but features must be passed with a single argument, shouldn't be hard to fix but I don't need it.)

Ideally I would add --release to control debug-assertions, but I probably would prefer to look into rust-analyzer internals more before, to see if there isn't a way to do that better directly in rust-analyzer. I've seen that it's possible to have multiple configs of the same crate simultaneously, which would mean that I might not need to restart rust-analyzer each time I change my virtual cargo command in Emacs. But not sure when I'll get time to look at this.

[bors]

☔ The latest upstream changes (presumably #12252) made this pull request unmergeable. Please resolve the merge conflicts.

[Veykril]

Sorry for the constant conflicts, had to touch up the config stuff a lot the past days :/ Will come back to this PR next week

[ia0]

Sorry for the constant conflicts, had to touch up the config stuff a lot the past days :/

No problem, this PR is very small so it's very easy to fix conflicts.

Will come back to this PR next week

Sounds good, thanks!

[Veykril]

So this seems fine to me, though given Florian's point it is somewhat questionable whether we want to do this. As was said, this entire config is kind of an escape hatch currently and ideally shouldn't exist. On the other hand given it is a thing already, I don't have a strong reason to not just accept all here either.

[Veykril]

This should be unnecessary

[ia0]

Indeed. Not sure why I had to add it before.

[Veykril]

This needs to use anyOf now for validity reasons in vscode, check the current version of "CargoFeatures" of what I mean

[ia0]

Done.

[ia0]

Yes, I agree it doesn't feel great to add to the debt. I would be willing to help although I'm not sure I'll have the time for the amount of effort that is needed.

In my opinion, the main issue of rust-analyzer (and also the reason why there's the unsetTest hack) is the lack of coherence with respect to cargo+rustc. Ideally, rust-analyzer should share the same stack as cargo and rustc. By coherence, I mean that rust-analyzer should have the same opinion about the code being analyzed as cargo and rustc have when they compile/analyze it (this should be a consequence of sharing the same code, not because rust-analyzer duplicates cargo and rustc's logic because this is not maintenable). And to underline that coherence, I think rust-analyzer should provide a dynamic config describing which view of the code it is rendering. This should be expressed in the same terms as cargo and rustc. In particular, I would expect rust-analyzer to take a cargo command as config and show that. This config should be easily editable without rebooting rust-analyzer (from what I've read of the documentation, this should be already possible because the crate graph may contain multiple versions of the same crate with different configs or views in my terms). A view would be defined by everything cargo and rustc support: features, profiles, test, target, etc. I'm currently able to simulate that (module rust-analyzer restart when changing view) except for profiles (e.g. debug-assertions) but I didn't hit the issue yet. I'm able to control features, target, and now test with this PR.

Is there already an issue tracking such effort?

[ia0]

Done.

[ia0]

Indeed. Not sure why I had to add it before.

[Veykril]

Alright, trying to catch up on old PRs now finally. So, we actually unsert the test attribute for non-local crates since yesterday making this hack a lot less useful (its only needed for local crates now, and usually yuo don't want to use it there). So with that I am now actually inclined to not merge this, as the changes here don't really give any value anymore.

Sorry for that, but thanks nevertheless for the work you put into this!

[ia0]

Thanks for the feedback! I guess you're referring to #12599. Even if it still doesn't fix the problem, I agree that the problem is very minor compared to other issues rust-analyzer is facing. So it sounds good to me to close this to free some time for higher priority problems. We can get back to this when analysis is revamped to match what rust and cargo do (ideally using the same code).