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

Make resuming generators unsafe instead of the creation of immovable generators #49194

Merged
merged 1 commit into from
Mar 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
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
20 changes: 10 additions & 10 deletions src/doc/unstable-book/src/language-features/generators.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ fn main() {
return "foo"
};

match generator.resume() {
match unsafe { generator.resume() } {
GeneratorState::Yielded(1) => {}
_ => panic!("unexpected value from resume"),
}
match generator.resume() {
match unsafe { generator.resume() } {
GeneratorState::Complete("foo") => {}
_ => panic!("unexpected value from resume"),
}
Expand Down Expand Up @@ -69,9 +69,9 @@ fn main() {
};

println!("1");
generator.resume();
unsafe { generator.resume() };
println!("3");
generator.resume();
unsafe { generator.resume() };
println!("5");
}
```
Expand All @@ -92,7 +92,7 @@ The `Generator` trait in `std::ops` currently looks like:
pub trait Generator {
type Yield;
type Return;
fn resume(&mut self) -> GeneratorState<Self::Yield, Self::Return>;
unsafe fn resume(&mut self) -> GeneratorState<Self::Yield, Self::Return>;
}
```

Expand Down Expand Up @@ -175,8 +175,8 @@ fn main() {
return ret
};

generator.resume();
generator.resume();
unsafe { generator.resume() };
unsafe { generator.resume() };
}
```

Expand All @@ -200,7 +200,7 @@ fn main() {
type Yield = i32;
type Return = &'static str;

fn resume(&mut self) -> GeneratorState<i32, &'static str> {
unsafe fn resume(&mut self) -> GeneratorState<i32, &'static str> {
use std::mem;
match mem::replace(self, __Generator::Done) {
__Generator::Start(s) => {
Expand All @@ -223,8 +223,8 @@ fn main() {
__Generator::Start(ret)
};

generator.resume();
generator.resume();
unsafe { generator.resume() };
unsafe { generator.resume() };
}
```

Expand Down
2 changes: 1 addition & 1 deletion src/liballoc/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@ impl<T> Generator for Box<T>
{
type Yield = T::Yield;
type Return = T::Return;
fn resume(&mut self) -> GeneratorState<Self::Yield, Self::Return> {
unsafe fn resume(&mut self) -> GeneratorState<Self::Yield, Self::Return> {
(**self).resume()
}
}
12 changes: 8 additions & 4 deletions src/libcore/ops/generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,11 @@ pub enum GeneratorState<Y, R> {
/// return "foo"
/// };
///
/// match generator.resume() {
/// match unsafe { generator.resume() } {
/// GeneratorState::Yielded(1) => {}
/// _ => panic!("unexpected return from resume"),
/// }
/// match generator.resume() {
/// match unsafe { generator.resume() } {
/// GeneratorState::Complete("foo") => {}
/// _ => panic!("unexpected return from resume"),
/// }
Expand Down Expand Up @@ -98,6 +98,10 @@ pub trait Generator {
/// generator will continue executing until it either yields or returns, at
/// which point this function will return.
///
/// The function is unsafe because it can be used on an immovable generator.
/// After such a call, the immovable generator must not move again, but
/// this is not enforced by the compiler.
///
/// # Return value
///
/// The `GeneratorState` enum returned from this function indicates what
Expand All @@ -116,7 +120,7 @@ pub trait Generator {
/// been returned previously. While generator literals in the language are
/// guaranteed to panic on resuming after `Complete`, this is not guaranteed
/// for all implementations of the `Generator` trait.
fn resume(&mut self) -> GeneratorState<Self::Yield, Self::Return>;
unsafe fn resume(&mut self) -> GeneratorState<Self::Yield, Self::Return>;
}

#[unstable(feature = "generator_trait", issue = "43122")]
Expand All @@ -125,7 +129,7 @@ impl<'a, T> Generator for &'a mut T
{
type Yield = T::Yield;
type Return = T::Return;
fn resume(&mut self) -> GeneratorState<Self::Yield, Self::Return> {
unsafe fn resume(&mut self) -> GeneratorState<Self::Yield, Self::Return> {
(**self).resume()
}
}
10 changes: 5 additions & 5 deletions src/librustc_mir/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2247,7 +2247,7 @@ let mut b = || {
yield (); // ...is still in scope here, when the yield occurs.
println!("{}", a);
};
b.resume();
unsafe { b.resume() };
```

At present, it is not permitted to have a yield that occurs while a
Expand All @@ -2265,7 +2265,7 @@ let mut b = || {
yield ();
println!("{}", a);
};
b.resume();
unsafe { b.resume() };
```

This is a very simple case, of course. In more complex cases, we may
Expand All @@ -2283,7 +2283,7 @@ let mut b = || {
yield x; // ...when this yield occurs.
}
};
b.resume();
unsafe { b.resume() };
```

Such cases can sometimes be resolved by iterating "by value" (or using
Expand All @@ -2298,7 +2298,7 @@ let mut b = || {
yield x; // <-- Now yield is OK.
}
};
b.resume();
unsafe { b.resume() };
```

If taking ownership is not an option, using indices can work too:
Expand All @@ -2314,7 +2314,7 @@ let mut b = || {
yield x; // <-- Now yield is OK.
}
};
b.resume();
unsafe { b.resume() };

// (*) -- Unfortunately, these temporaries are currently required.
// See <https://github.com/rust-lang/rust/issues/43122>.
Expand Down
12 changes: 2 additions & 10 deletions src/librustc_mir/transform/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,21 +125,13 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
&AggregateKind::Array(..) |
&AggregateKind::Tuple |
&AggregateKind::Adt(..) => {}
&AggregateKind::Closure(def_id, _) => {
&AggregateKind::Closure(def_id, _) |
&AggregateKind::Generator(def_id, _, _) => {
let UnsafetyCheckResult {
violations, unsafe_blocks
} = self.tcx.unsafety_check_result(def_id);
self.register_violations(&violations, &unsafe_blocks);
}
&AggregateKind::Generator(def_id, _, interior) => {
let UnsafetyCheckResult {
violations, unsafe_blocks
} = self.tcx.unsafety_check_result(def_id);
self.register_violations(&violations, &unsafe_blocks);
if !interior.movable {
self.require_unsafe("construction of immovable generator")
}
}
}
}
self.super_rvalue(rvalue, location);
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-pass/dynamic-drop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ fn generator(a: &Allocator, run_count: usize) {
);
};
for _ in 0..run_count {
gen.resume();
unsafe { gen.resume(); }
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/test/run-pass/generator/conditional-drop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ fn t1() {
};

let n = A.load(Ordering::SeqCst);
a.resume();
unsafe { a.resume() };
assert_eq!(A.load(Ordering::SeqCst), n + 1);
a.resume();
unsafe { a.resume() };
assert_eq!(A.load(Ordering::SeqCst), n + 1);
}

Expand All @@ -58,8 +58,8 @@ fn t2() {
};

let n = A.load(Ordering::SeqCst);
a.resume();
unsafe { a.resume() };
assert_eq!(A.load(Ordering::SeqCst), n);
a.resume();
unsafe { a.resume() };
assert_eq!(A.load(Ordering::SeqCst), n + 1);
}
2 changes: 1 addition & 1 deletion src/test/run-pass/generator/control-flow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ fn finish<T>(mut amt: usize, mut t: T) -> T::Return
where T: Generator<Yield = ()>
{
loop {
match t.resume() {
match unsafe { t.resume() } {
GeneratorState::Yielded(()) => amt = amt.checked_sub(1).unwrap(),
GeneratorState::Complete(ret) => {
assert_eq!(amt, 0);
Expand Down
4 changes: 2 additions & 2 deletions src/test/run-pass/generator/drop-env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ fn t1() {
};

let n = A.load(Ordering::SeqCst);
drop(foo.resume());
drop(unsafe { foo.resume() });
assert_eq!(A.load(Ordering::SeqCst), n);
drop(foo);
assert_eq!(A.load(Ordering::SeqCst), n + 1);
Expand All @@ -50,7 +50,7 @@ fn t2() {
};

let n = A.load(Ordering::SeqCst);
drop(foo.resume());
drop(unsafe { foo.resume() });
assert_eq!(A.load(Ordering::SeqCst), n + 1);
drop(foo);
assert_eq!(A.load(Ordering::SeqCst), n + 1);
Expand Down
6 changes: 4 additions & 2 deletions src/test/run-pass/generator/issue-44197.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ fn bar2(baz: String) -> impl Generator<Yield = String, Return = ()> {
}

fn main() {
assert_eq!(bar(String::new()).resume(), GeneratorState::Yielded(String::new()));
assert_eq!(bar2(String::new()).resume(), GeneratorState::Complete(()));
unsafe {
assert_eq!(bar(String::new()).resume(), GeneratorState::Yielded(String::new()));
assert_eq!(bar2(String::new()).resume(), GeneratorState::Complete(()));
}
}
4 changes: 3 additions & 1 deletion src/test/run-pass/generator/iterator-count.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@ use std::ops::{GeneratorState, Generator};

struct W<T>(T);

// This impl isn't safe in general, but the generator used in this test is movable
// so it won't cause problems.
impl<T: Generator<Return = ()>> Iterator for W<T> {
type Item = T::Yield;

fn next(&mut self) -> Option<Self::Item> {
match self.0.resume() {
match unsafe { self.0.resume() } {
Copy link
Member

@cramertj cramertj Mar 20, 2018

Choose a reason for hiding this comment

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

This impl isn't actually sound anymore since the iterator can move-- can you leave a comment? (I think it's fine to leave it because the Pin version will come soon, but there should probably be a note about it)

GeneratorState::Complete(..) => None,
GeneratorState::Yielded(v) => Some(v),
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-pass/generator/live-upvar-across-yield.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,5 @@ fn main() {
let mut a = || {
b(yield);
};
a.resume();
unsafe { a.resume() };
}
2 changes: 1 addition & 1 deletion src/test/run-pass/generator/nested_generators.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ fn main() {
yield 2;
};

match sub_generator.resume() {
match unsafe { sub_generator.resume() } {
GeneratorState::Yielded(x) => {
yield x;
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/run-pass/generator/panic-drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ fn main() {

assert_eq!(A.load(Ordering::SeqCst), 0);
let res = panic::catch_unwind(panic::AssertUnwindSafe(|| {
foo.resume()
unsafe { foo.resume() }
}));
assert!(res.is_err());
assert_eq!(A.load(Ordering::SeqCst), 1);
Expand All @@ -57,7 +57,7 @@ fn main() {

assert_eq!(A.load(Ordering::SeqCst), 1);
let res = panic::catch_unwind(panic::AssertUnwindSafe(|| {
foo.resume()
unsafe { foo.resume() }
}));
assert!(res.is_err());
assert_eq!(A.load(Ordering::SeqCst), 1);
Expand Down
4 changes: 2 additions & 2 deletions src/test/run-pass/generator/panic-safe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ fn main() {
};

let res = panic::catch_unwind(panic::AssertUnwindSafe(|| {
foo.resume()
unsafe { foo.resume() }
}));
assert!(res.is_err());

for _ in 0..10 {
let res = panic::catch_unwind(panic::AssertUnwindSafe(|| {
foo.resume()
unsafe { foo.resume() }
}));
assert!(res.is_err());
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/run-pass/generator/resume-after-return.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,12 @@ fn main() {
yield;
};

match foo.resume() {
match unsafe { foo.resume() } {
GeneratorState::Complete(()) => {}
s => panic!("bad state: {:?}", s),
}

match panic::catch_unwind(move || foo.resume()) {
match panic::catch_unwind(move || unsafe { foo.resume() }) {
Ok(_) => panic!("generator successfully resumed"),
Err(_) => {}
}
Expand Down
Loading