Introduce a no-op PlaceMention statement for let _ =.

[cjgillot]

Fixes #54003
Fixes #80059
Split from #101500

This PR introduces a new PlaceMention statement dedicated to matches that neither introduce bindings nor ascribe types. Without this, all traces of the match would vanish from MIR, making it impossible to diagnose unsafety or use in #101500.

This allows to mark let _ = <unsafe union access or dereference> as requiring an unsafe block.
Nominating for lang team, as this introduces an extra error.

[rust-highfive]

r? @TaKO8Ki

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

[rustbot]

This PR changes MIR

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

[lukas-code]

Does this also fix #80059?

[cjgillot]

@lukas-code, yes, I added a test.

[RalfJung]

This sounds great!

For Miri's rust-lang/miri#2360 it'd be awesome if those FakeRead could stick around in MIR a bit longer than they currently do (i.e., the pass that removes them should not be run with -Zmir-opt-level=0). Then we can also detect UB from incorrect union accesses / derefs for a _ place. (But if it's better to do that in a future PR, that's also fine.)

[JakobDegen]

We currently spec FakeRead as being a semantic nop; we could make an exception for the ForWildcard case, but it might be cleaner to have a new StatementKind::PlaceComputation or something like that

[cjgillot]

I made this a FakeRead because that's was the easiest. If a dedicated statement is better, I can add one. How should it be named?

[cjgillot]

We currently spec FakeRead as being a semantic nop; we could make an exception for the ForWildcard case, but it might be cleaner to have a new StatementKind::PlaceComputation or something like that

@JakobDegen I don't see the issue. FakeRead is an analysis helper for borrowck and unsafeck, and this PR uses it like this.

[JakobDegen]

I don't see the issue. FakeRead is an analysis helper for borrowck and unsafeck, and this PR uses it like this.

Sorry, that was in response to Ralf's comment about rust-lang/miri#2360 . For the purposes in this PR what you have is completely fine (in other words, if we wanted to use this to fix the miri issue it would stop being an analysis helper)

[est31]

I've tried the example in #80059 for destructuring assignments to _ (so I removed the let to have fn foo(ptr: *const bool) { _ = *ptr; }), and there are similarly no errors. Does the PR fix the situation there too?

[cjgillot]

@est31 it does. Added to the test.

[est31]

@cjgillot thanks! Would it be useful to add it to the test for #54003 as well?

[cjgillot]

@est31 this is the test for #54003.

[est31]

I meant one with _ = only, without a let.

[pnkfelix]

We talked about this in the T-lang meeting.

The main concern, from the language side, was that we had carefully calibrated a boundary for what kinds of things should be considered "syntactic checks" (checking for missing unsafe { ... } is an instance of that), vs "flow analysis" (checking that your variable is initialized is an instance of that).
So that leads to an assumption that unsafeck should "naturally" be performed on something that reflects the structure of the AST, like THIR.

• A contributor even spent some time on trying to port the unsafeck to THIR (or perhaps HIR, not sure from comments)
• Anyway that work (Port improved unused_unsafe check to -Zthir_unsafeck #99379) was never finished, and the PR was abandoned.

So, there's an architectural question for T-compiler: I believe this PR is adding something useful today, and so I suspect we should land it.

But: Do we think there's any chance we'll resurrect #99379, or otherwise try to do unsafeck on THIR, in the near term? I would like an answer from T-compiler about that before moving forward on this PR.

@rustbot label I-compiler-nominated

[cjgillot]

When I originally wrote this commit, I did not intend it to be useful for unsafeck. My focus was #101500, which is "flow analysis".

While I agree with the syntactic/dataflow separation between THIR and MIR, I still think that this PR is useful:

• it solves the linked issues now, without having to wait for the better THIR-based unsafeck;
• it (possibly) eases the transition to THIR-based unsafeck, by removing a difference in behaviour;
• it adds the necessary information to MIR for Perform unused assignment and unused variables lints on MIR. #101500 to happen.

[RalfJung]

But: Do we think there's any chance we'll resurrect #99379, or otherwise try to do unsafeck on THIR, in the near term? I would like an answer from T-compiler about that before moving forward on this PR.

If we do, then this PR just means MIR and THIR unsafety checking are more in line with each other, right? I don't quite understand what the concern is here.

[RalfJung]

While I agree with the syntactic/dataflow separation between THIR and MIR, I still think that this PR is useful:

I think this PR is crucial since it is a first step towards resolving rust-lang/miri#2360.

[RalfJung]

Btw, this will also fix #79735, won't it?

[cjgillot]

Btw, this will also fix #79735, won't it?

It should. I'll test it when I get my laptop working again.

A bit more problematic: this PR also introduces an error in this snippet, which currently passes borrow checking.

fn foo(mut n: Option<usize>) {
    let _ = if let Some(ref mut s) = n {
        s
    } else {
        &mut 0
        //~^ ERROR temporary value dropped while borrowed
    };
}

This error already happens for let _a = and match { ... } { _ => {} }. Should the error be kept or worked around?

[cjgillot]

Updated coverage info.
@bors r=lcnr

[bors]

📌 Commit 684de04 has been approved by lcnr

It is now in the queue for this repository.

[bors]

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

[bors]

⌛ Testing commit 684de04 with merge d583342...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing d583342 to master...

[rust-timer]

Finished benchmarking commit (d583342): 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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.3%, -2.1%]	2
All ❌✅ (primary)	-	-	0

Cycles

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