Add check to compare an impl method type with trait bounds

closes rust-lang#2687

Author: fhahn

Diff for: src/librustc/middle/typeck/check/mod.rs

@@ -112,15 +112,16 @@ use middle::lang_items::TypeIdLangItem;
 use util::common::{block_query, indenter, loop_query};
 use util::ppaux;
 use util::ppaux::{UserString, Repr};
-use util::nodemap::{FnvHashMap, NodeMap};
+use util::nodemap::NodeMap;
+use collections::{HashMap, HashSet};
 
 use std::cell::{Cell, RefCell};
-use collections::HashMap;
 use std::mem::replace;
 use std::result;
 use std::vec;
 use std::vec_ng::Vec;
 use std::vec_ng;
+use std::str::StrVector;
 use syntax::abi::AbiSet;
 use syntax::ast::{Provided, Required};
 use syntax::ast;
@@ -864,25 +865,62 @@ fn compare_impl_method(tcx: ty::ctxt,
            return;
         }
 
-        // FIXME(#2687)---we should be checking that the bounds of the
-        // trait imply the bounds of the subtype, but it appears we
-        // are...not checking this.
-        if impl_param_def.bounds.trait_bounds.len() !=
-            trait_param_def.bounds.trait_bounds.len()
-        {
+        // check that the bounds of the trait imply the bounds of the implementation type
+        let mut enforced_bounds = HashSet::new();
+        let mut unspecified_bounds = ~[];
+        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) &&
+                        impl_bound.def_id == trait_bound.def_id {
+                    enforced_bounds.insert(trait_bound.def_id);
+                    specified = true;
+                    break
+                } else {
+                    for s_trait_bound in ty::trait_supertraits(tcx, trait_bound.def_id).iter() {
+                        if !enforced_bounds.contains(&trait_bound.def_id) &&
+                                impl_bound.def_id == s_trait_bound.def_id {
+                            enforced_bounds.insert(trait_bound.def_id);
+                            specified = true;
+                            break
+                        }
+                    }
+                }
+            }
+            if !specified {
+                unspecified_bounds.push(format!("`{}`", impl_bound.repr(tcx)));
+            }
+        }
+
+        let unenforced_bounds: ~[~str] =
+            trait_param_def.bounds.trait_bounds.iter().filter_map(|&trait_bound| {
+            if !enforced_bounds.contains(&trait_bound.def_id) {
+                Some(format!("`{}`", trait_bound.repr(tcx)))
+            } else {
+                None
+            }
+        }).collect();
+
+        if unenforced_bounds.len() > 0 {
+             tcx.sess.span_err(
+                impl_m_span,
+                format!("in method `{method}`, \
+                        {nbounds, plural, =1{trait bound} other{trait bounds}} \
+                        {bounds} not enforced by this implementation",
+                        method = token::get_ident(trait_m.ident),
+                        nbounds = unenforced_bounds.len(),
+                        bounds = unenforced_bounds.connect(", ")));
+        }
+        if unspecified_bounds.len() > 0 {
             tcx.sess.span_err(
                 impl_m_span,
                 format!("in method `{method}`, \
-                        type parameter {typaram} has \
-                        {nimpl, plural, =1{# trait bound} other{# trait bounds}}, \
-                        but the corresponding type parameter in \
-                        the trait declaration has \
-                        {ntrait, plural, =1{# trait bound} other{# trait bounds}}",
+                        implementation {nbounds, plural, =1{bound} other{bounds}} \
+                        {bounds} {nbounds, plural, =1{was} other{were}} \
+                        not specified in trait definition",
                         method = token::get_ident(trait_m.ident),
-                        typaram = i,
-                        nimpl = impl_param_def.bounds.trait_bounds.len(),
-                        ntrait = trait_param_def.bounds.trait_bounds.len()));
-            return;
+                        nbounds = unspecified_bounds.len(),
+                        bounds = unspecified_bounds.connect(", ")));
         }
     }
 

Diff for: src/librustc/middle/typeck/collect.rs

@@ -1034,6 +1034,17 @@ pub fn ty_generics(ccx: &CrateCtxt,
                 TraitTyParamBound(ref b) => {
                     let ty = ty::mk_param(ccx.tcx, param_ty.idx, param_ty.def_id);
                     let trait_ref = instantiate_trait_ref(ccx, b, ty);
+                    if added_bounds.contains_key(&trait_ref.def_id) {
+                        ccx.tcx.sess.span_warn(
+                            b.path.span,
+                            format!("duplicated bound `{}`, ignoring it",
+                                    trait_ref.repr(ccx.tcx)));
+                        continue;
+
+                    } else {
+                        added_bounds.insert(trait_ref.def_id, ());
+                    }
+
                     if !ty::try_add_builtin_trait(
                         ccx.tcx, trait_ref.def_id,
                         &mut param_bounds.builtin_bounds)

Diff for: src/test/compile-fail/typeck-trait-bounds-impl-comparison.rs

@@ -0,0 +1,71 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+//
+// Make sure rustc checks the type parameter bounds in implementations of traits,
+// see #2687
+
+trait A {}
+
+trait B: A {}
+
+trait C: A {}
+
+trait Foo {
+    fn test_error1_fn<T: Eq>(&self);
+    fn test_error2_fn<T: Eq + Ord>(&self);
+    fn test_error3_fn<T: Eq + Ord>(&self);
+    fn test3_fn<T: Eq + Ord>(&self);
+    fn test4_fn<T: Eq + Ord>(&self);
+    fn test_error5_fn<T: A>(&self);
+    fn test_error6_fn<T: A + Eq>(&self);
+    fn test_error7_fn<T: A>(&self);
+    fn test_error8_fn<T: B>(&self);
+}
+
+impl Foo for int {
+    // invalid bound for T, was defined as Eq in trait
+    fn test_error1_fn<T: Ord>(&self) {}
+    //~^ ERROR bound `std::cmp::Eq` not enforced by this implementation
+    //~^^ ERROR implementation bound `std::cmp::Ord` was not specified in trait definition
+
+    // invalid bound for T, was defined as Eq + Ord in trait
+    fn test_error2_fn<T: Eq + B>(&self) {}
+    //~^ ERROR bound `std::cmp::Ord` not enforced by this implementation
+    //~^^ ERROR implementation bound `B` was not specified in trait definition
+
+    // invalid bound for T, was defined as Eq + Ord in trait
+    fn test_error3_fn<T: B + Eq>(&self) {}
+    //~^ ERROR bound `std::cmp::Ord` not enforced by this implementation
+    //~^^ ERROR implementation bound `B` was not specified in trait definition
+
+    // multiple bounds, same order as in trait
+    fn test3_fn<T: Ord + Eq>(&self) {}
+
+    // multiple bounds, different order as in trait
+    fn test4_fn<T: Eq + Ord>(&self) {}
+
+    // parameters in impls must be equal or more general than in the defining trait
+    fn test_error5_fn<T: B>(&self) {}
+    //~^ ERROR bound `A` not enforced by this implementation
+    //~^^ ERROR implementation bound `B` was not specified in trait definition
+
+    fn test_error6_fn<T: A>(&self) {}
+    //~^ ERROR bound `std::cmp::Eq` not enforced by this implementation
+
+    fn test_error7_fn<T: A + Eq>(&self) {}
+    //~^ ERROR implementation bound `std::cmp::Eq` was not specified in trait definition
+
+    fn test_error8_fn<T: C>(&self) {}
+    //~^ ERROR implementation bound `C` was not specified in trait definition
+    //~^^ ERROR bound `B` not enforced by this implementation
+
+}
+
+fn main() {}

Diff for: src/test/run-pass/typeck-contravariant-trait-bounds-impl.rs

@@ -0,0 +1,66 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+//
+// Tests contravariant type parameters in implementations of traits, see #2687
+
+trait A {
+    fn a(&self) -> int;
+}
+
+impl A for u8 {
+    fn a(&self) -> int {
+        8
+    }
+}
+
+impl A for u32 {
+    fn a(&self) -> int {
+        32
+    }
+}
+
+trait B: A {
+    fn b(&self) -> int;
+}
+
+impl B for u32 {
+    fn b(&self) -> int {
+        -1
+    }
+}
+
+trait Foo {
+    fn test_fn<T: B>(&self, x: T) -> int;
+    fn test_duplicated_bounds1_fn<T: B+B>(&self) -> int;
+    //~^ warn: duplicated bound `B`, ignoring it
+    fn test_duplicated_bounds2_fn<T: B>(&self) -> int;
+}
+
+impl Foo for int {
+    fn test_fn<T: A>(&self, x: T) -> int {
+        x.a()
+    }
+
+    fn test_duplicated_bounds1_fn<T: B+B>(&self) -> int {
+        //~^ warn: duplicated bound `B`, ignoring it
+        99
+    }
+
+    fn test_duplicated_bounds2_fn<T: B+B>(&self) -> int {
+        //~^ warn: duplicated bound `B`, ignoring it
+        199
+    }
+}
+
+fn main() {
+    let x: int = 0;
+    assert!(8 == x.test_fn(5u8));
+    assert!(32 == x.test_fn(5u32));
+}