Skip to content

DST syntax #13398

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

Closed
wants to merge 4 commits into from
Closed

DST syntax #13398

wants to merge 4 commits into from

Conversation

nrc
Copy link
Member

@nrc nrc commented Apr 8, 2014

Now with proper checking of enums and allows unsized fields as the last field in a struct or variant. This PR only checks passing of unsized types and distinguishing them from sized ones. To be safe we also need to control storage.

Closes issues #12969 and #13121, supersedes #13375 (all the discussion there is valid here too).

@nrc nrc mentioned this pull request Apr 8, 2014
@nrc
Copy link
Member Author

nrc commented Apr 8, 2014

@nikomatsakis r?

let arg_tys: Vec<ty::t> = ty::ty_fn_args(ctor_ty).iter().map(|a| *a).collect();
for i in range(0, args.len()) {
let t = arg_tys.get(i);
// Allow the last field in an enum to be unsized.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not obvious to me why this is allowed. It would be nice to extend this comment explaining why this is allowed. ( @nick29581 thanks for the explaining it to me )

@nikomatsakis nikomatsakis self-assigned this Apr 10, 2014
@pnkfelix
Copy link
Member

@flaper87 Can you more concretely explain your objection to trait A for type { }?

I think the analogous change, assuming we use the Sized? (or Unsized?) generalization marker, would be trait A for Sized? { ... }, not this trait A? { ... } thing that you wrote above, which I would definitely object to.

The whole point is that we are making a statement about the types that may implement this trait, not about the trait itself. (i.e. that was @nikomatsakis objection to e.g. unsized trait Foo { ... } Perhaps that is too subtle a distinction to attempt to encode in this manner.

I don't see how trait type A {} would be clearer, in terms of the above issue. (Or really, I just don't see how it would clearer at all, it just seems jumbled to me.)

@flaper87
Copy link
Contributor

@pnkfelix sorry for the quite vague comment.

My concern is about how trait A for type {} is read. The main thing I dislike is the use of a keyword type in the trait definition.

I believe trait A for Sized? { ... } is a better option. As mentioned on IRC, I don't mind it being a bit wordy if it makes the trait definition clearer and easier to read. I really would like it to express what it does by just reading it w/o requiring deep language knowledge.

You also mentioned trait A for Unsized? Self { ... } on IRC and I think it's even more explicit and clear than the other options we have.

@nrc
Copy link
Member Author

nrc commented Apr 20, 2014

The exact syntax is still open discussion - my preference is also for the Sized? notation. Given that is might take a while to settle on syntax, I would like to land this as is and fix the syntax as a follow-up.

@nrc
Copy link
Member Author

nrc commented Apr 20, 2014

Addressed @flaper87's comments.

r?

@@ -3379,14 +3432,16 @@ pub fn check_representable(tcx: &ty::ctxt,
/// is representable, but not instantiable.
pub fn check_instantiable(tcx: &ty::ctxt,
sp: Span,
item_id: ast::NodeId) {
item_id: ast::NodeId) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: place return type on next line

@nikomatsakis
Copy link
Contributor

This patch passes all compile-fail tests, at least:

diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs
index 5aebd3b..cd33dbf 100644
--- a/src/librustc/middle/ty.rs
+++ b/src/librustc/middle/ty.rs
@@ -1996,6 +1996,10 @@ pub fn type_is_sendable(cx: &ctxt, t: ty::t) -> bool {
     type_contents(cx, t).is_sendable(cx)
 }

+pub fn type_is_sized(cx: &ctxt, t: ty::t) -> bool {
+    type_contents(cx, t).is_sized(cx)
+}
+
 pub fn type_interior_is_unsafe(cx: &ctxt, t: ty::t) -> bool {
     type_contents(cx, t).interior_unsafe()
 }
@@ -2565,35 +2569,6 @@ pub fn type_is_machine(ty: t) -> bool {
     }
 }

-// Is the type's representation size known at compile time?
-#[allow(dead_code)] // leaving in for DST
-pub fn type_is_sized(cx: &ctxt, ty: ty::t) -> bool {
-    match get(ty).sty {
-        ty_param(tp) => {
-            assert_eq!(tp.def_id.krate, ast::LOCAL_CRATE);
-
-            let ty_param_defs = cx.ty_param_defs.borrow();
-            let param_def = ty_param_defs.get(&tp.def_id.node);
-            param_def.bounds.builtin_bounds.contains_elem(BoundSized)
-        },
-        ty_self(def_id) => {
-            let trait_def = lookup_trait_def(cx, def_id);
-            trait_def.bounds.contains_elem(BoundSized)
-        },
-        ty_struct(def_id, ref substs) => {
-            let flds = lookup_struct_fields(cx, def_id);
-            let mut tps = flds.iter().map(|f| lookup_field_type(cx, def_id, f.id, substs));
-            !tps.any(|ty| !type_is_sized(cx, ty))
-        }
-        ty_tup(ref ts) => !ts.iter().any(|t| !type_is_sized(cx, *t)),
-        ty_enum(did, ref substs) => {
-            let variants = substd_enum_variants(cx, did, substs);
-            !variants.iter().any(|v| v.args.iter().any(|t| !type_is_sized(cx, *t)))
-        }
-        _ => true
-    }
-}
-
 // Whether a type is enum like, that is an enum type with only nullary
 // constructors
 pub fn type_is_c_like_enum(cx: &ctxt, ty: t) -> bool {

@nrc
Copy link
Member Author

nrc commented Apr 21, 2014

With more changes

@nrc
Copy link
Member Author

nrc commented Apr 22, 2014

r=nikomatsakis

bors added a commit that referenced this pull request Apr 23, 2014
Now with proper checking of enums and allows unsized fields as the last field in a struct or variant. This PR only checks passing of unsized types and distinguishing them from sized ones. To be safe we also need to control storage.

Closes issues #12969 and #13121, supersedes #13375 (all the discussion there is valid here too).
@bors bors closed this Apr 23, 2014
@nrc nrc deleted the unsized-enum branch April 23, 2014 09:09
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 26, 2024
Fixes rust-lang#13375

I've added the lint next to the other attribute-related ones. Not sure
if this is the correct place, since while we are looking after the
`packed`-attribute (there is nothing we can do about types defined
elsewhere), we are more concerned about the type's representation set by
the attribute (instead of "duplicate attributes" and such).

The lint simply looks at the attributes themselves without concern for
the item-kind, since items where `repr` is not allowed end up in a
compile-error anyway.

I'm somewhat concerned about the level of noise this lint would cause
if/when it goes into stable, although it does _not_ come up in
`lintcheck`.

```
changelog: [`repr_packed_without_abi`]: Initial implementation
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants