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 generator types always Freeze. #65783

Closed
wants to merge 1 commit into from

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented Oct 24, 2019

When working on #57017, I discovered that generators are currently
considered Freeze based on the witness types they contain (just like
any other auto trait). That is, a generator which contains a !Freeze
type (e.g. Cell<T>) will also be !Freeze.

However, I believe that it is sound to always treat generator types as
Freeze, regardless of what types they may store internally. The only
possible of interacting with a generator is through the resume method
of the generator trait. resume takes a Pin<&mut Self>, which
requires a &mut Self to be created. In other words, given only a
shared reference to a generator (&{generator}), it is impossible to
mutate the generator. Note that is true regardless of whether or not
types inside the generator have interior mutability - they can only
change when resume is called, which is impossible to call using a
shared reference.

The motivation for this PR is to support further work on #57017. My
approach is to delay resolution of auto-trait predicates (e.g.
{generator}: Send) until after we've constructed the generator MIR
(specifically after we've run the StateTransform MIR transformation
pass). However, the const qualification pass (which runs before
StateTransform) explicitly checks whether types are Freeze. There are
several ways of resolving this:

  1. Refactor const-qualification to avoid the need to check whether
    generators are Freeze. This might involve running (part of ) const
    qualification after the StateTransform pass runs.
  2. Use the current, more conservative approach for generator when
    checking Freeze - that is, use the witness types computed from the
    HIR.
  3. Make generators always Freeze.

Option 1 and 2 introduce a large amount of additional complexity for the
sole purpose of supporting generators. Option 3 (this PR), requires only
a minor change to SelectionContext. Theoretically, it could also
improve performance, since we now perform less work when checking for
Freeze types.

This should probably have a test, but I'm not really sure how to write
one, considering that Freeze is private.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 24, 2019
When working on rust-lang#57017, I discovered that generators are currently
considered `Freeze` based on the witness types they contain (just like
any other auto trait). That is, a generator which contains a `!Freeze`
type (e.g. `Cell<T>`) will also be `!Freeze`.

However, I believe that it is sound to always treat generator types as
`Freeze`, regardless of what types they may store internally. The only
possible of interacting with a generator is through the `resume` method
of the generator trait. `resume` takes a `Pin<&mut Self>`, which
requires a `&mut Self` to be created. In other words, given only a
shared reference to a generator (`&{generator}`), it is impossible to
mutate the generator. Note that is true regardless of whether or not
types *inside* the generator have interior mutability - they can only
change when `resume` is called, which is impossible to call using a
shared reference.

The motivation for this PR is to support further work on rust-lang#57017. My
approach is to delay resolution of auto-trait predicates (e.g.
`{generator}: Send)` until after we've constructed the generator MIR
(specifically, after we've run the `StateTransform` MIR transformation
pass). However, the const qualification pass (which runs before
`StateTransform`) explictly checks whether types are `Freeze`. There are
several ways of resolving this:

1. Refactor const-qualification to avoid the need to check whether
generators are `Freeze`. This might involve running (part of ) const
qualification after the `StateTransform` pass runs.
2. Use the current, more conservative approach for generator when
checking `Freeze` - that is, use the witness types computed from the
HIR.
3. Make generators always `Freeze`.

Option 1 and 2 introduce a large amount of additional complexity for the
sole purpose of supporting generators. Option 3 (this PR), requires only
a minor change to `SelectionContext`. Theoretically, it could also
improve performance, since we now perform less work when checking for
`Freeze` types.

This should probably have a test, but I'm not really sure how to write
one, considering that `Freeze` is private.
@eddyb
Copy link
Member

eddyb commented Oct 25, 2019

r? @Zoxc cc @RalfJung @nikomatsakis

@rust-highfive rust-highfive assigned Zoxc and unassigned eddyb Oct 25, 2019
@Zoxc
Copy link
Contributor

Zoxc commented Oct 25, 2019

The motivation for this PR is to support further work on #57017. My
approach is to delay resolution of auto-trait predicates (e.g.
{generator}: Send) until after we've constructed the generator MIR
(specifically after we've run the StateTransform MIR transformation
pass).

You definitely shouldn't be doing that. StateTransform should run after MIR optimizations and we can't have {generator}: Send) depend on optimizations.

@RalfJung
Copy link
Member

However, I believe that it is sound to always treat generator types as
Freeze, regardless of what types they may store internally. The only
possible of interacting with a generator is through the resume method
of the generator trait. resume takes a Pin<&mut Self>, which
requires a &mut Self to be created. In other words, given only a
shared reference to a generator (&{generator}), it is impossible to
mutate the generator. Note that is true regardless of whether or not
types inside the generator have interior mutability - they can only
change when resume is called, which is impossible to call using a
shared reference.

Uh, this is subtle. So basically you are saying if we have a type like

struct MyType<T>(UnsafeCell<T>);

impl<T> MyType<T> {
  fn new() -> Self { ... }
  fn do(&mut self) { ... }
}

Then we can unsafe impl<T> Freeze for MyType<T>.

That... puts some assumptions about the Rust type system on its head. So far "Freeze = contains no UnsafeCell"; that would no longer be the case. So what does this mean in terms of UB? So far, as far as I am concerned, UnsafeCell is the only type here that actually affects whether some code is UB or not. So, it would not actually be UB to use interior mutability on MyType as there is an UnsafeCell. But with that view, this PR is incorrect as the compiler relies on mutating shared frozen types to be UB.

So to have something like this PR, we'd have to attach the UB to the Freeze trait instead of UnsafeCell, I think. Basically this makes UnsafeCell useless; Cell could instead just impl !Freeze for Cell to get the desired behavior. But I have no idea if such a scheme can actually work, at first sight it seems like using traits instead of types makes many things much more complicated and I do not have a good handle of what the consequences are.

This needs an RFC, I would say.

@Centril Centril added needs-rfc This change is large or controversial enough that it should have an RFC accepted before doing it. T-lang Relevant to the language team, which will review and decide on the PR/issue. S-blocked Status: 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 Oct 25, 2019
@nikomatsakis
Copy link
Contributor

I think maybe we should close this PR until we work out better what plan we want?

@Aaron1011
Copy link
Member Author

@RalfJung; The way I thought of it, generators would be treated in the same way as references with respect to Freeze. While generators, unlike references, contain their sub-types directly within the layout, they behave like references in that they introduce a layer of indirection.

I agree that this whole issue is very subtle, and should have an RFC.

@Aaron1011
Copy link
Member Author

@Zoxc: I replied to your comment in PR #65782, since it's more relevant in that thread.

@RalfJung
Copy link
Member

RalfJung commented Oct 26, 2019

The way I thought of it, generators would be treated in the same way as references with respect to Freeze. While generators, unlike references, contain their sub-types directly within the layout, they behave like references in that they introduce a layer of indirection.

So far everything in the compiler and Stacked Borrows assumes that "layer of indirection" is about pointer indirections. You can't just re-define that; after all even with your PR, moving a generator moves its interior -- there is observably no layer of indirection. There is a layer of abstraction, but that's not the same thing.

So what is your thought on the question I raised in my post regarding UB? Is writing to an &MyType<i32> UB or not? UnsafeCell is all about making certain things not UB that usually would be UB; to change that you'll have to make a proposal for how exactly UB could be defined (and checked in Miri) using Freeze instead of UnsafeCell.

@Ixrec
Copy link
Contributor

Ixrec commented Oct 26, 2019

Possibly stupid question: Could we make generator types wrap all of their mutable captures in UnsafeCells?

@Centril Centril added the F-coroutines `#![feature(coroutines)]` label Nov 10, 2019
@Aaron1011
Copy link
Member Author

I'm closing this - while I still think the underlying idea might work (even if it's by special-casing generators), it no longer seems to be necessary for #65782. Other changes to that PR should allow us to compute <generator>: Freeze without introducing a cycle, so the immediate motivation for this PR is gone.

@Aaron1011 Aaron1011 closed this Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-coroutines `#![feature(coroutines)]` needs-rfc This change is large or controversial enough that it should have an RFC accepted before doing it. S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants