[Unique-Usman]

View all comments

Change description

This PR changes how #[cfg] behaves in expression positions where removing the expression would result in invalid syntax.

For example:

let _ = 1 + #[cfg(unix)] 2;

This currently works on Unix because the condition is true, but becomes invalid on non-Unix targets when the expression is removed.

The proposed change rejects cfg in these positions regardless of whether the condition is true or false. The goal is to avoid code whose validity depends on the target and to make the behavior more predictable as stmt_expr_attributes is stabilized.

Summary of the discussion

@petrochenkov pointed out that this differs from the usual token-based model for macros: #[cfg(true)] expr produces expr, while #[cfg(false)] expr produces no tokens. From that perspective, only the latter naturally results in an error.

On the other hand, @estebank pointed out that these cases are easy to introduce accidentally and may only be discovered when the code is compiled for another target. This becomes more important with the stabilization of expression attributes.

We also discussed that this is technically a breaking change, since some currently accepted code such as #[cfg(true)] inside a macro invocation would stop compiling (#159580 (comment)). The conclusion was that this is effectively closing a stability hole, and the amount of affected code is expected to be very small.

The current implementation also uses RemoveNodeNotSupported for these cases and adds coverage for both #[cfg(true)] and #[cfg(false)].

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 74 candidates
• Random selection from 16 candidates

[Unique-Usman]

@estebank

[petrochenkov]

I don't think we should do this.

cfg should ideally behave like a macro, unless necessary for backward compatibility, that also means following the token-based model.

#[my_attr] expr is valid if it expands into tokens that can be parsed as an expression.

• #[cfg(false)] expr expands into /*empty*/, which cannot be parsed as an expression, so it's an error.
• #[cfg(true)] expr expands into expr, which can be parsed as an expression, so it's not an error.

[estebank]

@petrochenkov should the toolchain provide guidance for things that are likely to cause trouble because they are always a footgun regardless?

Code like let _ = 1 + #[cfg(unix)] 2; will never make sense, but could easily slip into a codebase without anyone noticing until a downstream tries to build on Windows, and then it becomes a compile error. I 100% agree that the general behavior of cfg attributes should not diverge from macros in general. But I am convinced we should strive to make the toolchain as proactive as possible in avoiding user mistakes. A custom #[my_cfg(..)] wouldn't have the benefit of this proactive, of course, making it slightly worse to use than the built-in. I think having this check is important for the effort to stabilize attributes in expressions, which I think has enough consensus on a subset that can be safely stabilized. If that happens, the prevalence of these kind of attributes might be higher than it is now.

Edit: even if we don't emit an error, shouldn't this case in particular be at least deny-by-default lint, given that it is syntactically correct, but unlikely to be what the user intended?

[mejrs]

cfg should ideally behave like a macro

#159581 won't stabilize attribute macros on expressions, so should we move #[cfg(pred)] expr to be feature gated by proc_macro_hygiene, like attribute macros currently are?

Also if we ever get a way for macros to overload based on what position (type, expr, etc) they're in (perhaps similar to macro_attr and macro_derive), then that would make the proposed behavior feel more natural.

[petrochenkov]

should the toolchain provide guidance for things that are likely to cause trouble because they are always a footgun regardless?

Yes, this just looks like a lint material to me.
The compiler knows additional details about some specific macro and can say that using it in some contexts is not a good idea, even if it technically works.

Are you sure cfg in these positions is always unstable?
In that case we can report a hard error just as a conservative measure to avoid stabilizing a questionable case, or keep it feature-gated as @mejrs says.

[mejrs]

Code like let _ = 1 + #[cfg(unix)] 2; will never make sense, but could easily slip into a codebase without anyone noticing until a downstream tries to build on Windows, and then it becomes a compile error.

For an example, see https://github.com/PyO3/pyo3/blob/09b8d484b3f79b8519cac63be9d71a107cb14952/pyo3-ffi/examples/string-sum/src/lib.rs#L36 where the value of an array length depends on conditional compilation.

It would be quite tempting to try to write that as...

const SLOTS_LEN: usize = 1 + #[cfg(Py_3_12)] 1 + #[cfg(Py_GIL_DISABLED)] 1 + #[cfg(Py_3_15)] 4);

...and end up in one of those situations where it looks like something that does what you expect, only for it to turn out you're wrong. It would just be another gotcha that most people are going to run into at some point.

[petrochenkov]

Are you sure cfg in these positions is always unstable?

This is technically a breaking change, because it closes a stability hole.
This code works now, but will stop working after this change.

macro_rules! mac {
    ($expr:expr) => { $expr.clone() }
}

fn main() {
    let _ = mac!(#[cfg(true)] 10);
}

[petrochenkov]

Let's land this, it will simplify the stabilization of stmt_expr_attributes.
The breakage is very unlikely in practice and we'll know whether it happens from beta crater runs, crater is too busy now for a separate run.
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[petrochenkov]

Could you also add this as a test case?

#[derive(Clone)]
enum E {
    V1 = #[cfg(true)] 1,
    V2 = #[cfg_attr(true, cfg(true))] 2,
}

Both variants should produce errors.
This executes configure_expr, which is an eager cfg expansion logic performed by derives.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[petrochenkov]

Thanks!
@bors r+

[rust-bors]

📋 This PR cannot be approved because it currently has the following label: S-waiting-on-t-lang.

[petrochenkov]

@bors r+

[rust-bors]

📌 Commit be649c0 has been approved by petrochenkov

It is now in the queue for this repository.