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

comparing bounds on impl method type parameters with bounds declared on trait is completely bogus #2687

Closed
nikomatsakis opened this issue Jun 21, 2012 · 12 comments
Labels
A-typesystem Area: The type system E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Milestone

Comments

@nikomatsakis
Copy link
Contributor

Right now, a fn implementing an iface method must match the purity of the iface method exactly. This is too strict. We should allow a pure fn to implement an impure fn, and both pure/impure to implement an unsafe fn.

UPDATE: Updated title to reflect the real problem here. There is a FIXME in the code in the relevant area. If one declares a trait like:

trait Foo {
    fn m<T:Eq>(...);
}

then in the impl, the m method should take one type parameter with an Eq bound. We don't really check this correctly though. As far as I can tell, we just check that m has one type parameter with one bound, but not precisely what kind of bound it is.

@nikomatsakis
Copy link
Contributor Author

Similarly, in ac1f84c, we require that the bounds on the method parameters be precisely equivalent, which is stronger than necessary. It is sufficient (I believe) that the bounds on the trait be stronger than the bounds on the implementation.

nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Oct 27, 2012
@thestinger
Copy link
Contributor

Is this still an issue? It seems to be fixed now.

@nikomatsakis
Copy link
Contributor Author

Still an issue with respect to type parameter bounds.

@pcwalton
Copy link
Contributor

I don't believe this is backwards incompatible, renominating.

@graydon
Copy link
Contributor

graydon commented Jun 6, 2013

accepted for feature-complete milestone

@huonw
Copy link
Member

huonw commented Jul 23, 2013

The following appears to work, but I assume that this is what @thestinger was talking about:

trait Foo {
    unsafe fn x();
}

impl Foo for int {
    unsafe fn x() {}
}
impl Foo for uint {
    fn x() {}
}

fn main() {}

@metajack
Copy link
Contributor

triage bump. nothing to add.

@pnkfelix
Copy link
Member

Accepted for P-backcompat-lang

@nikomatsakis
Copy link
Contributor Author

Added UPDATE to main bug description above.

@fhahn
Copy link
Contributor

fhahn commented Feb 13, 2014

I had a look at this issue and tried to create a test case. @nikomatsakis does this test case covers the issue you mentioned?
This test case fails because it seems like the parameter types are checked in librustc/middle/typeck/check/vtable.rs

@flaper87
Copy link
Contributor

@fhahn what error does it fail with? In the impl you need to use a different type bound for test_fn in order to test this issue. To be more precise, the bounds of the implementation should be implied by the bounds of the trait. For example, this should fail:

trait ParamTrait {}

struct Param;

impl ParamTrait for Param {}

struct Bar;

trait Foo {
    fn test_fn<T:Eq>(&self, x: T, y: T);
}

impl Foo for Bar {
    // The type param of the implementation is not
    // the same as specified in the trait `Foo` nor implied
    // by it.
    fn test_fn<T: ParamTrait>(&self, x: T, y: T) {}
}

fn main() {
    let x: Bar = Bar;
    x.test_fn(Param, Param);
}

@fhahn
Copy link
Contributor

fhahn commented Feb 27, 2014

@flaper87 thanks for the hint, I guess your updated example should indeed test this issue. I'll start working on a patch.

fhahn added a commit to fhahn/rust that referenced this issue Mar 14, 2014
fhahn added a commit to fhahn/rust that referenced this issue Mar 14, 2014
@emberian emberian reopened this Jun 19, 2014
bors added a commit that referenced this issue Jul 3, 2014
…felix

with the corresponding trait parameter bounds.

This is a version of the patch in PR #12611 by Florian Hahn, modified to
address Niko's feedback.

It does not address the issue of duplicate type parameter bounds, nor
does it address the issue of implementation-defined methods that contain
*fewer* bounds than the trait, because Niko's review indicates that this
should not be necessary (and indeed I believe it is not). A test has
been added to ensure that this works.

This will break code like:

    trait Foo {
        fn bar<T:Baz>();
    }

    impl Foo for Boo {
        fn bar<T:Baz + Quux>() { ... }
        //             ^~~~ ERROR
    }

This will be rejected because the implementation requires *more* bounds
than the trait. It can be fixed by either adding the missing bound to
the trait:

    trait Foo {
        fn bar<T:Baz + Quux>();
        //             ^~~~
    }

    impl Foo for Boo {
        fn bar<T:Baz + Quux>() { ... }  // OK
    }

Or by removing the bound from the impl:

    trait Foo {
        fn bar<T:Baz>();
    }

    impl Foo for Boo {
        fn bar<T:Baz>() { ... }  // OK
        //       ^ remove Quux
    }

This patch imports the relevant tests from #2687, as well as the test
case in #5886, which is fixed as well by this patch.

Closes #2687.
Closes #5886.

[breaking-change]

r? @pnkfelix
RalfJung pushed a commit to RalfJung/rust that referenced this issue Nov 27, 2022
ensure current getrandom works with strict provenance
Aaron1011 pushed a commit to Aaron1011/rust that referenced this issue Jan 6, 2023
ensure current getrandom works with strict provenance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-typesystem Area: The type system E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
10 participants