DST coercions and DST fields in structs #14397

nrc · 2014-05-24T05:27:45Z

DST coercions and DST fields in structs

The commits are not quite stand alone, I should probably squash them together before landing. In particular if you review the individual commits, then you'll see some scrappy stuff that gets fixed in later commits. But reading the commits in order might be easier to get an overall idea of what is going on.

The first commit includes putting back time zone into our time library - @pcwalton removed that as part of his de-~str'ing, but I had already converted it to use StrBuf, so we may as well leave it in. Update: no longer, this is removed in a later commit.

nrc · 2014-05-24T05:28:17Z

@nikomatsakis r?

alexcrichton · 2014-05-24T06:26:57Z

src/rt/rust_builtin.c

 #endif

-    tm_to_rust_tm(&tm, timeptr, gmtoff, nsec);
+    tm_to_rust_tm(&tm, timeptr, gmtoff, zone, nsec);


Looks like this filed picked up some extra code that was removed recently.

This is deliberate - as far as I could tell, it was only removed because it used ~str and I had already changed it locally to use StrBuf, so figured I should put it back in with the StrBuf version.

If that's the case, can you separate out this commit? It's unfortunate to see this jumbled up with the other DST commits.

I also don't think that this is the best way of tackling this problem. This is hard-coding the representation of StrBuf, and transitively Vec. There are no comments either here, in strbuf.rs or in vec.rs referencing that one needs to be updated with the others (and I don't think there should be comments).

This also looks very dangerous in term of pointers. The StrBuf type is deallocated with jemalloc, but it doesn't look like it's explicitly allocated with jemalloc here (but I may be missing something). Basically, I think that the time zone support needs to be redone with the C interop if we want to bring this back.

The representation of vec was hard-coded in before, likewise absence of comments, so I don't think this makes it worse (I agree it is terrible, though). I didn't look too closely at the alloc/dealloc side of things, but my impression was that all allocation happened on the Rust side so we should be OK.

TBH, I don't want to get into implementing a good solution here, I'd be happy to either take this all back out or to put it into a separate commit.

I think it may be best to separate to a separate commit or PR (to make it easier to review). I feel like hard-coding is worse now that String is just a library type, not a language-defined type (as ~str was), but we can always explore our options here.

Long term, it seems like these functions could change to receiving some extra arguments:

uint8_t* timezone_out, const size_t timezone_capacity, size_t* timezone_written_len

so the Rust code creates space for the zone, and passes in the pointer and capacity, then this function writes out the length that was actually written. (this also allows us to be slightly more efficient on the Rust side by using an on-stack [u8, .. 64], and only allocating when we know exactly how long the time zone string is. /me prematurely optimises.

nrc · 2014-06-20T23:37:43Z

@nikomatsakis r?

This needs another rebase already, but given that the last one took nearly a week, I wanted to get this up for review before I do that. I don't expect anything significant to change. It passes make check locally.

It is not worth looking at individual commits since the later ones change a lot of what happens in earlier ones. But I haven't squashed them because I thought it might be informative to see the steps taken (at the coarsest level) to get to where I got to.

With these changes we do deep coercions in the sense of structs with value fields which get unsized to produce an unsized struct. But we don't yet do the deep coercions of pointers.

There are still a few special cases on vec/str vs unsized, these are mostly where traits are different. I hope these go away when I do the DST-ification of traits.

nrc · 2014-06-23T01:08:23Z

rebased

michaelwoerister · 2014-06-26T09:31:11Z

src/librustc/middle/trans/debuginfo.rs

-            match ty::get(mt.ty).sty {
-                ty::ty_vec(ref mt, None) => {
-                    vec_slice_metadata(cx, t, mt.ty, unique_type_id, usage_site_span)
+        // FIXME Can we do better than this for unsized vec/str fields?


I think it would be better to specify the length of the array as 0. It's also what GCC does in this situation, I think:
https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
LLVM should know how to handle it (though I haven't tried).

brson · 2014-06-27T00:44:02Z

@pnkfelix Can you help review this? Let's try to get it merged and not wait for @nikomatsakis.

pnkfelix · 2014-06-27T15:40:14Z

@brson sounds good

nikomatsakis · 2014-06-28T04:52:07Z

So -- I did read over this patch as best I could before I left. Sorry I didn't get time to write comments. I just wanted to say that, generally speaking, everything I read looked good to me, but I wasn't able to really dive in.

nikomatsakis · 2014-06-28T05:20:38Z

src/librustc/middle/typeck/infer/sub.rs

+            (&ty::ty_infer(TyVar(_)), &ty::ty_str) |
+            (&ty::ty_infer(TyVar(_)), &ty::ty_vec(_, None)) => {
+                Err(ty::terr_sorts(expected_found(self, a, b)))
+            }


I'm not sure I follow this. Eventually we will want to be able to unify type variables with ty_str etc -- but I guess not in this patch, right?

Yeah, that is right. Unifying ty_str etc., at the moment breaks the world because we have loads of impls for both &T and &[T]. But once the trait reform lands, that should no longer be a problem. Even without trait reform, with complete DST we should be able to fix all those impls.

nrc · 2014-07-07T01:58:48Z

@pnkfelix r? (needs a rebase, but I don't expect that to change anything significant)

huonw · 2014-07-07T02:02:04Z

The rebase will presumably remove all the ~[] (well, Box<[]>) handling from the libraries, which seems relatively significant?

nrc · 2014-07-07T02:03:48Z

@huonw I think this has been rebased since that happened (unless there has been more). In any case, that shouldn't affect the implementation which I think will take the most effort to review.

huonw · 2014-07-07T02:04:43Z

Well, there's still mentions of ~[T] in the diff, but yeah, I imagine the librustc parts are the interesting bits.

pnkfelix · 2014-07-07T17:18:03Z

src/libcollections/dlist.rs

@@ -977,7 +977,8 @@ mod tests {
        let n = list_from([1,2,3]);
        spawn(proc() {
            check_links(&n);
-            assert_eq!(&[&1,&2,&3], n.iter().collect::<Vec<&int>>().as_slice());
+            let a: &[_] = &[&1,&2,&3];
+            assert_eq!(a, n.iter().collect::<Vec<&int>>().as_slice());


here and elsewhere: Does the <expr> as <type> syntax work yet for coercing a sized type to an unsized one?

E.g. could we here instead write assert_eq!(&[&1,&2,&3] as &[_], n.iter().collect::<Vec<&int>>().as_slice()); ?

I am not sure if it would actually make things look nicer, but it might help stress that a coercion is happening here, not just a type assertion.

It does not. From what I could tell at the time, the as syntax was for casts and coercions were always implicit. In retrospect, I think I am wrong about that. I envisaged addressing this with type ascription. But I think that highlights some potentially confusing overlap between type ascription and explicit casts.

(I might be using the term "coercion" incorrectly here. anyway the point is whether we have any expression form, other than a let nested in an ExprBlock, for turning a thin pointer into a fat one. But it sounds like we do not have that yet.)

@nick29581 Anyway, if possible, when you squash your commits together, I would appreciate it if you put these preparatory refactorings on a separate early commit, so that people looking at the history can easily focus in on the substansive changes.

pnkfelix · 2014-07-10T12:01:40Z

src/librustc/middle/ty.rs


-    /// Convert from ~[]/&[] to &&[] or str
-    AutoBorrowVecRef(Region, ast::Mutability),
+    /// Convert [T, ..n] to ~[T]


I do not understand this conversion, at least not as described in the comment. How can you convert an arbitrary [T, ..n] (e.g. perhaps stack-allocated) to a ~[T] (now Box<[T]> in modern rust)?

Or is there a typo in the comment? Is this actually ~[T, ..n] to ~[T] ?

pnkfelix · 2014-07-10T12:31:18Z

@nick29581 while attempting to explore the impact of this code on a local build of rustc, I found a bug where some DST manipulations lead to an LLVM assert fail. For now I will transcribe the test case here, but I am not sure if you would prefer for me to report such cases by other means.

#![allow(dead_code)]

extern crate debug;

fn main() {
    struct Foo<type T> { y: T }

    let f : Foo<[int, ..3]> = Foo { y: [5i, 6, 7] };
    println!("&f.y: {:?}", &f.y);

    let g : &Foo<[int]> = &f;
    println!("&g.y: {:?}", &g.y);

    let h = &Foo { y: *g };
    println!("&h.y: {:?}", &h.y);
}

yields:

% ./x86_64-apple-darwin/stage2/bin/rustc --version
rustc 0.11.0-pre (2973ca6 2014-07-07 13:49:43 +1200)
host: x86_64-apple-darwin
% ./x86_64-apple-darwin/stage2/bin/rustc /tmp/g.rs && ./g
Assertion failed: (getOperand(0)->getType() == cast<PointerType>(getOperand(1)->getType())->getElementType() && "Ptr must be a pointer to Val type!"), function AssertOK, file ../../../../../src/llvm/lib/IR/Instructions.cpp, line 1085.
Abort trap: 6
%

pnkfelix · 2014-07-10T12:33:39Z

@nick29581 (by the way it is entirely possible that the above test case is supposed to be illegal under DST. I am still not 100% clear on whether the official plan allows a deref of a fat pointers to be copied by value to construct another dst instance, which is what I was testing in the aforementioned test case, which is trying to construct a &Foo<Foo<[int]>> incrementally.)

pnkfelix · 2014-07-10T12:41:32Z

src/librustc/middle/ty.rs

    type_contents(cx, ty).is_sized(cx)
 }

+// Return the smallest part of t which is unsized. Fails if t is sized.


The meaning of the word "smallest" here is not obvious. For example, I would think that [int] is "smaller" than [[int]], but from reading the code, it seems the two are equal under this measure. Perhaps you just mean "strip a unsized struct down to the non-struct unsized type that caused it to become unsized."

(My example [[int]] was not well formed. But I think my point still stands. Let me know if I need to elaborate more)

pnkfelix · 2014-07-10T12:46:04Z

Another bug, this one is an ICE:

#![allow(dead_code)]

extern crate debug;

fn main() {
    struct Foo<type T> { y: T }

    let f : Foo<[int, ..3]> = Foo { y: [5i, 6, 7] };
    println!("&f.y: {:?}", &f.y);

    let g : Box<Foo<[int]>> = box f;
    println!("&g.y: {:?}", &g.y);
}

yields:

% ./x86_64-apple-darwin/stage2/bin/rustc --version 
rustc 0.11.0-pre (2973ca6 2014-07-07 13:49:43 +1200)
host: x86_64-apple-darwin
% ./x86_64-apple-darwin/stage2/bin/rustc /tmp/g.rs && ./g
/tmp/g.rs:11:31: 11:36 error: internal compiler error: unsupported adjustment
/tmp/g.rs:11     let g : Box<Foo<[int]>> = box f;
                                           ^~~~~
note: the compiler hit an unexpected failure path. this is a bug.
note: we would appreciate a bug report: http://doc.rust-lang.org/complement-bugreport.html
note: run with `RUST_BACKTRACE=1` for a backtrace
task 'rustc' failed at 'Box<Any>', /Users/fklock/Dev/Mozilla/rust-dstnrc/src/libsyntax/diagnostic.rs:106


%

gist of backtrace: https://gist.github.com/pnkfelix/5dd6b31964f5db3a2f87

A slightly simpler version of the same program has the same ICE, I believe:

    let f : Box<Foo<[int, ..3]>> = box Foo { y: [5i, 6, 7] };
    let g : Box<Foo<[int]>> = f;
    println!("&g.y: {:?}", &g.y);

(but a version that just does f: Box<[int, ..3]> and g: Box<[int]> does not ICE, which is good since it does seem like much of the existing tests are geared around those cases.)

pnkfelix · 2014-07-10T13:14:58Z

Also, presumably this is meant to close #13121. (Noting that here so that links show up over there.)

(Originally I also put #13716 next to #13121 above, but then I looked again at the description for #13716 and it explicitly talks about using the as operator to coerce e.g. support &[int, ..5] as &[int], which @nick29581 and I established above is not part of this PR. So I guess #13716 should stay open.)

pnkfelix · 2014-07-10T13:26:37Z

Q for @nick29581 : is impl W for W { ... } where W is a trait, is that part of DST?

(My head still spins every time I look at a declaration like impl W for W { ... }, but I am under the impression that we do not want a trait T, when regarded as an unsized type, to automatically implement T. Perhaps that impression is incorrect at this point.)

Here is the example that I was trying to hack together when I encountered the "need" for impl W for W { ... }.

#![allow(dead_code)]

extern crate debug;

fn main() {
    struct Foo<type T> { x: int, y: T }
    trait W for type { fn weight(&self) -> int; }
    #[cfg(buggy)]
    impl W for W {
        fn weight(&self) -> int { (*self).weight() }
    }
    impl W for (int, int) {
        fn weight(&self) -> int { self.val0() + self.val1() }
    }
    impl<type X:W> W for Foo<X> {
        fn weight(&self) -> int { self.x + self.y.weight() }
    }

    let f : Foo<(int,int)> = Foo { x: 1i, y: (2i, 3i) };
    let g : &Foo<(int, int)> = &f;
    let h : &Foo<W> = g;
    println!("h.y.weight(): {:?}", h.y.weight());
    println!("h.weight(): {:?}", h.weight());
}

When you compile without --cfg buggy, you get "failed to find an implementation of trait main::W for main::W". If you add --cfg buggy, you get:

% ./x86_64-apple-darwin/stage2/bin/rustc --cfg buggy /tmp/h.rs && ./h
error: internal compiler error: unexpected failure
note: the compiler hit an unexpected failure path. this is a bug.
note: we would appreciate a bug report: http://doc.rust-lang.org/complement-bugreport.html
note: run with `RUST_BACKTRACE=1` for a backtrace
task 'rustc' failed at 'get_base_type() returned a type that wasn't an enum, struct, or trait', /Users/fklock/Dev/Mozilla/rust-dstnrc/src/librustc/middle/typeck/coherence.rs:169

gist of the backtrace for the above: https://gist.github.com/pnkfelix/ad6cf810a87c2876cc57

pnkfelix · 2014-07-10T13:43:48Z

src/librustuv/lib.rs

@@ -40,6 +40,7 @@ via `close` and `delete` methods.
 #![crate_type = "rlib"]
 #![crate_type = "dylib"]

+#![allow(unknown_features)]


what feature in librustuv is this enabling? I do not see any new features being enabled.

assuming this line stays, presumably it should have a "remove after snapshot" note added above it, right?

pnkfelix · 2014-07-10T14:11:12Z

@nick29581 okay, phew. done with the review.

I largely am rubber-stamping the trans changes, though obviously there are problems embedded within them, at least that's what I infer from the backtraces in the test case failures I have posted in earlier comments.

Everything else looks reasonable.

brson · 2014-07-14T23:03:42Z

@pnkfelix @nick29581 It sounds like everything is good to go here. Can we merge?

nrc · 2014-07-15T04:47:44Z

@brson No, I need to address all these review comments and then rebase. I'll try and do that asap.

nrc · 2014-08-04T08:36:46Z

@pnkfelix r? I think I have made all the review changes you requested. I think there is enough to make it worthwhile reviewing the changes. They are all in the last commit. Now I need to squash and rebase...

pnkfelix · 2014-08-04T09:17:06Z

@nick29581 okay I will try to get to it today

pnkfelix · 2014-08-07T09:57:26Z

@nick29581 the cases from this comment still ICE. But maybe that need not hold up this PR.

nrc · 2014-08-07T10:21:08Z

GAH! I was sure I fixed that. Presumably I then broke it again whilst fixing something else. But I thought I even added a test. Maybe I missed that comment completely. Oh well, I'll look again.

pnkfelix · 2014-08-07T10:29:22Z

src/librustc/middle/typeck/infer/coercion.rs

               a.inf_str(self.get_ref().infcx), sty_a,
               b.inf_str(self.get_ref().infcx));

+        // Note, we want to avoid unnecessary unsizing. We don't want to coerce to
+        // a DST unless we have to. This currently comes out in the wash since
+        // we can't unify [T] with U. But to properly support DST, we need to allow


Is there an issue open somewhere for adding support for unifying [T] with U?

I didn't see one when skimming the list on the #12938 metabug.

Might be good to open an issue and put it on the metabug and this comment.

pnkfelix · 2014-08-07T10:31:20Z

@nick29581 I don't mind if we land this PR and leave the ICE for later.

The PR looks good to me; r=me after your next round of rebasing. :)

[breaking-change] 1. The internal layout for traits has changed from (vtable, data) to (data, vtable). If you were relying on this in unsafe transmutes, you might get some very weird and apparently unrelated errors. You should not be doing this! Prefer not to do this at all, but if you must, you should use raw::TraitObject rather than hardcoding rustc's internal representation into your code. 2. The minimal type of reference-to-vec-literals (e.g., `&[1, 2, 3]`) is now a fixed size vec (e.g., `&[int, ..3]`) where it used to be an unsized vec (e.g., `&[int]`). If you want the unszied type, you must explicitly give the type (e.g., `let x: &[_] = &[1, 2, 3]`). Note in particular where multiple blocks must have the same type (e.g., if and else clauses, vec elements), the compiler will not coerce to the unsized type without a hint. E.g., `[&[1], &[1, 2]]` used to be a valid expression of type '[&[int]]'. It no longer type checks since the first element now has type `&[int, ..1]` and the second has type &[int, ..2]` which are incompatible. 3. The type of blocks (including functions) must be coercible to the expected type (used to be a subtype). Mostly this makes things more flexible and not less (in particular, in the case of coercing function bodies to the return type). However, in some rare cases, this is less flexible. TBH, I'm not exactly sure of the exact effects. I think the change causes us to resolve inferred type variables slightly earlier which might make us slightly more restrictive. Possibly it only affects blocks with unreachable code. E.g., `if ... { fail!(); "Hello" }` used to type check, it no longer does. The fix is to add a semicolon after the string.

nrc · 2014-08-26T04:11:02Z

@pnkfelix Please could you r? The last two commits? The first of the two is a bunch of changes which came up during rebasing to current master. The final one is a tiny optimisation so we can pass the codegen tests. The other commits are just squashed and rebased versions of commits you have previously reviewed.

@pcwalton

DST coercions and DST fields in structs The commits are not quite stand alone, I should probably squash them together before landing. In particular if you review the individual commits, then you'll see some scrappy stuff that gets fixed in later commits. But reading the commits in order might be easier to get an overall idea of what is going on. The first commit includes putting back time zone into our time library - @pcwalton removed that as part of his de-~str'ing, but I had already converted it to use StrBuf, so we may as well leave it in. Update: no longer, this is removed in a later commit.

alexcrichton reviewed May 24, 2014
View reviewed changes

nrc changed the title ~~DST coercions and representaton for ~[T]~~ DST coercions and DST fields in structs Jun 20, 2014

michaelwoerister reviewed Jun 26, 2014
View reviewed changes

nikomatsakis reviewed Jun 28, 2014
View reviewed changes

nrc closed this Jul 7, 2014

nrc deleted the coerce branch July 7, 2014 01:53

nrc reopened this Jul 7, 2014

pnkfelix reviewed Jul 7, 2014
View reviewed changes

pnkfelix reviewed Jul 10, 2014
View reviewed changes

pnkfelix reviewed Aug 7, 2014
View reviewed changes

pnkfelix mentioned this pull request Aug 22, 2014

Implement DST #12938

Closed

23 tasks

nrc added 5 commits August 26, 2014 12:37

Use the slice repr for ~[T]

34d607f

Use temp vars for implicit coercion to ^[T]

37a94b8

Rebasing changes

52ef462

Optimise a particularly clown shoes example of DST codegen

08364a4

nrc force-pushed the coerce branch from 120e056 to 08364a4 Compare August 26, 2014 04:09

bors merged commit 08364a4 into rust-lang:master Aug 26, 2014

DST coercions and DST fields in structs #14397

DST coercions and DST fields in structs #14397

Conversation

nrc commented May 24, 2014

nrc commented May 24, 2014

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

nrc commented Jun 20, 2014

nrc commented Jun 23, 2014

Choose a reason for hiding this comment

brson commented Jun 27, 2014

pnkfelix commented Jun 27, 2014

nikomatsakis commented Jun 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrc commented Jul 7, 2014

huonw commented Jul 7, 2014

nrc commented Jul 7, 2014

huonw commented Jul 7, 2014

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 commented Jul 10, 2014

pnkfelix commented Jul 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Jul 10, 2014

pnkfelix commented Jul 10, 2014

pnkfelix commented Jul 10, 2014

Choose a reason for hiding this comment

pnkfelix commented Jul 10, 2014

brson commented Jul 14, 2014

nrc commented Jul 15, 2014

nrc commented Aug 4, 2014

pnkfelix commented Aug 4, 2014

pnkfelix commented Aug 7, 2014

nrc commented Aug 7, 2014

Choose a reason for hiding this comment

pnkfelix commented Aug 7, 2014

nrc commented Aug 26, 2014