Tracking issue for tweaks to dyn compatibility (RFC 2027)

[withoutboats]

This is the tracking issue for RFC 2027, tweaks to dyn-compatibility¹ (rust-lang/rfcs#2027)

• Implement (Object safe for dispatch #57545)
• Document
• Stabilize

Footnotes

1. Formerly known as "object safety". ↩

[withoutboats]

If anyone can mentor I'd be excited to try to implement this.

From my naive perspective, it seems like what we need to do is stop checking object safety at certain points (possibly determining if a type is well formed?) and start checking it at other points (checking if the object type implements its corresponding trait). More insight would be great, cc'ing @eddyb

[eddyb]

I'm afraid @nikomatsakis understands the details here much better than me.

[withoutboats]

I experimented with implementing this, but I definitely don't know what I'm doing. I noticed some interesting cases.

trait Foo: Sized { }

fn foo(_: &Foo) { panic!() }

fn main {
    let x: Box<Foo>;
}

Today these types are just invalid types, but I believe this RFC would make them valid. You would just never be able to initialize x or call foo. I didn't consider these in the RFC, but it seems fine to me.

[JeanMertz]

Hey @withoutboats, I was just wondering if anything has changed for this RFC since it was approved?

I don’t have the skills to tackle this, and you probably don’t have the time, but I wanted to “bump” this anyway. Perhaps it can get some more mentoring love to move it forward?

[withoutboats]

I still wish this would be implemented, but nothing has changed and I also don't have the knowledge of compiler internals to mentor anyone on it.

[bovinebuddha]

So, I took a crack at this over the holidays. Tried to wrap my head around the compiler internals.

I think I've got most of the RFC working. Currently trying to put all the relevant code behind a feature gate, I'll put up a PR in a couple of days when that's working. All the tests that were broken, ~30 IIRC (at least of those I managed to run), failed because of anticipated reasons. So all good so far.

Just a few corner cases I've been thinking about. The RFC is a little vague on some of the details of what exactly should compile or not.

• What should the feature gate be called? I used 'object_safe_for_dispatch' from the RFC but I think something that indicates that it's more about 'non-object safety trait objects are legal', or maybe 'symmetric_object_safety'.

• How do we alert users about its existence?The RFC basically turns previously illegal code into legal code, but there is no 'good' place to do emit_feature_err!("Static dispatch for non-object safe traits not stable"). Should we add some extra text to E0038 ('cannot be made into an object') to clarify that there is a feature unlocking the error?

• Should the following code compile:

trait Foo {
    fn foo() {}
}

trait Bar: Sized {}

impl Foo for dyn Bar {}

fn static_poly<T: Foo>() {
    T::foo();
}

fn main() {
    Bar::foo(); // Works now!
    static_poly::<dyn Bar>(); // Does not work currently
}

• What about if I add the following:

fn inferred_poly<T: Foo>(t: &T) {
    T::foo();
}

fn infer_using_trait_obj(t: &dyn Bar) {
    inferred_poly(t); // Does not work currently
}

• I currently removed the automatic 'impl Trait for dyn Trait' for non object-safe traits in src/librustc/traits/select.rs which I think is part of the old style trait solving, and not using chalk. Can I switch to use the new trait style solving with chalk somehow? Or is it used under the hood? The rustc guide isn't really clear on if there two styles coexist or if there is a WIP opt-in somewhere.

• Also, the RFC mentions we could possibly allow manual 'impl Trait for dyn Trait'. Is this something we would want? I figure it would be most prudent to add a new error forbidding this, and a future RFC could add it later.

Sorry for the Wall of Text.

TL/DR - Expect a pull request in a couple of days, some thorny questions remain.

@withoutboats

[withoutboats]

@bovinebuddha Is it intentional in your code samples that you do not do T: ?Sized + Foo? It seems necessary that the code you wrote does not compile because dyn Bar does not implement Sized, and all type parameters are implicitly bound Sized unless you add ?Sized.

[bovinebuddha]

Ah... What a gotcha! Quite new to Rust, forgot about traits defaulting to !Sized.

Adding that solved made the call in main() compile. The inferred call still doesn't compile with the following message:

23 | fn infer_using_trait_obj(t: &dyn Bar) {
   |                             -------- help: add explicit lifetime `'static` to the type of `t`: `&'static (dyn Bar + 'static)`
24 |     inferred_poly(t);
   |     ^^^^^^^^^^^^^ lifetime `'static` required

I'm not 100% confident in exactly how traits and lifetimes intermingle. Changing to the following did not do the trick:

fn inferred_poly<'a, T: Foo + ?Sized + 'a>(t: &'a T) {
    T::foo();
}

and neither did adding the caller to:

fn infer_using_trait_obj<'a>(t: &'a (dyn Bar + 'a)) {
    inferred_poly(t);
}

which resulted in this:

error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
  --> src/main.rs:24:19
   |
24 |     inferred_poly(t);
   |                   ^
   |
note: first, the lifetime cannot outlive the lifetime 'a as defined on the function body at 23:26...
  --> src/main.rs:23:26
   |
23 | fn infer_using_trait_obj<'a>(t: &'a (dyn Bar + 'a)) {
   |                          ^^
   = note: ...so that the expression is assignable:
           expected &dyn Bar
              found &(dyn Bar + 'a)
   = note: but, the lifetime must be valid for the static lifetime...
   = note: ...so that the types are compatible:
           expected Foo
              found Foo

[withoutboats]

The reason is that you have only implemented Foo for dyn Bar + 'static (because thats the inference in the impl header position).

All of these are normal & expected errors, then (even if they are confusing!). Seems like you got the feature working :)

[bovinebuddha]

Yay :)

I realized the problem I had with the feature gate was that I wrote #[feature...] instead of #![feature...], so that's working now.

Is there any way of writing the lifetimes so that the inferred method compiles though? Or should it even compile in any form? I tried adding separate lifetimes for the referance and the trait bound and an outlives relationship but that didn't do the trick...

[withoutboats]

either of these changes should make it compile:

impl<'a> Foo for dyn Bar + 'a { }

fn infer_using_trait_obj(t: &dyn Bar + 'static) { } 

[bovinebuddha]

Ah, now I reread your post and actually understood what you wrote. That's what you get for coding late at night. Gonna see if it the generic impl will do the trick tomorrow.

[bovinebuddha]

Btw, that was the problem I had with 'impl Trait for dyn Trait', didn't realize there was an implicit 'static there. This means that this actually compiles for me:

#![feature(object_safe_for_dispatch)]

trait Bad {
    fn bad_stat() { println!("trait bad_stat") }
    fn bad_virt(&self) { println!("trait bad_virt") }
    fn bad_indirect(&self) { Self::bad_stat() }
}

trait Good {
    fn good_virt(&self) { panic!() }
    fn good_indirect(&self) { panic!() }
}

impl<'a> Bad for dyn Bad + 'a {
    fn bad_stat() { println!("obj bad_stat") }
    fn bad_virt(&self) { println!("obj bad_virt") }
}

struct Struct {}

impl Bad for Struct {}

impl Good for Struct {}

fn main() {

    let s = Struct {};

    // Manually call static
    Struct::bad_stat();
    <dyn Bad>::bad_stat();

    let good: &dyn Good = &s;

    let bad = unsafe {std::mem::transmute::<&dyn Good, &dyn Bad>(good)};

    // Call virtual
    s.bad_virt();
    bad.bad_virt();

    // Indirectly call static
    s.bad_indirect();
    bad.bad_indirect();
    
}

and produces

trait bad_stat
obj bad_stat
trait bad_virt
obj bad_virt
trait bad_stat
obj bad_stat

[crlf0710]

I don't know if this is too late, but i just realized that after implementing this RFC we'll lose the ability of the compiler automatically checking object safety when we use the trait objects but didn't generating one...

If we're writing a library and designing traits, we might easily create functions that's never callable at all by accident after this change...

[mjbshaw]

This might be a terrible idea, but what if making a trait non-object-safe triggered a lint, and then if it was indeed intentional the author could add a #[allow(...)] to silence the lint?

[bovinebuddha]

I don't really think this will be an issue. If you do write a function which isn't callable (which it technically is though, just not with a safe to construct argument), I would expect the library to actually try and use the trait object, and this will lead to a compile time error for the library writer.

The only 'hidden' errors I think you might get is if you are writing a library with traits that are not handled by the library in itself, and you really really want these to be object safe for your users. I guess such libraries do exist, but I don't know if this is a problem we need to handle.

Maybe, just maybe, there should be some way of hinting to the compiler that this trait should really be object safe. OTOH, that check is one unit test away.

[mikeyhew]

@bovinebuddha how is your implementation coming along? I've done some work on object safety in the compiler would be happy to chat about it in Discord and look at your code.

[bovinebuddha]

@mikeyhew I think I got the implementation down. There's a pull request at https://github.com/rust-lang/rust/pull/57545. Please, check out the code and any suggestions or comments would be much appreciated!

I seem to have been stuck in review limbo unfortunately. I also had some conflicts to resolve, just rebased and pushed, hopefully it will pass on Travis. (I got some weird llvm errors building locally. For some reasons I get local edits to .cpp files in src/llvm-project when running ./x.py sometimes, I think that might have caused problems).

Cheers

[mikeyhew]

@bovinebuddha great! I will take a look at your PR and comment there.

[Dylan-DPC-zz]

@mikeyhew unfortunately that PR was inactive so we closed it. So if you are still interested, you can implement it.

[memoryruins]

#![feature(object_safe_for_dispatch)] is now available on nightly!
Implemented by @bovinebuddha and @nikomatsakis in #57545

[joshtriplett]

Paging @nikomatsakis, it would help to have a summary of this to know what state it's in and whether it's on a path to stabilization.