Add DatatypeDef #22564

Aatch · Feb 20, 2015

Add a DatatypeDef type that contains information for both structs and enums. Most places already treated structs like single-variant enums or vice-versa, so this actually simplifies a lot of code.

There is a small but significant performance increase in typechecking and translation (pre-optimisations) of about 5% in both phases. All the other phases seemed to have some performance improvements too, but the differences were too small to be meaningful.

This change is also a step towards overall simplification of the type system code. Given how many places are now treating enums and structs completely identically, a move towards a unified ty_data type is somewhat feasible.

rust-highfive · Feb 20, 2015

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

eddyb · Feb 20, 2015

I would prefer a name like DataType, but that's... bikeshedding.
cc @nikomatsakis @nick29581

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc/lint/builtin.rs

+        let ty = match ty::expr_ty_opt(cx.tcx, e) {
+            Some(ty) => ty,
+            None => cx.tcx.sess.span_bug(e.span, "No type for expression"),
+        };


Doesn't/can't ty::expr_ty handle this?

Ah right, that reminds me, I was going to fold the span_bug stuff into ty::expr_ty. Makes debugging easier.

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc/middle/mem_categorization.rs

+            let enum_def = ty::lookup_datatype_def(self.tcx(), enum_did);
+            // Univariant enums don't need a downcast
+            if !enum_def.is_univariant() {
+                self.cat_downcast(pat, cmt.clone(), cmt.ty, variant_did)


Down... cast? This is pre-existing, but what does it actually mean? Some sort of ancient refinement relic?

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc/middle/traits/coherence.rs

-            def_id.krate == ast::LOCAL_CRATE
+        ty::ty_enum(def, _) |
+        ty::ty_struct(def, _) => {
+            def.def_id.krate == ast::LOCAL_CRATE


Makes me wonder if we should have an is_local() method on DefId so we can do def.def_id.is_local().

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc/middle/ty.rs

@@ -629,6 +616,7 @@ pub struct CtxtArenas<'tcx> {
    substs: TypedArena<Substs<'tcx>>,
    bare_fn: TypedArena<BareFnTy<'tcx>>,
    region: TypedArena<Region>,
+    def: TypedArena<DatatypeDef<'tcx>>,


The field could use a more descriptive name (I would confuse it with middle::def::Def).

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc/middle/ty.rs

+    pub name: Name,
+    pub disr_val: Disr,
+    pub fields: Vec<FieldTy<'tcx>>,
+    pub vis: Visibility,


I thought enum variants are never private.

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc/middle/ty.rs

+    pub name: Name,
+    pub vis: ast::Visibility,
+    pub origin: DefId,
+    pub ty: Cell<Ty<'tcx>>


This needs a comment explaining why Cell is used here.
Similarly, the distinction between id and origin is not obvious, could also use a comment.

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc/middle/ty.rs

+    }
+}
+
+impl<'tcx> fmt::Debug for DatatypeDef<'tcx> {


Shouldn't this be a Repr implementation?

I added the Debug impl because sty derives Debug meaning that DatatypeDef need to impl Debug too.

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc/middle/ty.rs

@@ -3181,21 +3295,20 @@ pub fn sequence_element_type<'tcx>(cx: &ctxt<'tcx>, ty: Ty<'tcx>) -> Ty<'tcx> {
    }
 }

-pub fn simd_type<'tcx>(cx: &ctxt<'tcx>, ty: Ty<'tcx>) -> Ty<'tcx> {
+pub fn simd_type<'tcx>(_cx: &ctxt<'tcx>, ty: Ty<'tcx>) -> Ty<'tcx> {


This and a few other functions might benefit from not taking the unused tcx argument at all.

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/llvm

@@ -1 +1 @@
-Subproject commit 4891e6382e3e8aa89d530aa18427836428c47157


Rebase failure (run make and let it update submodules and/or avoid committing everything).

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc/middle/ty.rs

+
+    pub fn is_tuple_variant(&self) -> bool {
+        !self.fields.is_empty() &&
+            self.fields.iter().all(|f| f.name == token::special_names::unnamed_field)


I would avoid comparing to unnamed_field outside of an is_unnamed method on FieldTy.

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc_trans/trans/adt.rs

            for (j, field) in fields.iter().enumerate() {
-                let field_ty = ty::lookup_field_type(tcx, def_id, field.id, substs);
+                let field_ty = field.ty.get().subst(tcx, substs);


Maybe .ty.get() would be better off as .ty() - can the ty field be fully private?
Would be nice to know one cannot simply field.ty.set(ty).

Good point, you'd still need a set_ty method or similar, but I could always do a check to make sure that the type already set on the field is ty_err (meaning that you can only set it from ty_err to something else).

eddyb · Feb 20, 2015

eddyb reviewed Feb 20, 2015

View changes

eddyb · Feb 20, 2015

src/librustc_trans/trans/intrinsic.rs

-            if !return_type_is_void(ccx, tp_ty) {
-                // Just zero out the stack slot. (See comment on base::memzero for explanation)
+            if !return_type_is_void(tp_ty) {
+                // Just zero out the stack slot. (See comment on base::memzero for explaination)


Typo? Why was this comment changed in the first place?

Ah, there was a conflict and I missed that difference.

eddyb · Feb 20, 2015

LGTM modulo some nits, but I'd be happy with a second pair of eyes.

alexcrichton · Feb 20, 2015

r? @nikomatsakis

(I'm probably not the right reviewer for this)

nikomatsakis · Feb 25, 2015

Sorry for the delay. I somehow missed this PR.

nikomatsakis · Feb 25, 2015

nikomatsakis reviewed Feb 25, 2015

View changes

nikomatsakis · Feb 25, 2015

src/librustc/middle/ty.rs

+
+    pub fn ty(&self) -> Ty<'tcx> {
+        self.ty.get()
+    }


The current setup, iiuc, initially stores a ty_err and then updates it to the actual type. This seems like it will hide bugs where we read the type of a field before it is fully initialized. I would prefer to use Cell<Option<Ty<'tcx>>> and initially set it to None. Then ty can call self.ty.get().unwrap(), which will give the same (stronger) guarantees we get today. Similarly set_ty can check that the type is set exactly once. (There is no space inefficiency because Ty<'tcx> is a non-nullable-pointer.)

nikomatsakis · Feb 25, 2015

ok, I have to run for the moment. I'm a big fan of this general approach. I've been wanting to see the struct/enum code made nicer (and unified!) for a long time, and I think moving away from storing all the information in distinct side-tables is generally a good idea. I've only had time though to read over the high-level details of what is here, so I'll want some more time to soak in finer points.

nikomatsakis · Feb 25, 2015

nikomatsakis reviewed Feb 25, 2015

View changes

nikomatsakis · Feb 25, 2015

src/librustc_privacy/lib.rs

-                    ty::ty_struct(ctor_id, _) => {
-                        // RFC 736: ensure all unmentioned fields are visible.
-                        // Rather than computing the set of unmentioned fields
-                        // (i.e. `all_fields - fields`), just check them all.


Nit: Please don't drop this comment.

nikomatsakis · Feb 25, 2015

nikomatsakis reviewed Feb 25, 2015

View changes

nikomatsakis · Feb 25, 2015

src/rust-installer

@@ -1 +1 @@
-Subproject commit 60fd8abfcae50629a3fc664bd809238fed039617
+Subproject commit e577c97b494be2815b215e3042207d6d4b7c5516


this subproject commit is presumably unintentional?

nikomatsakis · Feb 25, 2015

I think I'd like to see the Cell<Option<Ty<'tcx>>> replaced with an Ivar<Ty<'tcx>>, where Ivar would be a type in librustc/middle/util/ivar.rs:

/// A "write once" cell.
pub struct Ivar<T:Copy> {
    data: Cell<Option<T>>
}

impl<T:Copy> Ivar<T> {
    pub fn get(&self) -> Option<T> {
        self.data.get()
    }

    pub fn fulfill(&self, value: T) {
        assert!(self.data.get().is_none());
        self.data.set(value);
    }

    pub fn unwrap(&self) -> T {
        self.get().unwrap()
    }
}

This has the advantage that (a) privacy ensures that we will only write to the cell once and (b) it can, in the future, be made Sync (we'd have to use atomic writes).

Aatch · Feb 26, 2015

I've squashed all the previous commits since rebasing was getting difficult due to functions being removed/replaced and me having to manually re-remove them post-rebase.

I have also added the Ivar type and used it for DatatypeDef

nikomatsakis · Feb 26, 2015

nikomatsakis reviewed Feb 26, 2015

View changes

nikomatsakis · Feb 26, 2015

src/librustc/metadata/encoder.rs

-        encode_struct_fields(rbml_w, &fields[..], def_id);
+          /* Encode def_ids for each field and method
+          for methods, write all the stuff get_trait_method
+          needs to know*/


Pre-existing,but this comment formatting is terrible.

nikomatsakis · Feb 26, 2015

@Aatch I commented on a few points that were made before but not corrected in your latest rebase. Overall I'm pretty happy with this.

One thing I was wondering -- perhaps it would make sense to merge ty_struct and ty_enum into ty_data or something like that. Just something to think about, for now.

nikomatsakis · Feb 26, 2015

Modulo those nits though I'm ready to r+.

pnkfelix · Feb 26, 2015

pnkfelix reviewed Feb 26, 2015

View changes

pnkfelix · Feb 26, 2015

src/librustc/util/ivar.rs

+    data: Cell<Option<T>>
+}
+
+impl<T:Copy> Ivar<T> {


I personally know what an "I-var" is, but it might be nice to either have some comments here just saying what the big picture is about them, or a pointer to http://en.wikipedia.org/wiki/Futures_and_promises or something.

(This comment should have been on the struct definition itself)

Aatch · Feb 27, 2015

@nikomatsakis yeah, I looked at merging them into a ty_data, but it's a non-trivial change on top of this already fairly extensive change. Definitely something to do in the near future though.

nikomatsakis · Feb 27, 2015

r+ from me, but of course there are tidy errors it looks like

bors · Feb 28, 2015

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

Ms2ger · Feb 28, 2015

There's a lot of &x[] slicing syntax, that I believe to be deprecated.

nikomatsakis · Feb 28, 2015

I was thinking about Ivar and I remembered that as I originally envisioned it, Ivar wouldn't require T:Copy but, rather, would take advantage of the fact that because it is only set once, it can return an &T to the contents even without using a RefCell (this would require unsafe code, of course). This doesn't have to go in the PR, I'm just noting it for future reference. ;)

eddyb · Feb 28, 2015

@nikomatsakis sounds like an "OnceCell".

pnkfelix · Mar 1, 2015

@eddyb I am not 100% sure I would apply the Cell terminology here, since part of the point is that these things do not start with an initial value, while all of the current kinds of cells do start with an initial value, right?

eddyb · Mar 1, 2015

@pnkfelix fair enough, I was just trying to force it into std::cell, but it probably fits better somewhere in std::sync.

nikomatsakis · Mar 17, 2015

@Aatch do you plan to rebase this?

nikomatsakis · Apr 8, 2015

@Aatch would you object if I rebased this?

Aatch · Apr 17, 2015

@nikomatsakis go for it. Sorry, I've been really busy with work + life recently, not much time for compiler-hacking.

nikomatsakis · Apr 17, 2015

@Aatch I started it. Lots of conflicts (of course). I might just try to cherry-pick and re-implement instead...gotta finish a few other things first.

alexcrichton · May 1, 2015

Closing in favor of @nikomatsakis's upcoming rebase

rust-highfive assigned alexcrichton Feb 20, 2015

rust-highfive assigned nikomatsakis and unassigned alexcrichton Feb 20, 2015

huonw referenced this pull request Feb 25, 2015
Merged
Handle stability of struct fields #22803

Aatch force-pushed the Aatch:ty-def branch from 1e9e7bc to 7df4133 Feb 26, 2015

Aatch force-pushed the Aatch:ty-def branch from 7df4133 to 777796b Feb 27, 2015

alexcrichton closed this May 1, 2015

rust-lang/rust

Join GitHub today

Add DatatypeDef #22564

Conversation

Aatch commented Feb 20, 2015

rust-highfive assigned alexcrichton Feb 20, 2015

This comment has been minimized.

rust-highfive commented Feb 20, 2015

This comment has been minimized.

eddyb commented Feb 20, 2015

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

eddyb reviewed Feb 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb commented Feb 20, 2015

This comment has been minimized.

alexcrichton commented Feb 20, 2015

rust-highfive assigned nikomatsakis and unassigned alexcrichton Feb 20, 2015

This comment has been minimized.

nikomatsakis commented Feb 25, 2015

nikomatsakis reviewed Feb 25, 2015 View changes

This comment has been minimized.

This comment has been minimized.

nikomatsakis commented Feb 25, 2015

nikomatsakis reviewed Feb 25, 2015 View changes

This comment has been minimized.

nikomatsakis reviewed Feb 25, 2015 View changes

This comment has been minimized.

This comment has been minimized.

nikomatsakis commented Feb 25, 2015

huonw referenced this pull request Feb 25, 2015

Aatch force-pushed the Aatch:ty-def branch from 1e9e7bc to 7df4133 Feb 26, 2015

This comment has been minimized.

Aatch commented Feb 26, 2015

nikomatsakis reviewed Feb 26, 2015 View changes

This comment has been minimized.

This comment has been minimized.

nikomatsakis commented Feb 26, 2015

This comment has been minimized.

nikomatsakis commented Feb 26, 2015

pnkfelix reviewed Feb 26, 2015 View changes

This comment has been minimized.

This comment has been minimized.

Aatch commented Feb 27, 2015

Aatch added some commits Jan 31, 2015

Aatch force-pushed the Aatch:ty-def branch from 7df4133 to 777796b Feb 27, 2015

This comment has been minimized.

nikomatsakis commented Feb 27, 2015

This comment has been minimized.

bors commented Feb 28, 2015

This comment has been minimized.

Ms2ger commented Feb 28, 2015

This comment has been minimized.

nikomatsakis commented Feb 28, 2015

This comment has been minimized.

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

eddyb reviewed Feb 20, 2015

View changes

nikomatsakis reviewed Feb 25, 2015

View changes

nikomatsakis reviewed Feb 25, 2015

View changes

nikomatsakis reviewed Feb 25, 2015

View changes

Aatch force-pushed the Aatch:ty-def branch from `1e9e7bc` to `7df4133` Feb 26, 2015

nikomatsakis reviewed Feb 26, 2015

View changes

pnkfelix reviewed Feb 26, 2015

View changes

Aatch force-pushed the Aatch:ty-def branch from `7df4133` to `777796b` Feb 27, 2015