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)

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