Skip to content

Allow partially moved values in match#103208

Merged
bors merged 6 commits intorust-lang:masterfrom
cjgillot:match-fake-read
Oct 27, 2023
Merged

Allow partially moved values in match#103208
bors merged 6 commits intorust-lang:masterfrom
cjgillot:match-fake-read

Conversation

@cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Oct 18, 2022

This PR attempts to unify the behaviour between let _ = PLACE, let _: TY = PLACE; and match PLACE { _ => {} }.
The logical conclusion is that the match version should not check for uninitialised places nor check that borrows are still live.

The match PLACE {} case is handled by keeping a FakeRead in the unreachable fallback case to verify that PLACE has a legal value.

Schematically, match PLACE { arms } in surface rust becomes in MIR:

PlaceMention(PLACE)
match PLACE {
  // Decision tree for the explicit arms
  arms,
  // An extra fallback arm
  _ => {
    FakeRead(ForMatchedPlace, PLACE);
    unreachable
  }
}

match *borrow { _ => {} } continues to check that *borrow is live, but does not read the value.
match *borrow {} both checks that *borrow is live, and fake-reads the value.

Continuation of #102256 #104844

Fixes #99180 #53114

@cjgillot cjgillot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 18, 2022
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 18, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 18, 2022

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

@rust-highfive
Copy link
Contributor

r? @eholk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Oct 18, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@pnkfelix
Copy link
Contributor

The lang team wants to have a design meeting about PR #102256 and PR #103208 ; @cjgillot I propose that you and I work together to make a design doc about these changes in terms of what is surfaced at the language level.

@Mark-Simulacrum Mark-Simulacrum removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 25, 2022
@scottmcm
Copy link
Member

On the let _: Ty = b; example:

What happens there when it's more than just an ascription because it's a coercion site?

For example,

let b: Box<String> = …;
let _: Box<dyn Display> = b;
return b;

I guess that the coercion becomes an explicit statement in MIR, so that would still be rejected even though the AscribeUserType in MIR wouldn't be considered a use any more?

@eholk
Copy link
Contributor

eholk commented Oct 25, 2022

I'm probably not the best person to review this, so I'll re-roll.

r? @rust-lang/compiler

@rust-highfive rust-highfive assigned lcnr and unassigned eholk Oct 25, 2022
@bors
Copy link
Collaborator

bors commented Oct 31, 2022

☔ The latest upstream changes (presumably #103787) made this pull request unmergeable. Please resolve the merge conflicts.

@lcnr
Copy link
Contributor

lcnr commented Oct 31, 2022

r? @oli-obk maybe 🤔 cc @RalfJung

@rustbot rustbot assigned oli-obk and unassigned lcnr Oct 31, 2022
@oli-obk oli-obk added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Oct 31, 2022
@bors
Copy link
Collaborator

bors commented Nov 17, 2022

☔ The latest upstream changes (presumably #103138) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 29, 2022

Triage: This is still waiting on #102256

@Dylan-DPC Dylan-DPC removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 14, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jan 1, 2023

The Miri subtree was changed

cc @rust-lang/miri

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 26, 2023
@rfcbot
Copy link

rfcbot commented Oct 26, 2023

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.

This will be merged soon.

@RalfJung
Copy link
Member

The Miri tests LGTM. @oli-obk wrote

The impl lgtm.

but that was in May. Oli if you could briefly skim this again to make sure it still looks good to you that would be great. :)

@oli-obk
Copy link
Contributor

oli-obk commented Oct 27, 2023

Did another review of the impl and mir opt test changes. Nothing in mir or mir building has changed that would affect these changes, and they look good to me (I don't remember them from the last review, so this was a review from scratch 😅)

@bors r=oli-obk,RalfJung

@bors
Copy link
Collaborator

bors commented Oct 27, 2023

📌 Commit 479fb4b has been approved by oli-obk,RalfJung

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-team labels Oct 27, 2023
@bors
Copy link
Collaborator

bors commented Oct 27, 2023

⌛ Testing commit 479fb4b with merge 59bb950...

@bors
Copy link
Collaborator

bors commented Oct 27, 2023

☀️ Test successful - checks-actions
Approved by: oli-obk,RalfJung
Pushing 59bb950 to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (59bb950): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.9% [1.2%, 3.5%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 635.347s -> 636.148s (0.13%)
Artifact size: 304.54 MiB -> 304.51 MiB (-0.01%)

@RalfJung
Copy link
Member

It took more than a year, but it's finally done. :) Thanks a lot @cjgillot for seeing this through to the end!

@WaffleLapkin
Copy link
Member

Should we add relnotes Marks issues that should be documented in the release notes of the next release. here? Might be useful to include in the release notes that we now accept more code

@RalfJung
Copy link
Member

Sure, why not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-opsem Relevant to the opsem team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MIR building optimizes away some place expressions around "_" patterns even with -Zmir-opt-level=0