From bd5fa7532d9f9944b6347624553f420bd7c89132 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 5 Oct 2016 10:17:14 -0400 Subject: [PATCH] cleanup error reporting and add `ui` tests --- src/librustc/infer/error_reporting.rs | 90 ++++++++++++++----- src/librustc/infer/mod.rs | 39 +++++++- src/librustc/traits/error_reporting.rs | 47 ++++++++-- src/librustc/traits/fulfill.rs | 2 +- src/librustc/traits/mod.rs | 6 +- src/librustc/traits/structural_impls.rs | 14 ++- src/librustc_typeck/check/compare_method.rs | 6 +- src/librustc_typeck/check/regionck.rs | 14 +-- .../compare-method/proj-outlives-region.rs} | 3 +- .../proj-outlives-region.stderr | 11 +++ .../compare-method/region-unrelated.rs} | 0 .../ui/compare-method/region-unrelated.stderr | 11 +++ src/test/ui/compare-method/region.rs | 27 ++++++ src/test/ui/compare-method/region.stderr | 11 +++ src/test/ui/update-references.sh | 4 +- 15 files changed, 229 insertions(+), 56 deletions(-) rename src/test/{compile-fail/traits-elaborate-type-region-proj.rs => ui/compare-method/proj-outlives-region.rs} (89%) create mode 100644 src/test/ui/compare-method/proj-outlives-region.stderr rename src/test/{compile-fail/traits-elaborate-type-region-unrelated.rs => ui/compare-method/region-unrelated.rs} (100%) create mode 100644 src/test/ui/compare-method/region-unrelated.stderr create mode 100644 src/test/ui/compare-method/region.rs create mode 100644 src/test/ui/compare-method/region.stderr diff --git a/src/librustc/infer/error_reporting.rs b/src/librustc/infer/error_reporting.rs index 5e3925b0b3c..c9850eb650a 100644 --- a/src/librustc/infer/error_reporting.rs +++ b/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,58 @@ 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); - } - None => { - other_errors.push(error.clone()); - } + SubSupConflict(ref var_origin, ref sub_origin, sub, ref sup_origin, sup) => { + debug!("processing SubSupConflict sub: {:?} sup: {:?}", sub, sup); + if let SubregionOrigin::CompareImplMethodObligation { .. } = *sub_origin { + // As above, when comparing an impl method + // against a trait method, it is not helpful + // to suggest changes to the impl method. + } else if let SubregionOrigin::CompareImplMethodObligation { .. } = *sup_origin { + // See above. + } else 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 +645,18 @@ 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 + } = origin { + self.report_extra_impl_obligation(span, + item_name, + impl_item_def_id, + trait_item_def_id, + &format!("`{}: {}`", bound_kind, sub)) + .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 +974,16 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { ""); err } + infer::CompareImplMethodObligation { span, + item_name, + impl_item_def_id, + trait_item_def_id } => { + self.report_extra_impl_obligation(span, + item_name, + impl_item_def_id, + trait_item_def_id, + &format!("`{}: {}`", sup, sub)) + } } } @@ -1792,6 +1829,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"); + } } } } diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs index af994e884fe..9d0d0b063be 100644 --- a/src/librustc/infer/mod.rs +++ b/src/librustc/infer/mod.rs @@ -360,6 +360,15 @@ 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, + }, } /// Places that type/region parameters can appear. @@ -1152,16 +1161,16 @@ 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_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)) }) } @@ -1792,6 +1801,30 @@ impl<'tcx> SubregionOrigin<'tcx> { AddrOf(a) => a, AutoBorrow(a) => a, SafeDestructor(a) => a, + CompareImplMethodObligation { span, .. } => span, + } + } + + pub fn from_cause(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 } => + 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, + }, + + _ => default(), } } } diff --git a/src/librustc/traits/error_reporting.rs b/src/librustc/traits/error_reporting.rs index 9435f96c08a..56ab8cb22ce 100644 --- a/src/librustc/traits/error_reporting.rs +++ b/src/librustc/traits/error_reporting.rs @@ -36,6 +36,7 @@ use util::nodemap::{FnvHashMap, FnvHashSet}; use std::cmp; use std::fmt; +use syntax::ast; use syntax_pos::Span; use errors::DiagnosticBuilder; @@ -417,6 +418,32 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { self.report_overflow_error(&cycle[0], false); } + pub fn report_extra_impl_obligation(&self, + error_span: Span, + item_name: ast::Name, + _impl_item_def_id: DefId, + trait_item_def_id: DefId, + requirement: &fmt::Display) + -> DiagnosticBuilder<'tcx> + { + let mut err = + struct_span_err!(self.tcx.sess, + error_span, + E0276, + "impl has stricter requirements than trait"); + + if let Some(trait_item_span) = self.tcx.map.span_if_local(trait_item_def_id) { + err.span_label(trait_item_span, + &format!("definition of `{}` from trait", item_name)); + } + + err.span_label( + error_span, + &format!("impl has extra requirement {}", requirement)); + + err + } + pub fn report_selection_error(&self, obligation: &PredicateObligation<'tcx>, error: &SelectionError<'tcx>) @@ -424,12 +451,16 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { let span = obligation.cause.span; let mut err = match *error { SelectionError::Unimplemented => { - if let ObligationCauseCode::CompareImplMethodObligation = obligation.cause.code { - span_err!( - self.tcx.sess, span, E0276, - "the requirement `{}` appears on the impl \ - method but not on the corresponding trait method", - obligation.predicate); + if let ObligationCauseCode::CompareImplMethodObligation { + item_name, impl_item_def_id, trait_item_def_id + } = obligation.cause.code { + self.report_extra_impl_obligation( + span, + item_name, + impl_item_def_id, + trait_item_def_id, + &format!("`{}`", obligation.predicate)) + .emit(); return; } else { match obligation.predicate { @@ -492,7 +523,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { ty::Predicate::RegionOutlives(ref predicate) => { let predicate = self.resolve_type_vars_if_possible(predicate); - let err = self.region_outlives_predicate(span, + let err = self.region_outlives_predicate(&obligation.cause, &predicate).err().unwrap(); struct_span_err!(self.tcx.sess, span, E0279, "the requirement `{}` is not satisfied (`{}`)", @@ -886,7 +917,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> { &parent_predicate, &data.parent_code); } - ObligationCauseCode::CompareImplMethodObligation => { + ObligationCauseCode::CompareImplMethodObligation { .. } => { err.note( &format!("the requirement `{}` appears on the impl method \ but not on the corresponding trait method", diff --git a/src/librustc/traits/fulfill.rs b/src/librustc/traits/fulfill.rs index ded2fdc58b4..906da429036 100644 --- a/src/librustc/traits/fulfill.rs +++ b/src/librustc/traits/fulfill.rs @@ -526,7 +526,7 @@ fn process_predicate<'a, 'gcx, 'tcx>( } ty::Predicate::RegionOutlives(ref binder) => { - match selcx.infcx().region_outlives_predicate(obligation.cause.span, binder) { + match selcx.infcx().region_outlives_predicate(&obligation.cause, binder) { Ok(()) => Ok(Some(Vec::new())), Err(_) => Err(CodeSelectionError(Unimplemented)), } diff --git a/src/librustc/traits/mod.rs b/src/librustc/traits/mod.rs index 7ba10d9c0a5..64016da4de7 100644 --- a/src/librustc/traits/mod.rs +++ b/src/librustc/traits/mod.rs @@ -138,7 +138,11 @@ pub enum ObligationCauseCode<'tcx> { ImplDerivedObligation(DerivedObligationCause<'tcx>), - CompareImplMethodObligation, + CompareImplMethodObligation { + item_name: ast::Name, + impl_item_def_id: DefId, + trait_item_def_id: DefId + }, } #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/src/librustc/traits/structural_impls.rs b/src/librustc/traits/structural_impls.rs index 022566642f6..57ae176c0e9 100644 --- a/src/librustc/traits/structural_impls.rs +++ b/src/librustc/traits/structural_impls.rs @@ -195,8 +195,14 @@ impl<'a, 'tcx> Lift<'tcx> for traits::ObligationCauseCode<'a> { super::ImplDerivedObligation(ref cause) => { tcx.lift(cause).map(super::ImplDerivedObligation) } - super::CompareImplMethodObligation => { - Some(super::CompareImplMethodObligation) + super::CompareImplMethodObligation { item_name, + impl_item_def_id, + trait_item_def_id } => { + Some(super::CompareImplMethodObligation { + item_name: item_name, + impl_item_def_id: impl_item_def_id, + trait_item_def_id: trait_item_def_id, + }) } } } @@ -459,7 +465,7 @@ impl<'tcx> TypeFoldable<'tcx> for traits::ObligationCauseCode<'tcx> { super::FieldSized | super::ConstSized | super::SharedStatic | - super::CompareImplMethodObligation => self.clone(), + super::CompareImplMethodObligation { .. } => self.clone(), super::ProjectionWf(proj) => super::ProjectionWf(proj.fold_with(folder)), super::ReferenceOutlivesReferent(ty) => { @@ -492,7 +498,7 @@ impl<'tcx> TypeFoldable<'tcx> for traits::ObligationCauseCode<'tcx> { super::FieldSized | super::ConstSized | super::SharedStatic | - super::CompareImplMethodObligation => false, + super::CompareImplMethodObligation { .. } => false, super::ProjectionWf(proj) => proj.visit_with(visitor), super::ReferenceOutlivesReferent(ty) => ty.visit_with(visitor), diff --git a/src/librustc_typeck/check/compare_method.rs b/src/librustc_typeck/check/compare_method.rs index 0dc3cf7e467..a5b3811ec61 100644 --- a/src/librustc_typeck/check/compare_method.rs +++ b/src/librustc_typeck/check/compare_method.rs @@ -363,7 +363,11 @@ pub fn compare_impl_method<'a, 'tcx>(ccx: &CrateCtxt<'a, 'tcx>, let cause = traits::ObligationCause { span: impl_m_span, body_id: impl_m_body_id, - code: traits::ObligationCauseCode::CompareImplMethodObligation, + code: traits::ObligationCauseCode::CompareImplMethodObligation { + item_name: impl_m.name, + impl_item_def_id: impl_m.def_id, + trait_item_def_id: trait_m.def_id, + }, }; fulfillment_cx.borrow_mut().register_predicate_obligation( diff --git a/src/librustc_typeck/check/regionck.rs b/src/librustc_typeck/check/regionck.rs index 23201acf8ee..536c4a9d524 100644 --- a/src/librustc_typeck/check/regionck.rs +++ b/src/librustc_typeck/check/regionck.rs @@ -356,7 +356,7 @@ impl<'a, 'gcx, 'tcx> RegionCtxt<'a, 'gcx, 'tcx> { debug!("visit_region_obligations: r_o={:?} cause={:?}", r_o, r_o.cause); let sup_type = self.resolve_type(r_o.sup_type); - let origin = self.code_to_origin(r_o.cause.span, sup_type, &r_o.cause.code); + let origin = self.code_to_origin(&r_o.cause, sup_type); self.type_must_outlive(origin, sup_type, r_o.sub_region); } @@ -366,16 +366,10 @@ impl<'a, 'gcx, 'tcx> RegionCtxt<'a, 'gcx, 'tcx> { } fn code_to_origin(&self, - span: Span, - sup_type: Ty<'tcx>, - code: &traits::ObligationCauseCode<'tcx>) + cause: &traits::ObligationCause<'tcx>, + sup_type: Ty<'tcx>) -> SubregionOrigin<'tcx> { - match *code { - traits::ObligationCauseCode::ReferenceOutlivesReferent(ref_type) => - infer::ReferenceOutlivesReferent(ref_type, span), - _ => - infer::RelateParamBound(span, sup_type), - } + SubregionOrigin::from_cause(cause, || infer::RelateParamBound(cause.span, sup_type)) } /// This method populates the region map's `free_region_map`. It walks over the transformed diff --git a/src/test/compile-fail/traits-elaborate-type-region-proj.rs b/src/test/ui/compare-method/proj-outlives-region.rs similarity index 89% rename from src/test/compile-fail/traits-elaborate-type-region-proj.rs rename to src/test/ui/compare-method/proj-outlives-region.rs index 42a2e820d2e..631da7140ea 100644 --- a/src/test/compile-fail/traits-elaborate-type-region-proj.rs +++ b/src/test/ui/compare-method/proj-outlives-region.rs @@ -18,8 +18,7 @@ trait Master<'a, T: ?Sized, U> { // `U::Item: 'a` does not imply that `U: 'a` impl<'a, U: Iterator> Master<'a, U::Item, U> for () { - fn foo() where U: 'a { } - //~^ ERROR parameter type `V` may not live long enough + fn foo() where U: 'a { } //~ ERROR E0276 } fn main() { diff --git a/src/test/ui/compare-method/proj-outlives-region.stderr b/src/test/ui/compare-method/proj-outlives-region.stderr new file mode 100644 index 00000000000..b36d64e3b9c --- /dev/null +++ b/src/test/ui/compare-method/proj-outlives-region.stderr @@ -0,0 +1,11 @@ +error[E0276]: impl has stricter requirements than trait + --> $DIR/proj-outlives-region.rs:21:5 + | +16 | fn foo() where T: 'a; + | --------------------- definition of `foo` from trait +... +21 | fn foo() where U: 'a { } //~ ERROR E0276 + | ^^^^^^^^^^^^^^^^^^^^^^^^ impl has extra requirement `U: 'a` + +error: aborting due to previous error + diff --git a/src/test/compile-fail/traits-elaborate-type-region-unrelated.rs b/src/test/ui/compare-method/region-unrelated.rs similarity index 100% rename from src/test/compile-fail/traits-elaborate-type-region-unrelated.rs rename to src/test/ui/compare-method/region-unrelated.rs diff --git a/src/test/ui/compare-method/region-unrelated.stderr b/src/test/ui/compare-method/region-unrelated.stderr new file mode 100644 index 00000000000..fb3511867e5 --- /dev/null +++ b/src/test/ui/compare-method/region-unrelated.stderr @@ -0,0 +1,11 @@ +error[E0276]: impl has stricter requirements than trait + --> $DIR/region-unrelated.rs:21:5 + | +16 | fn foo() where T: 'a; + | --------------------- definition of `foo` from trait +... +21 | fn foo() where V: 'a { } + | ^^^^^^^^^^^^^^^^^^^^^^^^ impl has extra requirement `V: 'a` + +error: aborting due to previous error + diff --git a/src/test/ui/compare-method/region.rs b/src/test/ui/compare-method/region.rs new file mode 100644 index 00000000000..1942ca9504c --- /dev/null +++ b/src/test/ui/compare-method/region.rs @@ -0,0 +1,27 @@ +// Copyright 2016 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![allow(dead_code)] + +// Test that we elaborate `Type: 'region` constraints and infer various important things. + +trait Master<'a, 'b> { + fn foo(); +} + +// `U: 'a` does not imply `V: 'a` +impl<'a, 'b> Master<'a, 'b> for () { + fn foo() where 'a: 'b { } + //~^ ERROR parameter type `V` may not live long enough +} + +fn main() { + println!("Hello, world!"); +} diff --git a/src/test/ui/compare-method/region.stderr b/src/test/ui/compare-method/region.stderr new file mode 100644 index 00000000000..78e00e066bd --- /dev/null +++ b/src/test/ui/compare-method/region.stderr @@ -0,0 +1,11 @@ +error[E0276]: impl has stricter requirements than trait + --> $DIR/region.rs:21:5 + | +16 | fn foo(); + | --------- definition of `foo` from trait +... +21 | fn foo() where 'a: 'b { } + | ^^^^^^^^^^^^^^^^^^^^^^^^^ impl has extra requirement `'a: 'b` + +error: aborting due to previous error + diff --git a/src/test/ui/update-references.sh b/src/test/ui/update-references.sh index f0a6f8a3d44..aa99d35f7aa 100755 --- a/src/test/ui/update-references.sh +++ b/src/test/ui/update-references.sh @@ -36,12 +36,12 @@ while [[ "$1" != "" ]]; do STDOUT_NAME="${1/%.rs/.stdout}" shift if [ -f $BUILD_DIR/$STDOUT_NAME ] && \ - ! (diff $BUILD_DIR/$STDOUT_NAME $MYDIR/$STDOUT_NAME > /dev/null); then + ! (diff $BUILD_DIR/$STDOUT_NAME $MYDIR/$STDOUT_NAME >& /dev/null); then echo updating $MYDIR/$STDOUT_NAME cp $BUILD_DIR/$STDOUT_NAME $MYDIR/$STDOUT_NAME fi if [ -f $BUILD_DIR/$STDERR_NAME ] && \ - ! (diff $BUILD_DIR/$STDERR_NAME $MYDIR/$STDERR_NAME > /dev/null); then + ! (diff $BUILD_DIR/$STDERR_NAME $MYDIR/$STDERR_NAME >& /dev/null); then echo updating $MYDIR/$STDERR_NAME cp $BUILD_DIR/$STDERR_NAME $MYDIR/$STDERR_NAME fi