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

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

Closed
eddyb opened this issue May 25, 2015 · 8 comments
Labels
A-DSTs Area: Dynamically-sized types (DSTs)

Comments

@eddyb
Copy link
Member

eddyb commented May 25, 2015

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 eddyb added the A-DSTs Area: Dynamically-sized types (DSTs) label May 25, 2015
@eddyb
Copy link
Member Author

eddyb commented May 25, 2015

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

@pnkfelix
Copy link
Member

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
Copy link
Member Author

eddyb commented May 26, 2015

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

@pnkfelix
Copy link
Member

@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 rust-lang/rfcs#1122 actually attempts to actually define backwards-compatibility hazards of this sort...)

@eddyb
Copy link
Member Author

eddyb commented May 26, 2015

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
Copy link
Member

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

Oh, of course

@nikomatsakis
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-DSTs Area: Dynamically-sized types (DSTs)
Projects
None yet
Development

No branches or pull requests

4 participants