Repository navigation
[nll] _ patterns should not count as borrows #53114
Description
Activity
OK, the code is sort of inconsistent right now with respect to what
let _ = foopermits. Here is my analysis of the current state:Thing AST MIR Example let _ = <unsafe-field>💚 💚 playground match <unsafe_field> { _ => () }❌ ❌ playground let _ = <moved>💚 💚 playground match <moved> { _ => () }❌ ❌ playground let _ = <borrowed>💚 💚 playground match <borrowed> { _ => () }💚 ❌ playground The explanations for what is going on here in the code are:
let _ = ...processes the pattern in a simplified, irrefutable mode during MIR processing. This mode does not insert the dummy accesses that the match mode does (because there is no need, generally speaking). This means thatlet _ = some.pathis always a no-op and just doesn't appear in the MIR at all.match <place> { _ => () }in contrast uses the "match mode" which can handle any number of arms.- Unsafe code checking happens on MIR, which I now think is mildly wrong (HAIR seems correct to me)
I'm not 100% sure why the old borrow check acepts
match <borrowed> { _ => () }; the EUV does seem to issue a "match discriminant" borrow formatch <..>. I'd have to dig in more.Reacted by scottmcm, runiq and Volodymyr LisivkaGiven the chart above, it seems like we could call the current behavior of NLL a kind of "bug fix" when it comes to
match, though I personally find the inconsistency withlet _ = <borrowed>disquieting.cc @rust-lang/lang -- I'd like opinions on what behavior we think should happen around
_patterns. In general, my mental model has been that_patterns are "no-ops". They do not "access or touch" the value that they are matching against. And that is certainly true some of the time. For example, we accept this code:fn main() { let foo = (Box::new(22), Box::new(44)); match foo { (_, y) => () } drop(foo.0); // ok, foo.0 was not moved }
However, we seem to require that you can only
matchon things are that are "fully present" and not (e.g.) partially moved. Hence it is an error to swap thedropandmatchin that example.This requirement that the match discriminant be valid is actually consistent with NLL's view on matching: NLL views the match desugaring as first borrowing the value that we are going to match upon (with a shared borrow). This borrow persists until an arm is chosen. This prevents match guards from doing weird stuff like mutating the value we are matching on. Requiring the match discriminant to be valid is also consistent with some of the discussions we've had about how to think about exhaustiveness and matches with zero arms like
match x { }.That said I also think that
let _ = ...andmatch <block> { _ => () }should be equivalent. So perhaps we should "fix" the behavior oflet_ = ...? Or do we special-casematchexpressions with a single arm to say that they do not have a "tracking borrow" (in which case we would accept the examples above).I would also -- personally -- potentially draw a distinction between the "unsafe check" and the checks for what data is moved. I don't think of the unsafe check as a "flow sensitive" check but rather something very simple -- if you access the field, even in dead code, you must be in an unsafe block. Hence I am not too thrilled that
let _ = <unsafe fields>would type-check, and I consider that a regression from the MIR-based borrow check. In fact, this is basically why I don't think that MIR is a good fit for unsafe checking (HAIR would be better).Reacted by scottmcm and Taylor CramerWhile I've learned that
_is a complete no-op, I've always found it weird thatlet _ = mutex.lock().unwrap();
and
let _guard = mutex.lock().unwrap();
do fundamentally different things.
I expected
_to behave as if the compiler gave it a name, like how argument-position'_works.Reacted by runiq, Volodymyr Lisivka and Dan Aloni@scottmcm understood — but I think too late to change that. =)
Update: (It would also mean that there is no way to extract some fields of a struct without moving the rest... unless we had some other pattern for "don't touch")
Reacted by scottmcm and runiqThis is what I personally think the table should look like:
Thing AST MIR Example let _ = <unsafe-field>❌ ❌ playground match <unsafe_field> { _ => () }❌ ❌ playground let _ = <moved>💚 💚 playground match <moved> { _ => () }💚 💚 playground let _ = <borrowed>💚 💚 playground match <borrowed> { _ => () }💚 💚 playground My reasoning is this:
- I think "unsafe checks" are not "flow-sensitive" checks -- they are just based on what you do and where, not whether the code is reachable etc.
- On the other hand, initialization and borrow checkers are flow-sensitive, and hence
let _ =is known not to touch the thing that is being matched against (same withmatchwith one_pattern).- This is concordant with the "most likely" way to understand exhaustiveness, I think, which is that
match foo { _ => () }basically is a non-access and hence cannot be UB.
- This is concordant with the "most likely" way to understand exhaustiveness, I think, which is that
I think the way I would implement this in MIR desugaring is:
- Move unsafe checking to operate on HAIR, I guess, or else add some kind of "unsafe_check()" instruction that hangs around just so that the unsafe checker can see it.
- If you are lowering a match with one arm, then use the "irrefutable" logic for handling it (since the pattern must be irrefutable) -- this will, in turn, mean that
match foo { _ => () }doesn't add anything extra- In particular, it doesn't add the implicit borrows or other accesses that would otherwise result
This is not entirely "backwards compatible", but accepting
let _ = <unsafe field>is a regression in any case, one which was incompletely fixed. In general the move to doing unsafe checking on MIR led to a number of complications around dead code, which is why I would prefer to move away from that.Reacted by Vadim Petrochenkov, Taylor Cramer and Mazdak Farrokhzad@nikomatsakis so I think the packed field access checking needs to be on MIR (to have access to the final place operations), but raw pointer dereferences and union field accesses could trivially be on HAIR (and we should maybe also move some linting from HIR to HAIR).
so I think the packed field access checking needs to be on MIR (to have access to the final place operations)
Hmm, that's surprising to me. Maybe I'm forgetting which bits of desugaring are done on HAIR vs MIR -- I would think that the field accesses would be quite easy to spot. @eddyb can you give me an example of where MIR would be better?
@nikomatsakis Patterns are probably the biggest thing that still exists in HAIR but not MIR. But if we have types in every node then it's probably plausible to handle it.
@eddyb ah, I see, you mean that we'd have to check in two places basically? (e.g., struct patterns)
Seems true, but yes since the patterns themselves are fully explicit and have types also like it's not that hard to handle.
60 remaining items
discussed with @Centril during pre-triage.
At this point, I think we should put P-high priority on ensuring the tests are present (or add them); its embarrassing that sat so long.
as for the behavior change for
match.... I probably figure that's still P-medium.Marking P-high to reflect the above.
Ah, look at this, I'm the one who marked it as P-high and explicitly said that the behavior change for match itself is P-medium
@rustbot label: -E-needs-test
As noted in the table in the issue description, there are essentially two tasks that remain here:
- change
let _ = <unsafe-field>to be rejected instead of accepted. This has its own dedicated issue inlet _ = <access to unsafe field>currently type-checks #54003 - change
match <moved> { _ => () }to be accepted instead of rejected. This does not have its own dedicated issue, but its a pretty niche feature request and seems easy to categorize as P-medium.
- change
- added a commit that references this issue
on Oct 28, 2023
Historically, we have considered
let _ = footo be a no-op. That is, it does not read or "access"fooin any way. This is why the following code compiles normally. However, it does NOT compile with NLL, because we have an "artificial read" of the matched value (or so it seems):Found in liner-0.4.4.
I believe that we added this artificial read in order to fix #47412, which had to do with enum reads and so forth. It seems like that fix was a bit too strong (cc @eddyb).
UPDATE: Current status as of 2018-10-02
let _ = <unsafe-field>match <unsafe_field> { _ => () }let _ = <moved>match <moved> { _ => () }let _ = <borrowed>match <borrowed> { _ => () }