fix: fix the capture behavior of if let in closures

[Embers-of-the-Fire]

View all comments

Closes #153982.
TL;DR This patch adds the missing capture behavior change for if let statements introduced in RFC 2229.

This patch converts

self.walk_local(init, pat, None, || self.borrow_expr(init, BorrowKind::Immutable))?;

into

self.walk_local(init, pat, None, || Ok(()))?;

so that if let now behaves like let.

[rustbot]

r? @mati865

rustbot has assigned @mati865.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 13 candidates

[meithecatte]

Could you add a test case that's a bit more like #153982, so that there's also a field that doesn't get captured in the first place?

[Kivooeo]

cc @Nadrieril

[meithecatte]

So, this is technically a breaking change, much like #138961, right? It'd be nice to have a proof-of-concept test on which the borrow checker will start erroring, at which point we'd probably wanna do a crater run...

View changes since this review

[Kivooeo]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: dec9417 (dec9417b8611e34e787a3e4c37686b5131f9e5c5, parent: 562dee4820c458d823175268e41601d4c060588a)

[Kivooeo]

I'm wondering which mode we should run crater in. Would cargo check be sufficient, or should we use build or build and test instead?

[Embers-of-the-Fire]

Would cargo check be sufficient

AFAIK check should be sufficient since this patch modifies something rather high in the compiler, and in theory, strong dependencies on the size of closures shouldn't be very common. The most likely result is that we may break the unit tests for some of the lower level libraries, but it’s unlikely that this will affect functionality.

[meithecatte]

Since this PR changes the drop order in some cases, it is technically conceivable that some code would have an observable change in behavior after this PR gets merged. It is also technically possible that a crate's test suite would notice this. I would however be extremely surprised if someone has written such code and made a test suite good enough to be able to detect this – previous experience with crater runs for similar adjustments confirms this.

If it was my call to make, I'd run a check run and assume that in the exceedingly unlikely case where someone does depend on this, it'll get caught by the full crater run for the beta.

But my call to make it is not.

[theemathas]

It can technically change run time behavior by changing drop order. I think that's kind of unlikely to be relied on though.

[Kivooeo]

Based on the discussion above, I'm going with just check.

@craterbot run mode=check-only

[craterbot]

👌 Experiment pr-154210 created and queued.
🤖 Automatically detected try build dec9417
⚠️ Try build based on commit 66777ad, but latest commit is 4d78fd1. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[jieyouxu]

It can technically change run time behavior by changing drop order. I think that's kind of unlikely to be relied on though.

If this can change runtime behavior (ergo, a stable breaking change), then IMO this definitely justifies a full build-and-test not just check-only. I imagine lang would likewise want as much of the full picture as possible on ecosystem impact when making the accept/reject call.

[Nadrieril]

@craterbot cancel

[craterbot]

🗑️ Experiment pr-154210 deleted!

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[Nadrieril]

All the crater regressions I've looked at look unrelated to this PR

[theemathas]

@rustbot labels +I-lang-nominated

I'm unsure if this a T-lang FCP, but nominated just in case.

In edition 2021+, match and let with a pattern can cause partial captures in closures. Previously, if let did not also have this same partial capture behavior. This PR changes if let so that it can cause partial captures in closures in edition 2021+.

[traviscross]

This seems to be a correct consistency fix to me. Based on @Nadrieril's report that all the crater regressions he's looked at seem unrelated (thanks for having a look), I propose we do this.

@rfcbot fcp merge lang

[rust-rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

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!

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.

[joshtriplett]

Since this is changing exactly what closures capture, is there a test to make sure that this doesn't introduce new UB-detection issues that rhyme with
rust-lang/reference#2121 ? (Not an expert here, just asking for confirmation that it won't introduce new such issues.)

[tmandry]

The potential drop order change is a concern, but since we managed to do it in #138961 with match I think if let should be fine.

@rfcbot reviewed

[joshtriplett]

Going to go ahead and check a box on the assumption that #154210 (comment) is already covered; if there's any issue on that front, please do raise a concern.

@rfcbot reviewed

[traviscross]

cc @rust-lang/opsem

[rust-rfcbot]

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

[RalfJung]

cc @rust-lang/opsem

This is more a question of how the generated MIR for these patterns looks like, which we're not the experts on... @Nadrieril are there tests we should have to ensure all the intended UB is captured by the generated MIR?

[Nadrieril]

@Nadrieril are there tests we should have to ensure all the intended UB is captured by the generated MIR?

I'm not sure I follow, do you mean Miri tests?

[RalfJung]

Yes.

[Nadrieril]

match and if let will have the same capture behavior after this, so you could duplicate any test with a match inside a closure and turn it into an if let (and/or vice-versa), but if you trust that the capturing and lowering is indeed the same now then you don't have to do anything.

[RalfJung]

Apparently the lowering was not the same before this PR so it may be worth having some sort of smoke test here?

Is there some test where Miri's verdict (UB vs non-UB) changes with this PR due to the changed capture behavior?

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.