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

[mati865]

I'm not a good person to review it. Does anybody from this thread want to take it over? Thanks in advance.

[meithecatte]

@mati865 I don't have reviewer permissions, but the change looks good to me, and it's touching pretty much the one file in the entire compiler which I'd say I'm actually qualified to review.

Otherwise, I suppose @Nadrieril would surely be familiar with this code?

View changes since this review

[Nadrieril]

r? me

Looks good to me, I am happy to merge implementation-wise.

Not sure what to make of the crater run, should we rerun on the spurious results again?

[Nadrieril]

I'm also unsure whether this needs t-lang input: this is clearly a bug in the compiler, where the behavior differs from the merged RFC. I'll tentatively say this is purely t-compiler territory unless the breaking changes are big.

[JonathanBrouwer]

@craterbot run mode=build-and-test crates=https://crater-reports.s3.amazonaws.com/pr-154210-1/retry-regressed-list.txt p=1

[craterbot]

👌 Experiment pr-154210-2 created and queued.
🤖 Automatically detected try build dec9417
⚠️ Try build based on commit 66777ad, but latest commit is 60dd108. 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

[mati865]

I'm also unsure whether this needs t-lang input: this is clearly a bug in the compiler, where the behavior differs from the merged RFC. I'll tentatively say this is purely t-compiler territory unless the breaking changes are big.

I didn't follow the entire discussion but it's labelled as requiring a fcp from lang team.

[craterbot]

🚧 Experiment pr-154210-2 is now running

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

[craterbot]

🎉 Experiment pr-154210-2 is completed!
📊 54 regressed and 60 fixed (61350 total)
📊 1145 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-154210-2/retry-regressed-list.txt ↩

[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+.