Immovable types prototype where the Move trait is builtin #44917

Zoxc · 2017-09-29T07:56:48Z

No description provided.

rust-highfive · 2017-09-29T07:57:01Z

r? @pnkfelix

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

Zoxc · 2017-09-29T07:57:22Z

@bors try

bors · 2017-09-29T07:57:22Z

@Zoxc: 🔑 Insufficient privileges: and not in try users

kennytm · 2017-09-29T07:59:48Z

@bors try

bors · 2017-09-29T07:59:58Z

⌛ Trying commit 6d6240e with merge 3c96da1...

DO NOT MERGE: Immovable types prototype where the Move trait is builtin

bors · 2017-09-29T09:29:13Z

☀️ Test successful - status-travis
State: approved= try=True

aidanhs · 2017-09-29T11:14:54Z

Crater run requested by zoxc/eddyb and now started.

plietar · 2017-09-29T11:22:34Z

src/libcore/cell.rs

+#[cfg(not(stage0))]
+#[unstable(feature = "immovable_types", issue = "0")]
+#[lang = "movable_cell"]
+#[derive(Eq, PartialEq, Ord, PartialOrd, Copy, Clone, Hash, Debug)]


These shouldn't be derived as the implementations would take pointers to the inner value.

Zoxc · 2017-10-04T08:14:42Z

This currently contains a hack where it catches all panics when building move data, a proper solution would be to ignore the errors and let borrowck handle them, see #44830.

This also causes FnOnce::Output and Deref::Target to no longer implement Move, which is a breaking change.

bors · 2017-10-04T23:12:18Z

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

bors · 2017-10-08T20:40:58Z

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

aidanhs · 2017-10-08T21:26:32Z

At long last! Sorry for the delay, internal s3 errors delayed things a fair amount.

Cargobomb results: http://cargobomb-reports.s3.amazonaws.com/pr-44917/index.html. 'Blacklisted' crates (spurious failures etc) can be found here.
If you see any spurious failures not on the list, please make a PR against that file.

Zoxc · 2017-10-08T22:07:07Z

I looked over the results.

Errors due to duplicate bounds #21974 (comment):

nalgebra 0.10.1
number_traits 0.1.2
rowcol 0.3.3
svgdom 0.7.0
svgdom 0.6.0
svgdom 0.3.1
vek 0.2.2

Errors due to where bounds guiding type inference astray:

rust-install 0.0.4
idcontain 0.7.2

The above errors is also expected to appear due to the addition of the DynSized trait.

Errors due to FnOnce::Output not implementing Move:

parsell 0.6.5

Erros due to Deref::Target not implementing Move:

shawshank 0.2.3
thunk 0.3.0

Zoxc · 2017-10-08T22:14:09Z

I fixed the MoveData hack now that #45016 finally landed.

pnkfelix · 2017-10-11T21:09:49Z

@Zoxc are you looking for review comments at this point?

pnkfelix · 2017-10-11T21:12:24Z

(@Zoxc to be clear, I am aware you've been working on this a while and discussing in the chat rooms. But I'm not sure if you expect the PR to actually be reviewed; as far as I know there isn't an RFC for this change, am I wrong?)

pnkfelix · 2017-10-11T21:13:23Z

@Zoxc having said that, I would like to at least coordinate the MIR-dataflow/MoveData changes made here so that they align with changes that I'm planning for the MoveData in MIR-borrowck.

Zoxc · 2017-10-11T21:23:02Z

@pnkfelix This is ready to be reviewed.

There is an RFC, although it was not been merged. This feature is required for experiments with immovable generators though (which does have a eRFC). The compiler team was also in favor of merging the DynTrait PR, which also lacks a RFC and has similar breakage.

arielb1 · 2017-10-12T11:56:14Z

src/libcore/cell.rs

@@ -1267,3 +1269,35 @@ fn assert_coerce_unsized(a: UnsafeCell<&i32>, b: Cell<&i32>, c: RefCell<&i32>) {
    let _: Cell<&Send> = b;
    let _: RefCell<&Send> = c;
 }
+
+/// A cell that is always movable, even if the interior type is immovable.


~~Can't a normal Cell take T: ?Move? I suppose that wouldn't work because we need a lang-item.~~

bors · 2017-11-21T12:36:08Z

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

carols10cents · 2017-11-27T15:02:26Z

Friendly ping to keep this on your radar @pnkfelix !

pnkfelix · 2017-11-27T15:20:27Z

src/libcore/marker.rs

+#[cfg(not(stage0))]
+#[lang = "move"]
+#[unstable(feature = "immovable_types", issue = "0")]
+pub unsafe trait Move: ?Move {


weird: do you have to have ?Move constraint on this trait?

Just curious, since I noticed that trait Sized does not have an analogous ?Sized bound...

Auto traits cannot have supertraits because that is unsound. Also we do not want Move to require Move. That would probably result in interesting things happening.

pnkfelix · 2017-11-27T15:37:39Z

src/librustc_passes/ast_validation.rs

@@ -91,19 +91,6 @@ impl<'a> AstValidator<'a> {
        }
    }

-    fn no_questions_in_bounds(&self, bounds: &TyParamBounds, where_: &str, is_trait: bool) {


You've removed all the calls to this method. I infer that it because you want to assume that traits automatically start off as implementing Move, and thus you need to be able to put : ?Move as bound on trait definitions in order to override that initial default.

But shouldn't we replace the old function with one that is specialized to ?Sized? Or is there some reason why we want to stop erroring in response to something like trait X : ?Sized { fn x(&self); } ? (see update in followup comment below)

oh never mind, I see that you did add such a check in fn conv_object_ty_poly_trait_ref

pnkfelix · 2017-11-27T15:44:28Z

src/test/compile-fail/maybe-bounds.rs


-type A1 = Tr + ?Sized; //~ ERROR `?Trait` is not permitted in trait object types
-type A2 = for<'a> Tr + ?Sized; //~ ERROR `?Trait` is not permitted in trait object types
+type A1 = Tr + ?Sized; //~ WARN default bound relaxed


hmm... I'm a little worried that this error has gone away. (I'd need to review the details about why its an error in the first place, but assuming that there was a good reason for it being an error, I assume that those same reason still hold here?)

Or was this just a case of "trait objects are implicitly unsized and thus it makes no sense to put ?Sized in them" ?

Trait objects previously had no default bounds, so the dyn TraitA + ?TraitB syntax wasn't useful. However now you can use it with Move, so dyn Trait implements Move, dyn Trait + ?Move does not.

pnkfelix · 2017-11-27T15:48:15Z

src/test/compile-fail/move-trait-leaks-in-impl-trait.rs

+
+fn main() {
+    test(immovable());
+    //~^ ERROR the trait bound `std::marker::Immovable: std::marker::Move` is not satisfied


hmm. This is certainly fine as a conservative choice for now (I do see that the RFC says "Existential types (impl Trait) are Move if their underlying type are Move" which I take to be an "if and only if").

But I find it interesting because I would have guessed that since no borrows are taken of the value returned from immovable(), nor do we call any methods on it that take &self/&mut self, that it would be safe to subsequently move it in the call to test(..)

It is safe to move it into test. The problem is that the test function has a Move bound, so it may rely on the type being movable after taking the address of it.

pnkfelix · 2017-11-27T15:51:48Z

src/test/compile-fail/phantom-oibit.rs

 use std::marker::{PhantomData};

-unsafe trait Zen {}
+unsafe trait Zen: ?Move {}


Did ... you need to add this? In other words, was it injecting new compiler errors otherwise? (If so, what were they?)

If not, then I assume you just added it because its the right way to preserve the previous semantics of the test here?

Auto traits cannot have supertraits because that is unsound, so we need to opt-out to Move, which is a default bound on Self for all traits.

pnkfelix · 2017-11-27T15:52:55Z

src/test/compile-fail/privacy-sanity.rs

-trait MarkerTr {}
+use std::marker::Move;
+
+trait MarkerTr: ?Move {}


(likewise here, I am again curious as to whether this was a necessary change, or just an artifact of your methodology for updating the tests...)

(I'll stop asking about the future instances of this pattern now, though.)

pnkfelix · 2017-11-27T15:55:48Z

src/test/ui/impl-trait/equality-recursive.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#![feature(conservative_impl_trait, specialization)]


I see you factored this test out from a separate piece of code that started failing (in a new way) in src/test/ui/impl-trait/equality.rs ... do we need to be concerned about this change in behavior? I.e. Does it need a separate issue filed for it, once this PR lands?

I split this out since this error happens in another compiler phase compared to the rest. I don't remember the details, but I probably did something which caused the compiler to exit earlier due to errors.

pnkfelix · 2017-11-27T15:58:58Z

Looks fine to me.

I had a bunch of questions that I posted as comments on the diff. But I don't think any of them actually warrant holding up trying to land this (assuming that we're collectively fine with landing this as an experimental extension for non-finalized RFC).

So, r=me after a rebase (but I'd love to see those questions answered!)

bjorn3 · 2017-11-27T18:27:48Z

src/test/compile-fail/no-move-immovable-box-content.rs

+    let a = new_box_immovable();
+    &*a; // ok
+    move_from_box(a); // ok
+}


Missing newline

bjorn3 · 2017-11-27T18:29:16Z

src/libstd/panic.rs

 #[stable(feature = "catch_unwind", since = "1.9.0")]
 impl<'a, T: ?Sized> !UnwindSafe for &'a mut T {}
+#[cfg(not(stage0))]
+#[stable(feature = "catch_unwind", since = "1.9.0")]
+impl<'a, T: ?Sized+?::marker::Move> !UnwindSafe for &'a mut T {}


Missing spaces around the +

bjorn3 · 2017-11-27T18:29:37Z

src/libstd/panic.rs

 #[stable(feature = "catch_unwind", since = "1.9.0")]
 impl<T: ?Sized> !RefUnwindSafe for UnsafeCell<T> {}
+#[cfg(not(stage0))]
+#[stable(feature = "catch_unwind", since = "1.9.0")]
+impl<T: ?Sized+?::marker::Move> !RefUnwindSafe for UnsafeCell<T> {}


Missing spaces around the +

bjorn3 · 2017-11-27T18:35:46Z

src/test/ui/impl-trait/equality-recursive.rs

+    }
+}
+
+fn main() {}


Missing newline

Zoxc · 2017-11-27T21:26:13Z

I'm not sure why @pnkfelix is still assigned here. I saw @arielb1 unassign him in the log. Anyway this is still blocked on @arielb1 and @nikomatsakis looking into fixing the breaking changes and looking into the changes on the src/test/ui/on-unimplemented/multiple-impls.stderr test.

kennytm · 2017-12-06T10:42:13Z

Hi @Zoxc, is this still blocked by the src/test/ui/on-unimplemented/multiple-impls.stderr test and the breaking changes?

Zoxc · 2017-12-06T10:48:23Z

Yes.

carols10cents · 2017-12-12T18:15:04Z

Friendly ping @arielb1 and @nikomatsakis, this comment seems to say this is waiting on you!

Anyway this is still blocked on @arielb1 and @nikomatsakis looking into fixing the breaking changes and looking into the changes on the src/test/ui/on-unimplemented/multiple-impls.stderr test.

kennytm · 2017-12-20T13:42:23Z

Triage — this PR appears to be still blocked by

Anyway this is still blocked on @arielb1 and @nikomatsakis looking into fixing the breaking changes and looking into the changes on the src/test/ui/on-unimplemented/multiple-impls.stderr test.

BTW @Zoxc There is merge conflict.

arielb1 · 2017-12-21T10:46:36Z

Status of the Move/DynSized PRs

I apologize that this was not communicated clearly enough, but there are breakage and language-design issues blocking these PRs. We are probably not going to merge them until these are solved:

First, there's a trait-system inference issue that means that without NLL and another fix, this PR "semi-randomly" breaks user code, which means that, as both NLL and not causing unnecessary breakage are fairly high priority, and these PRs are less so, we should probably delay this PR until NLL stabilizes.
We only discovered this when we have done the crater run on the Move PR. Thanks @Zoxc for getting to this state where this is our worst technical issue.
Second, as you have seen, adding more "Sized-style" traits has an ergonomics disadvantage which we didn't appreciate enough when we have first seen this PR.
We (or rather the lang team) need to find the best compromise in this area, which I'm not that sure about. Feel free to join them on this issue - they should have more time after the holidays, now that the impl period is over.

pnkfelix · 2018-01-04T16:23:32Z

rust-lang/rfcs#2255 seems relevant to @arielb1's second point (quoted here)

Second, as you have seen, adding more "Sized-style" traits has an ergonomics disadvantage which we didn't appreciate enough when we have first seen this PR.
We (or rather the lang team) need to find the best compromise in this area, which I'm not that sure about. Feel free to join them on this issue - they should have more time after the holidays, now that the impl period is over.

Immovable generators This adds support for immovable generators which allow you to borrow local values inside generator across suspension points. These are declared using a `static` keyword: ```rust let mut generator = static || { let local = &Vec::new(); yield; local.push(0i8); }; generator.resume(); // ERROR moving the generator after it has resumed would invalidate the interior reference // drop(generator); ``` Region inference is no longer affected by the types stored in generators so the regions inside should be similar to other code (and unaffected by the presence of `yield` expressions). The borrow checker is extended to pick up the slack so interior references still result in errors for movable generators. This fixes #44197, #45259 and #45093. This PR depends on [PR #44917 (immovable types)](#44917), I suggest potential reviewers ignore the first commit as it adds immovable types.

rust-highfive assigned pnkfelix Sep 29, 2017

bors added a commit that referenced this pull request Sep 29, 2017

Auto merge of #44917 - Zoxc:move, r=<try>

3c96da1

DO NOT MERGE: Immovable types prototype where the Move trait is builtin

plietar reviewed Sep 29, 2017

View reviewed changes

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 29, 2017

Zoxc force-pushed the move branch 2 times, most recently from a74dede to 0f21a1c Compare October 4, 2017 06:53

Zoxc changed the title ~~DO NOT MERGE: Immovable types prototype where the Move trait is builtin~~ Immovable types prototype where the Move trait is builtin Oct 4, 2017

Zoxc force-pushed the move branch 3 times, most recently from 4f75c60 to 5a3ee2b Compare October 7, 2017 07:35

Zoxc force-pushed the move branch from 5a3ee2b to 8797d49 Compare October 8, 2017 22:13

pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 11, 2017

arielb1 reviewed Oct 12, 2017

View reviewed changes

pnkfelix self-assigned this Nov 21, 2017

pnkfelix reviewed Nov 27, 2017

View reviewed changes

pnkfelix added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2017

bjorn3 requested changes Nov 27, 2017

View reviewed changes

kennytm unassigned pnkfelix Dec 6, 2017

kennytm added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 20, 2017

arielb1 added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Dec 21, 2017

kennytm mentioned this pull request Dec 22, 2017

More implicit bounds (?Sized, ?DynSized, ?Move) rust-lang/rfcs#2255

Open

kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Dec 22, 2017

Zoxc closed this Dec 23, 2017

Immovable types prototype where the Move trait is builtin #44917

Immovable types prototype where the Move trait is builtin #44917

Conversation

Zoxc commented Sep 29, 2017

rust-highfive commented Sep 29, 2017

Zoxc commented Sep 29, 2017

bors commented Sep 29, 2017

kennytm commented Sep 29, 2017

bors commented Sep 29, 2017

bors commented Sep 29, 2017

aidanhs commented Sep 29, 2017

Choose a reason for hiding this comment

Zoxc commented Oct 4, 2017

bors commented Oct 4, 2017

bors commented Oct 8, 2017

aidanhs commented Oct 8, 2017 • edited Loading

Zoxc commented Oct 8, 2017 • edited Loading

Zoxc commented Oct 8, 2017

pnkfelix commented Oct 11, 2017

pnkfelix commented Oct 11, 2017

pnkfelix commented Oct 11, 2017

Zoxc commented Oct 11, 2017

arielb1 Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

bors commented Nov 21, 2017

carols10cents commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zoxc commented Nov 27, 2017

kennytm commented Dec 6, 2017

Zoxc commented Dec 6, 2017

carols10cents commented Dec 12, 2017

kennytm commented Dec 20, 2017

arielb1 commented Dec 21, 2017 • edited Loading

Status of the Move/DynSized PRs

pnkfelix commented Jan 4, 2018

aidanhs commented Oct 8, 2017 •

edited

Loading

Zoxc commented Oct 8, 2017 •

edited

Loading

arielb1 Oct 12, 2017 •

edited

Loading

pnkfelix Nov 27, 2017 •

edited

Loading

pnkfelix Nov 27, 2017 •

edited

Loading

pnkfelix Nov 27, 2017 •

edited

Loading

arielb1 commented Dec 21, 2017 •

edited

Loading