Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decide on path forward for attributes on expressions #127436

Open
traviscross opened this issue Jul 6, 2024 · 17 comments
Open

Decide on path forward for attributes on expressions #127436

traviscross opened this issue Jul 6, 2024 · 17 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. I-lang-nominated Nominated for discussion during a lang team meeting. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@traviscross
Copy link
Contributor

traviscross commented Jul 6, 2024

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:

@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. C-discussion Category: Discussion or questions that doesn't represent real issues. I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 6, 2024
@traviscross traviscross removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jul 6, 2024
@scottmcm
Copy link
Member

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
Copy link

rfcbot commented Jul 17, 2024

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 17, 2024
@traviscross
Copy link
Contributor Author

@rfcbot reviewed

In favor of taking an incremental step here.

@petrochenkov
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

petrochenkov commented Jul 17, 2024

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 #61733 (comment) is likely relevant to this case as well.

@joshtriplett
Copy link
Member

@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
Copy link
Member

tmandry commented Jul 23, 2024

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
Copy link
Contributor Author

traviscross commented Jul 23, 2024

@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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Member

👍 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
Copy link
Member

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
Copy link
Contributor

voidc commented Aug 2, 2024

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
Copy link
Member

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):

  • #[allow] on a block expression (one, two, three, four)
  • Same but with an unsafe {} block (message, the original bot invocation seems to be edited away)
  • Proc macro on block to simulate something similar to macro_rules! syntax? (message)
  • #[inline] on closures (one, two, three)
  • cfg-out a block (one, two, three)
  • Use one or another expression based on cfg (message)
  • Proc-macro on a closure (one, two)
  • #[rustfmt::skip] on an expression (unclear which) (one, two, three, four, +a case in a private discord chat)
  • "nothing to see here just getting around `attributes on expressions are experimental" (message)
  • #[allow(arithmetic_overflow)] 0usize - 1 (looks like for experiment, not real code?) (one, two)
  • cfg-out an assignment statement (one, two?, there were others, but I lost them...)
  • #[allow] on an .await in a statement in a macro (message)
  • #[allow] on an unsafe{} (one, two, three)
  • cfg-out an argument of concat!` (one, two)
  • proc-macro on a block (message)
  • #[allow] on += (message)
  • #[allow] on a closure (message)
  • #[cold] on a closure (message)

Conclude from that what you will.

@safinaskar
Copy link
Contributor

(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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. I-lang-nominated Nominated for discussion during a lang team meeting. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests