Don't move ?Trait bounds to param bounds if they're in where clauses

jackh726 · jackh726 · commit f1f1d56d93b4 · 2021-09-07T18:08:46.000-04:00
diff --git a/compiler/rustc_ast_lowering/src/item.rs b/compiler/rustc_ast_lowering/src/item.rs
@@ -2,7 +2,6 @@ use super::{AnonymousLifetimeMode, LoweringContext, ParamMode};
 use super::{ImplTraitContext, ImplTraitPosition};
 use crate::Arena;
 
-use rustc_ast::node_id::NodeMap;
 use rustc_ast::ptr::P;
 use rustc_ast::visit::{self, AssocCtxt, FnCtxt, FnKind, Visitor};
 use rustc_ast::*;
@@ -1351,8 +1350,11 @@ impl<'hir> LoweringContext<'_, 'hir> {
         generics: &Generics,
         itctx: ImplTraitContext<'_, 'hir>,
     ) -> GenericsCtor<'hir> {
-        // Collect `?Trait` bounds in where clause and move them to parameter definitions.
-        let mut add_bounds: NodeMap<Vec<_>> = Default::default();
+        // Error if `?Trait` bounds in where clauses don't refer directly to type paramters.
+        // Note: we used to clone these bounds directly onto the type parameter (and avoid lowering
+        // these into hir when we lower thee where clauses), but this makes it quite difficult to
+        // keep track of the Span info. Now, `is_unsized` in `AstConv` checks both param bounds and
+        // where clauses for `?Sized`.
         for pred in &generics.where_clause.predicates {
             if let WherePredicate::BoundPredicate(ref bound_pred) = *pred {
                 'next_bound: for bound in &bound_pred.bounds {
@@ -1368,7 +1370,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
                             {
                                 for param in &generics.params {
                                     if def_id == self.resolver.local_def_id(param.id).to_def_id() {
-                                        add_bounds.entry(param.id).or_default().push(bound.clone());
                                         continue 'next_bound;
                                     }
                                 }
@@ -1386,7 +1387,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
         }
 
         GenericsCtor {
-            params: self.lower_generic_params_mut(&generics.params, &add_bounds, itctx).collect(),
+            params: self.lower_generic_params_mut(&generics.params, itctx).collect(),
             where_clause: self.lower_where_clause(&generics.where_clause),
             span: self.lower_span(generics.span),
         }
@@ -1419,32 +1420,17 @@ impl<'hir> LoweringContext<'_, 'hir> {
                 ref bounded_ty,
                 ref bounds,
                 span,
-            }) => {
-                self.with_in_scope_lifetime_defs(&bound_generic_params, |this| {
-                    hir::WherePredicate::BoundPredicate(hir::WhereBoundPredicate {
-                        bound_generic_params: this.lower_generic_params(
-                            bound_generic_params,
-                            &NodeMap::default(),
-                            ImplTraitContext::disallowed(),
-                        ),
-                        bounded_ty: this.lower_ty(bounded_ty, ImplTraitContext::disallowed()),
-                        bounds: this.arena.alloc_from_iter(bounds.iter().map(
-                            |bound| match bound {
-                                // We used to ignore `?Trait` bounds, as they were copied into type
-                                // parameters already, but we need to keep them around only for
-                                // diagnostics when we suggest removal of `?Sized` bounds. See
-                                // `suggest_constraining_type_param`. This will need to change if
-                                // we ever allow something *other* than `?Sized`.
-                                GenericBound::Trait(p, TraitBoundModifier::Maybe) => {
-                                    hir::GenericBound::Unsized(this.lower_span(p.span))
-                                }
-                                _ => this.lower_param_bound(bound, ImplTraitContext::disallowed()),
-                            },
-                        )),
-                        span: this.lower_span(span),
-                    })
+            }) => self.with_in_scope_lifetime_defs(&bound_generic_params, |this| {
+                hir::WherePredicate::BoundPredicate(hir::WhereBoundPredicate {
+                    bound_generic_params: this
+                        .lower_generic_params(bound_generic_params, ImplTraitContext::disallowed()),
+                    bounded_ty: this.lower_ty(bounded_ty, ImplTraitContext::disallowed()),
+                    bounds: this.arena.alloc_from_iter(bounds.iter().map(|bound| {
+                        this.lower_param_bound(bound, ImplTraitContext::disallowed())
+                    })),
+                    span: this.lower_span(span),
                 })
-            }
+            }),
             WherePredicate::RegionPredicate(WhereRegionPredicate {
                 ref lifetime,
                 ref bounds,
diff --git a/compiler/rustc_ast_lowering/src/lib.rs b/compiler/rustc_ast_lowering/src/lib.rs
@@ -1313,7 +1313,6 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                     hir::TyKind::BareFn(this.arena.alloc(hir::BareFnTy {
                         generic_params: this.lower_generic_params(
                             &f.generic_params,
-                            &NodeMap::default(),
                             ImplTraitContext::disallowed(),
                         ),
                         unsafety: this.lower_unsafety(f.unsafety),
@@ -1998,30 +1997,25 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
     fn lower_generic_params_mut<'s>(
         &'s mut self,
         params: &'s [GenericParam],
-        add_bounds: &'s NodeMap<Vec<GenericBound>>,
         mut itctx: ImplTraitContext<'s, 'hir>,
     ) -> impl Iterator<Item = hir::GenericParam<'hir>> + Captures<'a> + Captures<'s> {
-        params
-            .iter()
-            .map(move |param| self.lower_generic_param(param, add_bounds, itctx.reborrow()))
+        params.iter().map(move |param| self.lower_generic_param(param, itctx.reborrow()))
     }
 
     fn lower_generic_params(
         &mut self,
         params: &[GenericParam],
-        add_bounds: &NodeMap<Vec<GenericBound>>,
         itctx: ImplTraitContext<'_, 'hir>,
     ) -> &'hir [hir::GenericParam<'hir>] {
-        self.arena.alloc_from_iter(self.lower_generic_params_mut(params, add_bounds, itctx))
+        self.arena.alloc_from_iter(self.lower_generic_params_mut(params, itctx))
     }
 
     fn lower_generic_param(
         &mut self,
         param: &GenericParam,
-        add_bounds: &NodeMap<Vec<GenericBound>>,
         mut itctx: ImplTraitContext<'_, 'hir>,
     ) -> hir::GenericParam<'hir> {
-        let mut bounds: Vec<_> = self
+        let bounds: Vec<_> = self
             .with_anonymous_lifetime_mode(AnonymousLifetimeMode::ReportError, |this| {
                 this.lower_param_bounds_mut(&param.bounds, itctx.reborrow()).collect()
             });
@@ -2057,12 +2051,6 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                 (param_name, kind)
             }
             GenericParamKind::Type { ref default, .. } => {
-                let add_bounds = add_bounds.get(&param.id).map_or(&[][..], |x| &x);
-                if !add_bounds.is_empty() {
-                    let params = self.lower_param_bounds_mut(add_bounds, itctx.reborrow());
-                    bounds.extend(params);
-                }
-
                 let kind = hir::GenericParamKind::Type {
                     default: default.as_ref().map(|x| {
                         self.lower_ty(x, ImplTraitContext::Disallowed(ImplTraitPosition::Other))
@@ -2123,11 +2111,8 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
         p: &PolyTraitRef,
         mut itctx: ImplTraitContext<'_, 'hir>,
     ) -> hir::PolyTraitRef<'hir> {
-        let bound_generic_params = self.lower_generic_params(
-            &p.bound_generic_params,
-            &NodeMap::default(),
-            itctx.reborrow(),
-        );
+        let bound_generic_params =
+            self.lower_generic_params(&p.bound_generic_params, itctx.reborrow());
 
         let trait_ref = self.with_in_scope_lifetime_defs(&p.bound_generic_params, |this| {
             // Any impl Trait types defined within this scope can capture
diff --git a/compiler/rustc_hir/src/hir.rs b/compiler/rustc_hir/src/hir.rs
@@ -441,7 +441,6 @@ pub enum GenericBound<'hir> {
     Trait(PolyTraitRef<'hir>, TraitBoundModifier),
     // FIXME(davidtwco): Introduce `PolyTraitRef::LangItem`
     LangItemTrait(LangItem, Span, HirId, &'hir GenericArgs<'hir>),
-    Unsized(Span),
     Outlives(Lifetime),
 }
 
@@ -461,7 +460,6 @@ impl GenericBound<'_> {
             GenericBound::Trait(t, ..) => t.span,
             GenericBound::LangItemTrait(_, span, ..) => *span,
             GenericBound::Outlives(l) => l.span,
-            GenericBound::Unsized(span) => *span,
         }
     }
 }
diff --git a/compiler/rustc_hir/src/intravisit.rs b/compiler/rustc_hir/src/intravisit.rs
@@ -871,7 +871,6 @@ pub fn walk_param_bound<'v, V: Visitor<'v>>(visitor: &mut V, bound: &'v GenericB
             visitor.visit_generic_args(span, args);
         }
         GenericBound::Outlives(ref lifetime) => visitor.visit_lifetime(lifetime),
-        GenericBound::Unsized(_) => {}
     }
 }
 
diff --git a/compiler/rustc_hir_pretty/src/lib.rs b/compiler/rustc_hir_pretty/src/lib.rs
@@ -2232,9 +2232,6 @@ impl<'a> State<'a> {
                 GenericBound::Outlives(lt) => {
                     self.print_lifetime(lt);
                 }
-                GenericBound::Unsized(_) => {
-                    self.s.word("?Sized");
-                }
             }
         }
     }
diff --git a/compiler/rustc_middle/src/ty/diagnostics.rs b/compiler/rustc_middle/src/ty/diagnostics.rs
@@ -2,7 +2,6 @@
 
 use crate::ty::TyKind::*;
 use crate::ty::{InferTy, TyCtxt, TyS};
-use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::{Applicability, DiagnosticBuilder};
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
@@ -114,10 +113,8 @@ fn suggest_removing_unsized_bound(
     def_id: Option<DefId>,
 ) {
     // See if there's a `?Sized` bound that can be removed to suggest that.
-    // First look at the `where` clause because we can have `where T: ?Sized`, but that
-    // `?Sized` bound is *also* included in the `GenericParam` as a bound, which breaks
-    // the spans. Hence the somewhat involved logic that follows.
-    let mut where_unsized_bounds = FxHashSet::default();
+    // First look at the `where` clause because we can have `where T: ?Sized`,
+    // then look at params.
     for (where_pos, predicate) in generics.where_clause.predicates.iter().enumerate() {
         match predicate {
             WherePredicate::BoundPredicate(WhereBoundPredicate {
@@ -140,7 +137,6 @@ fn suggest_removing_unsized_bound(
             }) if segment.ident.as_str() == param_name => {
                 for (pos, bound) in bounds.iter().enumerate() {
                     match bound {
-                        hir::GenericBound::Unsized(_) => {}
                         hir::GenericBound::Trait(poly, hir::TraitBoundModifier::Maybe)
                             if poly.trait_ref.trait_def_id() == def_id => {}
                         _ => continue,
@@ -173,7 +169,6 @@ fn suggest_removing_unsized_bound(
                         //             ^^^^^^^^^
                         (_, pos, _, _) => bounds[pos - 1].span().shrink_to_hi().to(bound.span()),
                     };
-                    where_unsized_bounds.insert(bound.span());
                     err.span_suggestion_verbose(
                         sp,
                         "consider removing the `?Sized` bound to make the \
@@ -189,8 +184,7 @@ fn suggest_removing_unsized_bound(
     for (pos, bound) in param.bounds.iter().enumerate() {
         match bound {
             hir::GenericBound::Trait(poly, hir::TraitBoundModifier::Maybe)
-                if poly.trait_ref.trait_def_id() == def_id
-                    && !where_unsized_bounds.contains(&bound.span()) =>
+                if poly.trait_ref.trait_def_id() == def_id =>
             {
                 let sp = match (param.bounds.len(), pos) {
                     // T: ?Sized,
diff --git a/compiler/rustc_save_analysis/src/dump_visitor.rs b/compiler/rustc_save_analysis/src/dump_visitor.rs
@@ -693,7 +693,6 @@ impl<'tcx> DumpVisitor<'tcx> {
                     (Some(self.tcx.require_lang_item(lang_item, Some(span))), span)
                 }
                 hir::GenericBound::Outlives(..) => continue,
-                hir::GenericBound::Unsized(_) => continue,
             };
 
             if let Some(id) = def_id {
diff --git a/compiler/rustc_typeck/src/astconv/mod.rs b/compiler/rustc_typeck/src/astconv/mod.rs
@@ -854,7 +854,13 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
     }
 
     // Returns `true` if a bounds list includes `?Sized`.
-    pub fn is_unsized(&self, ast_bounds: &[hir::GenericBound<'_>], span: Span) -> bool {
+    fn is_unsized(
+        &self,
+        ast_bounds: &[hir::GenericBound<'_>],
+        self_ty: Option<hir::HirId>,
+        where_clause: Option<&[hir::WherePredicate<'_>]>,
+        span: Span,
+    ) -> bool {
         let tcx = self.tcx();
 
         // Try to find an unbound in bounds.
@@ -868,11 +874,38 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
                 }
             }
         }
+        if let (Some(self_ty), Some(where_clause)) = (self_ty, where_clause) {
+            let self_ty_def_id = tcx.hir().local_def_id(self_ty).to_def_id();
+            for clause in where_clause {
+                match clause {
+                    hir::WherePredicate::BoundPredicate(pred) => {
+                        match pred.bounded_ty.kind {
+                            hir::TyKind::Path(hir::QPath::Resolved(_, path)) => match path.res {
+                                Res::Def(DefKind::TyParam, def_id) if def_id == self_ty_def_id => {}
+                                _ => continue,
+                            },
+                            _ => continue,
+                        }
+                        for ab in pred.bounds {
+                            if let hir::GenericBound::Trait(ptr, hir::TraitBoundModifier::Maybe) =
+                                ab
+                            {
+                                if unbound.is_none() {
+                                    unbound = Some(&ptr.trait_ref);
+                                } else {
+                                    tcx.sess.emit_err(MultipleRelaxedDefaultBounds { span });
+                                }
+                            }
+                        }
+                    }
+                    _ => {}
+                }
+            }
+        }
 
         let kind_id = tcx.lang_items().require(LangItem::Sized);
         match unbound {
             Some(tpb) => {
-                // FIXME(#8559) currently requires the unbound to be built-in.
                 if let Ok(kind_id) = kind_id {
                     if tpb.path.res != Res::Def(DefKind::Trait, kind_id) {
                         tcx.sess.span_warn(
@@ -940,8 +973,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
                         false,
                     );
                 }
-                hir::GenericBound::Trait(_, hir::TraitBoundModifier::Maybe)
-                | hir::GenericBound::Unsized(_) => {}
+                hir::GenericBound::Trait(_, hir::TraitBoundModifier::Maybe) => {}
                 hir::GenericBound::LangItemTrait(lang_item, span, hir_id, args) => self
                     .instantiate_lang_item_trait_ref(
                         lang_item, span, hir_id, args, param_ty, bounds,
@@ -970,22 +1002,33 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
     /// example above, but is not true in supertrait listings like `trait Foo: Bar + Baz`.
     ///
     /// `span` should be the declaration size of the parameter.
-    pub fn compute_bounds(
+    pub(crate) fn compute_bounds(
         &self,
         param_ty: Ty<'tcx>,
         ast_bounds: &[hir::GenericBound<'_>],
+        self_ty: Option<hir::HirId>,
+        where_clause: Option<&[hir::WherePredicate<'_>]>,
         sized_by_default: SizedByDefault,
         span: Span,
     ) -> Bounds<'tcx> {
-        self.compute_bounds_inner(param_ty, &ast_bounds, sized_by_default, span)
+        self.compute_bounds_inner(
+            param_ty,
+            &ast_bounds,
+            self_ty,
+            where_clause,
+            sized_by_default,
+            span,
+        )
     }
 
     /// Convert the bounds in `ast_bounds` that refer to traits which define an associated type
     /// named `assoc_name` into ty::Bounds. Ignore the rest.
-    pub fn compute_bounds_that_match_assoc_type(
+    pub(crate) fn compute_bounds_that_match_assoc_type(
         &self,
         param_ty: Ty<'tcx>,
         ast_bounds: &[hir::GenericBound<'_>],
+        self_ty: Option<hir::HirId>,
+        where_clause: Option<&[hir::WherePredicate<'_>]>,
         sized_by_default: SizedByDefault,
         span: Span,
         assoc_name: Ident,
@@ -1002,13 +1045,15 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
             }
         }
 
-        self.compute_bounds_inner(param_ty, &result, sized_by_default, span)
+        self.compute_bounds_inner(param_ty, &result, self_ty, where_clause, sized_by_default, span)
     }
 
     fn compute_bounds_inner(
         &self,
         param_ty: Ty<'tcx>,
         ast_bounds: &[hir::GenericBound<'_>],
+        self_ty: Option<hir::HirId>,
+        where_clause: Option<&[hir::WherePredicate<'_>]>,
         sized_by_default: SizedByDefault,
         span: Span,
     ) -> Bounds<'tcx> {
@@ -1017,7 +1062,11 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
         self.add_bounds(param_ty, ast_bounds, &mut bounds, ty::List::empty());
 
         bounds.implicitly_sized = if let SizedByDefault::Yes = sized_by_default {
-            if !self.is_unsized(ast_bounds, span) { Some(span) } else { None }
+            if !self.is_unsized(ast_bounds, self_ty, where_clause, span) {
+                Some(span)
+            } else {
+                None
+            }
         } else {
             None
         };
diff --git a/compiler/rustc_typeck/src/collect.rs b/compiler/rustc_typeck/src/collect.rs
diff --git a/compiler/rustc_typeck/src/collect/item_bounds.rs b/compiler/rustc_typeck/src/collect/item_bounds.rs
diff --git a/src/test/ui/maybe-bounds-where.rs b/src/test/ui/maybe-bounds-where.rs
diff --git a/src/test/ui/maybe-bounds-where.stderr b/src/test/ui/maybe-bounds-where.stderr

Original file line number	Diff line number	Diff line change
`@@ -441,7 +441,6 @@ pub enum GenericBound<'hir> {`
`441`	`441`	`Trait(PolyTraitRef<'hir>, TraitBoundModifier),`
`442`	`442`	// FIXME(davidtwco): Introduce `PolyTraitRef::LangItem`
`443`	`443`	`LangItemTrait(LangItem, Span, HirId, &'hir GenericArgs<'hir>),`
`444`		`- Unsized(Span),`
`445`	`444`	`Outlives(Lifetime),`
`446`	`445`	`}`
`447`	`446`
`@@ -461,7 +460,6 @@ impl GenericBound<'_> {`
`461`	`460`	`GenericBound::Trait(t, ..) => t.span,`
`462`	`461`	`GenericBound::LangItemTrait(_, span, ..) => *span,`
`463`	`462`	`GenericBound::Outlives(l) => l.span,`
`464`		`- GenericBound::Unsized(span) => *span,`
`465`	`463`	`}`
`466`	`464`	`}`
`467`	`465`	`}`
Original file line number	Diff line number	Diff line change
`@@ -871,7 +871,6 @@ pub fn walk_param_bound<'v, V: Visitor<'v>>(visitor: &mut V, bound: &'v GenericB`
`871`	`871`	`visitor.visit_generic_args(span, args);`
`872`	`872`	`}`
`873`	`873`	`GenericBound::Outlives(ref lifetime) => visitor.visit_lifetime(lifetime),`
`874`		`- GenericBound::Unsized(_) => {}`
`875`	`874`	`}`
`876`	`875`	`}`
`877`	`876`
Original file line number	Diff line number	Diff line change
`@@ -2232,9 +2232,6 @@ impl<'a> State<'a> {`
`2232`	`2232`	`GenericBound::Outlives(lt) => {`
`2233`	`2233`	`self.print_lifetime(lt);`
`2234`	`2234`	`}`
`2235`		`- GenericBound::Unsized(_) => {`
`2236`		`- self.s.word("?Sized");`
`2237`		`- }`
`2238`	`2235`	`}`
`2239`	`2236`	`}`
`2240`	`2237`	`}`
Original file line number	Diff line number	Diff line change
`@@ -693,7 +693,6 @@ impl<'tcx> DumpVisitor<'tcx> {`
`693`	`693`	`(Some(self.tcx.require_lang_item(lang_item, Some(span))), span)`
`694`	`694`	`}`
`695`	`695`	`hir::GenericBound::Outlives(..) => continue,`
`696`		`- hir::GenericBound::Unsized(_) => continue,`
`697`	`696`	`};`
`698`	`697`
`699`	`698`	`if let Some(id) = def_id {`