Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upUse pinning for generators to make trait safe #55704
+465
−201
Conversation
rust-highfive
assigned
dtolnay
Nov 5, 2018
This comment has been minimized.
This comment has been minimized.
|
r? @dtolnay (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
label
Nov 5, 2018
This comment has been minimized.
This comment has been minimized.
rust-highfive
assigned
withoutboats
and unassigned
dtolnay
Nov 5, 2018
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
- error[E0277]: the trait bound `[static generator@$DIR/static-not-unpin.rs:19:25: 21:6 _]: std::marker::Unpin` is not satisfied
+ error[E0277]: the trait bound `[static generator@$DIR/static-not-unpin.rs:19:25: 21:6 _]: std::pin::Unpin` is not satisfiedIt definitely mentions |
This comment has been minimized.
This comment has been minimized.
|
And testing on an |
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
So, that caused my new test case to pass, but broke a lot of other cases that relied on the previous ordering. I would have thought that the default ordering would be by declaration order, but I don't know how that could be different between my machines and travis'. |
Nemo157
force-pushed the
Nemo157:pinned-generators
branch
from
ded732d
to
dea099a
Nov 6, 2018
This comment has been minimized.
This comment has been minimized.
|
So this is basically what we always meant to do, just at the time Zoxc implemented the previous iteration of the generator API, std API changes look good. @bors r? @eddyb can you review the compiler changes here and also confirm that the generator transform doesn't need to be changed with this PR |
rust-highfive
assigned
eddyb
and unassigned
withoutboats
Nov 6, 2018
This comment has been minimized.
This comment has been minimized.
|
The generator transform should use cc @Zoxc |
This comment has been minimized.
This comment has been minimized.
|
I think I can see how to update the transform. At a guess it currently works because the output MIR is self-consistent with what type it thinks the generator argument is, and the type it thinks the argument is is layout compatible with the actual type passed in. There must not be any validation of the signature after the transform happens. |
This comment has been minimized.
This comment has been minimized.
|
I suggesting replacing |
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
I think I have to also update the debug-info generation: rust/src/librustc_codegen_llvm/mir/mod.rs Lines 593 to 598 in ddd4b19 I have a patch to do that, but I still have to figure out how to test it. There appear to be some debuginfo tests, but nothing covering generators, and when I run the existing tests I get EDIT: Found the gdb issue, need to sign it for macOS, guess I just need to look into how to write the test for it now. |
src/librustc_codegen_llvm/mir/mod.rs Outdated
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| // option. This file may not be copied, modified, or distributed | ||
| // except according to those terms. | ||
|
|
||
| // normalize-stderr-test "std::pin::Unpin" -> "std::marker::Unpin" |
This comment has been minimized.
This comment has been minimized.
Nemo157
Nov 8, 2018
Contributor
I have not been able to figure out why travis shows a different path for this compared to the two machines with rustc I've been able to test on. I attempted to trace through where the diagnostics get the path from, and as far as I can tell everything should be deterministic. The path chosen appears to come from the visible_parent_map built here, this is done as a BFS, and should be deterministic within a single crate. Following the order of an items children back from here through the different representations all the way to libsyntax there doesn't appear to be anything that should change the order, then in libsyntax an items children come out in parse order. std re-exports the core::marker module before the core::pin module, so the path chosen to be displayed should be std::marker::Pin.
Parse/declaration order is also consistent with the failures in https://travis-ci.org/rust-lang/rust/jobs/451369080, that test commit forced the child order while constructing visible_parent_map to be sorted by name. All the failures were swapping paths that were declared earlier with paths declared later but alphabetically sorted earlier.
This will also become moot soon as #55766 is proposing to remove the std::pin::Unpin re-export (although, it does plan to add it to the prelude which will then become the preferred path).
This comment has been minimized.
This comment has been minimized.
|
Travis appears to have found all the test cases I didn't... Other than the comment I just left this is complete as far as I'm aware. I'd prefer to land this with that workaround and I'll open an issue about the non-deterministic diagnostics. Let me know whether there's anything else and I can squash all the fixups in. |
Nemo157
referenced this pull request
Nov 9, 2018
Open
Tracking issue for RFC 2033: Experimentally add coroutines to Rust #43122
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
r? @Zoxc |
rust-highfive
assigned
Zoxc
and unassigned
eddyb
Nov 15, 2018
This comment has been minimized.
This comment has been minimized.
|
@Nemo157 Did you print the MIR output of the generator transformation to check that it is now correct and no longer just happens to work? |
This comment has been minimized.
This comment has been minimized.
|
@Zoxc yep, here's one MIR dump I had laying around still. Once I changed the generator transform itself I actually got an assertion during codegen because the signature didn't match, that's how I found the signature being generated here and updated that as well. So I'm fairly confident it's all lining up correctly now. |
src/librustc/traits/select.rs
Outdated
| if lang_items.unpin_trait() == Some(def_id) { | ||
| // OK to skip binder because the substs on generator types never | ||
| // touch bound regions, they just capture the in-scope | ||
| // type/region parameters |
This comment has been minimized.
This comment has been minimized.
Zoxc
Nov 15, 2018
Contributor
It is ok to skip the binder here because we aren't looking at any regions, just if the generator is immovable.
I'm not sure this is the correct place for this check. I'd expect it to be in some auto trait related code, creating !Unpin impls for immovable generators. This may have the same effect though.
This comment has been minimized.
This comment has been minimized.
Nemo157
Nov 15, 2018
Contributor
Updated comment.
That's what I was thinking of initially, but I couldn't find anything similar to that (if you have some pointers for where to look I can take another look at it). This definitely gives the correct error message.
This comment has been minimized.
This comment has been minimized.
Nemo157
Nov 15, 2018
Contributor
Actually, I think I've found a better place to put this. In assemble_candidates_from_auto_impls I can just suppress adding a candidate for static generators when the trait is Unpin. I'll check whether that works now.
This comment has been minimized.
This comment has been minimized.
Zoxc
Nov 15, 2018
Contributor
@nikomatsakis @arielb1 Where would the correct place to put this be? Or does it not matter since the user can't write impls since generators are unnameable?
| set_task_waker(lw, || match unsafe { Pin::get_mut_unchecked(self).0.resume() } { | ||
| // Safe because we're !Unpin + !Drop mapping to a ?Unpin value | ||
| let gen = unsafe { Pin::map_unchecked_mut(self, |s| &mut s.0) }; | ||
| set_task_waker(lw, || match gen.resume() { |
This comment has been minimized.
This comment has been minimized.
| use std::ops::Generator; | ||
|
|
||
| fn msg() -> u32 { | ||
| 0 | ||
| } | ||
|
|
||
| pub fn foo() -> impl Generator<Yield=(), Return=u32> { | ||
| pub fn foo() -> impl Generator<Yield=(), Return=u32> + Unpin { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Nemo157
Nov 15, 2018
Contributor
Unpin is necessary to use Pin::new, otherwise the tests would need to use the unsafe Pin::new_unchecked (this may not actually be necessary with impl Trait, Unpin probably leaks like other auto-traits, I didn't check that).
This comment has been minimized.
This comment has been minimized.
Zoxc
Nov 15, 2018
Contributor
Yeah, I was expecting Unpin to leak here, so this change shouldn't be necessary.
This comment has been minimized.
This comment has been minimized.
| || { | ||
| if false { | ||
| yield; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn bar<T: 'static>(t: T) -> Box<Generator<Yield = T, Return = ()>> { | ||
| pub fn bar<T: Unpin + 'static>(t: T) -> Box<Generator<Yield = T, Return = ()> + Unpin> { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Nemo157
Nov 15, 2018
Contributor
I think that's necessary because non-static generators are still subject to the normal structural Unpin checks. Rebuilding without it now to check (will be a while as I've got to do a fresh build).
This comment has been minimized.
This comment has been minimized.
Nemo157
Nov 15, 2018
Contributor
Yep, without it gives:
| |______^ within `[generator@/Users/Nemo157/sources/rust/src/test/run-pass/generator/auxiliary/xcrate.rs:25:14: 27:6 t:T _]`, the trait `std::marker::Unpin` is not implemented for `T`
This comment has been minimized.
This comment has been minimized.
Zoxc
Nov 15, 2018
Contributor
Is that correct? Should movable generators always implement Unpin? I why is Unpin an auto trait? I haven't looked at this stuff in a while =P
This comment has been minimized.
This comment has been minimized.
Nemo157
Nov 16, 2018
Contributor
So, I haven't spent time verifying this, but having non-static generators use the structural definition of pinning makes this sound code:
fn foo() -> impl Generator<Yield = usize, Return = ()> {
static || {
let x = 5;
let x = &x;
yield *x;
yield *x;
}
}
fn bar() -> impl Generator<Yield = usize, Return = ()> {
|| {
let mut foo = foo();
loop {
let val = {
let mut foo = unsafe { Pin::new_unchecked(&mut foo) };
match foo.resume() {
GeneratorState::Yielded(val) => val,
GeneratorState::Complete(()) => break,
}
};
yield val;
}
}
}I don't know if there would be a way to make this safe code, the stack pinning macro would require borrows to persist across yields, but maybe something like this would be useful?
It also seems to me that this should be investigated for both closures and generators at the same time, they likely have similar reasons to either propagate or force Unpin.
@withoutboats any opinion (and does closures interaction with Unpin matter for stabilization of Pin)?
This comment has been minimized.
This comment has been minimized.
withoutboats
Nov 16, 2018
•
Contributor
There should be some way to make generators which are checked for Unpin, so they can be moved in and out of pins (and thus moved between resume calls). Currently its not annotating the generator static.
It might not be correct for that behavior to check that their members are unpin, maybe it only should check that they don't themselves borrow across yields, and omit the generated negative impl of unpin.
I think the current closure behavior around Unpin is correct in that it is consistent with other auto traits like Send etc; if anything should change (not sure that it should) it would be generator syntax.
This comment has been minimized.
This comment has been minimized.
Nemo157
Nov 16, 2018
•
Contributor
maybe it only should check that they don't themselves borrow across yields, and omit the generated negative impl of unpin.
That's basically what is happening now, for static annotated generators there's an implicit negative impl of Unpin, for other generators there's no implicit impl (either positive or negative) and it's falling back to structural checking of the upvars and state that is live across yield points (the closed over environment).
We could force a positive impl of Unpin for non-static generators instead and say that it's unsound to create a pinned reference into the environment, even just between yield points. You would still be able to create pinned references to values that are only live between yield points since they are true stack variables. And if you need pinned references into the environment you can force the generator to be immovable via static. As far as I'm aware this wouldn't remove any capabilities; a non-static generator that is !Unpin because of structural recursion and a static generator appear the same externally, and when implementing the generator you always have the choice to add or remove the static annotation
Thinking about it a bit more I guess this doesn't matter for closure's as we don't have FnPin, there's no way to call a closure such that it could soundly create a pinned reference into its environment (EDIT: other than FnOnce, but at that point you can't move the closure after calling anyway).
This comment has been minimized.
This comment has been minimized.
Nemo157
Nov 21, 2018
Contributor
I've added an unconditional implementation of Unpin for movable generators.
| assert_eq!(generator.resume(), GeneratorState::Yielded(())); | ||
| assert_eq!(generator.resume(), GeneratorState::Complete(())); | ||
| } | ||
| let mut generator = unsafe { Pin::new_unchecked(&mut generator) }; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Nemo157
Nov 16, 2018
•
Contributor
Constructing a Pin to a reference to !Unpin value is unsafe. There is a safe macro-based solution to do so on the stack, but it's not included in std. I could use a Pin<Box> here instead safely, but I thought better to just keep it on the stack.
I can add a note about why this is safe (same reason as pin_mut! is safe, it shadows the owning binding so the value cannot be moved after this line).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
Nemo157
force-pushed the
Nemo157:pinned-generators
branch
2 times, most recently
from
aae512c
to
10fccf0
Nov 21, 2018
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
Nemo157
added some commits
Oct 4, 2018
Nemo157
force-pushed the
Nemo157:pinned-generators
branch
from
10fccf0
to
84363df
Dec 10, 2018
This comment has been minimized.
This comment has been minimized.
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Nemo157 commentedNov 5, 2018
•
edited
I'm unsure whether there needs to be any changes to the actual generator transform. Tests are passing so the fact that
Pin<&mut T>is fundamentally the same as&mut Tseems to allow it to still work, but maybe there's something subtle here that could go wrong.This is specified in RFC 2349 § Immovable generators (although, since that RFC it has become safe to create an immovable generator, and instead it's unsafe to resume any generator; with these changes both are now safe and instead the unsafety is moved to creating a
Pin<&mut [static generator]>which there are safe APIs for).CC #43122