rust-lang / rust

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

support `default impl` for specialization #37653

Open nikomatsakis opened 8 years ago

nikomatsakis commented 8 years ago

rust-lang/rfcs#1210 included support for default impl, which declared a set of defaults that other impls can inherit (if they correspond to a subset of the default impl's types). This was intended as a replacement/improvement of the existing default methods that appear in trait bodies (it was also originally called partial impl). Unfortunately, this feature is not yet implemented!

Some things to be sure of:

cc https://github.com/rust-lang/rust/issues/31844

nikomatsakis commented 8 years ago

cc @giannicic @aturon

giannicic commented 7 years ago

@nikomatsakis the PR addressing the 3rd task list item has been merged. Now i'm working on the first two points.

nikomatsakis commented 7 years ago

@giannicic awesome!

Phlosioneer commented 7 years ago

Any progress on this?

giannicic commented 7 years ago

@Phlosioneer Yes, the first point is done. Currently I'm working on the second one. I've looked at the trait resolution and identified where to place the default impl trait implementation check, now I'm figuring out how to properly implement this check because i need to differentiate it based on the cause of the obligation. For example, if i have a default impl that doesn't implement all the methods of the trait Foo, like the following

trait Foo {
    fn foo_one(&self) -> &'static str;
    fn foo_two(&self) -> &'static str;
}

default impl<T> Foo for T {
    fn foo_one(&self) -> &'static str {
        "generic"
    }
}

struct MyStruct;

a call like MyStruct.foo_one() should pass the trait resolution because of the first point. Instead if i call foo(MyStruct) with foo like fn foo<T: Foo>(x: T) -> &'static str the trait resolution should fail because of the second point. It could pass if the default impl implements all the methods of the Trait

petrochenkov commented 6 years ago

Can inherent impls be default?

This currently compiles successfully, but the compiler has zero tests for it, so it's probably unintended.

#![feature(specialization)]

struct S;

default impl S {}

fn main() {}

The general idea is reasonable though, e.g.

struct S<T>(T);

default impl<T> S<T> { fn f() { println!("Hello world!") } }
impl S<u8> { fn f() { println!("Goodbye world!") } } // This currently errors
petrochenkov commented 6 years ago

Can auto trait impls (including negative ones) be default?

This is currently accepted:

struct S;
struct Z;
default unsafe impl Send for S {}
default impl !Send for Z {}
petrochenkov commented 6 years ago

https://github.com/rust-lang/rust/pull/46455 conservatively makes both https://github.com/rust-lang/rust/issues/37653#issuecomment-348687794 and https://github.com/rust-lang/rust/issues/37653#issuecomment-348688785 errors.

Restioson commented 6 years ago

Really excited for full support of specialization (esp. point 2)! +1

giannicic commented 6 years ago

@petrochenkov as per RFC, there is no mentions about auto traits and a possible extension that covers inherent impls. But I suppose that it will not be implemented in the near future so, thank you for including an error about that in your PR :)

qnighy commented 6 years ago

As seen in the PR #45404, many people are confused about the syntax. The author of #45404, the author of #44790, the author of parquet-rs, and the poster of #48515 were at least confused about it.

Perhaps it is a good idea to propose a change to the syntax? For example resurrecting the partial impl keyword from rust-lang/rfcs#1210 or introduce an attribute like #[partial] default impl.

Phlosioneer commented 6 years ago

I don't think it's confusing, I think it just needs to be documented in the error messages of default impl. For example, in the parquet-rs error, the confusion would go away with a help message:

    = help: there is a default impl for the trait bound:
            `{{the default impl here}}`
            A default impl does not count as satisfying the trait bounds; try removing `default`.

Which would make the full error:

error[E0599]: no method named `set_data` found for type `encodings::decoding::PlainDecoder<T>` in the current scope
   --> src/column/reader.rs:443:18
    |
443 |       dictionary.set_data(page.buffer().clone(), num_values as usize)?;
    |                  ^^^^^^^^
    | 
   ::: src/encodings/decoding.rs:83:1
    |
83  | pub struct PlainDecoder<T: DataType> {
    | ------------------------------------ method `set_data` not found for this
    |
    = note: the method `set_data` exists but the following trait bounds were not satisfied:
            `encodings::decoding::PlainDecoder<T> : encodings::decoding::Decoder<_>`
    = help: items from traits can only be used if the trait is implemented and in scope
    = note: the following trait defines an item `set_data`, perhaps you need to implement it:
            candidate #1: `encodings::decoding::Decoder`
    = help: there is a default impl for the trait bound:
            `default impl<T: DataType> Decoder<T> for PlainDecoder<T>`
            A default impl does not count as satisfying the trait bounds; try removing `default`.

I think that does a really good job of not only pointing out the mistake with relevant info, but also teaching the user why it's an error and how it can be fixed.

@nikomatsakis @giannicic Pinging because of your work on #45404

Edit: formatting.

nikomatsakis commented 6 years ago

I am torn. On the one hand, I liked the "economy" of using default. I feel like having partial impl is its own sort of complexity. On the other hand, I think there is a kind of assumption that a default impl { .. } ought to be equivalent to impl { default ... }. I'm not sure though if that means that this is a natural rule that we ought to respect, or a matter of needing more education. Perhaps -- even -- it is the default keyword on functions and items that should be changed (e.g., to specializable or something).

Restioson commented 6 years ago

Personally, I was slightly confused as to why partial impl was scrapped in favour of default impl - default impl doesn't really sound like it can be partial...

RalfJung commented 4 years ago

all items in a default impl are (implicitly) default and hence specializable

I think this is a serious limitation, because it means there is no way to have a partial impl where some items are not specializable. I do not see that limitation discussed in the RFC, nor a motivation for why it might be required. It's a big RFC though, so maybe I missed it?

Here is a short discussion with a use-case for a partial unspecializable impl. Basically the idea is to implement some associated types of the trait, and then provide some default implementations that require this particular choice of associated type. I think the current approach makes that kind of reuse fundamentally impossible.

nikomatsakis commented 4 years ago

@RalfJung it was a deliberate simplification, but one that I'm still not 100% sure was the right call. The initial version of the RFC had partial impl and default fn as two distinct concepts. They were merged because it seemed pretty clear that it was going to be confusing somehow, particularly if we had partial default impl, but that did imply giving up on the ability to have some things fixed and some things not. You're right that the use case you cited then becomes impossible.

RalfJung commented 4 years ago

@nikomatsakis I see. IMO that limitation should be lifted before stabilization, it's rather surprising and frustrating that what seems like a purely syntactic choice makes some interesting specialization use-cases fundamentally impossible. Having default impl have both of these effects (enable partial impl and make every item overwritable) violates orthogonality and compositionality, values that otherwise Rust considers very important.

Could we get this listed as a concern to revisit in some tracking issue, maybe here?

nikomatsakis commented 4 years ago

@RalfJung I'd be willing to revisit this prior to stabilization, yes.

nikomatsakis commented 4 years ago

Actually, @RalfJung, it's already listed:

RalfJung commented 4 years ago

Oh, I was checking the items in this issue but not the main one. oops Thanks for adding a pointer to this sub-discussion here. :)

mankinskin commented 3 years ago

Will it be possible to define different default impls for differently trait bounded type parameters? e.g.

default impl<A: ToRoute> Routable for A {
    type Route = <A as ToRoute>::Route;
    fn route(&self) -> Self::Route {
        self.to_route()
    }
}

default impl<B: ToString> Routable for B {
    type Route = DefaultRoute;
    fn route(&self) -> Self::Route {
        DefaultRoute::from(self.to_string())
    }
}

In case of A: ToRoute and B: ToRoute + Debug, A should only match types not implementing Debug, i.e. the more specific trait bound has precedence.

joergbrech commented 2 years ago

I posted a suggestion on default impl in https://github.com/rust-lang/rust/issues/31844#issuecomment-995627907 and just realized it would have been more appropriate here. I don't know if my suggestion makes sense at all and if it isn't too late, since this issue is already well on its way. But I would love to hear your thoughts.

jplatte commented 2 years ago

@joergbrech I think you would be interested in the future possibilities section of RFC 2532 (associated type defaults).

joergbrech commented 2 years ago

@jplatte, thanks for the link. Yes default groups would be much more flexible!