Implemented checks for breaking traits.

We don't check trait items if the trait itself suffered breaking
changes.

Author: ibabushkin

src/semcheck/mapping.rs

@@ -17,10 +17,10 @@ use syntax::ast::Name;
 /// losslessly. The *access* to the stored data happens through the same API, however.
 #[derive(Default)]
 pub struct IdMapping {
-    /// Toplevel items' old `DefId` mapped to new `DefId`, as well as old and new exports.
+    /// Toplevel items' old `DefId` mapped to old and new `Def`.
     toplevel_mapping: HashMap<DefId, (Def, Def)>,
-    /// Trait items' old `DefId` mapped to new `DefId`.
-    trait_item_mapping: HashMap<DefId, (Def, Def)>,
+    /// Trait items' old `DefId` mapped to old and new `Def`.
+    trait_item_mapping: HashMap<DefId, (Def, Def, DefId)>,
     /// Other item's old `DefId` mapped to new `DefId`.
     internal_mapping: HashMap<DefId, DefId>,
     /// Children mapping, allowing us to enumerate descendants in `AdtDef`s.
@@ -43,8 +43,8 @@ impl IdMapping {
     }
 
     /// Add any trait item's old and new `DefId`s.
-    pub fn add_trait_item(&mut self, old: Def, new: Def) {
-        self.trait_item_mapping.insert(old.def_id(), (old, new));
+    pub fn add_trait_item(&mut self, old: Def, new: Def, trait_def_id: DefId) {
+        self.trait_item_mapping.insert(old.def_id(), (old, new, trait_def_id));
     }
 
     /// Add any other item's old and new `DefId`s.
@@ -95,6 +95,11 @@ impl IdMapping {
         }
     }
 
+    /// Return the `DefId` of the trait a given item belongs to.
+    pub fn get_trait_def(&self, item_def_id: &DefId) -> Option<DefId> {
+        self.trait_item_mapping.get(item_def_id).map(|t| t.2)
+    }
+
     /// Tell us whether a `DefId` is present in the mappings.
     pub fn contains_id(&self, old: DefId) -> bool {
         self.toplevel_mapping.contains_key(&old) ||
@@ -111,10 +116,11 @@ impl IdMapping {
     }
 
     /// Iterate over the toplevel and trait item pairs.
-    pub fn items<'a>(&'a self) -> impl Iterator<Item = &'a (Def, Def)> + 'a {
+    pub fn items<'a>(&'a self) -> impl Iterator<Item = (Def, Def)> + 'a {
         self.toplevel_mapping
             .values()
-            .chain(self.trait_item_mapping.values())
+            .cloned()
+            .chain(self.trait_item_mapping.values().map(|&(o, n, _)| (o, n)))
     }
 
     /// Iterate over the item pairs of all children of a given item.

src/semcheck/traverse.rs

@@ -43,12 +43,12 @@ pub fn run_analysis<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, old: DefId, new: DefI
     }
 
     // third pass
-    for &(old, new) in id_mapping.items() {
+    for (old, new) in id_mapping.items() {
         diff_bounds(&mut changes, tcx, old.def_id(), new.def_id());
     }
 
     // fourth pass
-    for &(old, new) in id_mapping.items() {
+    for (old, new) in id_mapping.items() {
         diff_types(&mut changes, &id_mapping, tcx, old, new);
     }
 
@@ -134,11 +134,11 @@ fn diff_structure<'a, 'tcx>(changes: &mut ChangeSet,
                         };
 
                         let output = o_vis == Public || n_vis == Public;
-                        changes.new_change(o.def.def_id(),
-                                           n.def.def_id(),
+                        changes.new_change(o_def_id,
+                                           n_def_id,
                                            o.ident.name,
-                                           tcx.def_span(o.def.def_id()),
-                                           tcx.def_span(n.def.def_id()),
+                                           tcx.def_span(o_def_id),
+                                           tcx.def_span(n_def_id),
                                            output);
 
                         if o_vis == Public && n_vis != Public {
@@ -419,13 +419,13 @@ fn diff_traits(changes: &mut ChangeSet,
                 let old_def_id = old_def.def_id();
                 let new_def_id = new_def.def_id();
 
-                id_mapping.add_trait_item(old_def, new_def);
-                changes.new_change(old_def.def_id(),
-                                   new_def.def_id(),
+                id_mapping.add_trait_item(old_def, new_def, old);
+                changes.new_change(old_def_id,
+                                   new_def_id,
                                    *name,
                                    tcx.def_span(old_def_id),
                                    tcx.def_span(new_def_id),
-                                   true);
+                                   true); // TODO: bad to do this unconditionally
 
                 diff_generics(changes, id_mapping, tcx, true, old_def_id, new_def_id);
                 diff_method(changes, tcx, old_item, new_item);
@@ -541,7 +541,9 @@ fn diff_types<'a, 'tcx>(changes: &mut ChangeSet<'tcx>,
     let old_def_id = old.def_id();
     let new_def_id = new.def_id();
 
-    if changes.item_breaking(old_def_id) {
+    if changes.item_breaking(old_def_id) ||
+            id_mapping.get_trait_def(&old_def_id)
+                .map_or(false, |did| changes.item_breaking(did)) {
         return;
     }
 
@@ -754,8 +756,12 @@ fn fold_to_new<'a, 'tcx, T>(id_mapping: &IdMapping,
             TyParam(param) => {
                 if param.idx != 0 { // `Self` is special
                     let old_did = index_map[&param.idx];
-                    let new_did = id_mapping.get_new_id(old_did);
-                    tcx.mk_param_from_def(&id_mapping.get_type_param(&new_did))
+                    if id_mapping.contains_id(old_did) {
+                        let new_did = id_mapping.get_new_id(old_did);
+                        tcx.mk_param_from_def(&id_mapping.get_type_param(&new_did))
+                    } else {
+                        tcx.mk_ty(TyParam(param))
+                    }
                 } else {
                     tcx.mk_ty(TyParam(param))
                 }

tests/cases/traits/new.rs

@@ -12,3 +12,17 @@ pub trait Abc {
     fn test8(_: &Self) -> u8;
     fn test9(&self) -> u8;
 }
+
+pub trait Bcd<A> {}
+
+pub trait Cde {}
+
+pub trait Def<A, B> {
+    // The method is not broken - the impls are, but calls should work as expected, as
+    // long as a proper impl is presented. Maybe this will need some more careful handling.
+    fn def(&self, a: B) -> bool;
+}
+
+pub trait Efg<A> {
+    fn efg(&self, a: A) -> bool;
+}

tests/cases/traits/old.rs

@@ -12,3 +12,15 @@ pub trait Abc {
     fn test8(&self) -> u8;
     fn test9(_: &Self) -> u8;
 }
+
+pub trait Bcd {}
+
+pub trait Cde<A> {}
+
+pub trait Def<A> {
+    fn def(&self, a: A) -> bool;
+}
+
+pub trait Efg<A, B> {
+    fn efg(&self, a: B) -> bool;
+}

tests/cases/traits/stdout

@@ -36,14 +36,6 @@ note: added defaulted item to trait (technically breaking)
 10 | |     }
    | |_____^
 
-warning: breaking changes in `test7`
-  --> $REPO_PATH/tests/cases/traits/new.rs:11:5
-   |
-11 |     fn test7() -> u16;
-   |     ^^^^^^^^^^^^^^^^^^
-   |
-   = warning: type error: expected u8, found u16 (breaking)
-
 warning: breaking changes in `test8`
   --> $REPO_PATH/tests/cases/traits/new.rs:12:5
    |
@@ -60,3 +52,41 @@ warning: technically breaking changes in `test9`
    |
    = note: added self-argument to method (technically breaking)
 
+warning: breaking changes in `Bcd`
+  --> $REPO_PATH/tests/cases/traits/new.rs:16:1
+   |
+16 | pub trait Bcd<A> {}
+   | ^^^^^^^^^^^^^^^^^^^
+   |
+   = warning: type parameter added (breaking)
+
+warning: breaking changes in `Cde`
+  --> $REPO_PATH/tests/cases/traits/new.rs:18:1
+   |
+18 | pub trait Cde {}
+   | ^^^^^^^^^^^^^^^^
+   |
+   = warning: type parameter removed (breaking)
+
+warning: breaking changes in `Def`
+  --> $REPO_PATH/tests/cases/traits/new.rs:20:1
+   |
+20 | / pub trait Def<A, B> {
+21 | |     // The method is not broken - the impls are, but calls should work as expected, as
+22 | |     // long as a proper impl is presented. Maybe this will need some more careful handling.
+23 | |     fn def(&self, a: B) -> bool;
+24 | | }
+   | |_^
+   |
+   = warning: type parameter added (breaking)
+
+warning: breaking changes in `Efg`
+  --> $REPO_PATH/tests/cases/traits/new.rs:26:1
+   |
+26 | / pub trait Efg<A> {
+27 | |     fn efg(&self, a: A) -> bool;
+28 | | }
+   | |_^
+   |
+   = warning: type parameter removed (breaking)
+