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

Add DynSized trait (rebase of #44469) #46108

Closed
wants to merge 17 commits into from

Conversation

@mikeyhew
Copy link
Contributor

mikeyhew commented Nov 20, 2017

I managed to rebase @plietar's PR #44469, and get all the tests passing.

This unfortunately conflicts with #44917 (by @Zoxc) in a lot of places, so one of us will have to do a rebase after the other gets merged.

r? @pnkfelix since you were reviewing #44469
cc @arielb1

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Nov 20, 2017

Letting auto trait imply ?Move and ?DynSized might be a good idea. It would at least avoid all the changes to the tests.

@nikomatsakis Any chance that bounds on Self will become sound for auto traits in the future?

@mikeyhew

This comment has been minimized.

Copy link
Contributor Author

mikeyhew commented Nov 20, 2017

A couple of interesting points that weren't mentioned in the conversation on #44469:

  • DynSized is an implicit supertrait of all traits, including Sized. This is in contrast to Sized, which is only an implicit bound on type parameters and associated types.

  • As a result, this messes up auto traits, which may not have super traits. After this PR, all auto traits must be updated to explicitly remove DynSized as a super trait:

    #![feature(optin_builtin_traits, dynsized)]
    use core::marker::DynSized;
    unsafe auto trait Auto: ?DynSized {}
  • Whenever another implicit supertrait is added, auto traits will break again. This will happen when #44917 lands, which adds an implicit supertrait Move.

Because of the potential for future breakage, it may make sense to special-case auto traits, so that they don't have any implicit supertraits. As long as auto traits can't have items, it hopefully should be possible to add or remove implicit supertraits without breaking. Or alternatively, if auto traits could be allowed to have supertraits, we could allow that, as @Zoxc has just mentioned

@mikeyhew

This comment has been minimized.

Copy link
Contributor Author

mikeyhew commented Nov 20, 2017

cc @bluss

@mikeyhew

This comment has been minimized.

Copy link
Contributor Author

mikeyhew commented Nov 20, 2017

@Zoxc I saw your comment just before I posted mine, just didn't want to rewrite everything. You basically said everything that I was saying, but in way fewer words :)

@mikeyhew

This comment has been minimized.

Copy link
Contributor Author

mikeyhew commented Nov 20, 2017

ugh... src/test/rustdoc/foreigntype.rs is failing, but I don't have that file in my local branch... guess I need to rebase

plietar and others added some commits Sep 3, 2017

Add a DynSized trait.
The DynSized trait is implemented by all types which have a size and alignment
known at runtime. This includes every type other than extern types, introduced
in RFC 1861 and implemented in #44295, which are completely opaque.

The main motivation for this trait is to prevent the use of !DynSized types as
struct tails. Consider for example the following types :
```rust
extern {
  type foo;
}

struct A<T: ?Sized> {
    a_x: u8
    a_y: T
}

struct B<T: ?Sized> {
    b_x: u8
    b_y: T
}
```

Before this change, the type `A<B<foo>>` is considered well-formed. However,
the alignment of `B<foo>` and thus the offset of the `a_y` field depends on the
alignment of `foo`, which is unknown.

By introducing this new trait, struct tails are now required to implement
`DynSized`, such that their alignment is known. The trait is an implicit bound,
making `A<B<foo>>` ill-formed.

Just like the `Sized` trait, the default bound can be opted-out by using
`?DynSized`.
fix some auto_trait tests by removing the DynSized bound
auto traits cannot have bounds, so they must remove the implicit
DynSized bound with `Self: ?DynSized`
don't print `DynSized` in conflicting trait impl error messages
This fixes a bunch of ui tests. It now either prints out `T`, `T:
?Sized`, or `T: ?DynSized`.
don't bug! if the unsized struct tail isn't [T], str, or a trait object
There are other types that can be in a DST struct’s tail: other DST
structs, DST tuples, and possibly more. The only one we don’t want to
allow is extern types, so I added a check for that
fix run-make/simd-ffi test
had to add the dynsized lang item, and add ?DynSized bound to auto traits
fix some rustdoc tests
Had to add ?DynSized to auto-traits
Update auto traits in the unstable book
Add a `?DynSized` unbound to the auto trait in the example, and explain
why it is necessary
Fix src/test/rustdoc/foreigntype.rs
extern types do not implement DynSized, so to implement a trait for one
you need to explicitly remove the DynSized bound for that trait

@mikeyhew mikeyhew force-pushed the mikeyhew:add-dynsized branch from 27133cc to b9a69c7 Nov 21, 2017

@liigo

This comment has been minimized.

Copy link
Contributor

liigo commented Nov 21, 2017

dynamically (at runtime) sized types are DynSized, that's ok.
but statically (known at compiletime) sized types are DynSized?? that's a bit weird.
Maybe you should rename DynSized. (ClearSized? VS Opaque types)

@mikeyhew

This comment has been minimized.

Copy link
Contributor Author

mikeyhew commented Nov 21, 2017

@liigo anything that is Sized is trivially also DynSized. If you know the size of something at compile time, then you know it a runtime. That's why DynSized is a supertrait of Sized, though that's not immediately obvious from the declaration of Sized because the DynSized bound is implicit

@mikeyhew

This comment has been minimized.

Copy link
Contributor Author

mikeyhew commented Nov 21, 2017

@iigo I'll try to explain that a bit better: DynSized (in this interpretation) doesn't mean "not statically sized", but rather "has a size that can be calculated at run time".

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Nov 24, 2017

Ping from triage @pnkfelix — will you be able to review this shortly?

@shepmaster shepmaster added the T-lang label Dec 1, 2017

@mikeyhew

This comment has been minimized.

Copy link
Contributor Author

mikeyhew commented Jan 11, 2018

  • T: ?Dynsized can still be owned, just not using Box. People will probably implement Drop for T by calling some C api function, and use a different owning pointer type that just calls ptr::drop_in_place when dropped.
  • The distinction between T and T: ?Sized will be clouded a bit if/when we have by-value DST.
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 11, 2018

@mikeyhew

just not using Box

Yes; not using the Rust allocator, basically. Not sure how important that is. (Sometimes I regret designing our allocators so that they supply the size of the value being freed, but I guess it's awfully late to try and reverse course there.)

The distinction between T and T: ?Sized will be clouded a bit if/when we have by-value DST.

Yes, true.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Jan 11, 2018

(Sometimes I regret designing our allocators so that they supply the size of the value being freed, but I guess it's awfully late to try and reverse course there.)

It seems like the use cases for size_of_val diminish significantly without this requirement. I wonder if we couldn't revisit this?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 23, 2018

OK, I feel bad that this PR is sitting here with no viable means to go forward or backward yet. I remain highly nervous about ? traits like ?DynSized. It still feels to me like this is overall a bad direction for Rust to be going in. But I don't want to get into a litany of these concerns (though it makes sense to write those out and work through them). I would rather focus on the fate of this PR.

i.e., do we want to move forward with ?DynSized even in an experimental capacity? Or should we close the PR?

(It seems clear we have to ensure it is 100% backwards compatible; I've kind of lost track of whether that is the case now.)

@mikeyhew if your main motivation is to explore custom DST, I'd rather just explore that directly, while leaving the questions around DynSized slightly unresolved. I continue to feel they are somewhat orthogonal questions.

@mikeyhew

This comment has been minimized.

Copy link
Contributor Author

mikeyhew commented Jan 24, 2018

  • @nikomatsakis and @withoutboats, and anyone else with concerns about ?DynSized and other ?-traits, a good place to discuss them is in rust-lang/rfcs#2255
  • Regarding backward compatibility, this PR is in theory backward compatible, but there is still this issue, which will cause some regressions. Back in October, @Zoxc listed crates that were affected by that issue in this comment
  • about custom DST and related stuff, I would also like to move ahead and experiment with that. What I can do is open a thread on internals and discuss the design that I have in mind, using the DynSized trait, and then you can poke and prod and we can discuss alternatives
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 24, 2018

@mikeyhew

a good place to discuss them is in rust-lang/rfcs#2255

agreed, this PR doesn't feel like the right place!

this PR is in theory backward compatible, but there is still this issue, which will cause some regressions

OK, I see, I remember that now. Thanks for the clarification.

What I can do is open a thread on internals and discuss the design that I have in mind, using the DynSized trait, and then you can poke and prod and we can discuss alternatives

Sounds like a plan.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Jan 25, 2018

FYI I've submitted rust-lang/rfcs#2310 as an alternative.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 26, 2018

@rfcbot fcp close

We discussed this in the @rust-lang/lang meeting today. As a result of that conversation, I'm going to move to close this PR. The general feeling was that the "jury is still out" on what approach to use for handling extern types and the related problems -- as @withoutboats and I have shared here, there is a general concern about the complexity costs of a ?DynSized-based solution (and ?-trait-bounds in general), particularly when weighed against the relative obscurity of extern types (it's a lot of language baggage for a corner case). On the other hand, @mikeyhew and others make a strong case for static guarantees.

Given this uncertainty, I at least am reluctant to land deep changes to the compiler at this time, and in particular not to the trait system (which I think is due for some overhauls). Past experience (e.g., with the ! type) shows that this often has unexpected consequences, causing regressions and other maintenance burden, which then distracts from higher priority goals.

Speaking personally: I do intend to leave a comment on rust-lang/rfcs#2255 "real soon now" trying to spell out some of the specific confusion that can arise from ?DynSized. I am also still intrigued by the possibility of finding an "ownership-based" way of thinking about optional traits along the lines I outlined before, though I don't know if that really leads anywhere.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 26, 2018

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Feb 3, 2018

Ping for ticky boxes @Zoxc, @arielb1, @jseyfried, @michaelwoerister!

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 5, 2018

I tagged @arielb1 (who should be moved to alumni status, really) as well as @michaelwoerister (who is on vacation afaik) and @jseyfried (who probably also should be moved to alumni status).

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 5, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

kennytm added a commit to kennytm/rfcbot-rs that referenced this pull request Feb 6, 2018

@aturon

This comment has been minimized.

Copy link
Member

aturon commented Feb 6, 2018

anp added a commit to rust-lang/rfcbot-rs that referenced this pull request Feb 7, 2018

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 15, 2018

The final comment period is now complete.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 15, 2018

OK, going to close. Thanks @mikeyhew

@SimonSapin SimonSapin referenced this pull request Mar 28, 2018

Open

Tracking issue for RFC 1861: Extern types #43467

1 of 3 tasks complete
@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Mar 29, 2018

@nikomatsakis The language team decision against ?DynSize looked to me like "jurry is still out, so the fact that the code is in flux (chalk, etc) we use as a tie-breaker". That sounds to me more like "not now" than "no never": is there a possibility for landing this on an experimental basis once the type system overhauls are done?

The problem seems to be ?-traits syntax more than semantics that people want. I say landing this on an experimental basis is best way to find a different syntax / explore downstream problems like custom DSTs. Otherwise too many chained problems are hard to untangle.

@mikeyhew

This comment has been minimized.

Copy link
Contributor Author

mikeyhew commented Mar 29, 2018

@Ericson2314 I agree with you, I think that ultimately having a trait or traits like DynSized is desired, and that the decision to close this PR wasn't a rejection of the underlying concept, but rather a "let's postpone this for now, and get a better idea of what we want to do first".

I've been meaning to comment on https://internals.rust-lang.org/t/pre-erfc-lets-fix-dsts/6663 and keep the discussion going, and I still think that is the best way forward – we should get an eRFC merged and start working on things experimentally.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Apr 4, 2018

@mikeyhew See the end of #43467 . Looks like DynSized is off the table for now, but hopefully it could come back as a non-default bound with {size,align}_of_bound being deprecated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.