Auto merge of #52990 - Aaron1011:fix/rustdoc-auto-trait-static, r=eddyb

Fix ICE when rustdoc encounters certain usages of HRTBs

Fixes #51236

Under certain circumstances, `AutoTraitFinder` could end up computing a `ParamEnv` involving two trait predicates that differed only in the region parameters involved. One of these parameters would be a HRTB, while the other would be a normal region parameter.

When this `ParamEnv` was later passed to `SelectionContext`, an `Ambiguity` error would occur, since the erased versions of these predicates would be identical. To solve the issue, we de-duplicate our list of predicates as we build it up. Whenever we encounter two predicates that differ only in their assignment of region parameters (a HRTB vs a normal lifetime parameter), we pick the HRTB. This corresponds to selecting a 'stricter' bound to display in the generated documentation: we're requiring that a particular type works for all possible lifetime parameters if it's going to implement a particular auto trait.

Author: bors

src/librustc/traits/auto_trait.rs

@@ -358,7 +358,8 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
                 &Err(SelectionError::Unimplemented) => {
                     if self.is_of_param(pred.skip_binder().trait_ref.substs) {
                         already_visited.remove(&pred);
-                        user_computed_preds.insert(ty::Predicate::Trait(pred.clone()));
+                        self.add_user_pred(&mut user_computed_preds,
+                                           ty::Predicate::Trait(pred.clone()));
                         predicates.push_back(pred);
                     } else {
                         debug!(
@@ -393,6 +394,92 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
         return Some((new_env, final_user_env));
     }
 
+    // This method is designed to work around the following issue:
+    // When we compute auto trait bounds, we repeatedly call SelectionContext.select,
+    // progressively building a ParamEnv based on the results we get.
+    // However, our usage of SelectionContext differs from its normal use within the compiler,
+    // in that we capture and re-reprocess predicates from Unimplemented errors.
+    //
+    // This can lead to a corner case when dealing with region parameters.
+    // During our selection loop in evaluate_predicates, we might end up with
+    // two trait predicates that differ only in their region parameters:
+    // one containing a HRTB lifetime parameter, and one containing a 'normal'
+    // lifetime parameter. For example:
+    //
+    // T as MyTrait<'a>
+    // T as MyTrait<'static>
+    //
+    // If we put both of these predicates in our computed ParamEnv, we'll
+    // confuse SelectionContext, since it will (correctly) view both as being applicable.
+    //
+    // To solve this, we pick the 'more strict' lifetime bound - i.e. the HRTB
+    // Our end goal is to generate a user-visible description of the conditions
+    // under which a type implements an auto trait. A trait predicate involving
+    // a HRTB means that the type needs to work with any choice of lifetime,
+    // not just one specific lifetime (e.g. 'static).
+    fn add_user_pred<'c>(&self, user_computed_preds: &mut FxHashSet<ty::Predicate<'c>>,
+                         new_pred: ty::Predicate<'c>) {
+        let mut should_add_new = true;
+        user_computed_preds.retain(|&old_pred| {
+            match (&new_pred, old_pred) {
+                (&ty::Predicate::Trait(new_trait), ty::Predicate::Trait(old_trait)) => {
+                    if new_trait.def_id() == old_trait.def_id() {
+                        let new_substs = new_trait.skip_binder().trait_ref.substs;
+                        let old_substs = old_trait.skip_binder().trait_ref.substs;
+                        if !new_substs.types().eq(old_substs.types()) {
+                            // We can't compare lifetimes if the types are different,
+                            // so skip checking old_pred
+                            return true
+                        }
+
+                        for (new_region, old_region) in new_substs
+                            .regions()
+                            .zip(old_substs.regions()) {
+
+                            match (new_region, old_region) {
+                                // If both predicates have an 'ReLateBound' (a HRTB) in the
+                                // same spot, we do nothing
+                                (
+                                    ty::RegionKind::ReLateBound(_, _),
+                                    ty::RegionKind::ReLateBound(_, _)
+                                ) => {},
+
+                                (ty::RegionKind::ReLateBound(_, _), _) => {
+                                    // The new predicate has a HRTB in a spot where the old
+                                    // predicate does not (if they both had a HRTB, the previous
+                                    // match arm would have executed).
+                                    //
+                                    // The means we want to remove the older predicate from
+                                    // user_computed_preds, since having both it and the new
+                                    // predicate in a ParamEnv would confuse SelectionContext
+                                    // We're currently in the predicate passed to 'retain',
+                                    // so we return 'false' to remove the old predicate from
+                                    // user_computed_preds
+                                    return false;
+                                },
+                                (_, ty::RegionKind::ReLateBound(_, _)) => {
+                                    // This is the opposite situation as the previous arm - the
+                                    // old predicate has a HRTB lifetime in a place where the
+                                    // new predicate does not. We want to leave the old
+                                    // predicate in user_computed_preds, and skip adding
+                                    // new_pred to user_computed_params.
+                                    should_add_new = false
+                                }
+                                _ => {}
+                            }
+                        }
+                    }
+                },
+                _ => {}
+            }
+            return true
+        });
+
+        if should_add_new {
+            user_computed_preds.insert(new_pred);
+        }
+    }
+
     pub fn region_name(&self, region: Region) -> Option<String> {
         match region {
             &ty::ReEarlyBound(r) => Some(r.name.to_string()),
@@ -555,15 +642,15 @@ impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
                     let substs = &p.skip_binder().trait_ref.substs;
 
                     if self.is_of_param(substs) && !only_projections && is_new_pred {
-                        computed_preds.insert(predicate);
+                        self.add_user_pred(computed_preds, predicate);
                     }
                     predicates.push_back(p.clone());
                 }
                 &ty::Predicate::Projection(p) => {
                     // If the projection isn't all type vars, then
                     // we don't want to add it as a bound
                     if self.is_of_param(p.skip_binder().projection_ty.substs) && is_new_pred {
-                        computed_preds.insert(predicate);
+                        self.add_user_pred(computed_preds, predicate);
                     } else {
                         match poly_project_and_unify_type(select, &obligation.with(p.clone())) {
                             Err(e) => {

src/librustdoc/clean/auto_trait.rs

@@ -418,8 +418,8 @@ impl<'a, 'tcx, 'rcx, 'cstore> AutoTraitFinder<'a, 'tcx, 'rcx, 'cstore> {
                                 name: name.to_string(),
                                 kind: GenericParamDefKind::Lifetime,
                             })
-                        }
-                        &ty::ReVar(_) | &ty::ReEarlyBound(_) => None,
+                        },
+                        &ty::ReVar(_) | &ty::ReEarlyBound(_) | &ty::ReStatic => None,
                         _ => panic!("Unexpected region type {:?}", r),
                     }
                 })

src/test/rustdoc/issue-51236.rs

@@ -0,0 +1,24 @@
+// Copyright 2018 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.
+
+use std::marker::PhantomData;
+
+pub mod traits {
+    pub trait Owned<'a> {
+        type Reader;
+    }
+}
+
+// @has issue_51236/struct.Owned.html
+// @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]//*/code' "impl<T> Send for \
+// Owned<T> where <T as Owned<'static>>::Reader: Send"
+pub struct Owned<T> where T: for<'a> ::traits::Owned<'a> {
+    marker: PhantomData<<T as ::traits::Owned<'static>>::Reader>,
+}

src/test/rustdoc/synthetic_auto/static-region.rs

@@ -0,0 +1,20 @@
+// Copyright 2018 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.
+
+pub trait OwnedTrait<'a> {
+    type Reader;
+}
+
+// @has static_region/struct.Owned.html
+// @has - '//*[@id="synthetic-implementations-list"]/*[@class="impl"]//*/code' "impl<T> Send for \
+// Owned<T> where <T as OwnedTrait<'static>>::Reader: Send"
+pub struct Owned<T> where T: OwnedTrait<'static> {
+    marker: <T as OwnedTrait<'static>>::Reader,
+}