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

`#[cfg_attr(..., path = "submod_gated.rs")] mod submod;` makes RA recognize submod_gated.rs, discarding cfg_attr and submod.rs #17371

Open zjp-CN opened 2 months ago

zjp-CN commented 2 months ago

rust-analyzer version: rust-analyzer 1.80.0-nightly (7c52d2d 2024-06-03)

rustc version: rustc 1.80.0-nightly (7c52d2db6 2024-06-03)

editor or extension: neovim

relevant settings: None

repository link (if public, optional): None. But I found the problem from embassy-executor. For reproducible code, see below.

code snippet to reproduce:

// src/main.rs
fn main() {
    submod::f();
}

#[cfg_attr(feature = "gated", path = "submod_gated.rs")]
mod submod;
// src/submod.rs
pub fn f() {
    println!("from submod.rs");
}
// src/submod_gated.rs
pub fn f() {
    println!("from submod_gated.rs");
}
# Cargo.toml
[features]
gated = []

Currently, RA marks submod.rs as file not included in crate hierarchy when gated feature is nowhere enabled, and treats submod_gated.rs as the default submod file, discarding cfg_attr.

Veykril commented 2 months ago

Hmm, the attributes are configured at that point, so the issue is probably in https://github.com/rust-lang/rust-analyzer/blob/0c795a7226a9cb2267c56d2f46aee698b8c78f78/crates/hir-expand/src/attrs.rs#L127-L156 somewhere

zjp-CN commented 2 months ago

As a sidenote, I find embassy codebase uses conditional compilation a lot by combining cfg_attr and path attribute macro .

So a similar case worth considering is like this:

#[cfg(feature = "_arch")]
#[cfg_attr(feature = "arch-avr", path = "arch/avr.rs")]
#[cfg_attr(feature = "arch-cortex-m", path = "arch/cortex_m.rs")]
#[cfg_attr(feature = "arch-riscv32", path = "arch/riscv32.rs")]
#[cfg_attr(feature = "arch-std", path = "arch/std.rs")]
#[cfg_attr(feature = "arch-wasm", path = "arch/wasm.rs")]
mod arch;

RA mistakenly recognizes arch/avr.rs as arch module file, when arch-riscv32 feature is enabled in the RA configuration file.

zjp-CN commented 2 months ago

Wow, this pattern causes 1K+ errors in embassy-hal-internal without enabling any feature in it.

(I could try to solve this issue, but don't have the time until next month.)

Edit: wait, cfg_attr is not the evil, they are caused by cfg attributes instead 😮

Veykril commented 2 months ago

Hmm those are rustc errors though, not rust-analyzer? This sounds like your cargo check for rust-analyzer is setup wrongly

Veykril commented 2 months ago

Do you have "rust-analyzer.cargo.features" / "rust-analyzer.check.features" set to "all"?

zjp-CN commented 2 months ago

Do you have "rust-analyzer.cargo.features" / "rust-analyzer.check.features" set to "all"?

No, I never have them in configuration file. That's what surprises me.

zjp-CN commented 2 months ago

Weird, these cfg error doesn't exist on vscode. embassy has own vscode settings which I didn't use in neovim.

I have to double check on my RA settings.

zjp-CN commented 2 months ago

Well, there are some interesting things going on:

{
    "rust-analyzer.cargo.features": [],
    "rust-analyzer.cargo.allFeatures": true,
}

It's valid, and RA only respects the second and causes the issue.

allFeatures is not mentioned in the manual page any more, can RA stops recognizing it? If allFeatures has to be kept, I think the documentation needs improvement which I can help.

Veykril commented 2 months ago

can RA stops recognizing it?

I'd love to do that but some of the third party clients have complained about us fixing up the config so we had to still accept that setting. We could maybe try removing that compat layer now. Accepting that is just a backwards compat change.

The bigger issue here is that https://github.com/mrcjkb/rustaceanvim/tree/master has a very bad default for that config here. There is a reason why allFeatures is disabled by default because crates tend to use non-additive features.

Veykril commented 2 months ago

See https://github.com/rust-lang/rust-analyzer/issues/4604#issuecomment-1117235051 and the following collapsed comments

Veykril commented 2 months ago

We could maybe just warn in the logs and the like if a client sends outdated config keys that are still accepted for now