Skip to content
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.

Sign up for GitHub

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

Jump to bottom

correct spans in parem_env to deduplicate errors #102502

Closed
wants to merge 3 commits into from
Closed

correct spans in parem_env to deduplicate errors #102502

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
33a9f16
avoid collecting `Vec` multiple times when normalize `param_env`
SparrowLii Sep 28, 2022
f0ba469
correct spans in `parem_env` to deduplicate errors
SparrowLii Sep 30, 2022
c600a8e
rename variable
SparrowLii Oct 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 27 additions & 23 deletions compiler/rustc_trait_selection/src/traits/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -219,6 +220,7 @@ fn do_normalize_predicates<'tcx>(
cause: ObligationCause<'tcx>,
elaborated_env: ty::ParamEnv<'tcx>,
predicates: Vec<ty::Predicate<'tcx>>,
Copy link
Contributor

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?

Copy link
Member Author

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.

outlives: bool,
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

@SparrowLii SparrowLii Oct 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. In param_env, normalization will be called twice according to whether it is a predicate of type TypeOutlives. This variable is used to distinguish between the two calls.

) -> Result<Vec<ty::Predicate<'tcx>>, ErrorGuaranteed> {
let span = cause.span;
// FIXME. We should really... do something with these region
Expand All @@ -235,7 +237,15 @@ 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| {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make this a separate local variable?
Would this be simpler as a loop mutating predicates?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing! I renamed it to normalized_predicates. I think It maybe not easy to use a mutating, since fully_normalize needs to take ownership of the elements in predicts. Similar methods that act on instances of TypeFoldable have this feature, and try_map_id is used to implement TypeFoldable for Vec.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, could you make a separate variable for the try_map_id call?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I renamed it to origin_predicate. We can pick what we want and sqaush them to one commit I think

if outlives
!= matches!(predicate.kind().skip_binder(), ty::PredicateKind::TypeOutlives(..))
{
Ok(predicate)
} else {
fully_normalize(&infcx, cause.clone(), elaborated_env, predicate)
}
}) {
Ok(predicates) => predicates,
Err(errors) => {
let reported = infcx.report_fulfillment_errors(&errors, None, false);
Expand All @@ -262,7 +272,15 @@ fn do_normalize_predicates<'tcx>(
);
}

match infcx.fully_resolve(predicates) {
match predicates.try_map_id(|predicate| {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -306,7 +324,7 @@ 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();
Expand Down Expand Up @@ -337,53 +355,39 @@ 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(),
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,
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),
Expand Down