Auto merge of #37167 - nikomatsakis:jroesch-issue-18937, r=pnkfelix

detect extra region requirements in impls

The current "compare method" check fails to check for the "region obligations" that accrue in the fulfillment context. This branch switches that code to create a `FnCtxt` so that it can invoke the regionck code. Previous crater runs (I haven't done one with the latest tip) have found some small number of affected crates, so I went ahead and introduced a warning cycle. I will kick off a crater run with this branch shortly.

This is a [breaking-change] because previously unsound code was accepted. The crater runs also revealed some cases where legitimate code was no longer type-checking, so the branch contains one additional (but orthogonal) change. It improves the elaborator so that we elaborate region requirements more thoroughly. In particular, if we know that `&'a T: 'b`, we now deduce that `T: 'b` and `'a: 'b`.

I invested a certain amount of effort in getting a good error message. The error message looks like this:

```
error[E0276]: impl has stricter requirements than trait
  --> traits-elaborate-projection-region.rs:33:5
   |
21 |     fn foo() where T: 'a;
   |     --------------------- definition of `foo` from trait
...
33 |     fn foo() where U: 'a { }
   |     ^^^^^^^^^^^^^^^^^^^^^^^^ impl has extra requirement `U: 'a`
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #18937 <#18937>
note: lint level defined here
  --> traits-elaborate-projection-region.rs:12:9
   |
12 | #![deny(extra_requirement_in_impl)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^
```

Obviously the warning only prints if this is a _new_ error (that resulted from the bugfix). But all existing errors that fit this description are updated to follow the general template. In order to get the lint to preserve the span-labels and the error code, I separate out the core `Diagnostic` type (which encapsulates the error code, message, span, and children) from the `DiagnosticBuilder` (which layers on a `Handler` that can be used to report errors). I also extended `add_lint` with an alternative `add_lint_diagnostic` that takes in a full diagnostic (cc @jonathandturner for those changes). This doesn't feel ideal but feels like it's moving in the right direction =).

r? @pnkfelix
cc @arielb1

Fixes #18937

Author: bors

src/librustc/infer/error_reporting.rs

@@ -245,6 +245,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
         debug!("report_region_errors: {} errors after preprocessing", errors.len());
 
         for error in errors {
+            debug!("report_region_errors: error = {:?}", error);
             match error.clone() {
                 ConcreteFailure(origin, sub, sup) => {
                     self.report_concrete_failure(origin, sub, sup).emit();
@@ -299,44 +300,64 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
         let mut bound_failures = Vec::new();
 
         for error in errors {
+            // Check whether we can process this error into some other
+            // form; if not, fall through.
             match *error {
                 ConcreteFailure(ref origin, sub, sup) => {
                     debug!("processing ConcreteFailure");
-                    match free_regions_from_same_fn(self.tcx, sub, sup) {
-                        Some(ref same_frs) => {
-                            origins.push(
-                                ProcessedErrorOrigin::ConcreteFailure(
-                                    origin.clone(),
-                                    sub,
-                                    sup));
-                            append_to_same_regions(&mut same_regions, same_frs);
-                        }
-                        _ => {
-                            other_errors.push(error.clone());
-                        }
+                    if let SubregionOrigin::CompareImplMethodObligation { .. } = *origin {
+                        // When comparing an impl method against a
+                        // trait method, it is not helpful to suggest
+                        // changes to the impl method.  This is
+                        // because the impl method signature is being
+                        // checked using the trait's environment, so
+                        // usually the changes we suggest would
+                        // actually have to be applied to the *trait*
+                        // method (and it's not clear that the trait
+                        // method is even under the user's control).
+                    } else if let Some(same_frs) = free_regions_from_same_fn(self.tcx, sub, sup) {
+                        origins.push(
+                            ProcessedErrorOrigin::ConcreteFailure(
+                                origin.clone(),
+                                sub,
+                                sup));
+                        append_to_same_regions(&mut same_regions, &same_frs);
+                        continue;
                     }
                 }
-                SubSupConflict(ref var_origin, _, sub_r, _, sup_r) => {
-                    debug!("processing SubSupConflict sub: {:?} sup: {:?}", sub_r, sup_r);
-                    match free_regions_from_same_fn(self.tcx, sub_r, sup_r) {
-                        Some(ref same_frs) => {
-                            origins.push(
-                                ProcessedErrorOrigin::VariableFailure(
-                                    var_origin.clone()));
-                            append_to_same_regions(&mut same_regions, same_frs);
+                SubSupConflict(ref var_origin, ref sub_origin, sub, ref sup_origin, sup) => {
+                    debug!("processing SubSupConflict sub: {:?} sup: {:?}", sub, sup);
+                    match (sub_origin, sup_origin) {
+                        (&SubregionOrigin::CompareImplMethodObligation { .. }, _) => {
+                            // As above, when comparing an impl method
+                            // against a trait method, it is not helpful
+                            // to suggest changes to the impl method.
                         }
-                        None => {
-                            other_errors.push(error.clone());
+                        (_, &SubregionOrigin::CompareImplMethodObligation { .. }) => {
+                            // See above.
+                        }
+                        _ => {
+                            if let Some(same_frs) = free_regions_from_same_fn(self.tcx, sub, sup) {
+                                origins.push(
+                                    ProcessedErrorOrigin::VariableFailure(
+                                        var_origin.clone()));
+                                append_to_same_regions(&mut same_regions, &same_frs);
+                                continue;
+                            }
                         }
                     }
                 }
                 GenericBoundFailure(ref origin, ref kind, region) => {
                     bound_failures.push((origin.clone(), kind.clone(), region));
+                    continue;
                 }
                 ProcessedErrors(..) => {
                     bug!("should not encounter a `ProcessedErrors` yet: {:?}", error)
                 }
             }
+
+            // No changes to this error.
+            other_errors.push(error.clone());
         }
 
         // ok, let's pull together the errors, sorted in an order that
@@ -630,6 +651,19 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                 format!("the associated type `{}`", p),
         };
 
+        if let SubregionOrigin::CompareImplMethodObligation {
+            span, item_name, impl_item_def_id, trait_item_def_id, lint_id
+        } = origin {
+            self.report_extra_impl_obligation(span,
+                                              item_name,
+                                              impl_item_def_id,
+                                              trait_item_def_id,
+                                              &format!("`{}: {}`", bound_kind, sub),
+                                              lint_id)
+                .emit();
+            return;
+        }
+
         let mut err = match *sub {
             ty::ReFree(ty::FreeRegion {bound_region: ty::BrNamed(..), ..}) => {
                 // Does the required lifetime have a nice name we can print?
@@ -947,6 +981,18 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                     "");
                 err
             }
+            infer::CompareImplMethodObligation { span,
+                                                 item_name,
+                                                 impl_item_def_id,
+                                                 trait_item_def_id,
+                                                 lint_id } => {
+                self.report_extra_impl_obligation(span,
+                                                  item_name,
+                                                  impl_item_def_id,
+                                                  trait_item_def_id,
+                                                  &format!("`{}: {}`", sup, sub),
+                                                  lint_id)
+            }
         }
     }
 
@@ -1792,6 +1838,11 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                     "...so that references are valid when the destructor \
                      runs");
             }
+            infer::CompareImplMethodObligation { span, .. } => {
+                err.span_note(
+                    span,
+                    "...so that the definition in impl matches the definition from the trait");
+            }
         }
     }
 }

src/librustc/infer/mod.rs

@@ -355,6 +355,19 @@ pub enum SubregionOrigin<'tcx> {
 
     // Region constraint arriving from destructor safety
     SafeDestructor(Span),
+
+    // Comparing the signature and requirements of an impl method against
+    // the containing trait.
+    CompareImplMethodObligation {
+        span: Span,
+        item_name: ast::Name,
+        impl_item_def_id: DefId,
+        trait_item_def_id: DefId,
+
+        // this is `Some(_)` if this error arises from the bug fix for
+        // #18937. This is a temporary measure.
+        lint_id: Option<ast::NodeId>,
+    },
 }
 
 /// Places that type/region parameters can appear.
@@ -1147,16 +1160,18 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
     }
 
     pub fn region_outlives_predicate(&self,
-                                     span: Span,
+                                     cause: &traits::ObligationCause<'tcx>,
                                      predicate: &ty::PolyRegionOutlivesPredicate<'tcx>)
         -> UnitResult<'tcx>
     {
         self.commit_if_ok(|snapshot| {
             let (ty::OutlivesPredicate(r_a, r_b), skol_map) =
                 self.skolemize_late_bound_regions(predicate, snapshot);
-            let origin = RelateRegionParamBound(span);
+            let origin =
+                SubregionOrigin::from_obligation_cause(cause,
+                                                       || RelateRegionParamBound(cause.span));
             self.sub_regions(origin, r_b, r_a); // `b : a` ==> `a <= b`
-            self.leak_check(false, span, &skol_map, snapshot)?;
+            self.leak_check(false, cause.span, &skol_map, snapshot)?;
             Ok(self.pop_skolemized(skol_map, snapshot))
         })
     }
@@ -1786,6 +1801,32 @@ impl<'tcx> SubregionOrigin<'tcx> {
             AddrOf(a) => a,
             AutoBorrow(a) => a,
             SafeDestructor(a) => a,
+            CompareImplMethodObligation { span, .. } => span,
+        }
+    }
+
+    pub fn from_obligation_cause<F>(cause: &traits::ObligationCause<'tcx>,
+                                    default: F)
+                                    -> Self
+        where F: FnOnce() -> Self
+    {
+        match cause.code {
+            traits::ObligationCauseCode::ReferenceOutlivesReferent(ref_type) =>
+                SubregionOrigin::ReferenceOutlivesReferent(ref_type, cause.span),
+
+            traits::ObligationCauseCode::CompareImplMethodObligation { item_name,
+                                                                       impl_item_def_id,
+                                                                       trait_item_def_id,
+                                                                       lint_id } =>
+                SubregionOrigin::CompareImplMethodObligation {
+                    span: cause.span,
+                    item_name: item_name,
+                    impl_item_def_id: impl_item_def_id,
+                    trait_item_def_id: trait_item_def_id,
+                    lint_id: lint_id,
+                },
+
+            _ => default(),
         }
     }
 }

src/librustc/lint/builtin.rs

@@ -198,6 +198,12 @@ declare_lint! {
     "patterns in functions without body were erroneously allowed"
 }
 
+declare_lint! {
+    pub EXTRA_REQUIREMENT_IN_IMPL,
+    Warn,
+    "detects extra requirements in impls that were erroneously allowed"
+}
+
 /// Does nothing as a lint pass, but registers some `Lint`s
 /// which are used by other parts of the compiler.
 #[derive(Copy, Clone)]
@@ -235,7 +241,8 @@ impl LintPass for HardwiredLints {
             HR_LIFETIME_IN_ASSOC_TYPE,
             LIFETIME_UNDERSCORE,
             SAFE_EXTERN_STATICS,
-            PATTERNS_IN_FNS_WITHOUT_BODY
+            PATTERNS_IN_FNS_WITHOUT_BODY,
+            EXTRA_REQUIREMENT_IN_IMPL
         )
     }
 }