implement new object safety rules

[nrc]

r? @nikomatsakis

[huonw]

I'm increasingly concerned about this from a library design view point. It requires defining and importing a whole pile of extra traits, and also requires a library author to remember and think about "enabling" trait objects for a trait.

E.g. Iterator and std::rand::Rng are traits for which it is sensible to make trait objects, but they have generic methods, and, the standard library/tests/compiler never make trait objects out of them, meaning this 'problem' will only be picked up by users. Separating out a trait to rectify this is backwards incompatible, and also may require users to manually implement multiple traits (not everything is like Iterator and Rng where the generic methods are default methods).

Most libraries will have less associated code than our stdlib and compiler, and so even less "chance" for a trait object to be made and hence a lower chance of detecting the problem locally.

[alexcrichton]

I agree with @huonw, and issues like #17701 worry me a great deal if that's what we're forced to modify to go down this route. I know of some current uses of boxed iterators that basically have no replacement if this lands. Are there possibly alternative solutions to rust-lang/rfcs#255 which don't involve this splitting up of traits?

[tbu-]

incclude -> include

[aturon]

I share @huonw and @alexcrichton's concerns about the impact of this on library APIs. In particular, @huonw's point that this

requires a library author to remember and think about "enabling" trait objects for a trait.

is a very good one that was not discussed at the meeting where we accepted this RFC. And as we're seeing with the changes needed for std alone, this looks like it would involve a fair amount of additional API complexity.

I think this RFC merits another round of discussion before we land these changes, where we think through the concrete implications on std. I also think we should consider the alternatives more closely.

That said, it's interesting to consider the design of Iterator with respect to trait objects. Yes, you can create trait objects around iterators today, but you lose most of the adapters -- and not for any good reason. Right now, these adapters are set up via default methods, which are basically not overridable because they nail down specific concrete iterator return types. If we did split this out as a core trait with e.g. just next, and an IteratorAdapters trait with the rest of the methods, we could provide a blanket impl of IteratorAdapters for any Iterator. That would allow the adapters to be used even for boxed iterators.

(On the other hand, in the future these adapter types could be provided as associated types via HKT, and then you could specialize the implementations for performance reasons...)

[nikomatsakis]

I'm happy to discuss it more, but I still think this is the right approach. I think it's a GOOD thing that trait authors have to think about enabling traits for objects at trait design time. Specifically, if you have some trait X that is not object safe, then it means that if we permit objects to be created they are second-class citizens: many of the methods of the trait are unavailable, and you cannot use them with generic functions (because the type X cannot implement the trait X).

Iterator makes for a good case study. The fact that we could create objects of type Iterator means that we think the current design is object-friendly, but actually it's quite hostile: you can call next on an Iterator<int>, but not map or filter. Moreover, if I have some generic function:

fn foo<T:Iterator<int>>(t: T) { ... }

then I can't use my Box<Iterator<int>> with this fn either.

It would be better if we made the core iterator trait object-safe and then implemented map and other things as extension traits. This would mean that iterators cannot override map and friends, but in exchange they would be usable on objects. Seems like a good trade to me personally.

[nikomatsakis]

Well, let me rephrase. I don't think it's "GOOD" that we don't detect failures until you try to use as an object. But I worry very much about composability -- and I don't like the idea of second-class objects floating around that have some but not all of the capabilities of their trait.

[nikomatsakis]

Just to make sure my point comes through: It's true that you don't detect the fact that a trait is not object-safe until you try to make an object, but in the current design, it's even worse: you don't detect the problems around trait objects until you actually try to use them in a non-trivial way.

(Ok, last comment, I promise)

[aturon]

@nikomatsakis I had the same thought about iterators, though in the long run it would be nice to be able to override adapter implementations when you can do something smarter for your particular iterator (though that requires HKT, etc.) Splitting up the trait will force the issue.

I take your point about needing to think about these issues when designing an API, but the worry is: what happens when some API you don't control fails to do so?

I suppose you can always work around it by creating your own trait, with just the object-callable methods, and a blanket impl that just calls out to the existing trait.

[aturon]

@nikomatsakis

Actually, a follow-up for iterators:

Suppose we went with the design where you can create trait objects for any trait, but that you only get DST-style impls for certain traits.

We would not get the automatic DST-style impl for Iterator, but we could always provide an impl ourselves. That would retain the simplicity of having a single trait, while allowing full-fledged use of boxed iterators.

Another, somewhat related worry: fully automatic DST-style impl of traits for trait objects means that you'll never be able to provide your own custom impl. Is that a problem? I think it's probably fine, but worth thinking about.

[nikomatsakis]

@aturon Yes, I realize it's a tradeoff. Clearly this is a point about which reasonable people can disagree.

I think I'd prefer to make the distinction sharper precisely because I think it's easily forgotten, so the more we gray the line the more people get confused. I think it's clear from this conversation that it's very easy to not realize the repercussions of generic methods in a trait and so forth. (It almost makes me wonder if it's worth having a different declaration form or some other way to opt-in to advanced features.)

It's worth considering what the workarounds are in both cases. Under this design, end users who want an object but can't have one have to make a copy of the trait that deletes the disallowed methods. This is annoying, but has the advantage that its limitations are clear: naturally you won't be able to the disallowed methods on the new trait, since they're not there, and naturally you won't be able to use the trait elsewhere.

Under the other design, the workaround is... well, I don't know what it is. I guess we could permit you to hand-write an impl that failed if you tried to use the negative methods.

Regarding overriding map -- it occurs to me that impl specialization could give us a "best of both worlds". That is, you can make the IteratorMap trait specializable -- and then specialize it when the iterable type is known to be your particular iterator. (Also, if we want to preserve the ability to override at all, we have to be very careful about the return type, as you are well aware, since you originally raised the issue.)

[nikomatsakis]

@aturon I guess that my specialized-based proposal and your impl Iterator for Iterator proposal wind up at the same place. As a relevant aside, many people I've spoken to about this issue over the years when we've been debating it :) have found the very notion of impl Iterator for Iterator confusing. Basically I think that the pun of using a trait name for both a type and an object is a good one so long as the brain can kind of elide the difference -- but when there start to be subtle differences, it can be hard to even talk about.

[nrc]

I don't trait objects are as common as we are imagining and therefore this is not as big a problem as it looks: e.g., there is not a single Iterator object in all the libraries and compiler. And in all that code there were only 3(?) traits which broke with this change. And I agree with Niko, in those cases, the split caused a better design - extracting the by_ref methods from Reader/Writer seems like a good idea.

We discussed @aturon's suggestion, the problem is that having T not implement T for some traits seems even worse than having T not implement T all the time - I would hate to have to explain that to someone. Whereas the object-safety rules are pretty easy to explain.

[nikomatsakis]

I have to run atm, but to argue devil's advocate: it is true that even if Trait impl Trait always holds, if you write a function like:

fn foo<T:Trait>(x: T) { ... }

it is STILL not usable with objects by default, it'd have to be:

fn foo<Sized? T:Trait>(x: T) { ... }

This weakens my argument considerably. :)

(Though I have been privately wondering if implied bounds could allow us to remove the Sized? default and just not have any defaults at all. I need to do some experiments here.)

[alexcrichton]

One of the key points here to me is that the author may not actually be the one who's actually thinking about the trait-object-ness of the trait. I think that the base of idiomatic Rust code is expanding quickly beyond the libraries and compiler, so I'm not sure how much longer we can use that as our metric to determine the impact of a change. Here it's easy to say that Iterator (or any other trait) is not being used as an object, but as the authors of the trait we don't know how to assess downstream code using our trait.

[nrc]

@alexcrichton when that is true we are in a library/client situation and we can expect library designers to think of this sort of thing, its just another part of library design.

We are pre-1.0, we can break downstream code as long as there is a good work around, which I think here there is. Do you know if iterator objects are common in downstream code? It seems a bad idea to conjecture here.

[alexcrichton]

I only personally know of the one example I linked to above, I'm unaware if there are more instances.

[reem]

@alexcrichton In the future, that use of Box<Iterator> could be replaced with an associated type + HKT:

pub trait Headers {
    // Theoretical
    pub type HeaderEntries<'a>: Iterator<(&'a str, Vec<&'a str>)>;

    fn find<'a>(&'a self, key: &str) -> Option<Vec<&'a str>>;
    fn has(&self, key: &str) -> bool;
    fn iter<'a>(&'a self) -> HeaderEntries<'a>;
}

That said, we can't do it today, which is critical since there is no promise of when if ever HKT will be added, and it seems it will almost certainly be after 1.0.

In my experience with downstream Rust code, Box<Iterator> is not particularly common as it is slow for Iterators common case, but use of trait objects for traits which are not fully object safe is much more common.

For instance, in Iron, we've got:

pub trait Chain: Handler {
    fn new<H: Handler>(H) -> Self;
    fn link<B, A>(&mut self, (B, A)) where A: AfterMiddleware, B: BeforeMiddleware;
    fn link_before<B>(&mut self, B) where B: BeforeMiddleware;
    fn link_after<A>(&mut self, A) where A: AfterMiddleware;
    fn around<A>(&mut self, A) where A: AroundMiddleware;
}

which is not object safe, but it is still useful to make Box<Chain> objects because you can still call the inherited Handler methods through one. In practice, this is usually accomplished through making it a Box<Handler>, but this is only one case. It's entirely possible that Chain could grow more methods which could be called through an Object, and losing the ability to do Box<Chain> would then be very painful.

Generally, it's sometimes useful to have methods which you can only call through T: Trait but still have the ability to do Box<Trait>. Under this proposal, we would have to have two different traits, which just feels unnecessarily unergonomic.

[nikomatsakis]

@reem in that case, it seems that you already have two traits -- why would you not make a Box<Handler> object?

[nikomatsakis]

Oh, I see, I misunderstood. You said you may want to add more methods in the future.

[nrc]

We discussed and approved this in today's meeting. I just rebased it.

r? @nikomatsakis

[aturon]

Can you instead provide a blanket impl of this trait for all Readers?

[aturon]

(That would avoid this being a breaking change.)

[aturon]

Thanks @nick29581 for adding the blanket impls.

It occurs to me that if we want to fully avoid breakage, we need to add these new traits to the prelude, which is fine by me. @alexcrichton is that ok with you?

[aturon]

@nick29581 I talked with @alexcrichton about this on IRC, and his feeling is that he'd rather stomach a bit of breakage than further pollute the prelude (we're not sure whether Reader/Writer should be there anyway).

So this is fine as-is.

Can you adjust the commit message that includes the [breaking-change] tag to show how to fix code that previously relied on these methods, i.e. by important the relevant trait?

[nikomatsakis]

r+ modulo new test