Reject cfg on expressions that cannot be safely removed

[Unique-Usman]

Previously, expressions behind stmt_expr_attributes were only rejected when the cfg condition evaluated to false. If the condition was true, the code compiled successfully.

This meant code like an attributed binary operand could compile on one platform but fail on another, even though removing the operand would leave an invalid expression.

This change always rejects cfg in expression positions where removing the expression would produce invalid code. Expression positions where removal is safe continue to work as before.

[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

[mejrs]

What this code does it emits this error if the cfg predicate is true, then if it's false we call expand_cfg_false and (if it's not the crate root) that emits the same error. Not only isn't that immediately obvious, it also suggests that there are still places where #[cfg(true)] is allowed but #[cfg(false)] is not.

Can you change it so there's only one place we emit this error (whether the predicate is true or not)?

Can you also change the error name and message to say that cfg is not supported in these positions?

View changes since the review

[mejrs]

(this check could also be moved elsewhere, I'm not sure what the best place for it is)

[petrochenkov]

Can you change it so there's only one place we emit this error (whether the predicate is true or not)?

The false version is reported in expand_cfg_false.

This seems like an ok place to do this check for the true case, if it's moved to expand_cfg_true, the match #159580 (comment) will be much larger at least.

[petrochenkov]

Not only isn't that immediately obvious, it also suggests that there are still places where #[cfg(true)] is allowed but #[cfg(false)] is not.

If expand_cfg_false is inlined and replaced with a similar match then it will be more clear.

[mejrs]

Can this file have some #[cfg(true)] as well?

View changes since the review

[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]

Could you use a match here?

• AstFragmentKind::Expr | AstFragmentKind::MethodReceiverExpr - error, AstFragmentKind::Crate - do nothing, everything else - unreachable!().

View changes since the review

[Unique-Usman]

yep, sure.

[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.