Addressed points raised in review.

Author: Alexander Regueiro

src/librustc/traits/util.rs

@@ -56,6 +56,7 @@ impl<'a, 'gcx, 'tcx> PredicateSet<'a, 'gcx, 'tcx> {
     }
 
     fn contains(&mut self, pred: &ty::Predicate<'tcx>) -> bool {
+        // See the `insert` method for why we use `anonymize_predicate` here.
         self.set.contains(&anonymize_predicate(self.tcx, pred))
     }
 
@@ -74,13 +75,14 @@ impl<'a, 'gcx, 'tcx> PredicateSet<'a, 'gcx, 'tcx> {
     }
 
     fn remove(&mut self, pred: &ty::Predicate<'tcx>) -> bool {
+        // See the `insert` method for why we use `anonymize_predicate` here.
         self.set.remove(&anonymize_predicate(self.tcx, pred))
     }
 }
 
 impl<'a, 'gcx, 'tcx, T: AsRef<ty::Predicate<'tcx>>> Extend<T> for PredicateSet<'a, 'gcx, 'tcx> {
     fn extend<I: IntoIterator<Item = T>>(&mut self, iter: I) {
-        for pred in iter.into_iter() {
+        for pred in iter {
             self.insert(pred.as_ref());
         }
     }
@@ -289,30 +291,33 @@ pub fn transitive_bounds<'cx, 'gcx, 'tcx>(tcx: TyCtxt<'cx, 'gcx, 'tcx>,
 /// `trait Foo = Bar + Sync;`, and another trait alias
 /// `trait Bar = Read + Write`, then the bounds would expand to
 /// `Read + Write + Sync + Send`.
+/// Expansion is done via a DFS (depth-first search), and the `visited` field
+/// is used to avoid cycles.
 pub struct TraitAliasExpander<'a, 'gcx: 'a + 'tcx, 'tcx: 'a> {
     stack: Vec<TraitAliasExpansionInfo<'tcx>>,
     /// The set of predicates visited from the root directly to the current point in the
-    /// expansion tree.
+    /// expansion tree (only containing trait aliases).
     visited: PredicateSet<'a, 'gcx, 'tcx>,
 }
 
+/// Stores information about the expansion of a trait via a path of zero or more trait aliases.
 #[derive(Debug, Clone)]
 pub struct TraitAliasExpansionInfo<'tcx> {
     pub items: SmallVec<[(ty::PolyTraitRef<'tcx>, Span); 4]>,
 }
 
 impl<'tcx> TraitAliasExpansionInfo<'tcx> {
-    fn new(trait_ref: ty::PolyTraitRef<'tcx>, span: Span) -> TraitAliasExpansionInfo<'tcx> {
-        TraitAliasExpansionInfo {
+    fn new(trait_ref: ty::PolyTraitRef<'tcx>, span: Span) -> Self {
+        Self {
             items: smallvec![(trait_ref, span)]
         }
     }
 
-    fn push(&self, trait_ref: ty::PolyTraitRef<'tcx>, span: Span) -> TraitAliasExpansionInfo<'tcx> {
+    fn push(&self, trait_ref: ty::PolyTraitRef<'tcx>, span: Span) -> Self {
         let mut items = self.items.clone();
         items.push((trait_ref, span));
 
-        TraitAliasExpansionInfo {
+        Self {
             items
         }
     }
@@ -330,6 +335,8 @@ impl<'tcx> TraitAliasExpansionInfo<'tcx> {
     }
 }
 
+/// Emits diagnostic information relating to the expansion of a trait via trait aliases
+/// (see [`TraitAliasExpansionInfo`]).
 pub trait TraitAliasExpansionInfoDignosticBuilder {
     fn label_with_exp_info<'tcx>(&mut self,
         info: &TraitAliasExpansionInfo<'tcx>,
@@ -365,24 +372,28 @@ pub fn expand_trait_aliases<'cx, 'gcx, 'tcx>(
 
 impl<'cx, 'gcx, 'tcx> TraitAliasExpander<'cx, 'gcx, 'tcx> {
     /// If `item` is a trait alias and its predicate has not yet been visited, then expands `item`
-    /// to the definition and pushes the resulting expansion onto `self.stack`, and returns `false`.
-    /// Otherwise, immediately returns `true` if `item` is a regular trait and `false` if it is a
+    /// to the definition, pushes the resulting expansion onto `self.stack`, and returns `false`.
+    /// Otherwise, immediately returns `true` if `item` is a regular trait, or `false` if it is a
     /// trait alias.
-    /// The return value indicates whether `item` should not be yielded to the user.
+    /// The return value indicates whether `item` should be yielded to the user.
     fn push(&mut self, item: &TraitAliasExpansionInfo<'tcx>) -> bool {
         let tcx = self.visited.tcx;
         let trait_ref = item.trait_ref();
         let pred = trait_ref.to_predicate();
 
         debug!("expand_trait_aliases: trait_ref={:?}", trait_ref);
 
-        self.visited.remove(&pred);
-
+        // Don't recurse unless this bound is a trait alias and isn't currently in the DFS stack of
+        // already-visited predicates.
         let is_alias = tcx.is_trait_alias(trait_ref.def_id());
         if !is_alias || self.visited.contains(&pred) {
             return !is_alias;
         }
 
+        // Remove the current predicate from the stack of already-visited ones, since we're doing
+        // a DFS.
+        self.visited.remove(&pred);
+
         // Get components of trait alias.
         let predicates = tcx.super_predicates_of(trait_ref.def_id());
 

src/librustc_typeck/astconv.rs

@@ -996,11 +996,12 @@ impl<'o, 'gcx: 'tcx, 'tcx> dyn AstConv<'gcx, 'tcx> + 'o {
             expanded_traits.partition(|i| tcx.trait_is_auto(i.trait_ref().def_id()));
         if regular_traits.len() > 1 {
             let extra_trait = &regular_traits[1];
-            let mut err = struct_span_err!(tcx.sess, extra_trait.bottom().1, E0225,
-                "only auto traits can be used as additional traits in a trait object");
-            err.label_with_exp_info(extra_trait, "additional non-auto trait");
-            err.span_label(regular_traits[0].top().1, "first non-auto trait");
-            err.emit();
+            struct_span_err!(tcx.sess, extra_trait.bottom().1, E0225,
+                "only auto traits can be used as additional traits in a trait object"
+            )
+                .label_with_exp_info(extra_trait, "additional non-auto trait")
+                .span_label(regular_traits[0].top().1, "first non-auto trait")
+                .emit();
         }
 
         if regular_traits.is_empty() && auto_traits.is_empty() {

src/test/ui/error-codes/E0225.rs

@@ -3,8 +3,8 @@
 trait Foo = std::io::Read + std::io::Write;
 
 fn main() {
-    let _: Box<std::io::Read + std::io::Write>;
+    let _: Box<dyn std::io::Read + std::io::Write>;
     //~^ ERROR only auto traits can be used as additional traits in a trait object [E0225]
-    let _: Box<Foo>;
+    let _: Box<dyn Foo>;
     //~^ ERROR only auto traits can be used as additional traits in a trait object [E0225]
 }

src/test/ui/maybe-bounds.rs

@@ -1,6 +1,6 @@
 trait Tr: ?Sized {} //~ ERROR `?Trait` is not permitted in supertraits
 
-type A1 = Tr + (?Sized);
-type A2 = for<'a> Tr + (?Sized);
+type A1 = dyn Tr + (?Sized);
+type A2 = dyn for<'a> Tr + (?Sized);
 
 fn main() {}

src/test/ui/traits/trait-alias/trait-alias-maybe-bound.rs

@@ -1,27 +1,29 @@
+// compile-pass
+
 // Test that `dyn ... + ?Sized + ...` resulting from the expansion of trait aliases is okay.
 
 #![feature(trait_alias)]
 
+trait Foo {}
+
 trait S = ?Sized;
 
 // Nest a couple of levels deep:
 trait _0 = S;
 trait _1 = _0;
 
 // Straight list expansion:
-type _T0 = dyn _1;
-//~^ ERROR at least one non-builtin trait is required for an object type [E0224]
+type _T0 = dyn _1 + Foo;
 
 // In second position:
-type _T1 = dyn Copy + _1;
+type _T1 = dyn Foo + _1;
 
 // ... and with an auto trait:
-type _T2 = dyn Copy + Send + _1;
+type _T2 = dyn Foo + Send + _1;
 
 // Twice:
 trait _2 = _1 + _1;
 
-type _T3 = dyn _2;
-//~^ ERROR at least one non-builtin trait is required for an object type [E0224]
+type _T3 = dyn _2 + Foo;
 
 fn main() {}

src/test/ui/traits/trait-alias/trait-alias-no-duplicates.rs

@@ -6,7 +6,7 @@
 
 use std::marker::Unpin;
 
-// Some arbitray object-safe trait:
+// Some arbitrary object-safe trait:
 trait Obj {}
 
 // Nest a few levels deep:

src/test/ui/traits/trait-alias/trait-alias-no-extra-traits.rs

@@ -5,7 +5,7 @@
 
 use std::marker::Unpin;
 
-// Some arbitray object-safe traits:
+// Some arbitrary object-safe traits:
 trait ObjA {}
 trait ObjB {}
 

src/test/ui/traits/trait-alias/trait-alias-object-wf.rs

@@ -1,6 +1,6 @@
 // run-pass
 
-// This test checks that trait objects involving trait aliases are well-formed.
+// This test hecks that trait objects involving trait aliases are well-formed.
 
 #![feature(trait_alias)]
 
@@ -14,49 +14,61 @@ trait _1 = _0 + Send + Sync;
 
 use std::marker::Unpin;
 
-type _T01 = dyn _0;
-type _T02 = dyn _1;
-type _T03 = dyn Unpin + _1 + Send + Sync;
+fn _f0() {
+    let _: Box<dyn _0>;
+    let _: Box<dyn _1>;
+    let _: Box<dyn Unpin + _1 + Send + Sync>;
+}
 
 // Include object safe traits:
 
-type _T10 = dyn Obj + _0;
-type _T11 = dyn Obj + _1;
-type _T12 = dyn Obj + _1 + _0;
+fn _f1() {
+    let _: Box<dyn Obj + _0>;
+    let _: Box<dyn Obj + _1>;
+    let _: Box<dyn Obj + _1 + _0>;
+}
 
 // And when the object safe trait is in a trait alias:
 
 trait _2 = Obj;
 
-type _T20 = dyn _2 + _0;
-type _T21 = dyn _2 + _1;
-type _T22 = dyn _2 + _1 + _0;
+fn _f2() {
+    let _: Box<dyn _2 + _0>;
+    let _: Box<dyn _2 + _1>;
+    let _: Box<dyn _2 + _1 + _0>;
+}
 
 // And it should also work when that trait is has auto traits to the right of it.
 
 trait _3 = Obj + Unpin;
 
-type _T30 = dyn _3 + _0;
-type _T31 = dyn _3 + _1;
-type _T32 = dyn _3 + _1 + _0;
+fn _f3() {
+    let _: Box<dyn _3 + _0>;
+    let _: Box<dyn _3 + _1>;
+    let _: Box<dyn _3 + _1 + _0>;
+}
 
 // Nest the trait deeply:
 
 trait _4 = _3;
 trait _5 = _4 + Sync + _0 + Send;
 trait _6 = _5 + Send + _1 + Sync;
 
-type _T60 = dyn _6 + _0;
-type _T61 = dyn _6 + _1;
-type _T62 = dyn _6 + _1 + _0;
+fn _f4() {
+    let _: Box<dyn _6 + _0>;
+    let _: Box<dyn _6 + _1>;
+    let _: Box<dyn _6 + _1 + _0>;
+}
 
 // Just nest the trait alone:
 
 trait _7 = _2;
 trait _8 = _7;
 trait _9 = _8;
 
-type _T9 = dyn _9;
+fn _f5() {
+    let _: Box<dyn _9>;
+}
 
 // First bound is auto trait:
 
@@ -65,7 +77,9 @@ trait _11 = Obj + Send;
 trait _12 = Sync + _11;
 trait _13 = Send + _12;
 
-type _T70 = dyn _0;
-type _T71 = dyn _3;
+fn f6() {
+    let _: Box<dyn _10>;
+    let _: Box<dyn _13>;
+}
 
 fn main() {}

src/test/ui/traits/trait-alias/trait-alias-only-maybe-bound.rs

@@ -0,0 +1,22 @@
+// Test that `dyn ?Sized` (i.e., a trait object with only a maybe buond) is not allowed, when just
+// `?Sized` results from trait alias expansion.
+
+#![feature(trait_alias)]
+
+trait S = ?Sized;
+
+// Nest a couple of levels deep:
+trait _0 = S;
+trait _1 = _0;
+
+// Straight list expansion:
+type _T0 = dyn _1;
+//~^ ERROR at least one non-builtin trait is required for an object type [E0224]
+
+// Twice:
+trait _2 = _1 + _1;
+
+type _T1 = dyn _2;
+//~^ ERROR at least one non-builtin trait is required for an object type [E0224]
+
+fn main() {}

src/test/ui/traits/trait-alias/trait-alias-maybe-bound.stderr renamed to src/test/ui/traits/trait-alias/trait-alias-only-maybe-bound.stderr

@@ -1,13 +1,15 @@
-// The purpose of this test is to demonstrate that `?Sized` is allowed in trait objects
-// (thought it has no effect).
+// compile-pass
 
-type _0 = dyn ?Sized;
-//~^ ERROR at least one non-builtin trait is required for an object type [E0224]
+// Test that `dyn ... + ?Sized + ...` is okay (though `?Sized` has no effect in trait objects).
 
-type _1 = dyn Clone + ?Sized;
+trait Foo {}
 
-type _2 = dyn Clone + ?Sized + ?Sized;
+type _0 = dyn ?Sized + Foo;
 
-type _3 = dyn ?Sized + Clone;
+type _1 = dyn Foo + ?Sized;
+
+type _2 = dyn Foo + ?Sized + ?Sized;
+
+type _3 = dyn ?Sized + Foo;
 
 fn main() {}

src/test/ui/traits/wf-trait-object-maybe-bound.rs

@@ -1,7 +1,7 @@
 // The purpose of this test is to demonstrate that duplicating object safe traits
 // that are not auto-traits is rejected even though one could reasonably accept this.
 
-// Some arbitray object-safe trait:
+// Some arbitrary object-safe trait:
 trait Obj {}
 
 // Demonstrate that recursive expansion of trait aliases doesn't affect stable behavior:

src/test/ui/traits/wf-trait-object-no-duplicates.rs

@@ -0,0 +1,6 @@
+// Test that `dyn ?Sized` (i.e., a trait object with only a maybe buond) is not allowed.
+
+type _0 = dyn ?Sized;
+//~^ ERROR at least one non-builtin trait is required for an object type [E0224]
+
+fn main() {}

src/test/ui/traits/wf-trait-object-only-maybe-bound.rs

@@ -1,15 +1,15 @@
 // run-pass
 
-// Ensure that `dyn $($AutoTrait) + ObjSafe` is well-formed.
+// Ensure that `dyn $($AutoTrait)+ ObjSafe` is well-formed.
 
 use std::marker::Unpin;
 
-// Some arbitray object-safe trait:
+// Some arbitrary object-safe trait:
 trait Obj {}
 
-type _0 = Unpin;
-type _1 = Send + Obj;
-type _2 = Send + Unpin + Obj;
-type _3 = Send + Unpin + Sync + Obj;
+type _0 = dyn Unpin;
+type _1 = dyn Send + Obj;
+type _2 = dyn Send + Unpin + Obj;
+type _3 = dyn Send + Unpin + Sync + Obj;
 
 fn main() {}