-
Notifications
You must be signed in to change notification settings - Fork 13.4k
correct spans in parem_env to deduplicate errors #102502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ use crate::infer::outlives::env::OutlivesEnvironment; | |
use crate::infer::{InferCtxt, TyCtxtInferExt}; | ||
use crate::traits::error_reporting::InferCtxtExt as _; | ||
use crate::traits::query::evaluate_obligation::InferCtxtExt as _; | ||
use rustc_data_structures::functor::IdFunctor; | ||
use rustc_errors::ErrorGuaranteed; | ||
use rustc_hir as hir; | ||
use rustc_hir::def_id::DefId; | ||
|
@@ -216,11 +217,12 @@ pub fn type_known_to_meet_bound_modulo_regions<'a, 'tcx>( | |
#[instrument(level = "debug", skip(tcx, elaborated_env))] | ||
fn do_normalize_predicates<'tcx>( | ||
tcx: TyCtxt<'tcx>, | ||
cause: ObligationCause<'tcx>, | ||
span: Span, | ||
mut causes: impl Iterator<Item = ObligationCause<'tcx>> + Debug, | ||
elaborated_env: ty::ParamEnv<'tcx>, | ||
predicates: Vec<ty::Predicate<'tcx>>, | ||
outlives: bool, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you make the parameter name more explicit? I don't understand what this toggles. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. In |
||
) -> Result<Vec<ty::Predicate<'tcx>>, ErrorGuaranteed> { | ||
let span = cause.span; | ||
// FIXME. We should really... do something with these region | ||
// obligations. But this call just continues the older | ||
// behavior (i.e., doesn't cause any new bugs), and it would | ||
|
@@ -235,7 +237,16 @@ fn do_normalize_predicates<'tcx>( | |
// them here too, and we will remove this function when | ||
// we move over to lazy normalization *anyway*. | ||
tcx.infer_ctxt().ignoring_regions().enter(|infcx| { | ||
let predicates = match fully_normalize(&infcx, cause, elaborated_env, predicates) { | ||
let predicates = match predicates.try_map_id(|predicate| { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you make this a separate local variable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for reviewing! I renamed it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, could you make a separate variable for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. I renamed it to |
||
if outlives | ||
!= matches!(predicate.kind().skip_binder(), ty::PredicateKind::TypeOutlives(..)) | ||
{ | ||
causes.next().unwrap(); | ||
Ok(predicate) | ||
} else { | ||
fully_normalize(&infcx, causes.next().unwrap(), elaborated_env, predicate) | ||
} | ||
}) { | ||
Ok(predicates) => predicates, | ||
Err(errors) => { | ||
let reported = infcx.report_fulfillment_errors(&errors, None, false); | ||
|
@@ -262,7 +273,15 @@ fn do_normalize_predicates<'tcx>( | |
); | ||
} | ||
|
||
match infcx.fully_resolve(predicates) { | ||
match predicates.try_map_id(|predicate| { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise. |
||
if outlives | ||
!= matches!(predicate.kind().skip_binder(), ty::PredicateKind::TypeOutlives(..)) | ||
{ | ||
Ok(predicate) | ||
} else { | ||
infcx.fully_resolve(predicate) | ||
} | ||
}) { | ||
Ok(predicates) => Ok(predicates), | ||
Err(fixup_err) => { | ||
// If we encounter a fixup error, it means that some type | ||
|
@@ -286,6 +305,7 @@ fn do_normalize_predicates<'tcx>( | |
|
||
// FIXME: this is gonna need to be removed ... | ||
/// Normalizes the parameter environment, reporting errors if they occur. | ||
#[inline] | ||
#[instrument(level = "debug", skip(tcx))] | ||
pub fn normalize_param_env_or_error<'tcx>( | ||
tcx: TyCtxt<'tcx>, | ||
|
@@ -306,13 +326,30 @@ pub fn normalize_param_env_or_error<'tcx>( | |
// parameter environments once for every fn as it goes, | ||
// and errors will get reported then; so outside of type inference we | ||
// can be sure that no errors should occur. | ||
let mut predicates: Vec<_> = | ||
let predicates: Vec<_> = | ||
util::elaborate_predicates(tcx, unnormalized_env.caller_bounds().into_iter()) | ||
.map(|obligation| obligation.predicate) | ||
.collect(); | ||
|
||
debug!("normalize_param_env_or_error: elaborated-predicates={:?}", predicates); | ||
|
||
normalize_param_env_with_causes( | ||
tcx, | ||
unnormalized_env, | ||
cause.span, | ||
std::iter::repeat(cause), | ||
predicates, | ||
) | ||
} | ||
|
||
#[instrument(level = "debug", skip(tcx))] | ||
pub fn normalize_param_env_with_causes<'tcx>( | ||
tcx: TyCtxt<'tcx>, | ||
unnormalized_env: ty::ParamEnv<'tcx>, | ||
span: Span, | ||
causes: impl Iterator<Item = ObligationCause<'tcx>> + Clone + Debug, | ||
predicates: Vec<ty::Predicate<'tcx>>, | ||
) -> ty::ParamEnv<'tcx> { | ||
let elaborated_env = ty::ParamEnv::new( | ||
tcx.intern_predicates(&predicates), | ||
unnormalized_env.reveal(), | ||
|
@@ -337,53 +374,41 @@ pub fn normalize_param_env_or_error<'tcx>( | |
// | ||
// This works fairly well because trait matching does not actually care about param-env | ||
// TypeOutlives predicates - these are normally used by regionck. | ||
let outlives_predicates: Vec<_> = predicates | ||
.drain_filter(|predicate| { | ||
matches!(predicate.kind().skip_binder(), ty::PredicateKind::TypeOutlives(..)) | ||
}) | ||
.collect(); | ||
|
||
debug!( | ||
"normalize_param_env_or_error: predicates=(non-outlives={:?}, outlives={:?})", | ||
predicates, outlives_predicates | ||
); | ||
let Ok(non_outlives_predicates) = do_normalize_predicates( | ||
let Ok(predicates) = do_normalize_predicates( | ||
tcx, | ||
cause.clone(), | ||
span.clone(), | ||
causes.clone(), | ||
elaborated_env, | ||
predicates, | ||
false, | ||
) else { | ||
// An unnormalized env is better than nothing. | ||
debug!("normalize_param_env_or_error: errored resolving non-outlives predicates"); | ||
return elaborated_env; | ||
}; | ||
|
||
debug!("normalize_param_env_or_error: non-outlives predicates={:?}", non_outlives_predicates); | ||
|
||
// Not sure whether it is better to include the unnormalized TypeOutlives predicates | ||
// here. I believe they should not matter, because we are ignoring TypeOutlives param-env | ||
// predicates here anyway. Keeping them here anyway because it seems safer. | ||
let outlives_env: Vec<_> = | ||
non_outlives_predicates.iter().chain(&outlives_predicates).cloned().collect(); | ||
let outlives_env = ty::ParamEnv::new( | ||
tcx.intern_predicates(&outlives_env), | ||
tcx.intern_predicates(&predicates), | ||
unnormalized_env.reveal(), | ||
unnormalized_env.constness(), | ||
); | ||
let Ok(outlives_predicates) = do_normalize_predicates( | ||
let Ok(predicates) = do_normalize_predicates( | ||
tcx, | ||
cause, | ||
span, | ||
causes, | ||
outlives_env, | ||
outlives_predicates, | ||
predicates, | ||
true | ||
) else { | ||
// An unnormalized env is better than nothing. | ||
debug!("normalize_param_env_or_error: errored resolving outlives predicates"); | ||
return elaborated_env; | ||
}; | ||
debug!("normalize_param_env_or_error: outlives predicates={:?}", outlives_predicates); | ||
|
||
let mut predicates = non_outlives_predicates; | ||
predicates.extend(outlives_predicates); | ||
debug!("normalize_param_env_or_error: final predicates={:?}", predicates); | ||
ty::ParamEnv::new( | ||
tcx.intern_predicates(&predicates), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
use rustc_data_structures::fx::FxIndexSet; | ||
use rustc_hir as hir; | ||
use rustc_hir::def_id::DefId; | ||
use rustc_infer::traits::util::elaborate_obligations; | ||
use rustc_infer::traits::Obligation; | ||
use rustc_middle::ty::{self, Binder, Predicate, PredicateKind, ToPredicate, Ty, TyCtxt}; | ||
use rustc_trait_selection::traits; | ||
|
||
|
@@ -110,7 +112,7 @@ fn param_env(tcx: TyCtxt<'_>, def_id: DefId) -> ty::ParamEnv<'_> { | |
} | ||
// Compute the bounds on Self and the type parameters. | ||
|
||
let ty::InstantiatedPredicates { mut predicates, .. } = | ||
let ty::InstantiatedPredicates { mut predicates, spans } = | ||
tcx.predicates_of(def_id).instantiate_identity(tcx); | ||
|
||
// Finally, we have to normalize the bounds in the environment, in | ||
|
@@ -132,6 +134,7 @@ fn param_env(tcx: TyCtxt<'_>, def_id: DefId) -> ty::ParamEnv<'_> { | |
|
||
let local_did = def_id.as_local(); | ||
let hir_id = local_did.map(|def_id| tcx.hir().local_def_id_to_hir_id(def_id)); | ||
let mut is_fn = false; | ||
|
||
let constness = match hir_id { | ||
Some(hir_id) => match tcx.hir().get(hir_id) { | ||
|
@@ -181,8 +184,11 @@ fn param_env(tcx: TyCtxt<'_>, def_id: DefId) -> ty::ParamEnv<'_> { | |
kind: | ||
hir::ItemKind::Fn(hir::FnSig { header: hir::FnHeader { constness, .. }, .. }, ..), | ||
.. | ||
}) | ||
| hir::Node::TraitItem(hir::TraitItem { | ||
}) => { | ||
is_fn = true; | ||
*constness | ||
} | ||
hir::Node::TraitItem(hir::TraitItem { | ||
kind: | ||
hir::TraitItemKind::Fn( | ||
hir::FnSig { header: hir::FnHeader { constness, .. }, .. }, | ||
|
@@ -214,8 +220,36 @@ fn param_env(tcx: TyCtxt<'_>, def_id: DefId) -> ty::ParamEnv<'_> { | |
_ => hir::CRATE_HIR_ID, | ||
}; | ||
|
||
let cause = traits::ObligationCause::misc(tcx.def_span(def_id), body_id); | ||
traits::normalize_param_env_or_error(tcx, unnormalized_env, cause) | ||
let span = tcx.def_span(def_id); | ||
|
||
if is_fn { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm skeptical that all this extra code is worthwhile... since all it's here for is to deduplicate a diagnostic that's still going to end up being an error. Especially because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing it out. If that isn't worthwhile, could you take a look at the first commit, which is just a little optimization of param_env, and I hope it's acceptable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure this is an optimization in reality? Maybe you should put it up as a separate commit (separate from the span modifications) so we can do a perf run on it. I am still not confident that the added complexity is worth it. |
||
let mut spans = spans.into_iter(); | ||
let obligations: Vec<_> = predicates | ||
.into_iter() | ||
.map(|predicate| { | ||
let cause = traits::ObligationCause::misc(spans.next().unwrap_or(span), body_id); | ||
Obligation::new(cause, ty::ParamEnv::empty(), predicate) | ||
}) | ||
.collect(); | ||
|
||
let (predicates, causes): (Vec<_>, Vec<_>) = elaborate_obligations(tcx, obligations) | ||
.map(|obligation| (obligation.predicate, obligation.cause)) | ||
.unzip(); | ||
|
||
traits::normalize_param_env_with_causes( | ||
tcx, | ||
unnormalized_env, | ||
span, | ||
causes.into_iter(), | ||
predicates, | ||
) | ||
} else { | ||
traits::normalize_param_env_or_error( | ||
tcx, | ||
unnormalized_env, | ||
traits::ObligationCause::misc(span, body_id), | ||
) | ||
} | ||
} | ||
|
||
/// Elaborate the environment. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
// compile-flags: -Zdeduplicate-diagnostics=yes | ||
|
||
fn f<T>(_: impl A<X = T>) {} | ||
//~^ ERROR the trait bound `T: B` is not satisfied [E0277] | ||
trait A: C<Z = <<Self as A>::X as B>::Y> { | ||
type X: B; | ||
} | ||
|
||
trait B { | ||
type Y; | ||
} | ||
|
||
trait C { | ||
type Z; | ||
} | ||
|
||
fn main() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
error[E0277]: the trait bound `T: B` is not satisfied | ||
--> $DIR/issue-102185.rs:3:17 | ||
| | ||
LL | fn f<T>(_: impl A<X = T>) {} | ||
| ^^^^^^^^ the trait `B` is not implemented for `T` | ||
| | ||
help: consider restricting type parameter `T` | ||
| | ||
LL | fn f<T: B>(_: impl A<X = T>) {} | ||
| +++ | ||
|
||
error: aborting due to previous error | ||
|
||
For more information about this error, try `rustc --explain E0277`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the use site, should we just take
&mut
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, as mentioned above, fully_normalize requires taking ownership of the predicats. I could not think of a good way to deal with this for a while.