Auto merge of rust-lang#38897 - nikomatsakis:issue-32330-followup, r=arielb1

make lifetimes that only appear in return type early-bound

This is the full and proper fix for rust-lang#32330. This also makes some effort to give a nice error message (as evidenced by the `ui` test), sending users over to the tracking issue for a fuller explanation and offering a `--explain` message in some cases.

This needs a crater run before we land.

r? @arielb1

Author: bors

src/librustc/infer/error_reporting.rs

@@ -72,13 +72,12 @@ use super::region_inference::SameRegions;
 use hir::map as hir_map;
 use hir;
 
-use lint;
 use hir::def_id::DefId;
 use infer;
 use middle::region;
 use traits::{ObligationCause, ObligationCauseCode};
 use ty::{self, TyCtxt, TypeFoldable};
-use ty::{Region, ReFree};
+use ty::{Region, ReFree, Issue32330};
 use ty::error::TypeError;
 
 use std::fmt;
@@ -610,6 +609,39 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
         self.tcx.note_and_explain_type_err(diag, terr, span);
     }
 
+    pub fn note_issue_32330(&self,
+                            diag: &mut DiagnosticBuilder<'tcx>,
+                            terr: &TypeError<'tcx>)
+    {
+        debug!("note_issue_32330: terr={:?}", terr);
+        match *terr {
+            TypeError::RegionsInsufficientlyPolymorphic(_, &Region::ReVar(vid)) |
+            TypeError::RegionsOverlyPolymorphic(_, &Region::ReVar(vid)) => {
+                match self.region_vars.var_origin(vid) {
+                    RegionVariableOrigin::EarlyBoundRegion(_, _, Some(Issue32330 {
+                        fn_def_id,
+                        region_name
+                    })) => {
+                        diag.note(
+                            &format!("lifetime parameter `{0}` declared on fn `{1}` \
+                                      appears only in the return type, \
+                                      but here is required to be higher-ranked, \
+                                      which means that `{0}` must appear in both \
+                                      argument and return types",
+                                     region_name,
+                                     self.tcx.item_path_str(fn_def_id)));
+                        diag.note(
+                            &format!("this error is the result of a recent bug fix; \
+                                      for more information, see issue #33685 \
+                                      <https://github.com/rust-lang/rust/issues/33685>"));
+                    }
+                    _ => { }
+                }
+            }
+            _ => { }
+        }
+    }
+
     pub fn report_and_explain_type_error(&self,
                                          trace: TypeTrace<'tcx>,
                                          terr: &TypeError<'tcx>)
@@ -629,6 +661,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
             }
         };
         self.note_type_err(&mut diag, &trace.cause, None, Some(trace.values), terr);
+        self.note_issue_32330(&mut diag, terr);
         diag
     }
 
@@ -1053,27 +1086,6 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
             err.emit();
         }
     }
-
-    pub fn issue_32330_warnings(&self, span: Span, issue32330s: &[ty::Issue32330]) {
-        for issue32330 in issue32330s {
-            match *issue32330 {
-                ty::Issue32330::WontChange => { }
-                ty::Issue32330::WillChange { fn_def_id, region_name } => {
-                    self.tcx.sess.add_lint(
-                        lint::builtin::HR_LIFETIME_IN_ASSOC_TYPE,
-                        ast::CRATE_NODE_ID,
-                        span,
-                        format!("lifetime parameter `{0}` declared on fn `{1}` \
-                                 appears only in the return type, \
-                                 but here is required to be higher-ranked, \
-                                 which means that `{0}` must appear in both \
-                                 argument and return types",
-                                region_name,
-                                self.tcx.item_path_str(fn_def_id)));
-                }
-            }
-        }
-    }
 }
 
 impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
@@ -1104,7 +1116,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                 format!(" for lifetime parameter {}in trait containing associated type `{}`",
                         br_string(br), type_name)
             }
-            infer::EarlyBoundRegion(_, name) => {
+            infer::EarlyBoundRegion(_, name, _) => {
                 format!(" for lifetime parameter `{}`",
                         name)
             }

src/librustc/infer/higher_ranked/mod.rs

@@ -622,51 +622,14 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
     /// hold. See `README.md` for more details.
     pub fn leak_check(&self,
                       overly_polymorphic: bool,
-                      span: Span,
+                      _span: Span,
                       skol_map: &SkolemizationMap<'tcx>,
                       snapshot: &CombinedSnapshot)
                       -> RelateResult<'tcx, ()>
     {
         debug!("leak_check: skol_map={:?}",
                skol_map);
 
-        // ## Issue #32330 warnings
-        //
-        // When Issue #32330 is fixed, a certain number of late-bound
-        // regions (LBR) will become early-bound. We wish to issue
-        // warnings when the result of `leak_check` relies on such LBR, as
-        // that means that compilation will likely start to fail.
-        //
-        // Recall that when we do a "HR subtype" check, we replace all
-        // late-bound regions (LBR) in the subtype with fresh variables,
-        // and skolemize the late-bound regions in the supertype. If those
-        // skolemized regions from the supertype wind up being
-        // super-regions (directly or indirectly) of either
-        //
-        // - another skolemized region; or,
-        // - some region that pre-exists the HR subtype check
-        //   - e.g., a region variable that is not one of those created
-        //     to represent bound regions in the subtype
-        //
-        // then leak-check (and hence the subtype check) fails.
-        //
-        // What will change when we fix #32330 is that some of the LBR in the
-        // subtype may become early-bound. In that case, they would no longer be in
-        // the "permitted set" of variables that can be related to a skolemized
-        // type.
-        //
-        // So the foundation for this warning is to collect variables that we found
-        // to be related to a skolemized type. For each of them, we have a
-        // `BoundRegion` which carries a `Issue32330` flag. We check whether any of
-        // those flags indicate that this variable was created from a lifetime
-        // that will change from late- to early-bound. If so, we issue a warning
-        // indicating that the results of compilation may change.
-        //
-        // This is imperfect, since there are other kinds of code that will not
-        // compile once #32330 is fixed. However, it fixes the errors observed in
-        // practice on crater runs.
-        let mut warnings = vec![];
-
         let new_vars = self.region_vars_confined_to_snapshot(snapshot);
         for (&skol_br, &skol) in skol_map {
             // The inputs to a skolemized variable can only
@@ -680,13 +643,6 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                 match *tainted_region {
                     ty::ReVar(vid) => {
                         if new_vars.contains(&vid) {
-                            warnings.extend(
-                                match self.region_vars.var_origin(vid) {
-                                    LateBoundRegion(_,
-                                                    ty::BrNamed(.., wc),
-                                                    _) => Some(wc),
-                                    _ => None,
-                                });
                             continue;
                         }
                     }
@@ -712,8 +668,6 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
             }
         }
 
-        self.issue_32330_warnings(span, &warnings);
-
         Ok(())
     }
 

src/librustc/infer/mod.rs

@@ -368,7 +368,7 @@ pub enum RegionVariableOrigin {
     Coercion(Span),
 
     // Region variables created as the values for early-bound regions
-    EarlyBoundRegion(Span, ast::Name),
+    EarlyBoundRegion(Span, ast::Name, Option<ty::Issue32330>),
 
     // Region variables created for bound regions
     // in a function or method that is called
@@ -1184,7 +1184,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                               span: Span,
                               def: &ty::RegionParameterDef)
                               -> &'tcx ty::Region {
-        self.next_region_var(EarlyBoundRegion(span, def.name))
+        self.next_region_var(EarlyBoundRegion(span, def.name, def.issue_32330))
     }
 
     /// Create a type inference variable for the given
@@ -1761,7 +1761,7 @@ impl RegionVariableOrigin {
             AddrOfRegion(a) => a,
             Autoref(a) => a,
             Coercion(a) => a,
-            EarlyBoundRegion(a, _) => a,
+            EarlyBoundRegion(a, ..) => a,
             LateBoundRegion(a, ..) => a,
             BoundRegionInCoherence(_) => syntax_pos::DUMMY_SP,
             UpvarRegion(_, a) => a

src/librustc/middle/resolve_lifetime.rs

@@ -31,7 +31,7 @@ use syntax::ptr::P;
 use syntax::symbol::keywords;
 use syntax_pos::Span;
 use errors::DiagnosticBuilder;
-use util::nodemap::{NodeMap, FxHashSet, FxHashMap, DefIdMap};
+use util::nodemap::{NodeMap, NodeSet, FxHashSet, FxHashMap, DefIdMap};
 use rustc_back::slice;
 
 use hir;
@@ -150,10 +150,14 @@ pub struct NamedRegionMap {
     // `Region` describing how that region is bound
     pub defs: NodeMap<Region>,
 
-    // the set of lifetime def ids that are late-bound; late-bound ids
-    // are named regions appearing in fn arguments that do not appear
-    // in where-clauses
-    pub late_bound: NodeMap<ty::Issue32330>,
+    // the set of lifetime def ids that are late-bound; a region can
+    // be late-bound if (a) it does NOT appear in a where-clause and
+    // (b) it DOES appear in the arguments.
+    pub late_bound: NodeSet,
+
+    // Contains the node-ids for lifetimes that were (incorrectly) categorized
+    // as late-bound, until #32330 was fixed.
+    pub issue_32330: NodeMap<ty::Issue32330>,
 
     // For each type and trait definition, maps type parameters
     // to the trait object lifetime defaults computed from them.
@@ -261,7 +265,8 @@ pub fn krate(sess: &Session,
     let krate = hir_map.krate();
     let mut map = NamedRegionMap {
         defs: NodeMap(),
-        late_bound: NodeMap(),
+        late_bound: NodeSet(),
+        issue_32330: NodeMap(),
         object_lifetime_defaults: compute_object_lifetime_defaults(sess, hir_map),
     };
     sess.track_errors(|| {
@@ -840,7 +845,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
         }
 
         let lifetimes = generics.lifetimes.iter().map(|def| {
-            if self.map.late_bound.contains_key(&def.lifetime.id) {
+            if self.map.late_bound.contains(&def.lifetime.id) {
                 Region::late(def)
             } else {
                 Region::early(&mut index, def)
@@ -1610,22 +1615,26 @@ fn insert_late_bound_lifetimes(map: &mut NamedRegionMap,
         // just mark it so we can issue warnings.
         let constrained_by_input = constrained_by_input.regions.contains(&name);
         let appears_in_output = appears_in_output.regions.contains(&name);
-        let will_change = !constrained_by_input && appears_in_output;
-        let issue_32330 = if will_change {
-            ty::Issue32330::WillChange {
-                fn_def_id: fn_def_id,
-                region_name: name,
-            }
-        } else {
-            ty::Issue32330::WontChange
-        };
+        if !constrained_by_input && appears_in_output {
+            debug!("inserting issue_32330 entry for {:?}, {:?} on {:?}",
+                   lifetime.lifetime.id,
+                   name,
+                   fn_def_id);
+            map.issue_32330.insert(
+                lifetime.lifetime.id,
+                ty::Issue32330 {
+                    fn_def_id: fn_def_id,
+                    region_name: name,
+                });
+            continue;
+        }
 
         debug!("insert_late_bound_lifetimes: \
-                lifetime {:?} with id {:?} is late-bound ({:?}",
-               lifetime.lifetime.name, lifetime.lifetime.id, issue_32330);
+                lifetime {:?} with id {:?} is late-bound",
+               lifetime.lifetime.name, lifetime.lifetime.id);
 
-        let prev = map.late_bound.insert(lifetime.lifetime.id, issue_32330);
-        assert!(prev.is_none(), "visited lifetime {:?} twice", lifetime.lifetime.id);
+        let inserted = map.late_bound.insert(lifetime.lifetime.id);
+        assert!(inserted, "visited lifetime {:?} twice", lifetime.lifetime.id);
     }
 
     return;

src/librustc/ty/mod.rs

@@ -606,6 +606,7 @@ pub struct RegionParameterDef {
     pub name: Name,
     pub def_id: DefId,
     pub index: u32,
+    pub issue_32330: Option<ty::Issue32330>,
 
     /// `pure_wrt_drop`, set by the (unsafe) `#[may_dangle]` attribute
     /// on generic parameter `'a`, asserts data of lifetime `'a`
@@ -622,8 +623,7 @@ impl RegionParameterDef {
     }
 
     pub fn to_bound_region(&self) -> ty::BoundRegion {
-        // this is an early bound region, so unaffected by #32330
-        ty::BoundRegion::BrNamed(self.def_id, self.name, Issue32330::WontChange)
+        ty::BoundRegion::BrNamed(self.def_id, self.name)
     }
 }
 

src/librustc/ty/sty.rs

@@ -58,7 +58,7 @@ pub enum BoundRegion {
     ///
     /// The def-id is needed to distinguish free regions in
     /// the event of shadowing.
-    BrNamed(DefId, Name, Issue32330),
+    BrNamed(DefId, Name),
 
     /// Fresh bound identifiers created during GLB computations.
     BrFresh(u32),
@@ -68,23 +68,18 @@ pub enum BoundRegion {
     BrEnv
 }
 
-/// True if this late-bound region is unconstrained, and hence will
-/// become early-bound once #32330 is fixed.
+/// When a region changed from late-bound to early-bound when #32330
+/// was fixed, its `RegionParameterDef` will have one of these
+/// structures that we can use to give nicer errors.
 #[derive(Copy, Clone, Debug, PartialEq, PartialOrd, Eq, Ord, Hash,
          RustcEncodable, RustcDecodable)]
-pub enum Issue32330 {
-    WontChange,
+pub struct Issue32330 {
+    /// fn where is region declared
+    pub fn_def_id: DefId,
 
-    /// this region will change from late-bound to early-bound once
-    /// #32330 is fixed.
-    WillChange {
-        /// fn where is region declared
-        fn_def_id: DefId,
-
-        /// name of region; duplicates the info in BrNamed but convenient
-        /// to have it here, and this code is only temporary
-        region_name: ast::Name,
-    }
+    /// name of region; duplicates the info in BrNamed but convenient
+    /// to have it here, and this code is only temporary
+    pub region_name: ast::Name,
 }
 
 // NB: If you change this, you'll probably want to change the corresponding

src/librustc/util/ppaux.rs

@@ -276,7 +276,7 @@ fn in_binder<'a, 'gcx, 'tcx, T, U>(f: &mut fmt::Formatter,
     let new_value = tcx.replace_late_bound_regions(&value, |br| {
         let _ = start_or_continue(f, "for<", ", ");
         let br = match br {
-            ty::BrNamed(_, name, _) => {
+            ty::BrNamed(_, name) => {
                 let _ = write!(f, "{}", name);
                 br
             }
@@ -286,8 +286,7 @@ fn in_binder<'a, 'gcx, 'tcx, T, U>(f: &mut fmt::Formatter,
                 let name = Symbol::intern("'r");
                 let _ = write!(f, "{}", name);
                 ty::BrNamed(tcx.hir.local_def_id(CRATE_NODE_ID),
-                            name,
-                            ty::Issue32330::WontChange)
+                            name)
             }
         };
         tcx.mk_region(ty::ReLateBound(ty::DebruijnIndex::new(1), br))
@@ -435,7 +434,7 @@ impl fmt::Display for ty::BoundRegion {
         }
 
         match *self {
-            BrNamed(_, name, _) => write!(f, "{}", name),
+            BrNamed(_, name) => write!(f, "{}", name),
             BrAnon(_) | BrFresh(_) | BrEnv => Ok(())
         }
     }
@@ -446,9 +445,9 @@ impl fmt::Debug for ty::BoundRegion {
         match *self {
             BrAnon(n) => write!(f, "BrAnon({:?})", n),
             BrFresh(n) => write!(f, "BrFresh({:?})", n),
-            BrNamed(did, name, issue32330) => {
-                write!(f, "BrNamed({:?}:{:?}, {:?}, {:?})",
-                       did.krate, did.index, name, issue32330)
+            BrNamed(did, name) => {
+                write!(f, "BrNamed({:?}:{:?}, {:?})",
+                       did.krate, did.index, name)
             }
             BrEnv => "BrEnv".fmt(f),
         }