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

Do not activate test feature #7225

Open
sanbox-irl opened this issue Jan 10, 2021 · 15 comments
Open

Do not activate test feature #7225

sanbox-irl opened this issue Jan 10, 2021 · 15 comments
Labels
S-actionable Someone could pick this issue up and work on it right now

Comments

@sanbox-irl
Copy link

Hello!

Right now, I believe Rust Analyzer hard codes activating test as a feature. This works well for writing tests, but has the unfortunate side effect that it's difficult to have any cfgs in the rest of a codebase, which should only be active during test.

In my specific usecase, I'm working on an opengl application, and i'd like to make a "HeadlessContext". I use it like this:

#[cfg(test)]
type Gl = HeadlessContext;

#[cfg(not(test))]
type Gl = Context; // the real opengl

Right now, in the above program, Rust Analyzer will infer in all code (outside of test) that I am using HeadlessContext, not Context, which leads to false positive errors (and false negative non-errors).

I don't think the solution to this problem is really that easy though -- if RA simply allowed me to turn off test as a condition, then writing Unit Tests would suddenly get much harder, since i would no longer have access to the RA in such a context. A bit hacky, but a solution where cfg(test) on modules is accepted, but cfg(test) on non-modules is not would fix this usecase, but seems very shaky and perhaps confusing to users.

Thank you for all the hard work on this excellent library! What do you think of the above?

@jonas-schievink
Copy link
Contributor

I'm running into this as well. We could use the new cfg infra to detect when toggling just test will enable an item, and offer a quickfix that does that (which I think entails changing how the CrateGraph gets built).

@lnicola lnicola added the S-actionable Someone could pick this issue up and work on it right now label Jan 18, 2021
lf- added a commit to lf-/rust-analyzer that referenced this issue Jun 14, 2021
Fixes crates which vanish when the 'test' cfg atom is set.

Fix rust-lang#7243.
Fix rust-lang#9203.
Fix rust-lang#7225.
@bors bors bot closed this as completed in 8b77e26 Jun 21, 2021
@jplatte
Copy link
Contributor

jplatte commented Jul 2, 2021

This was closed in #9227, but the fix seems incomplete (it requires the user to configure things to work correctly). I think in many cases such as #7243 the fix would simply be to have dependencies, maybe just ones outside of the workspace, be loaded without cfg(test) (possibly then loading them again with cfg(test) once a file from it is opened directly).

Can this be re-opened?

@trevyn
Copy link

trevyn commented Aug 5, 2021

Is there a way to detect rust-analyzer in a cfg? I have a crate that needs to be able to signal, via a feature flag, to a proc-macro impl subcrate that it's being tested. To make sure this is set, I have this guard:

#[cfg(all(not(feature = "test"), any(test, doctest)))]
compile_error!("turbosql must be tested with '--features test'");

Which works great, except that it shows up as an error in rust-analyzer. I'd rather detect and avoid this in the code itself, rather than as a user setting.

@Qubasa
Copy link

Qubasa commented Sep 15, 2021

Same problem

cassandracomar added a commit to cassandracomar/lsp-mode that referenced this issue Oct 18, 2021
this allows the user to still enable #[cfg(test)] in rust-analyzer
globally while not breaking dependencies that have different (broken)
behavior when #[cfg(test)] is set.

see:
    - rust-lang/rust-analyzer#7225
    - lf-/rust-analyzer@86c9e7b
cassandracomar added a commit to cassandracomar/lsp-mode that referenced this issue Oct 18, 2021
this allows the user to still enable #[cfg(test)] in rust-analyzer
globally while not breaking dependencies that have different (broken)
behavior when #[cfg(test)] is set.

see:
    - rust-lang/rust-analyzer#7225
    - lf-/rust-analyzer@86c9e7b
yyoncho pushed a commit to emacs-lsp/lsp-mode that referenced this issue Nov 8, 2021
this allows the user to still enable #[cfg(test)] in rust-analyzer
globally while not breaking dependencies that have different (broken)
behavior when #[cfg(test)] is set.

see:
    - rust-lang/rust-analyzer#7225
    - lf-/rust-analyzer@86c9e7b
@ia0
Copy link

ia0 commented Mar 16, 2022

Would it be a possible solution to be able to set the --cfg arguments the same way we can already set --features and --target? Even if it's specific to test.

@bjorn3
Copy link
Member

bjorn3 commented Mar 16, 2022

You can for test. You can use rust-analyzer.cargo.unsetTest to unset cfg(test) for specific crates.

@ia0
Copy link

ia0 commented Mar 16, 2022

But you have to specify the crate. Why is this not a boolean?

@bjorn3
Copy link
Member

bjorn3 commented Mar 16, 2022

Most of the time you don't want to apply it to the crate you are working on as it breaks ide functionality like autocompletion inside tests.

@ia0
Copy link

ia0 commented Mar 16, 2022

I would actually argue the opposite. Most of the time you want to toggle it for the crate you are working on and have it always unset for dependency crates. The rationale is this: rust-analyzer give you a view of what you can do on the command-line with cargo.

For example:

  • If I am running cargo test then I want test to be enabled for my crate (and disabled for dependencies).
  • If I am running cargo build then I want test to be disabled for all crates.
  • If I am running cargo build --features=foo then I want test to be disabled and feature="foo" enabled for my crate.
  • If I am running cargo build --target=thumbv7em-none-eabi then I want that target set for all crates.
  • And any combinations of those.

Each time I switch from working on one cargo command to another, I want rust-analyzer to adapt. For now I can do it for features and target (I update the config and restart rust-analyzer), but I cannot do it for test, it's always enabled (there's the variable you mentioned but that should be a bool, the same way features and target apply to the current crate and don't mention any crates).

@flodiebold
Copy link
Member

I agree that we usually don't want to set test for pure dependencies. The problem are workspace crates. When you're editing their tests, we need to analyze them with test, but when analyzing the code of other crates that depend on them, we should not be setting it. We currently can't do both at the same time, so right now we simply set test for everything and have the setting as a workaround. Ideally we would unset test for non-editable crates (pure dependencies), and analyze workspace crates both with and without test. Being able to easily switch configurations would help as well.

@ia0
Copy link

ia0 commented Mar 16, 2022

I don't think workspace crates should be analyzed under the same rust-analyzer instance with current technology. This brings too many problems (unnecessary load time, features ambiguity, test ambiguity, etc). Given how things seem to be implemented currently (rust-analyzer state seems to be tied to some specific rust invocation and some implicit crate to which features are provided), there should be one rust-analyzer instance per (crate-level) cargo invocation. I agree that ideally, rust-analyzer should probably be able to be tied to multiple rust invocations (and possibly multiple crates) which would permit to easily switch between features, targets, and test (and even crate) without recomputing the state. But I'm not even sure this is planned and will definitely be very long-term. So for now I think the focus should be on the user experience with a single rust invocation for a single crate. This objective seems reachable in the short-term. The workaround for workspace crates (resp. features, target, test) is to use one rust-analyzer per crate (resp. features, target, test). Or is there something I'm missing in the current design?

@flodiebold
Copy link
Member

I don't think workspace crates should be analyzed under the same rust-analyzer instance with current technology

That's how rust-analyzer works right now. There is no implicit main crate or anything like that; RA works on whole crate graphs and can in principle have multiple versions of the same crate in the graph. RA is not tied to some specific rustc invocation.

@ia0
Copy link

ia0 commented Mar 17, 2022

There is no implicit main crate

I guess you talk specifically about workspace crates right? Because otherwise (when the LSP workspace is an actual crate and not a Cargo workspace) how would cargo.features work? To which crate do they apply? There is a cargo manifest command run by rust-analyzer. This defines which "crate" (maybe it's a workspace but I think it's wrong to call rust-analyzer on a workspace, there is no support of that currently, many things are broken) is the main crate.

RA is not tied to some specific rustc invocation.

What about the cargo manifest call? Try to set cargo.features to something random and see what kind of errors you get:

rust-analyzer failed to load workspace: Failed to read Cargo metadata from Cargo.toml file [...]
`cargo metadata` exited with an error:
error: none of the selected packages contains these features: bar, foo, did you mean: std?

So it's trying to read a Cargo.toml file (I suspect the one of the LSP workspace) which is thus the main crate (if it happens to be a workspace you're doomed).

@bjorn3
Copy link
Member

bjorn3 commented Mar 17, 2022

A virtual workspace doesn't have a main crate.

maybe it's a workspace but I think it's wrong to call rust-analyzer on a workspace, there is no support of that currently, many things are broken

Cargo workspaces are fully supported AFAIK.

@ia0
Copy link

ia0 commented Mar 17, 2022

A virtual workspace doesn't have a main crate.

Ok so this is specific to Cargo workspaces. If the LSP workspace is a crate and not a Cargo workspace, then you have a main crate. Is this understanding correct? If yes, I think this use-case should be well-supported because that's the most simple one (i.e. at least features, target, and test should work, the first 2 already do, the 3rd is this issue).

Cargo workspaces are fully supported AFAIK.

For example features are not: #10298. While they work with a main crate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

No branches or pull requests

9 participants