Permit attributes on 'if' expressions

[Aaron1011]

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).
Both built-in attributes (e.g. #[allow], #[cfg]) and proc-macro attributes are supported.

We still do not accept attributes on 'other parts' of an if-else
chain. That is, the following code snippet still fails to parse:

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

Closes #68618

[Aaron1011]

r? @Centril

[Centril]

Looks good; I'd like to see some more tests though:

• Interactions with stmt_expr_attributes, specifically demonstrating that let _ = #[allow(...)] if ...; is feature gated.
• Testing that let _ = #[cfg(FALSE)] if true {}; results in "error: removing an expression is not supported in this position".
• Testing that #[cfg(FALSE)] and cfg_attr perform normally when attached onto if ... as a statement. (e.g. provoke a type error inside the block in the former case and a lint set to deny in the latter case, and ensure that the test is check-pass).

These mostly test aspects of statements and expressions themselves, but I'd like to be sure that there's nothing out of the ordinary here.

[Centril]

From #68658 (comment):

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.

Based on this, let's:

@rfcbot merge

[rfcbot]

Team member @Centril has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Centril
• @cramertj
• @eddyb
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[joshtriplett]

I'd like to request one more test, please:

let x = 1;

#[cfg(FALSE)]
if false {
    x = 2;
} else if true {
    x = 3;
} else {
    x = 4;
}
assert_eq!(x, 1);

This makes sure that the attribute gets applied to the entire chained if/else-if/else construct, and not just part of it.

[Aaron1011]

@Centril: I've added the additional tests you requested and removed the parser recovery test.

@joshtriplett: I've added the test you suggested.

[rust-highfive]

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-02-19T16:06:14.4258135Z ========================== Starting Command Output ===========================
2020-02-19T16:06:14.4295156Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/f2672492-244e-4e7c-89ba-7c05bab6e5a0.sh
2020-02-19T16:06:14.4295260Z 
2020-02-19T16:06:14.4297979Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-19T16:06:14.4303973Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69201/merge to s
2020-02-19T16:06:14.4305653Z Task         : Get sources
2020-02-19T16:06:14.4305688Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-19T16:06:14.4305723Z Version      : 1.0.0
2020-02-19T16:06:14.4305758Z Author       : Microsoft
---
2020-02-19T16:06:15.2833440Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-19T16:06:15.2936260Z ##[command]git config gc.auto 0
2020-02-19T16:06:15.3000888Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-19T16:06:15.3072873Z ##[command]git config --get-all http.proxy
2020-02-19T16:06:15.3205112Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69201/merge:refs/remotes/pull/69201/merge
---
2020-02-19T16:11:49.1750071Z    Compiling serde_json v1.0.40
2020-02-19T16:11:50.2881613Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2020-02-19T16:11:59.3565872Z     Finished release [optimized] target(s) in 1m 21s
2020-02-19T16:11:59.3652037Z tidy check
2020-02-19T16:11:59.9046588Z tidy error: /checkout/src/test/ui/if-attrs/gate-whole-expr.rs:4: tab character
2020-02-19T16:11:59.9047073Z tidy error: /checkout/src/test/ui/if-attrs/gate-whole-expr.rs:6: tab character
2020-02-19T16:11:59.9047394Z tidy error: /checkout/src/test/ui/if-attrs/gate-whole-expr.rs:7: tab character
2020-02-19T16:11:59.9047703Z tidy error: /checkout/src/test/ui/if-attrs/gate-whole-expr.rs:8: tab character
2020-02-19T16:11:59.9048022Z tidy error: /checkout/src/test/ui/if-attrs/gate-whole-expr.rs:9: tab character
2020-02-19T16:11:59.9048346Z tidy error: /checkout/src/test/ui/if-attrs/gate-whole-expr.rs:10: tab character
2020-02-19T16:11:59.9048655Z tidy error: /checkout/src/test/ui/if-attrs/gate-whole-expr.rs:11: tab character
2020-02-19T16:11:59.9048976Z tidy error: /checkout/src/test/ui/if-attrs/gate-whole-expr.rs:12: tab character
2020-02-19T16:11:59.9049284Z tidy error: /checkout/src/test/ui/if-attrs/gate-whole-expr.rs:13: tab character
2020-02-19T16:11:59.9050064Z tidy error: /checkout/src/test/ui/if-attrs/gate-whole-expr.rs:14: tab character
2020-02-19T16:12:01.8422230Z some tidy checks failed
2020-02-19T16:12:01.8422372Z Found 487 error codes
2020-02-19T16:12:01.8422437Z Found 0 error codes with no tests
2020-02-19T16:12:01.8422520Z Done!
2020-02-19T16:12:01.8422520Z Done!
2020-02-19T16:12:01.8422557Z 
2020-02-19T16:12:01.8422590Z 
2020-02-19T16:12:01.8423604Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2020-02-19T16:12:01.8423741Z 
2020-02-19T16:12:01.8423772Z 
2020-02-19T16:12:01.8423848Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2020-02-19T16:12:01.8423913Z Build completed unsuccessfully in 0:01:31
2020-02-19T16:12:01.8423913Z Build completed unsuccessfully in 0:01:31
2020-02-19T16:12:01.8467495Z == clock drift check ==
2020-02-19T16:12:01.8488385Z   local time: Wed Feb 19 16:12:01 UTC 2020
2020-02-19T16:12:02.0045624Z   network time: Wed, 19 Feb 2020 16:12:02 GMT
2020-02-19T16:12:02.0051506Z == end clock drift check ==
2020-02-19T16:12:02.7155115Z 
2020-02-19T16:12:02.7245306Z ##[error]Bash exited with code '1'.
2020-02-19T16:12:02.7259893Z ##[section]Finishing: Run build
2020-02-19T16:12:02.7273691Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69201/merge to s
2020-02-19T16:12:02.7275407Z Task         : Get sources
2020-02-19T16:12:02.7275476Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-19T16:12:02.7275526Z Version      : 1.0.0
2020-02-19T16:12:02.7275569Z Author       : Microsoft
2020-02-19T16:12:02.7275569Z Author       : Microsoft
2020-02-19T16:12:02.7275633Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-02-19T16:12:02.7275687Z ==============================================================================
2020-02-19T16:12:03.1035820Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-02-19T16:12:03.1075712Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/69201/merge to s
2020-02-19T16:12:03.1180888Z Cleaning up task key
2020-02-19T16:12:03.1181809Z Start cleaning up orphan processes.
2020-02-19T16:12:03.1285956Z Terminate orphan process: pid (4010) (python)
2020-02-19T16:12:03.1461964Z ##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[Centril]

r=me when FCP is done

[scottmcm]

@rfcbot reviewed

I don't have a complaint about this situation in particular, but do wonder about philosophy around attributes here. @Centril mentioned in the meeting that it's expressions in certain context that can allow attributes; I'd like a page in the reference or something about why particular things can or cannot have attributes. For example, the following currently doesn't work:

let x = #[cfg(true)] 4 #[cfg(not(true))] 5;

But in some sense there isn't a reason that it couldn't be made to work, since either way is a valid expression for the RHS of the binding.

So I'd just like a definition somewhere about why it's the right thing to do on an if expression, but not on a literal expression. (Sorry if it already exists somewhere, but I didn't see an answer in the OP, in the RFC proposal comment, or in skimming the thread.)

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[eddyb]

@scottmcm There is one interesting distinction: expressions if are allowed to be expression statements without a ;.
AFAIK, the shortest version of your example that does work is:

let x = {
    #[cfg(foo)] { 4 }
    #[cfg(not(foo))] { 5 }
};

This is because this parses even if you remove the #[cfg]s (and results in a mismatched types error, as it expects { 4 } to have the type ()).

[Centril]

The reason why e.g. #[cfg(stuff))] let x = 0; works but e.g. let x = #[cfg(true)] 4 #[cfg(not(true))] 5; doesn't can I think be summarized with "because monoids". In the former context, we have a list of statements, and lists have as their identity element the empty list. In the latter context, we have an expression to the right of =. Expressions do not have an identity element, so what do we do if both 4 and 5 are removed? Similarly, what do we do if both are kept? Remember that the conditional compilation engine does not perform any form of "there will only be one remaining" analysis after stripping is done. (This is ignoring the fact that the parser would not allow Attrs* 4 Attrs* 5, so we never reach conditional compilation to begin with.)