rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.6k stars 12.74k forks source link

Combining explicit T: Sized and T: ?Sized bounds is silently accepted and results in T: Sized. #25776

Closed eddyb closed 9 years ago

eddyb commented 9 years ago
fn foo<T: Sized + ?Sized>() {}

The above results in no warning or error in 1.0 and 1.1.

While this may seem harmless, the Sized bound can be propagated from a trait:

trait NotObjectSafe: Sized {...}
fn foo<T: NotObjectSafe + ?Sized>() {
    is_sized::<T>();
}
fn is_sized<T: Sized>() {}

This is a backwards-compatibility hazard, as NotObjectSafe cannot be made object-safe (by adding where Self: Sized to each method which requires it) without causing NotObjectSafe + ?Sized to start allowing unsized types for T, where it previously wouldn't have.

A concrete example is trait Clone: Sized which I believe could be object-safe if the Sized bound was moved to its two methods. Clone: Sized prevents, e.g. [T]: Copy where T: Copy which could be used to write constructors for Rc<[T]> (and perhaps even for Rc<Trait+Copy>).

It's also arguably unintuitive, as ?Sized has no effect but it may show up in documentation, advertising an usecase which isn't actually supported.

eddyb commented 9 years ago

cc @nikomatsakis @pnkfelix @brson Is this considered a bug?

pnkfelix commented 9 years ago

Certainly the current semantics (that ?Sized is essentially ignored) is what I expect.

a lint seems like a good idea to me.

I wouldn't want a hard error due to potential for macros to expand into such combinations.

eddyb commented 9 years ago

@pnkfelix what about the backwards compatibility issues? How do lints interact with stability guarantees?

pnkfelix commented 9 years ago

@eddyb my understanding is that we are allowing warning lints to start or stop firing with arbitrary releases of Rust; i.e. that there are no guarantees with respect to the stability of lints (or at least of lints that are not set to error-by-default; not sure how many of those we actually have).


The other supposed backwards compatibility issue you raise (or, if you prefer, the main such issue) is this:

NotObjectSafe cannot be made object-safe ... without causing T: NotObjectSafe + ?Sized to start allowing unsized types for T, where it previously wouldn't have.

But my understanding is that this would solely allow more code to compile than before -- it won't cause any potential code to start being rejected, right?

I ask because as I understand it, such a change is also not a backwards-compatibility hazard.

(Interestingly, I don't think the current draft https://github.com/rust-lang/rfcs/pull/1122 actually attempts to actually define backwards-compatibility hazards of this sort...)

eddyb commented 9 years ago

I was asking about lints being used to defend stability guarantees: i.e. can we make a backwards-incompatible change if the only uses would have required disabling an error-by-default lint?

In my example foo would stop compiling as the call to is_sized::<T> is no longer valid. Also, any by-value uses of T would cause errors.

pnkfelix commented 9 years ago

In my example foo would stop compiling as the call to is_sized::<T> is no longer valid.

Oh, of course

nikomatsakis commented 9 years ago

So @pnkfelix raised this question of the fact that Copy: Sized. Here are some (somewhat scattered) thoughts on this topic. TL;DR is that I think the current design is ok and there are lots of ways forward but I'm not sure on the best one.

First, Copy today combines two things: "affine" and "can be memcpy". We've thought about separating this in the past (Pod), but decided against it because it didn't seem worthwhile if the reason was simply to act as a lint. Unsized values present a more interesting case though.

I personally expect unsized values to be affine. Currently, they can never be moved by the compiler, but the only way that they could that I can see is we passed a pointer into the current location (since the size is not known to the compiler at compilation time). I've described this before as permitting unsized values to be passed as parameters. This only makes sense if all unsized things are affine, because otherwise when the callee mutates the value, it will mutate a copy that the caller can see, which violates the mental model.

If we did support moves, then you could move a [T] (or Trait) into an Rc -- for any T. I think this is probably what you really wanted to do, not to copy or clone. And if it was copy or clone that you wanted, you could achieve that for slices at least by calling to_vec first and then moving out of the vec.

It seems like wanting to copy/clone a [T] into an Rc will be relatively uncommon (versus move), and traits even more so. The former could also be achieved with a special case method that takes a &[T] for T: Clone and clones it into an Rc (which seems more general than the copy case). The trait case could be covered via a CloneInto trait, as has been tossed around from time to time.

Finally, we could add a Pod trait as has been discussed, with a blanket impl that says T: Copy => T: Pod. This would probably interact poorly with coherence in the absence of specialization, though. This would be similar to the prior proposal but with different aims, since here one would want to avoid linearity not because of a "lint-like" use but because of unsized values. Still, of all the ways forward, it's the least appealing to me.

nikomatsakis commented 9 years ago

I don't think there is an actual bug here... going to close. Re-open if you disagree.