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

#[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 this issue Jun 9, 2024 · 11 comments
Labels
A-third-party-client C-support Category: support questions

Comments

@zjp-CN
Copy link

zjp-CN commented Jun 9, 2024

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.

@zjp-CN zjp-CN added the C-bug Category: bug label Jun 9, 2024
@Veykril Veykril added the A-nameres name, path and module resolution label Jun 9, 2024
@Veykril
Copy link
Member

Veykril commented Jun 9, 2024

Hmm, the attributes are configured at that point, so the issue is probably in

let is_cfg_attr =
attr.path.as_ident().map_or(false, |name| *name == crate::name![cfg_attr]);
if !is_cfg_attr {
return smallvec![attr.clone()];
}
let subtree = match attr.token_tree_value() {
Some(it) => it,
_ => return smallvec![attr.clone()],
};
let (cfg, parts) = match parse_cfg_attr_input(subtree) {
Some(it) => it,
None => return smallvec![attr.clone()],
};
let index = attr.id;
let attrs = parts.enumerate().take(1 << AttrId::CFG_ATTR_BITS).filter_map(
|(idx, attr)| Attr::from_tt(db, attr, index.with_cfg_attr(idx)),
);
let cfg_options = &crate_graph[krate].cfg_options;
let cfg = Subtree { delimiter: subtree.delimiter, token_trees: Box::from(cfg) };
let cfg = CfgExpr::parse(&cfg);
if cfg_options.check(&cfg) == Some(false) {
smallvec![]
} else {
cov_mark::hit!(cfg_attr_active);
attrs.collect()
}
somewhere

@zjp-CN
Copy link
Author

zjp-CN commented Jun 10, 2024

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
Copy link
Author

zjp-CN commented Jun 14, 2024

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
Copy link
Member

Veykril commented Jun 14, 2024

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

@Veykril
Copy link
Member

Veykril commented Jun 14, 2024

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

@zjp-CN
Copy link
Author

zjp-CN commented Jun 14, 2024

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
Copy link
Author

zjp-CN commented Jun 14, 2024

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
Copy link
Author

zjp-CN commented Jun 15, 2024

Well, there are some interesting things going on:

  • it's true that cargo.allFeatures is enabled by neovim RA plugin mrcjkb/rustaceanvim
    • if I set "rust-analyzer.cargo.allFeatures": false, the problem in the original issue on #[cfg_attr(feature = "gated", path = "submod_gated.rs")] is gone. I believe the 1K+ errors in embassy-hal-internal will also disappear, though I haven't tested it.
    • note that the original issue doesn't actually exist: I've confirmed it because RA jumps to correct submod.rs since no all features are enable by default: Disable rust-analyzer.{cargo,checkOnSave}.allFeatures by default #4711
  • but I accidentally find a bug in RA: cargo.allFeatures seems deprecated since Config revamp #12010 , but it's still working. What if we both have a setting like this?
{
    "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
Copy link
Member

Veykril commented Jun 15, 2024

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 Veykril added C-support Category: support questions A-third-party-client and removed A-nameres name, path and module resolution C-bug Category: bug labels Jun 15, 2024
@Veykril
Copy link
Member

Veykril commented Jun 15, 2024

See #4604 (comment) and the following collapsed comments

@Veykril
Copy link
Member

Veykril commented Jun 15, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-third-party-client C-support Category: support questions
Projects
None yet
Development

No branches or pull requests

2 participants