rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.16k stars 12.69k forks source link

Decide on path forward for attributes on expressions #127436

Open traviscross opened 3 months ago

traviscross commented 3 months ago

Long ago, we adopted rust-lang/rfcs#16 ("attributes on statements and blocks"). However, it's long been blocked despite the known and compelling use cases for this.

Over in the tracking issue, #15701, @WaffleLapkin explains the nature of what is blocking this and proposes a path forward:

From what I understand the issue blocking this is ambiguity -- even if the RFC specifies what #[meow] 1 + 1 means, it's still not very readable. I think the path forward is to cut this feature to only allow attributes on things that are unambiguous, such as:

  • All kinds of braces: #[meow] (1 + 1), #[uwu] [1, 2, 3], #[purr] {} (parethesis/grouping expr, tuples, arrays, blocks)
  • Closures: #[kwncjhn] || 2
  • Expressions starting with a keyword: #[meow] if x {}, #[attr] loop { break 'rust; }, #[kva] while false {}, ...
  • etc

Then we can provide a suggestion to add parenthesis around the expression, if it is not supported:

error: meow meow meow ambiguous attribute
 --> src/main.rs:LL:CC
   |
LL |     let x = #[meow] 1 + 1;
   |
help: wrap the expression in parenthesis
   |
LL |     let x = #[meow] (1 + 1);
   |                     +     +
help: wrap the expression in parenthesis (alternative
   |
LL |     let x = (#[meow] 1) + 1;
   |             +         +

Let's nominate this for discussion so we can decide whether we can unblock this by adopting that proposal.

This may have relevance for whether libs-api would feel the need to stabilize this:

@rustbot labels +I-lang-nominated +T-lang +C-discussion

cc @rust-lang/lang @WaffleLapkin

Tracking:

scottmcm commented 3 months ago

We had a short discussion about this in the lang triage meeting today.

We'd like to unblock progress here, but I had some concerns that I thought Tyler did a good job of phrasing: there might be a different between technically unambiguous and socially unambiguous. I'd thus like to propose making a partial step, and potentially coming back later with additional motivation for more places if needed.

Proposal: We stabilize attributes on closures and on blocks, like these

let _x = #[foo] || 0;
let _y = #[foo] { 4 };
let _z = { #![foo] 2 };

@rfcbot fcp merge

(Noting that attributes on statements are already stable.)

rfcbot commented 3 months ago

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

Concerns:

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. See this document for info about what commands tagged team members can give me.

traviscross commented 3 months ago

@rfcbot reviewed

In favor of taking an incremental step here.

petrochenkov commented 3 months ago

Proposal: We stabilize attributes on closures and on blocks, like these

Do you plan to stabilize any attributes in list contexts like

call(#[foo] { block1 }, #[bar] { block2 })

? (Macro attributes in particular.)

In any case I'll need to audit the technical details before any stabilization is merged, please ping me if necessary. (Can't do it right now, unfortunately.)

scottmcm commented 3 months ago

@petrochenkov Are you thinking of that in terms of whether cfgs on those can remove the expressions from the list? Or is it something else? (I'm not familiar with the gotchas for these things, so I could easily be missing something.)

petrochenkov commented 3 months ago

It needs to be decided how proc macros see the commas, or other separators in similar cases.

Ideally proc macros should be able to turn 1 expression into multiple (including 0) expressions in this position, similarly to cfgs or macros in list contexts without separators. So it would be reasonable if the separators were included into both input and output tokens streams (there are probably other alternatives, but they do not fit into the token-based model as well). The "reparse context" bit from https://github.com/rust-lang/rust/issues/61733#issuecomment-509626449 is likely relevant to this case as well.

joshtriplett commented 3 months ago

@scottmcm For closures with keywords in front of them (e.g. move), are you proposing exclusively #[attr] move || ... or also move #[attr] || ...? I'd assume the former and not the latter.

tmandry commented 3 months ago

I'm aligned on @scottmcm's proposal, assuming we can resolve concerns mentioned by @petrochenkov.

@rfcbot reviewed @rfcbot concern macros-in-lists

Ideally proc macros should be able to turn 1 expression into multiple (including 0) expressions in this position, similarly to cfgs or macros in list contexts without separators.

It's definitely not clear to me what we want here. The original RFC makes no mention of macros or list contexts at all. Perhaps we should not include macros in this stabilization, and consider that question separately?

traviscross commented 3 months ago

@petrochenkov / @WaffleLapkin: Do you have any recommendations for what the correct semantics are, and what we could stabilize here as a conservative first step?

scottmcm commented 3 months ago

Thanks for filing that, TC. I'm also feeling unconfident in my own proposal, after the explorations. @rfcbot concern we-should-figure-this-out-better

@WaffleLapkin, do you know which things people are more looking to be able to do right now that they can't? What are people looking to be able to attribute?

traviscross commented 3 months ago

@scottmcm: The use case that brought this to my attention was that libs-api seems likely to add a must_use function due to this limitation:

nikomatsakis commented 3 months ago

Good catch @petrochenkov -- I agree that it'd be nice for the macro to be able to remove items from the list at least (not sure about expanding to 1+). I'm not sure if including the comma is the best way to do that, versus (for example) just saying that the comma is removed if the macro returns no tokens.

I'm therefore inclined to say, let's exclude macro attributes in list position from stabilization.

joshtriplett commented 3 months ago

:+1: for excluding them for now. I think there are reasonable steps we could take here, namely allowing expansions to produce a top-level , separator, as well as checking if an expansion produced zero tokens and removing the following , if so. But that probably shouldn't be part of what was intended to be a conservative initial step.

programmerjake commented 3 months ago

I'll note that attributes on expressions are already stable in attribute proc macro inputs: (error caused here is by the proc macro, rustc itself doesn't cause an error afaict. makes me wish there was a handy proc macro in the playground that would just dump the input tokens instead of erroring) https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=88e28454bd778e29488bfd8bf6aa2b43

voidc commented 3 months ago

In #124099, I implemented @WaffleLapkin's suggestion to disallow ambiguous attributes on expressions. However, this change turned out to cause regressions in several crates that incorrectly use expression attributes and was subsequently reverted. This showed that the ambiguity is already causing issues and should be deprecated regardless of what subset of the feature is stabilized. To avoid the breakage of #124099, we can emit future compatibility warnings for ambiguous attributes instead of erroring.

WaffleLapkin commented 2 months ago

do you know which things people are more looking to be able to do right now that they can't? What are people looking to be able to attribute?

@scottmcm here are the cases I was able to find (basically all of them from Rust Community Discord):

Conclude from that what you will.

safinaskar commented 2 weeks ago

(Noting that attributes on statements are already stable.)

They sometimes don't work:

fn main() {
    #[allow(unused_variable)]
    assert_eq!({ let a = 2; 3 }, 3);
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7aa43c90479959bb93e8d299d3adc8d3