Syntactically permit attributes on 'if' expressions

[Aaron1011]

Fixes #68618

Previously, attributes on 'if' expressions (e.g. #[attr] if true {})
were disallowed during parsing. This made it impossible for macros to
perform any custom handling of such attributes (e.g. stripping them
away), since a compilation error would be emitted before they ever had a
chance to run.

This PR permits attributes on 'if' expressions ('if-attrs' from here on)
syntactically, i.e. during parsing. We instead deny if-attrs
during AST validation, which occurs after all macro expansions have run.

This is a conservative change which allows more code to be processed by
macros. It does not commit us to semantically accepting if-attrs. For
example, the following code is not allowed even with this PR:

fn builtin_attr() {
    #[allow(warnings)] if true {}
}

fn custom_attr() {
    #[my_proc_macro_attr] if true {}
}

However, the following code is accepted

#[cfg(FALSE)]
fn foo() {
    #[allow(warnings)] if true {}
    #[my_custom_attr] if true {}
}

#[my_custom_attr]
fn use_within_body() {
    #[allow(warnings)] if true {}
    #[my_custom_attr] if true {}
}

[rust-highfive]

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

[Aaron1011]

r? @Centril

[Aaron1011]

In the process of writing this, I discovered that the following syntax does not even parse:

fn main() {
    if true {
    } #[attr] else {
    }
}

gives the error:

error: expected expression, found keyword `else`
 --> src/main.rs:3:15
  |
3 |     } #[attr] else {
  |               ^^^^ expected expression

That is, we don't even recognize #[attr] as an attribute when it appears anywhere on an 'else if' or 'else' (e.g. #[attr] else if, else #[attr] if, #[attr] else).

Personally, I think this should be accepted syntactically as well. However, I think that would probably need an FCP (if not an RFC), since it would require changing what the parser even recognizes, not syntax that we parse but explicitly reject.

[Centril]

Can you test the interactions with from the RFC?:

#[cfg(not(foo))]
if cond1 {
} else #[cfg(not(bar))] if cond2 {
} else #[cfg(not(baz))] {
}

Would be good to also test things like:

fn bar() {}
fn foo() {
    bar(#[cfg(F)] if true {}); // The `match` equivalent is allowed.
}

..as well as some procedural macro attribute being used like #[my_proc_attr] if true { ... }

[Aaron1011]

#[cfg(not(foo))]
if cond1 {
} else #[cfg(not(bar))] if cond2 {
} else #[cfg(not(baz))] {
}

doesn't parse - I have somthing similar in src/test/ui/parser/if-attrs/else-attrs.rs

[Centril]

Right, hmm... the check-pass test does exercise the config behavior, but for extra robustness, could we add something like this?

// run-pass

fn main() {
    #[cfg(FALSE)]
    if true { panic!() }
}

[Aaron1011]

I'm confused - the whole point of this PR is that if-attrs are allowed syntactically, but denied semantically. Your example will error with "attributes are not yet allowed on if expressions" both before and after this PR - it's just that the error is being emitted from a different place now.

[Centril]

Ah right; I was thinking that cfg expansion might strip out the attribute. Can you add #[cfg(FALSE)] and #[cfg_attr(FALSE, whatever)] to this file to exercise that behavior for conditional compilation specifically?

[Aaron1011]

@Centril: Actually, you're right - that code does compile with this PR, though I think it probably shouldn't. Should I modify cfg-expansion to deny #[cfg] attributes on if expressions?

[Centril]

Yeah, that does seem like the conservative choice, although I have no idea how complicated it would be. Remember to also cover cfg_attr. But this is something that @petrochenkov should r+ :)

[Centril]

That #[attr] else does not parse is not surprising. The grammar for if is and should be if $expr $block (else $block)?. That is, else is not an expression in the AST by itself.

Maybe else #[attr] if should work, although that seems like a separate issue.

[Centril]

Also, let's cc @rust-lang/lang in case there are objections to this. I personally think we could also semantically allow proc macro attributes here, as it doesn't feel that confusing to have the attributes apply to the exterior as opposed to the interior. We might also want to run this through FCP possibly, let's wait and see. :)

[Aaron1011]

I personally think we could also semantically allow proc macro attributes here

That would be gated as a proc-macro expression attribute, right?

[Centril]

That would be gated as a proc-macro expression attribute, right?

I think so, but I would prefer for @petrochenkov to sign off on those changes as macros is not my expertise.

[Aaron1011]

I'm not particularly happy with how this PR is denying #[cfg] and #[cfg_attr]. However, I think any other approach would be much messier, or would require refactoring a large amount of the cfg-checking code.

[Centril]

Left some comments on Discord: https://discordapp.com/channels/442252698964721669/443151243398086667/672271149740326922

[Centril]

Reassigning to r? @petrochenkov for the config implementation since that's not my wheelhouse.

[bors]

☔ The latest upstream changes (presumably #68133) made this pull request unmergeable. Please resolve the merge conflicts.

[petrochenkov]

Perhaps it's time to just remove the restriction?

It's pretty established now that macros and attributes work with expressions/statements and not arbitrary syntactic fragments, so the choice in favor of the alternative 2 from https://github.com/rust-lang/rfcs/blob/master/text/0016-more-attributes.md#if is unlikely.

I can't say I like the "figure gating" this PR is trying to do.
(Also, it looks like macro attributes are not gated here, but they work in accordance with the alternative 1.)

[petrochenkov]

@Centril
Could you discuss the removal of this restriction with lang team?

[Centril]

I would love to discuss it with the team. (I agree with your assessment re. removing restriction wholesale and not being a fan of the complex gating here.)

[nikomatsakis]

Can somebody summarize precisely what changes are being proposed here? It seems like there was some iteration.

[Aaron1011]

@nikomatsakis: In its current form, this PR syntactically permits attributes on if expressions (e.g. #[attr] if true {} else if false {} else {}). The following are still denied:

• Post-macro-expansion attributes on if expressions (e.g. attributes are denied semantically). This includes #[cfg], #[cfg_attr], and custom attributes.
• Attributes on other parts of an 'if-else' chain (e.g. if true {} #[first_attr] else if {} #[second_attr] else {}) - these still don't even parse.

@petrochenkov and @Centril suggested allowing attributes semantically as well. This would permit attributes on if expressions after macro expansion, including #[cfg], #[cfg_attr], and custom attrs.

I do not want to consider attributes on other parts of the 'if-else' chain (e.g. if true {} #[first_attr] else if {} #[second_attr] else {}) as part of this PR. After discussing this on Discord with @Centril, I've come to believe that the only possible interpretations both have significant drawbacks:

1. Parse expressions like if true {} else #[attr] if false {} else {} like this:

if true {} else {
    #[attr]
    if false {
    } else {}
}

That is, parse the attribute as applying to the entire remaining part of the if-else chain, not just the particular 'branch' being annotated. This is the most 'natural' way to parse it based on how 'if-else' chains are parsed. However, I think it's extremely counter-intuitive, and would be an enormous footgun when combined with #[cfg].

2. Parse if-else chains with attributes only applying to the 'branch' they appear directly on. That is, the #[attr] in if true {} #[attr] else if false {} else {} only applies to else if false {}, not else if false {} else {}.

This approach would either require additional lookahead during parsing (to see if an else comes after the attribute, or would require us to syntactically accept code like this:

if true {} #[weird_attr] /* note the lack of `else` */

Personally, I don't think having attributes available in more places makes the drawbacks worth it. Either approach would probably need RFC, given that it would be a much larger change than what this PR currently does.

[Ixrec]

FWIW, if I had a use case for attributes on "else blocks", my first thought would be attaching those attributes to the "blocks" / pairs of curly braces, which bypasses most of the weirdness:

if true #[attr] {} else if false #[attr] {} else #[attr] {}

And if you really need your attribute to munch the else keywords for some reason, then we just tell you to put the attribute on the whole if expression (it's hard to imagine a case where you wouldn't want that anyway).

But I agree that any variation of this deserves a separate RFC with some concrete use cases.

[Centril]

We discussed this in today's language team meeting (2020-02-13).

Those present, in particular myself and @joshtriplett, felt that of the options available (1. keep the status quo, 2. accept only syntactically, 3. accept semantically also), that the 3rd option would be the simplest, and least complex approach.

Specifically, we would like to allow #[attr] on if both syntactically (in the parser) and semantically (expansion / conditional compilation, subject to existing feature gates for stmt_expr_attributes, e.g. #[allow()] match 0 { _ => {} }; (ungated) vs. let _ = #[allow()] match 0 { _ => {} }; (gated)).

In reaching this conclusion, we did consult RFC 16 regarding attributes on if expressions. We noted in particular that neither of else #[cfg(not(bar))] if cond2 or else #[cfg(not(baz))] {. Moreover, as @petrochenkov notes in #68658 (comment):

It's pretty established now that macros and attributes work with expressions/statements and not arbitrary syntactic fragments, so the choice in favor of the alternative 2 from https://github.com/rust-lang/rfcs/blob/master/text/0016-more-attributes.md#if is unlikely.

We also noted that while many users interpret code in a linear / textual fashion as opposed to as a tree-like structure, we still believe that the semantics of e.g. #[allow(...)] if ... is something that can be learned as necessary.

@Aaron1011 Can you please adjust the PR to remove the semantic restrictions and add relevant tests (including interactions with stmt_expr_attributes)? I'll start FCP when done.

[Aaron1011]

@Centril: I'm going to open a new PR, since almost all of this discussion in this PR doesn't apply once we allow if-attrs semantically.

[Aaron1011]

Closing in favor of #69201, which allows if-attrs both syntactically and semantically.