RFC: DynSized without ?DynSized — Lint against use of `extern type` in `size_of_val`, and more

[kennytm]

Rendered

cc #2255
cc @mikeyhew, @mystor (#1993), @withoutboats

[Ixrec]

Tell me if I'm going crazy here, but given that...

Since “breaking changes to the standard library are not possible” using epochs, it may be impossible to [Replace all #[assume_dyn_sized] by proper T: DynSized bounds].

and

Even with this RFC ... dropping a Box will [either] panic [or] at best leaks the memory, and at worst overwrites unrelated memory and causes undefined behavior.

the RFC's final alternative of a "post-monomorphization error" actually seems like a strong contender to me (I mean in addition to the proposed trait and/or lints, not instead of), since it seems to be the only way to make foreign types actually typesafe.

Or is the proposed lint expected to be exhaustive/"actually typesafe", and the distinction between a lint and a post-monomorphization error merely academic here?

[eddyb]

Please don't refer to errors by error codes, or the error message, but rather for the semantic reason for the error.

Also note that other than resource limits, all the post-monomorphization errors come from unstable features (mostly inline assembly and platform intrinsics, both of which shouldn't have stabilized forms that don't move the errors to earlier checks).

[mikeyhew]

@kennytm, I have to say this RFC is really well written and researched. This kind of writing is painful for me, and when I write my first RFC, it's definitely going to be a lot shorter.

I haven't fully read everything yet, so I'll probably add more comments later.

It looks like your proposal is, basically, to still add the DynSized trait, without making it a default bound; except that instead of generating an error when the DynSized trait bound is unfulfilled, we output a warning, and presumably panic at runtime if the monomorphized type is indeed an extern type. Is that an somewhat-accurate depiction of what you are suggesting?

It seems rather complicated, but then again, most of the potential solutions, except maybe C++-style post-monomorphization errors, are fairly complicated.

In RFC 1861 (extern types), @canndrew writes in the Drawbacks section:

Very slight addition of complexity to the language.

Perhaps we should ask him what sort of implementation he had in mind 😄

[kennytm]

@mikeyhew Thanks :)

This proposal will add DynSized, yes. And yes, this stabilize the current behavior where size_of_val::<ExternType> still needs to be successfully monomorphized, either to a run-time panic or return 0.

This RFC is complicated since we want to avoid default bound, i.e. ?Sized is still ?Sized, and if we want DynSized, we need to write ?Sized + DynSized.

This proposal does not make failing to fulfill DynSized a warning (that is an alternative). If we substitute an extern type into T: DynSized + ?Sized, it will still be a compile-time error. And to the type inference engine, #[assume_dyn_sized] T: ?Sized still means T: ?Sized.

The problem is that, as DynSized is not implied, we cannot modify any existing std items from T: ?Sized to T: DynSized + ?Sized without breaking API compatibility.

Thus we cheat by introducing #[assume_dyn_sized], which does not modify the std API (size_of_val still takes T: ?Sized), so all existing code can still compile. But we emit a lint — which doesn't break compatibility — whenever T is not DynSized. So in the end we get want we want: a static check, while preserving stability ✌️.

Due to how epoch works, we could turn the lint into an error in the next epoch (2018). But the current epoch (2015) must still be supported, thus the #[assume_dyn_sized] hack must stay forever.

[mystor]

I'm not a big fan of the need to keep that hack around forever, but the rest of the system seems good to me. I didn't like the need for the ?DynSized default bound in my original RFC, and I'm glad we're finding some way to get by without it.

My understanding is that we are not adding post-monomorphization errors, correct? Rather we're emitting a warning for almost all callers of size_of_val and other types with ?Sized bounds which can't support non-DynSized types, which we will try to turn into an error in the future? We won't have to explain the assume_dyn_sized hack to people who want to learn rust in the future I hope.

[kennytm]

@mystor Yeah, #[assume_dyn_sized] should be an implementation detail that shouldn't need to be taught unless you want to study the standard library. Hence this RFC suggests that rustdoc should render #[assume_dyn_sized] T: X as T: DynSized + X to completely conceal this attribute outside of the original source.

[RalfJung]

Yeah, #[assume_dyn_sized] should be an implementation detail that shouldn't need to be taught unless you want to study the standard library. Hence this RFC suggests that rustdoc should render #[assume_dyn_sized] T: X as T: DynSized + X to completely conceal this attribute outside of the original source.

However, this also means that other libraries have no choice but do an API-breaking update by the time the lint becomes a hard error as they cannot benefit from this libstd-only hack, right?

[kennytm]

@RalfJung Yes, but it requires a new epoch. You could still stay in the 2015 epoch if a hard error is not wanted, but that's basically lying to yourself that "my code still works".

From what I checked there isn't really a lot of public interfaces expecting DynSized, so hopefully not too many packages need to update the major version.

[comex]

Now that I've actually read this RFC properly... I don't think #[assume_dyn_sized] actually needs to be a special-case compiler hack! It does require something that the compiler currently doesn't support, but that something is general-purpose; arguably just a bugfix, even.

It falls out of specialization that we can write:

trait HopefullyDynSized {}

impl<T: ?Sized> HopefullyDynSized for T {}

impl<T: ?Sized + DynSized> HopefullyDynSized for T {}

Now, what if we mark the first impl as deprecated?

#[deprecated(note = "how about adding a DynSized bound, dummy")]
impl<T: ?Sized> HopefullyDynSized for T {}

Currently… it compiles but has no effect: the compiler doesn't seem to produce a lint in any scenario, even if the impl is used with fully concrete !DynSized types. (playground link)

Relevant bug report: #39935: #[deprecated] does nothing if placed on an impl item.

But ideally, rustc could lint whenever trait resolution chooses that impl during the type checking stage (as opposed to trans/monomorphization). This wouldn't require any special extra queries, 'just' linting based on something it already knows. That said, the implementation might be nontrivial: AFAICT, trait resolution currently has no lints at all, and I'm not sure whether it keeps track of which span(s) are responsible for a trait obligation, which it'd need in order to know where to attach the lint… But that's clearly a solvable problem.

Then, instead of #[assume_dyn_sized], you could just write T: HopefullyDynSized. (Trait name is a placeholder, feel free to suggest better ones. ;p)

[kennytm]

@comex Interesting. As you mentioned, this relies on rust-lang/rust#39935, which is also what forces us to insta-stable all new impls. I wonder if fixing this would be easy ☺️

[stevenblenkinsop]

One thing this RFC does is double down on the idea that anything with a known size can be moved, since as long as something is DynSized (or assumed to be so), the standard library is allowed to move it. Right now, the Pin API doesn't require it, but if Rust ever decided to adopt first-class immovable types, you would probably have to add something like DynMove to mark things which can be moved dynamically, which would be distinct from DynSized set out here. Adding DynSized without DynMove now would make designing first-class immovable types at some point down the road more complicated, since not only would you have to deal with every T: Sized currently being movable, but also with every T: DynSized being dynamically movable. I'm not sure this is worth considering—Rust might never want first-class immovable types—but I thought I would note it here anyways.

[nikomatsakis]

@rfcbot fcp postpone

I am going to move to postpone this RFC. At present, there is only one kind of type for which this is relevant (extern types) and even more specifically one set of functions (size_of_val, align_of_val). We have a pending decision in rust-lang/rust#49708 that basically aims to layout a policy around size_of_val and align_of_val where they will panic/abort (tbd) when invoked on such a type. We would also expect lints to help people detect this whenever possible. (This proposal is quite similar to this RFC, in fact.)

This doesn't however foreclose adding a DynSized trait in the future, particularly if we move foward with some kind of custom DST proposal -- and one could imagine deprecating the existing methods and adding others that carry a DynSized bound, for example. In that case, some of the ideas from this RFC seem potentially useful and relevant, hence the proposal to postpone until that time (as ever, this is not meant to imply that would adopt this exact proposal, but rather that it contains useful ingredients).

[rfcbot]

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

• @aturon
• @cramertj
• @eddyb
• @joshtriplett
• @nikomatsakis
• @nrc
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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.

[rfcbot]

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

[rfcbot]

The final comment period, with a disposition to postpone, as per the review above, is now complete.

By the power vested in me by Rust, I hereby postpone this RFC.