Add test for comparing impl method type with trait bounds #12611

fhahn · 2014-02-27T23:00:02Z

This patch checks if all type bounds of an argument in an implementation of a trait can be found in the trait definition.

I'm not entirely sure about the wording of the error message though.

alexcrichton · 2014-02-27T23:11:14Z

I'm not sure this is quite exhaustive enough just yet, it looks like it would still allow code of the form:

trait A {
    fn foo<T: Eq + Ord>(&self);
}

impl A for int {
    fn foo<T: Ord + Ord>(&self) {}
}

alexcrichton · 2014-02-27T23:12:02Z

src/test/compile-fail/invalid-trait-bounds.rs

+
+struct Bar;
+impl Foo for Bar {
+    fn test_fn<T:ParamTrait>(&self, _: T) {} //~ ERROR: in method


Can you test the error message a little more exhaustively than just "in method" ?

I've added a couple of more tests and better error message checks.

flaper87 · 2014-02-28T11:28:58Z

I think this case is missing too:

trait A {}

trait B: A {}

trait C {
    fn test<T: A>(&self) {}
}

impl C for  int {
    fn test<T: B>(&self) {}
}

fn main() { }

In the example above the bounds of the method test in the trait C are stronger than the bounds of its implementation. This should be fine, but not the other way around, since A is implied by B.

(unless I'm missing something)

EDIT: I meant to say:

trait A {}

trait B: A {}

trait C {
    fn test<T: B>(&self) {}
}

impl C for  int {
    fn test<T: A>(&self) {}
}

fn main() { }

fhahn · 2014-02-28T22:56:54Z

I have a question concerning the following trait:

trait A {
    fn foo<T: Eq + Ord>(&self);
}

Are the following two implementations considered equivalent?

impl A for int {
    fn foo<T: Eq + Ord>(&self) {}
}

impl A for int {
    fn foo<T: Ord + Eq>(&self) {}
}

My PR at the moment assumes that the order must be equal as well (the second case would be invalid), but this behavior could be changed easily.

flaper87 · 2014-02-28T23:20:17Z

@fhahn bounds are commutative

flaper87 · 2014-02-28T23:31:10Z

src/librustc/middle/typeck/check/mod.rs

@@ -874,6 +872,33 @@ fn compare_impl_method(tcx: ty::ctxt,
                        trait_param_def.bounds.trait_bounds.len()));
            return;
        }
+        for (i, impl_bound) in impl_param_def.bounds.trait_bounds.iter().enumerate() {


I think this for loop could be simplified by using ty::each_bound_trait_and_supertraits. I'm sorry, I don't have much time to come up with a working example but I'd recommend you to take a look there, it iterates over the bounds and supertraits.

alexcrichton · 2014-02-28T23:34:59Z

I'm not entirely convinced that this is the right algorithm, and travis seems to agree with me. I'm not sure where supetraits come in to this verification, but I may just not be understanding what's going on.

fhahn · 2014-03-02T16:32:42Z

@flaper87 I'm not sure if your example from above should be valid.

trait A {
    fn foo();
}

trait B: A {
     fn bar();
}

trait C {
    fn test<T: A>(&self, x: T);
}

impl C for  int {
    fn test<T: B>(&self, x: T) {
         x.bar();
    }
}

As far as I understand, trait bounds should work the following way, but I could easily miss something.
The trait C says the function test can be called with an parameter which implements A. The implementation of C for int breaks this contract, because there the function test cannot be called with a trait just implementing A. Shouldn't arguments in subtypes of traits be contravariant, e.g. the call fn test<T: A>(&self, x: T) should be valid for every implementation of that trait. With contravariant arguments, the following would be possible:

trait A {
    fn foo();
}

trait B: A {
     fn bar();
}

trait C {
    fn test<T: B>(&self, x: T);
}

impl C for  int {
    fn test<T: A>(&self, x: T) {
         x.foo();
    }
}

This implementation of trait C does not break it's contract, because the function test for int could be called with an argument with type B, because the type in the implementation is more general.

flaper87 · 2014-03-03T13:21:00Z

@fhahn I'm sorry, it looks like I mistakenly inverted the bounds of the trait and its implementation. I meant to say what you wrote in your last comment. In other words, the idea is to make the bounds on the trait stronger than the bounds on the implementation. See this comment

flaper87 · 2014-03-03T13:21:33Z

Also, I'd like @nikomatsakis to double check this PR.

nikomatsakis · 2014-03-03T15:12:56Z

ok.

fhahn · 2014-03-03T22:07:33Z

I've update the patch. It now accepts contravariant argument bounds in implementations. I guess this should be documented as well? I've added a simple run-pass testcase with contravariant arguments.

The patch in it's current form requires the bounds to be unique, so the following definition would be problematic:

trait C {
    fn test<T: Eq + Eq>(&self);
}

impl C for  int {
    fn test<T: Eq + Eq >(&self) {}
}

I'm not sure if a bound definition with non-unique traits should be valid. If they are valid, should fn test<T: Eq + Eq >(&self); be equal to fn test<T: Eq >(&self);?

flaper87 · 2014-03-03T22:24:56Z

@fhahn we should probably raise a warning for non-unique bounds and drop 1 of them. I don't think that should be valid.

flaper87 · 2014-03-06T08:39:31Z

This needs to be rebased. @nikomatsakis any comments on this PR ?

nikomatsakis · 2014-03-06T13:52:00Z

Sorry, I plan to review, but it will take me some time. This week is very busy.

fhahn · 2014-03-07T22:29:12Z

I've rebased the PR. It raises errors for the following two cases:

a type parameter was specified in the trait definition but not in the implementation
a type parameter was specified in the implementation but not in the trait

Type parameters in the implementation can be supertypes of the type parameters in the trait.

The other commit raises a warning for duplicated type parameters and drops the duplicated parameter. One thing that isn't warned about at the moment are duplicated subtypes and supertypes.

Should I document this behaviour?

flaper87 · 2014-03-07T23:15:19Z

src/librustc/middle/typeck/collect.rs

@@ -1010,11 +1012,23 @@ pub fn ty_generics(ccx: &CrateCtxt,
            builtin_bounds: ty::EmptyBuiltinBounds(),
            trait_bounds: ~[]
        };
+
+        let mut added_bounds = HashMap::new();


What about using a MutableSet here?

MutableSet is a trait??

You probably mean HashSet.

erm yeah, sorry! I meant HashSet (I'm falling asleep right now)

Just to make sure there's no confusion. I meant HashSet

Update: Which is implemented using a map anyway, it just removes the thinking of map here.

Thanks, I've changed HashMap to HashSet

closes rust-lang#2687

alexcrichton · 2014-03-19T04:34:23Z

@nikomatsakis re-r? (just to make sure)

flaper87 · 2014-03-19T14:41:55Z

Seems like travis has some legitimate failures.

fhahn · 2014-03-19T21:56:24Z

I've tried to use infer::mk_sub_trait_refs as @nikomatsakis suggested, because his example did not throw an error with the previous check.

As far as I understand, infer::mk_sub_trait_refs should return result::Ok() if the trait bound is a subtype of the implementation bound (= the implementation bound is a super type of the trait bound), the relevant code is here: fhahn@a1bedb7#diff-31c6a728eb925b9f6b0124e93948d270R875 .

But I guess I'm using infer::mk_sub_trait_refs wrong?

nikomatsakis · 2014-03-20T14:36:22Z

Sorry, I'm very slow getting to reviews right now. This is on my list of "reviews to do", however.

nikomatsakis · 2014-03-21T09:50:55Z

src/librustc/middle/typeck/check/mod.rs

+        for impl_bound in impl_param_def.bounds.trait_bounds.iter() {
+            let mut specified = false;
+            for trait_bound in trait_param_def.bounds.trait_bounds.iter() {
+                if !enforced_bounds.contains(&trait_bound.def_id) {


This still looks wrong. I don't think we can key on def-id. I think you just need to remove this set and do something simpler. I imagine two nested loops (here I wrote it using filter/any). Basically you want to know: Are there any impl bounds which are not implied by some trait bound? If so, that's an error.

let failed_bounds = impl_bounds.filter(|ib| trait_bounds.any(|tb| implies(infcx, tb, ib))); fn implies(infcx, trait_bound, impl_bound) { is impl_bound a sub trait ref of trait_bound? }

Note that this doesn't take the full generality into account that we could. e.g., If the trait has Ord and the impl has Eq, that should be ok. We should be able to do that too; it's a matter of expanding out the "trait_bounds" vector to account for super bounds (or possibly changing mk_sub_trait_refs, I've been debating that in my head just now). But I think we can leave that aside for now, it'd really only be important for #5236 which is not on the table presently.

nikomatsakis · 2014-03-21T09:53:36Z

ok, I apologize for the delay, I left some comments, ping me when updated, and find me on IRC if you have any questions! Thanks very much for working on this.

alexcrichton · 2014-04-03T18:46:21Z

Closing due to inactivity, but feel free to reopen with a rebase!

with the corresponding trait parameter bounds. This is a version of the patch in PR rust-lang#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 rust-lang#2687, as well as the test case in rust-lang#5886, which is fixed as well by this patch. Closes rust-lang#2687. Closes rust-lang#5886. [breaking-change]

@pnkfelix

…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

alexcrichton reviewed Feb 27, 2014
View reviewed changes

flaper87 reviewed Feb 28, 2014
View reviewed changes

flaper87 reviewed Mar 7, 2014
View reviewed changes

fhahn added 3 commits March 14, 2014 21:36

Add check to compare an impl method type with trait bounds

d9346d8

closes rust-lang#2687

Check duplicated paramter bounds, refs rust-lang#2687

e7d98d5

Use mk_sub_trait_refs

a1bedb7

nikomatsakis reviewed Mar 21, 2014
View reviewed changes

alexcrichton closed this Apr 3, 2014

pcwalton mentioned this pull request Jul 2, 2014

librustc: Properly compare implementation method type parameter bounds #15325

Closed

fhahn deleted the issue-2687-cmp-bounds branch July 7, 2014 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for comparing impl method type with trait bounds #12611

Add test for comparing impl method type with trait bounds #12611

fhahn commented Feb 27, 2014

alexcrichton commented Feb 27, 2014

alexcrichton Feb 27, 2014

fhahn Feb 28, 2014

flaper87 commented Feb 28, 2014

fhahn commented Feb 28, 2014

flaper87 commented Feb 28, 2014

flaper87 Feb 28, 2014

alexcrichton commented Feb 28, 2014

fhahn commented Mar 2, 2014

flaper87 commented Mar 3, 2014

flaper87 commented Mar 3, 2014

nikomatsakis commented Mar 3, 2014

fhahn commented Mar 3, 2014

flaper87 commented Mar 3, 2014

flaper87 commented Mar 6, 2014

nikomatsakis commented Mar 6, 2014

fhahn commented Mar 7, 2014

flaper87 Mar 7, 2014

huonw Mar 7, 2014

flaper87 Mar 7, 2014

flaper87 Mar 7, 2014

fhahn Mar 10, 2014

alexcrichton commented Mar 19, 2014

flaper87 commented Mar 19, 2014

fhahn commented Mar 19, 2014

nikomatsakis commented Mar 20, 2014

nikomatsakis Mar 21, 2014

nikomatsakis commented Mar 21, 2014

alexcrichton commented Apr 3, 2014

Add test for comparing impl method type with trait bounds #12611

Add test for comparing impl method type with trait bounds #12611

Conversation

fhahn commented Feb 27, 2014

alexcrichton commented Feb 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flaper87 commented Feb 28, 2014

fhahn commented Feb 28, 2014

flaper87 commented Feb 28, 2014

Choose a reason for hiding this comment

alexcrichton commented Feb 28, 2014

fhahn commented Mar 2, 2014

flaper87 commented Mar 3, 2014

flaper87 commented Mar 3, 2014

nikomatsakis commented Mar 3, 2014

fhahn commented Mar 3, 2014

flaper87 commented Mar 3, 2014

flaper87 commented Mar 6, 2014

nikomatsakis commented Mar 6, 2014

fhahn commented Mar 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Mar 19, 2014

flaper87 commented Mar 19, 2014

fhahn commented Mar 19, 2014

nikomatsakis commented Mar 20, 2014

Choose a reason for hiding this comment

nikomatsakis commented Mar 21, 2014

alexcrichton commented Apr 3, 2014