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

Object safety check fixes #18527

Merged
merged 2 commits into from
Nov 3, 2014
Merged

Object safety check fixes #18527

merged 2 commits into from
Nov 3, 2014

Conversation

bkoropoff
Copy link
Contributor

Closes #18490

r? @nick29581

This also fixes rust-lang#18490 as a side-effect by avoiding a later
out-of-bounds slice.
@bkoropoff bkoropoff changed the title Fix object safety check Object safety check fixes Nov 2, 2014
@aturon
Copy link
Member

aturon commented Nov 2, 2014

Part of the motivation for the object safety change was that if Trait is object safe then Box<Trait>: Trait. But if Trait has static methods, then there's no way to automatically implement it for Box<Trait>, sadly.

(That said, static methods weren't discussed when making the decision, and seem to be a significant downside to this change, so we may need to reconsider the whole thing.)

@bkoropoff
Copy link
Contributor Author

That's a good point. The current behavior permits static methods as long as they follow the other rules (e.g. not referring to Self), so I erred on the side of permissivity. Should I tweak the patch to just avoid the panic and maintain the status quo otherwise?

@nrc
Copy link
Member

nrc commented Nov 2, 2014

@aturon to be precise with the static method issue, is this the situation you worry about:

trait T {
    fn foo();
}

fn bar<X: T>(x: Box<T>) {
    X::foo();
}

fn main() {
    let y: Box<T> = ...;
    bar(y);
}

Where the monomorphised version of X::foo() for X as T can't have an impl?

@aturon
Copy link
Member

aturon commented Nov 2, 2014

@nick29581 Yes, that's one example.

More generally, there have been a lot of complaints about the new object safety rules not allowing you to have constructors-like methods (that produce Self) even though these are, in some sense, perfectly reasonable for trait objects. After all, you still construct the initial object with a concrete type before boxing it up, and after that point all actual methods may be perfectly usable with trait objects. Having to split out the constructor(s) into a separate trait seems rather unfortunate.

Unlike other examples we've seen, where splitting the trait made things more flexible, this seems to unequivocally make the API worse.

Some real-life examples where the splitting up is having to occur:

https://github.com/hyperium/hyper/blob/master/src/header/mod.rs#L26-L43
https://github.com/hyperium/hyper/blob/master/src/net.rs#L44-L61

@nrc
Copy link
Member

nrc commented Nov 2, 2014

@bkoropoff Yeah, I think taking a patch like this is the thing to do for now - given that we don't use object safety for the auto-impl stuff, this seems like the right fix (and brings us as close as possible to the old situation of per-method object safety).

We do need to think about object safety in general though, it seems like all static methods object-unsafe once we do the auto-impls.

@nrc
Copy link
Member

nrc commented Nov 3, 2014

I opened rust-lang/rfcs#428 to discuss the proper fix for the object-safety/static method issue.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Nov 3, 2014
@bors bors merged commit 107af28 into rust-lang:master Nov 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: 'rustc' panicked at 'assertion failed: *start <= *end' in slice.rs:396
4 participants