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

More expressions correctly are marked to end with curly braces #118880

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

GearsDatapacks
Copy link
Contributor

@GearsDatapacks GearsDatapacks commented Dec 12, 2023

Fixes #118859, and replaces the mentioned match statement with an exhaustive list, so that this code doesn't get overlooked in the future

@rustbot
Copy link
Collaborator

rustbot commented Dec 12, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nnethercote (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 12, 2023
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs tests for every one of these expressions. Please add them to some relevant subdirectory in tests/ui, maybe just tests/ui/parser, though you could add a subdirectory to tests/ui/parser... up to you.

Also, this causes some code to go from passing to failing, right? If so, we need to nominate this for T-lang discussion.

@dtolnay
Copy link
Member

dtolnay commented Dec 12, 2023

Also, this causes some code to go from passing to failing, right? If so, we need to nominate this for T-lang discussion.

Not any stable code. The only behavior changes in the PR are to const {}, do yeet, and become, all of which are properly feature gated in the parser even inside of #[cfg(any())] code.

#[cfg(any())]
fn repro() {
    let _ = const {} else {};
    let _ = do yeet {} else {};
    let _ = become {} else {};
}
error[E0658]: inline-const is experimental
 --> src/lib.rs:3:13
  |
3 |     let _ = const {} else {};
  |             ^^^^^
  |
  = note: see issue #76001 <https://github.com/rust-lang/rust/issues/76001> for more information
  = help: add `#![feature(inline_const)]` to the crate attributes to enable

error[E0658]: `do yeet` expression is experimental
 --> src/lib.rs:4:13
  |
4 |     let _ = do yeet {} else {};
  |             ^^^^^^^^^^
  |
  = note: see issue #96373 <https://github.com/rust-lang/rust/issues/96373> for more information
  = help: add `#![feature(yeet_expr)]` to the crate attributes to enable

error[E0658]: `become` expression is experimental
 --> src/lib.rs:5:13
  |
5 |     let _ = become {} else {};
  |             ^^^^^^^^^
  |
  = note: see issue #112788 <https://github.com/rust-lang/rust/issues/112788> for more information
  = help: add `#![feature(explicit_tail_calls)]` to the crate attributes to enable

Break(_, _)
| Range(_, _, _)
| Ret(_)
| Yield(_)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this more precise? The only pattern that must be matched here is Yield(None). Same with a few of the other below patterns.

@compiler-errors
Copy link
Member

Not any stable code. The only behavior changes in the PR are to const {}, do yeet, and become, all of which are properly feature gated in the parser even inside of #[cfg(any())] code.

Right, I misread that match in this PR's implementation.

What about MacCall though? This PR doesn't affect let _ = mac! {} else {}; That behavior still seems inconsistent and just seems to be just added to the "okay" arm of the match.

@dtolnay
Copy link
Member

dtolnay commented Dec 12, 2023

The only behavior changes in the PR are to const {}, do yeet, and become, all of which are properly feature gated in the parser even inside of #[cfg(any())] code.

But I would like @rust-lang/lang to comment on other cases ending in brace, not touched in this PR, which are stable.

// ExprKind::FormatArgs
let _ = format_args! {
    "..."
} else {
    return;
};
// ExprKind::MacCall
let Ok(_) = writeln! {
    io::sink(),
    "...",
} else {
    return;
};
// ExprKind::IncludedBytes
let b"..." = include_bytes! {
    concat!(env!("OUT_DIR"), "/the_bytes")
} else {
    return;
};
// ExprKind::InlineAsm
let 0isize = asm! {
    ...
} else {
    return;
};

and finally, unstable offset_of.

@WaffleLapkin WaffleLapkin added the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 13, 2023
@WaffleLapkin
Copy link
Member

Nominating for T-lang discussion as per the comment above

@nnethercote
Copy link
Contributor

Michael is already effectively serving as reviewer, and clearly knows more about this particular piece of code than I do. Therefore:

r? @compiler-errors

@GearsDatapacks
Copy link
Contributor Author

I have added tests for these cases, and made the match expression more explicit.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 13, 2023
@rustbot

This comment was marked as resolved.

@WaffleLapkin WaffleLapkin removed the has-merge-commits PR has merge commits, merge with caution. label Dec 13, 2023
@WaffleLapkin
Copy link
Member

(removed the has-merge-commits PR has merge commits, merge with caution. because it looks like the merge commits in question were removed)

@traviscross
Copy link
Contributor

traviscross commented Dec 13, 2023

@rustbot labels -I-lang-nominated

We discussed this in the T-lang meeting today. There was a feeling that the current behavior may not in fact be ambiguous and that it may be fine to leave it as-is. However, we were a bit unsure about the ask.

@dtolnay: If there's a specific rule that you would propose here for the handling of this kind of thing, please describe that and why it would be better and then renominate this issue for further discussion.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 13, 2023
@WaffleLapkin
Copy link
Member

WaffleLapkin commented Dec 13, 2023

We've talked about this today in @rust-lang/lang triage meeting. We were not sure what you are asking/proposing, @dtolnay, maybe you can clarify?

Specifically it was unclear why we would want to prohibit macro examples in your comment (#118880 (comment)). Since macros use ! there is no potential for ambiguity.

We even talked about maybe instead allowing more expressions ending in } before else {, as there shouldn't actually be ambiguity and it's not fully clear why this restriction is necessary at all.

Reading the test (my personal opinions):

  • {}, S{}, const{}, unsafe{} — don't see any ambiguity, we could just allow that
  • for, while, a = — they all have () type, so are useless with let-else, fine to disallow to prevent confusion
  • if+else, loop, match — those can be useful with with let-else but also might be confusing 🤷🏻

oops, was writing the comment at the same time as @traviscross and github did not show his post to me

@dtolnay
Copy link
Member

dtolnay commented Dec 14, 2023

A design aspect of the let-else RFC that was important enough to feature in the 6-sentence summary is that the EXPRESSION in the syntax let PATTERN: TYPE = EXPRESSION else DIVERGING_BLOCK; must not end in }.

This restriction ended up in the RFC based on multiple rounds of feedback.

  • The feature originally envisioned by the author did not have this restriction. It only had the essential restriction that EXPRESSION must not be if … {…} (+else if) because in that case the new syntax would collide with stable syntax that already had a different meaning.

  • Based on a suggestion in Zulip by bstrie shortly before the publication of the RFC, the syntax was made more restrictive to say EXPRESSION must not be what the Reference calls ExpressionWithBlock. The change was made in rust-lang/rfcs@3438586 and means the expression must not be any of {…}, unsafe {…}, loop {…}, while … {…}, for … in … {…}, if … {…} (+else if +else), or match … {…}.

  • The wording to disallow the above expressions was made more precise in response to Niko's non-blocking concern. The wording improvement was made in rust-lang/rfcs@89c5b6e without changing the intent of the restriction.

  • In response to steffahn's insightful comment and a related older unresolved review by nagisa and followup, followed by several +1 comments in GitHub and Zulip, and digama0's review, the syntax was made more restrictive again to disallow EXPRESSION from being anything having the last token }. This change was rust-lang/rfcs@c0c2fcc. It disallows async {…}, try {…}, const {…}, Struct {…}, |…| {…}, &{…}, break {…}, return {…} and many others. This restriction was not implemented by the compiler.

  • With further iteration from Zulip, the wording was clarified in rust-lang/rfcs@b6b87d6 to emphasize that any expression ending in } before macro expansion is not allowed. mac! {…} is one such example. This restriction was not implemented by the compiler and the cases in More expressions correctly are marked to end with curly braces #118880 (comment) all violate it.

The compiler's implementation of let-else did not implement the restriction described by the accepted RFC, so some programs that the RFC says are invalid syntax are accepted by stable Rust compilers since 1.65.0.

To understand why disallowing only ExpressionWithBlock is not sufficient, see the code samples containing -if in steffahn's comment.

Separately from the ambiguities explained in that comment, steffahn also was I think the first to note a human value in having } else { always mean an else corresponding to an if, as opposed to an else corresponding to a let. This is voiced in steffahn's comment linked above, as well as subsequent discussion on Zulip arriving at agreement about the final strengthenings ("on the grounds that it makes it easy to distinguish let from let-else by looking for an else not preceded by }").

rust-lang/rfcs#3137 (comment) has a summary of the consensus from Zulip and the state of the RFC after the second strengthening of the restriction.

rust-lang/rfcs#3137 (comment) expresses the author's fondness in favor of the second strengthened restriction, "to be clear to human readers", which I also agree with, and is further reinforced by a lang team consensus expressed by Josh in Zulip: "Based on the lang team consensus, the thing we wanted to avoid was let PAT = some_construct_with { braces } else { diverge };"

At the time, only one single exception to the must-not-end-in-} form of the restriction was still being considered, and that was very explicitly the specific case of {…}, i.e. let … = {…} else {…}; with nothing between the = and first {. "So, nothing that ends in } and doesn't start with { unless it's wrapped in parentheses or braces." Ultimately there was not consensus about this exception, there was strong opposition in Zulip and it was never written into the RFC or implemented in the compiler.

The issue of } before else did not get listed as an unresolved question in the final draft of the RFC which got merged. This is because the topic was sufficiently resolved by the discussion described above and the lang team consensus via Zulip, and all parties were in agreement about this iteration of the restriction. So there was no further discussion of it in the tracking issue nor during stabilization aside from one brief comment reinforcing that the restriction is intentional and future possibilities are left open.

Mentioning some of the contributors whose comments I have paraphrased, in case their view has changed since then or has not been adequately captured here: @Fishrock123 @digama0 @steffahn @camsteffen @bstrie

@dtolnay: If there's a specific rule that you would propose here for the handling of this kind of thing, please describe that and why it would be better and then renominate this issue for further discussion.

The rule I like is that EXPRESSION must not end in }, which was the RFC final draft.

To the extent that the compiler is going to accept some EXPRESSION which end in } (which is not my preference), I think those cases should be approved by the lang team since so far no exceptions to must-not-end-in-} have been through team approval.

@dtolnay dtolnay added the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 14, 2023
@compiler-errors
Copy link
Member

Thanks for the detailed history, @dtolnay.

@GearsDatapacks: As for this PR, the fact that these remaining expressions can't be fixed without breaking existing code should be recorded in a FIXME, so it isn't forgotten. Since the PR only changes unstable syntax, I can r+ it after that.

Also, please squash all these commits into one.

@rustbot author

…sion kinds to specify whether they contain a brace

Add inline const and other possible curly brace expressions to expr_trailing_brace

Add tests for `}` before `else` in `let...else` error

Change to explicit cases for expressions with optional values when being checked for trailing braces

Add tests for more complex cases of `}` before `else` in `let..else` statement

Move other possible `}` cases into separate arm and add FIXME for future reference
@GearsDatapacks
Copy link
Contributor Author

I added the FIXME, and squashed the commits into one.

@camsteffen
Copy link
Contributor

Is the FIXME actually fixable? If it's an edition thing, how to we record todos for the next edition?

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 17, 2023

📌 Commit 1fc6dbc has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 17, 2023
@compiler-errors
Copy link
Member

compiler-errors commented Dec 17, 2023

Is the FIXME actually fixable?

Not clear.

If it's an edition thing, how to we record todos for the next edition?

I also have no idea. I think we may want to open a new tracking issue after T-lang weighs in on the existing inconsistencies, and we can add the relevant 2024 tags to that issue.

@dtolnay
Copy link
Member

dtolnay commented Dec 17, 2023

Is the FIXME actually fixable?

Yes.

Currently on crates.io, here is the prevalence of each kind of expression in the EXPRESSION part of a let-else (using syn::Expr's naming):

  • 12750 Expr::MethodCall
  • 4914 Expr::Path
  • 3275 Expr::Call (function call)
  • 2681 Expr::Field
  • 1752 Expr::Reference
  • 1069 Expr::Try (question mark)
  • 478 Expr::Await
  • 405 Expr::Index
  • 339 Expr::Unary
  • 187 Expr::Tuple
  • 81 Expr::Macro
  • 66 Expr::Paren
  • 5 Expr::Binary
  • 3 Expr::Array
  • 1 Expr::Block
  • 1 Expr::ForLoop
  • 1 Expr::If
  • 1 Expr::Loop
  • 1 Expr::Match
  • 1 Expr::While
  • 0 all others (Assign, Async, Break, Cast, Closure, Const, Continue, Let, Lit, Range, Repeat, Return, Struct, TryBlock, Unsafe, Yield)

Of the 81 macro calls, all 81 use parenthesis delimiter. 0 use square bracket delimiter or brace delimiter.

@digama0
Copy link
Contributor

digama0 commented Dec 17, 2023

I'm curious about the Expr::Block, Expr::ForLoop and Expr::While examples because Expr::Block is supposed to be specifically disallowed and the other two should be a type error (you can't refutably match something with type ()).

I'm surprised the Expr::Paren case isn't more common, because of the let PAT = ({ .. }) else { ... } pattern that was discussed during the RFC.

@dtolnay
Copy link
Member

dtolnay commented Dec 17, 2023

It's parser test cases from rust-analyzer in ra_ap_parser 0.0.189.
https://github.com/rust-lang/rust-analyzer/tree/master/crates/parser/test_data/parser/err

test_data/parser/err/0049_let_else_right_curly_brace_for.rs
test_data/parser/err/0050_let_else_right_curly_brace_loop.rs
test_data/parser/err/0051_let_else_right_curly_brace_match.rs
test_data/parser/err/0052_let_else_right_curly_brace_while.rs
test_data/parser/err/0053_let_else_right_curly_brace_if.rs
test_data/parser/inline/err/0017_let_else_right_curly_brace.rs

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2023
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#118880 (More expressions correctly are marked to end with curly braces)
 - rust-lang#118928 (fix: Overlapping spans in delimited meta-vars)
 - rust-lang#119022 (Remove unnecessary constness from ProjectionCandidate)
 - rust-lang#119052 (Avoid overflow in GVN constant indexing.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6269bf1 into rust-lang:master Dec 17, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 17, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2023
Rollup merge of rust-lang#118880 - GearsDatapacks:issue-118859-fix, r=compiler-errors

More expressions correctly are marked to end with curly braces

Fixes rust-lang#118859, and replaces the mentioned match statement with an exhaustive list, so that this code doesn't get overlooked in the future
@dtolnay
Copy link
Member

dtolnay commented Dec 17, 2023

I am moving the lang-nominated label over to #119057 for followup.

@rustbot label -I-lang-nominated

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 17, 2023
fmease added a commit to fmease/rust that referenced this pull request May 22, 2024
Disallow cast with trailing braced macro in let-else

This fixes an edge case I noticed while porting rust-lang#118880 and rust-lang#119062 to syn.

Previously, rustc incorrectly accepted code such as:

```rust
let foo = &std::ptr::null as &'static dyn std::ops::Fn() -> *const primitive! {
    8
} else {
    return;
};
```

even though a right curl brace `}` directly before `else` in a `let...else` statement is not supposed to be valid syntax.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2024
Rollup merge of rust-lang#125049 - dtolnay:castbrace, r=compiler-errors

Disallow cast with trailing braced macro in let-else

This fixes an edge case I noticed while porting rust-lang#118880 and rust-lang#119062 to syn.

Previously, rustc incorrectly accepted code such as:

```rust
let foo = &std::ptr::null as &'static dyn std::ops::Fn() -> *const primitive! {
    8
} else {
    return;
};
```

even though a right curl brace `}` directly before `else` in a `let...else` statement is not supposed to be valid syntax.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let-else not checking for curly brace of inline const before else
10 participants