Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Evaluate place expression in PlaceMention #104844

Merged
merged 2 commits into from
Apr 22, 2023

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Nov 24, 2022

#102256 introduces a PlaceMention(place) MIR statement which keep trace of let _ = place statements from surface rust, but without semantics.

This PR proposes to change the behaviour of let _ = patterns with respect to the borrow-checker to verify that the bound place is live.

Specifically, consider this code:

let _ = {
    let a = 5;
    &a
};

This passes borrowck without error on stable. Meanwhile, replacing _ by _: _ or _p errors with "error[E0597]: a does not live long enough", see playground.

This PR does not change how _ patterns behave with respect to initializedness: it remains ok to bind a moved-from place to _.

The relevant test is tests/ui/borrowck/let_underscore_temporary.rs. Crater check found no regression.

For consistency, this PR changes miri to evaluate the place found in PlaceMention, and report eventual dangling pointers found within it.

r? @RalfJung

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 24, 2022
@cjgillot cjgillot marked this pull request as ready for review November 24, 2022 19:48
@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2022

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

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 in compiler/rustc_codegen_cranelift

cc @bjorn3

This PR changes MIR

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

The Miri subtree was changed

cc @rust-lang/miri

Comment on lines 121 to 119
// Evaluate the place expression, without reading from it.
PlaceMention(box place) | AscribeUserType(box (place, _), _) => {
let _ = self.eval_place(*place)?;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, AscribeUserType also does this? That is slightly surprising.

Is it possible to add tests to Miri that hit both of these cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should always be a PlaceMention before an AscribeUserType, so theoretically AscribeUserType can never fail. So I don't really know if we should check both, or just PlaceMention.

/// binding. This is especially useful for `let _ = PLACE;` bindings that desugar to a single
/// `PlaceMention(PLACE)`.
///
/// When executed at runtime this is a nop.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated.

Suggested change
/// When executed at runtime this is a nop.
/// When executed at runtime, this computes the given place, but then discards
/// it without doing a load. It is UB if the place is not pointing to live memory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AscribeUserType would probably need something similar, judging from the case you added in Miri?

Comment on lines +55 to +58
// `PlaceMention` and `AscribeUserType` both evaluate the place, which must not
// contain dangling references.
PlaceContext::NonUse(NonUseContext::PlaceMention) |
PlaceContext::NonUse(NonUseContext::AscribeUserTy) |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know borrowck, so someone else will have to review this part.
But doesn't this do a check that the place is actually initialized? Just to avoid UB it would be sufficient to check that the place is live, but maybe borrowck does not have such a check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking that the place is initialized will break a lot of code, because we allow binding moved-from places to _. For instance in test src/test/ui/binding/issue-53114-borrow-checks.rs.

Copy link
Member

@RalfJung RalfJung Nov 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes an init check is not needed. I just thought DefUse::Use does an init check (since usually when we use a place, we load from it, and it must be init). But it seems I am wrong about that?

@bors

This comment was marked as resolved.

@cjgillot cjgillot changed the title Evaluate place expression in PlaceMention and AscribeUserType Evaluate place expression in PlaceMention Nov 27, 2022
@bors

This comment was marked as resolved.

@RalfJung RalfJung added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2022
@RalfJung
Copy link
Member

Marking as blocked on #102256

@cjgillot cjgillot force-pushed the mention-eval-place branch 2 times, most recently from 7c62ed8 to fd1a0fc Compare January 11, 2023 17:30
@bors

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@bors

This comment was marked as resolved.

@cjgillot cjgillot force-pushed the mention-eval-place branch 2 times, most recently from 37219ed to ac2ba0e Compare February 28, 2023 17:38
@bors

This comment was marked as resolved.

@cjgillot cjgillot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 15, 2023
@bors
Copy link
Contributor

bors commented Apr 19, 2023

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 19, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 22, 2023
@rfcbot
Copy link

rfcbot commented Apr 22, 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.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Apr 22, 2023
@cjgillot
Copy link
Contributor Author

@bors r=jackh726,RalfJung

@bors
Copy link
Contributor

bors commented Apr 22, 2023

📌 Commit 2870d26 has been approved by jackh726,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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 22, 2023
@bors
Copy link
Contributor

bors commented Apr 22, 2023

⌛ Testing commit 2870d26 with merge 21fab43...

@bors
Copy link
Contributor

bors commented Apr 22, 2023

☀️ Test successful - checks-actions
Approved by: jackh726,RalfJung
Pushing 21fab43 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 22, 2023
@bors bors merged commit 21fab43 into rust-lang:master Apr 22, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 22, 2023
@cjgillot cjgillot deleted the mention-eval-place branch April 22, 2023 12:40
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (21fab43): 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)
3.4% [3.4%, 3.4%] 1
Improvements ✅
(primary)
-2.1% [-2.1%, -2.1%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.1% [-2.1%, -2.1%] 1

Cycles

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.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) - - 0

flip1995 pushed a commit to flip1995/rust that referenced this pull request May 5, 2023
…726,RalfJung

Evaluate place expression in `PlaceMention`

rust-lang#102256 introduces a `PlaceMention(place)` MIR statement which keep trace of `let _ = place` statements from surface rust, but without semantics.

This PR proposes to change the behaviour of `let _ =` patterns with respect to the borrow-checker to verify that the bound place is live.

Specifically, consider this code:
```rust
let _ = {
    let a = 5;
    &a
};
```

This passes borrowck without error on stable. Meanwhile, replacing `_` by `_: _` or `_p` errors with "error[E0597]: `a` does not live long enough", [see playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c448d25a7c205dc95a0967fe96bccce8).

This PR *does not* change how `_` patterns behave with respect to initializedness: it remains ok to bind a moved-from place to `_`.

The relevant test is `tests/ui/borrowck/let_underscore_temporary.rs`. Crater check found no regression.

For consistency, this PR changes miri to evaluate the place found in `PlaceMention`, and report eventual dangling pointers found within it.

r? `@RalfJung`
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 25, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 27, 2023
…RalfJung

Allow partially moved values in match

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:
```rust
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 ~rust-lang#102256 ~rust-lang#104844

Fixes rust-lang#99180 rust-lang#53114
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Oct 28, 2023
Allow partially moved values in match

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:
```rust
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 ~rust-lang/rust#102256 ~rust-lang/rust#104844

Fixes rust-lang/rust#99180 rust-lang/rust#53114
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. 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. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.