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

support default impl for specialization #37653

Open
1 of 3 tasks
nikomatsakis opened this issue Nov 8, 2016 · 24 comments
Open
1 of 3 tasks

support default impl for specialization #37653

nikomatsakis opened this issue Nov 8, 2016 · 24 comments
Labels
A-specialization Area: Trait impl specialization A-trait-system Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. F-specialization `#![feature(specialization)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 8, 2016

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:

  • a default impl need not include all items from the trait
  • a default impl alone does not mean that a type implements the trait
  • all items in a default impl are (implicitly) default and hence specializable

cc #31844

@nikomatsakis nikomatsakis added A-trait-system Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-specialization Area: Trait impl specialization E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Nov 8, 2016
@nikomatsakis
Copy link
Contributor Author

cc @giannicic @aturon

bors added a commit that referenced this issue Apr 27, 2017
#37653 support `default impl` for specialization

this commit implements the first step of the `default impl` feature:

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

In order to test this feature I've copied all the tests provided for the
`default` method implementation (in run-pass/specialization and
compile-fail/specialization directories) and moved the `default` keyword
from the item to the impl.
See [referenced](#37653) issue for further info

r? @aturon
@giannicic
Copy link
Contributor

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

@nikomatsakis
Copy link
Contributor Author

@giannicic awesome!

@Phlosioneer
Copy link
Contributor

Any progress on this?

@giannicic
Copy link
Contributor

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

petrochenkov commented Dec 2, 2017

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

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

#46455 conservatively makes both #37653 (comment) and #37653 (comment) errors.

@Restioson
Copy link

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

@giannicic
Copy link
Contributor

@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 :)

bors added a commit that referenced this issue Jan 14, 2018
syntax: Rewrite parsing of impls

Properly parse impls for the never type `!`
Recover from missing `for` in `impl Trait for Type`
Prohibit inherent default impls and default impls of auto traits (#37653 (comment), #37653 (comment))
Change wording in more diagnostics to use "auto traits"
Fix some spans in diagnostics
Some other minor code cleanups in the parser
Disambiguate generics and qualified paths in impls (parse `impl <Type as Trait>::AssocTy { ... }`)
Replace the future-compatibility hack from #38268 with actually parsing generic parameters
Add a test for #46438
bors added a commit that referenced this issue Feb 16, 2018
#37653 support `default impl` for specialization

this commit implements the second part of the `default impl` feature:

>  - a `default impl` need not include all items from the trait
>  - a `default impl` alone does not mean that a type implements the trait

The first point allows rustc to compile and run something like this:

```
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;

fn  main() {
    assert!(MyStruct.foo_one() == "generic");
}
```

but it shows a proper error if trying to call `MyStruct.foo_two()`

The second point allows a `default impl` to be considered as not implementing the `Trait` if it doesn't implement all the trait items.
The tests provided (in the compile-fail section) should cover all the possible trait resolutions.
Let me know if some tests is missed.

See [referenced ](#37653) issue for further info

r? @nikomatsakis
@qnighy
Copy link
Contributor

qnighy commented Feb 25, 2018

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

Phlosioneer commented Feb 25, 2018

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

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

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

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

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

RalfJung commented Apr 21, 2020

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

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

@nikomatsakis
Copy link
Contributor Author

Actually, @RalfJung, it's already listed:

  • Should we have default impl (where all items are default) or partial impl (where default is opt-in)

@RalfJung
Copy link
Member

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

mankinskin commented Dec 21, 2020

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

I posted a suggestion on default impl in #31844 (comment) 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
Copy link
Contributor

jplatte commented Dec 16, 2021

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

@joergbrech
Copy link

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

@matthewjasper matthewjasper added the F-specialization `#![feature(specialization)]` label Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-specialization Area: Trait impl specialization A-trait-system Area: Trait system C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. F-specialization `#![feature(specialization)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests