Skip to content

if let pre-stabilization preview #140908

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Kivooeo
Copy link
Contributor

@Kivooeo Kivooeo commented May 10, 2025

I didn’t quite find the courage to open a stabilization PR right away — I thought it might make more sense to start with a review and see what the team thinks first

I went through the full set of tests and also studied #132833, which stabilized let_chains and includes a some of discussion and evidence that if let guards behave correctly

My conclusion is that the feature appears sound and well-tested, and I believe it’s ready for stabilization. If I missed anything, please let me know

Since development seems to have stalled, and the test suite already covers a wide range of interactions (including drop order, temporaries, mutability, macros and a LOT more), I believe this would be a productive and low-risk step toward stabilization

If this direction looks good to everyone, I’d be happy to try opening a proper stabilization PR — not sure how tricky it’ll be, but I’d love to gain the experience :D

Tracking issue #51114
Reference adding documentaion for if let issue rust-lang/reference#1823
rendered

Pre-stabilization checklist

  • Tests cover drop order, move, async, const, bindings, temporaries, mutability, macros, and a lot more (see tests for if let guard)
  • irrefutable_let_patterns lint works as expected
  • Verify other lints (unused_variables, unreachable_patterns, etc.)
  • Documentation updated in src/doc/if-let-guard-review.md and if let guards documentation reference#1823
    (The if-let-guard-review.md document is a comprehensive write-up summarizing the state of the if let guards feature, including tests, drop semantics, and stabilization considerations. Since it’s more of a review and discussion document rather than final documentation, I don’t think it belongs in src/doc)
  • Confirm 2024-only stabilization with team
  • Open stabilization PR after team approval

r? @est31

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2025
@rust-log-analyzer

This comment has been minimized.

Here `v` is `&Vec`, and the guard borrows parts of it; those references are valid in the arm body. If a guard binds by value
(e.g. `if let x = some_moveable`), the usual move/borrow rules apply (see below), but in all cases the scopes follow the match-arm rules.

Moreover, an `if let` guard cannot break exhaustiveness: each arm is either taken or skipped in the usual way.
Copy link
Member

Choose a reason for hiding this comment

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

I'd expect exhaustiveness checks to work like with normal guards: they don't play into exhaustiveness checks at all.

In effect, the drop semantics are the same as for an ordinary match guard or an `if let` in an `if` expression: no unexpected
extension of lifetimes. (In Rust 2024 edition, there is a finer rule that even in `if let`
expressions temporaries drop before the `else` block; but for match guards the effect is that temporaries from the
guard are dropped before the arm body.)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should take the approach that we adopt 2024 drop order behaviour on all editions. As it's a green field new feature, we can do that. Because I see that there is edition differences on the example from #104843 adapted to if let guards: there is a broken MIR ICE on 2021, but no ICE on 2024.

When it comes to #103476 (comment) , I can't reproduce it on the 2021 edition.

@est31
Copy link
Member

est31 commented May 10, 2025

I'd also check if the lints for bindings properly visit if let guards: they are a new concept, so maybe not all lints are adapted to them. E.g. do we check for irrefutable tails via the irrefutable_let_patterns lint (see #94951 for the work I did on let chains)? Irrefutable suffixes need to be allowed though as there is no easy way to write those (due to the match making things conditional).

@est31
Copy link
Member

est31 commented May 10, 2025

Also, from experience from let chains and let else we should really ensure that we get the drop order right. I think if let guards are a bit more rarely to be used than former two features, so users won't be doing as much experimentation with it. On the other hand, having done both let chains and let else gave us some experience around drop order which we can apply to if let guards as well (tests in the testsuite as a minimum).

As a procedural note, even though I have responded, PRs adding documents are generally not the place the Rust community has discussions. Either one can discuss it in https://internals.rust-lang.org/ or on zulip. But it's also fine to file a stabilization PR right away, at the cost that it might be open for years :).

@Kivooeo
Copy link
Contributor Author

Kivooeo commented May 11, 2025

@est31 Thanks! I actually have quite a bit of free time right now, so I’ll spend it digging into this further. Also, I completely forgot about the internals forum — you’re right, it would probably be a better place to post my write-up, maybe with a Gist link to the full review

My original thinking was to open a PR just to get some feedback on the general state of the feature, and then either close it or follow it up with a proper stabilization PR

I’ll check whether the lints behave correctly later today

One thing I’m wondering about: how can I enable the 2024 edition drop order behavior in older editions?

Maybe it would make sense to rename the current PR to something like “pre-stabilization review” and add a checklist of steps toward stabilization? Would that be a reasonable approach?

@est31
Copy link
Member

est31 commented May 11, 2025

add a checklist of steps toward stabilization?

I personally love checklists, they allow organizing things really well. With github checklists though, especially in PRs or issues, people have an expectation that you keep them up to date. So I'd suggest making one but making it clear that it's not the "real" one, just a draft for the actual stabilization report.

One thing I’m wondering about: how can I enable the 2024 edition drop order behavior in older editions?

I suppose you can take a look how the 2024 drop order was introduced. It should be possible to adjust that code to support special treatment for if-let guards. The most relevant reading in that regard is these two PRs:

@Kivooeo
Copy link
Contributor Author

Kivooeo commented May 11, 2025

@est31 I'm wondering if we should enable if let guards only for edition 2024 and later. I tried removing the edition checks and applying the IfLetRescope behavior in edition 2021, but it led to MIR issues (e.g., internal compiler errors related to drop scopes). Given that let_chains were also stabilized only for edition 2024 to avoid similar backward compatibility pitfalls, maybe it makes sense to take the same approach here? We could open an issue later if compatibility between editions becomes essential, but for now, it seems reasonable to restrict this feature to edition 2024. After all, it is stable (at least in 2024 edition), and thanks to if let rescope, it is now highly predictable and stable, especially for things like move and drop semantics

I guess if we can start from stabilizing it in 2024 edition only, then I think it's a good point to open stabilization PR from

@Kivooeo
Copy link
Contributor Author

Kivooeo commented May 11, 2025

irrefutable_let_patterns
Also, works as intented, will try to check other lints today, maybe will found something
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=8accdf1497ec546de7be072b832b6de0

@Kivooeo Kivooeo changed the title Internal review for if-let guards feature stabilization if let pre-stabilization preview May 11, 2025
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 11, 2025
@rust-log-analyzer

This comment has been minimized.

@est31
Copy link
Member

est31 commented May 11, 2025

It might be that the errors you mentionhave an easy fix. In any case, as I've said before, let's discuss this in a github issue, an internals.rust-lang.org thread, or a zulip thread.

Oh and before I forget it, rustfmt support is quite important too. It needs to be tracked for stabilization.

@Kivooeo Kivooeo closed this May 11, 2025
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)


# RECOMMENDATIONS

* Fix missing copyright/licensing information: For one or more files, the tool
  cannot find copyright and/or licensing information. You typically do this by
  adding 'SPDX-FileCopyrightText' and 'SPDX-License-Identifier' tags to each
  file. The tutorial explains additional ways to do this:
  <https://reuse.software/tutorial/>

  local time: Sun May 11 11:52:09 UTC 2025
  network time: Sun, 11 May 2025 11:52:09 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@Jules-Bertholet
Copy link
Contributor

an internals.rust-lang.org thread

https://internals.rust-lang.org/t/pre-stabilization-if-let-guards/22901

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants