8000 Evaluate place expression in `PlaceMention` by cjgillot · Pull Request #104844 · rust-lang/rust · GitHub
[go: up one dir, main page]

Skip to content

Evaluate place expression in PlaceMention #104844

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

Merged
merged 2 commits into from
Apr 22, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Evaluate place expression in PlaceMention.
  • Loading branch information
cjgillot committed Apr 21, 2023
commit ddfa2463e205a1bcae51aeb2698f09b4b8288e3d
8 changes: 5 additions & 3 deletions compiler/rustc_borrowck/src/def_use.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,16 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::UniqueBorrow) |

// `PlaceMention` and `AscribeUserType` both evaluate the place, which must not
// contain dangling references.
PlaceContext::NonUse(NonUseContext::PlaceMention) |
PlaceContext::NonUse(NonUseContext::AscribeUserTy) |
10000
Comment on lines +55 to +58
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?


PlaceContext::MutatingUse(MutatingUseContext::AddressOf) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::AddressOf) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::Inspect) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::Copy) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::Move) |
PlaceContext::NonUse(NonUseContext::AscribeUserTy) |
PlaceContext::MutatingUse(MutatingUseContext::Retag) =>
Some(DefUse::Use),

Expand All @@ -72,8 +76,6 @@ pub fn categorize(context: PlaceContext) -> Option<DefUse> {
PlaceContext::MutatingUse(MutatingUseContext::Drop) =>
Some(DefUse::Drop),

// This statement exists to help unsafeck. It does not require the place to be live.
PlaceContext::NonUse(NonUseContext::PlaceMention) => None,
// Debug info is neither def nor use.
PlaceContext::NonUse(NonUseContext::VarDebugInfo) => None,

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/invalidation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
}
// Only relevant for mir typeck
StatementKind::AscribeUserType(..)
// Only relevant for unsafeck
// Only relevant for liveness and unsafeck
| StatementKind::PlaceMention(..)
// Doesn't have any language semantics
| StatementKind::Coverage(..)
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_borrowck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,7 @@ impl<'cx, 'tcx> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtx
}
// Only relevant for mir typeck
StatementKind::AscribeUserType(..)
// Only relevant for unsafeck
// Only relevant for liveness and unsafeck
| StatementKind::PlaceMention(..)
// Doesn't have any language semantics
| StatementKind::Coverage(..)
Expand Down
10 changes: 8 additions & 2 deletions compiler/rustc_const_eval/src/interpret/step.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,14 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

Intrinsic(box intrinsic) => self.emulate_nondiverging_intrinsic(intrinsic)?,

// Statements we do not track.
PlaceMention(..) | AscribeUserType(..) => {}
// Evaluate the place expression, without reading from it.
PlaceMention(box place) => {
let _ = self.eval_place(*place)?;
}

// This exists purely to guide borrowck lifetime inference, and does not have
// an operational effect.
AscribeUserType(..) => {}

// Currently, Miri discards Coverage statements. Coverage statements are only injected
// via an optional compile time MIR pass and have no side effects. Since Coverage
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_middle/src/mir/syntax.rs
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,8 @@ pub enum StatementKind<'tcx> {
/// This is especially useful for `let _ = PLACE;` bindings that desugar to a single
/// `PlaceMention(PLACE)`.
///
/// 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.
///
/// Disallowed after drop elaboration.
Copy link
Member

Choose a reason for hiding this comment

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

Given that this sticks around for Miri, this comment does not seem to be true any more?

PlaceMention(Box<Place<'tcx>>),
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/tests/ui/option_if_let_else.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ fn else_if_option(string: Option<&str>) -> Option<(bool, &str)> {
fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
let _ = string.map_or(0, |s| s.len());
let _ = num.as_ref().map_or(&0, |s| s);
let _ = num.as_mut().map_or(&mut 0, |s| {
let _ = num.as_mut().map_or(&0, |s| {
*s += 1;
s
});
Expand All @@ -34,7 +34,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
s += 1;
s
});
let _ = num.as_mut().map_or(&mut 0, |s| {
let _ = num.as_mut().map_or(&0, |s| {
*s += 1;
s
});
Expand Down
4 changes: 2 additions & 2 deletions src/tools/clippy/tests/ui/option_if_let_else.rs
Original file line numbe F438 r Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
*s += 1;
s
} else {
&mut 0
&0
};
let _ = if let Some(ref s) = num { s } else { &0 };
let _ = if let Some(mut s) = num {
Expand All @@ -46,7 +46,7 @@ fn unop_bad(string: &Option<&str>, mut num: Option<i32>) {
*s += 1;
s
} else {
&mut 0
&0
Copy link
Member
@RalfJung RalfJung Mar 18, 2023

Choose a reason for hiding this comment

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

What are these clippy changes about?

EDIT: Oh I see, with &mut 0 these are temporaries that now lead to errors, but with &0 promotion kicks in.

};
}

Expand Down
8 changes: 4 additions & 4 deletions src/tools/clippy/tests/ui/option_if_let_else.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@ LL | let _ = if let Some(s) = &mut num {
LL | | *s += 1;
LL | | s
LL | | } else {
LL | | &mut 0
LL | | &0
LL | | };
| |_____^
|
help: try
|
LL ~ let _ = num.as_mut().map_or(&mut 0, |s| {
LL ~ let _ = num.as_mut().map_or(&0, |s| {
LL + *s += 1;
LL + s
LL ~ });
Expand Down Expand Up @@ -76,13 +76,13 @@ LL | let _ = if let Some(ref mut s) = num {
LL | | *s += 1;
LL | | s
LL | | } else {
LL | | &mut 0
LL | | &0
LL | | };
| |_____^
|
help: try
|
LL ~ let _ = num.as_mut().map_or(&mut 0, |s| {
LL ~ let _ = num.as_mut().map_or(&0, |s| {
LL + *s += 1;
LL + s
LL ~ });
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Make sure we find these even with many checks disabled.
//@compile-flags: -Zmiri-disable-alignment-check -Zmiri-disable-stacked-borrows -Zmiri-disable-validation

fn main() {
let p = {
let b = Box::new(42);
&*b as *const i32
};
let _ = unsafe { *p }; //~ ERROR: dereferenced after this allocation got freed
panic!("this should never print");
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
error: Undefined Behavior: pointer to ALLOC was dereferenced after this allocation got freed
--> $DIR/dangling_pointer_deref_underscore.rs:LL:CC
|
LL | let _ = unsafe { *p };
| ^^ pointer to ALLOC was dereferenced after this allocation got freed
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
= note: inside `main` at $DIR/dangling_pointer_deref_underscore.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to previous error

30 changes: 29 additions & 1 deletion tests/ui/borrowck/let_underscore_temporary.rs
F438
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// check-pass
// check-fail

fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
let _ = if let Some(s) = *string { s.len() } else { 0 };
Expand All @@ -8,6 +8,7 @@ fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
s
} else {
&mut 0
//~^ ERROR temporary value dropped while borrowed
};
let _ = if let Some(ref s) = num { s } else { &0 };
let _ = if let Some(mut s) = num {
Expand All @@ -21,6 +22,33 @@ fn let_underscore(string: &Option<&str>, mut num: Option<i32>) {
s
} else {
&mut 0
//~^ ERROR temporary value dropped while borrowed
};
}

fn let_ascribe(string: &Option<&str>, mut num: Option<i32>) {
let _: _ = if let Some(s) = *string { s.len() } else { 0 };
let _: _ = if let Some(s) = &num { s } else { &0 };
let _: _ = if let Some(s) = &mut num {
*s += 1;
s
} else {
&mut 0
//~^ ERROR temporary value dropped while borrowed
};
let _: _ = if let Some(ref s) = num { s } else { &0 };
let _: _ = if let Some(mut s) = num {
s += 1;
s
} else {
0
};
let _: _ = if let Some(ref mut s) = num {
*s += 1;
s
} else {
&mut 0
//~^ ERROR temporary value dropped while borrowed
};
}

Expand Down
79 changes: 79 additions & 0 deletions tests/ui/borrowck/let_underscore_temporary.stderr
| | |
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
error[E0716]: temporary value dropped while borrowed
--> $DIR/let_underscore_temporary.rs:10:14
|
LL | let _ = if let Some(s) = &mut num {
| _____________-
LL | | *s += 1;
LL | | s
LL | | } else {
LL | | &mut 0
| | ^ creates a temporary value which is freed while still in use
LL | |
LL | | };
| | -
| | |
| |_____temporary value is freed at the end of this statement
| borrow later used here
|
= note: consider using a `let` binding to create a longer lived value

error[E0716]: temporary value dropped while borrowed
--> $DIR/let_underscore_temporary.rs:24:14
|
LL | let _ = if let Some(ref mut s) = num {
| _____________-
LL | | *s += 1;
LL | | s
LL | | } else {
LL | | &mut 0
| | ^ creates a temporary value which is freed while still in use
LL | |
LL | | };
| | -
| |_____temporary value is freed at the end of this statement
| borrow later used here
|
= note: consider using a `let` binding to create a longer lived value

error[E0716]: temporary value dropped while borrowed
--> $DIR/let_underscore_temporary.rs:36:14
|
LL | let _: _ = if let Some(s) = &mut num {
| ________________-
LL | | *s += 1;
LL | | s
LL | | } else {
LL | | &mut 0
| | ^ creates a temporary value which is freed while still in use
LL | |
LL | | };
| | -
| | |
| |_____temporary value is freed at the end of this statement
| borrow later used here
|
= note: consider using a `let` binding to create a longer lived value

error[E0716]: temporary value dropped while borrowed
--> $DIR/let_underscore_temporary.rs:50:14
|
LL | let _: _ = if let Some(ref mut s) = num {
| ________________-
LL | | *s += 1;
LL | | s
LL | | } else {
LL | | &mut 0
| | ^ creates a temporary value which is freed while still in use
LL | |
LL | | };
| | -
| | |
| |_____temporary value is freed at the end of this statement
| borrow later used here
|
= note: consider using a `let` binding to create a longer lived value

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0716`.
2 changes: 0 additions & 2 deletions tests/ui/span/send-is-not-static-std-sync-2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ LL | };
error[E0597]: `x` does not live long enough
--> $DIR/send-is-not-static-std-sync-2.rs:31:25
|
LL | let (_tx, rx) = {
| --- borrow later used here
LL | let x = 1;
| - binding `x` declared here
LL | let (tx, rx) = mpsc::channel();
Expand Down
0