Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 13 additions & 2 deletions compiler/rustc_borrowck/src/type_check/canonical.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,8 +343,15 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
return;
}

// FIXME: Ideally MIR types are normalized, but this is not always true.
let mir_ty = self.normalize(mir_ty, Locations::All(span));
// This is a hack. `body.local_decls` are not necessarily normalized in the old
// solver due to not deeply normalizing in writeback. So we must re-normalize here.
//
// I am not sure of a test case where this actually matters. There is a similar
// hack in `equate_inputs_and_outputs` which does have associated test cases.
let mir_ty = match self.infcx.tcx.next_trait_solver_globally() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mir_ty = match self.infcx.tcx.next_trait_solver_globally() {
let mir_ty = match self.infcx.next_trait_solver() {

true => mir_ty,
false => self.normalize(mir_ty, Locations::All(span)),
};

let cause = ObligationCause::dummy_with_span(span);
let param_env = self.infcx.param_env;
Expand All @@ -353,6 +360,10 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
ConstraintCategory::Boring,
type_op::custom::CustomTypeOp::new(
|ocx| {
// The `AscribeUserType` query would normally emit a wf
// obligation for the unnormalized user_ty here. This is
// where the "incorrectly skips the WF checks we normally do"
// happens
let user_ty = ocx.normalize(&cause, param_env, user_ty);
ocx.eq(&cause, param_env, user_ty, mir_ty)?;
Ok(())
Expand Down
73 changes: 50 additions & 23 deletions compiler/rustc_borrowck/src/type_check/input_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,31 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
);
}

// FIXME(BoxyUwU): This should probably be part of a larger borrowck dev-guide chapter
//
/// Enforce that the types of the locals corresponding to the inputs and output of
/// the body are equal to those of the (normalized) signature.
///
/// This is necessary for two reasons:
/// - Locals in the MIR all start out with `'erased` regions and then are replaced
/// with unconstrained nll vars. If we have a function returning `&'a u32` then
/// the local `_0: &'?10 u32` needs to have its region var equated with the nll
/// var representing `'a`. i.e. borrow check must uphold that `'?10 = 'a`.
/// - When computing the normalized signature we may introduce new unconstrained nll
/// vars due to higher ranked where clauses ([#136547]). We then wind up with implied
/// bounds involving these vars.
///
/// For this reason it is important that we equate with the *normalized* signature
/// which was produced when computing implied bounds. If we do not do so then we will
/// wind up with implied bounds on nll vars which cannot actually be used as the nll
/// var never gets related to anything.
///
/// For 'closure-like' bodies this function effectively relates the *inferred* signature
/// of the closure against the locals corresponding to the closure's inputs/output. It *does
/// not* relate the user provided types for the signature to the locals, this is handled
/// separately by: [`TypeChecker::check_signature_annotation`].
///
/// [#136547]: <https://www.github.com/rust-lang/rust/issues/136547>
#[instrument(skip(self), level = "debug")]
pub(super) fn equate_inputs_and_outputs(&mut self, normalized_inputs_and_output: &[Ty<'tcx>]) {
let (&normalized_output_ty, normalized_input_tys) =
Expand Down Expand Up @@ -173,38 +198,40 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
);
}

// Return types are a bit more complex. They may contain opaque `impl Trait` types.
Copy link
Member Author

Choose a reason for hiding this comment

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

Am I missing something here? The actual logic here is no more complex than any of the previous relations we do.

let mir_output_ty = self.body.local_decls[RETURN_PLACE].ty;
// Equate expected output ty with the type of the RETURN_PLACE in MIR
let mir_output_ty = self.body.return_ty();
let output_span = self.body.local_decls[RETURN_PLACE].source_info.span;
self.equate_normalized_input_or_output(normalized_output_ty, mir_output_ty, output_span);
}

#[instrument(skip(self), level = "debug")]
fn equate_normalized_input_or_output(&mut self, a: Ty<'tcx>, b: Ty<'tcx>, span: Span) {
if let Err(_) =
self.eq_types(a, b, Locations::All(span), ConstraintCategory::BoringNoLocation)
{
Comment on lines -184 to -186
Copy link
Member Author

Choose a reason for hiding this comment

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

i assume removing this "fast path" is perf sensitive so will do a perf run 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

like 0.2% in a few secondary benchmarks #151374 (comment)

I don't think a simple cleanup of yeeting this is worth it given those results?

Copy link
Contributor

Choose a reason for hiding this comment

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

can you instead do

if infcx.next_Trait_solver() {
    return eq_types();
}

and then keep the old body with updated comments, rn the formatting is kinda ass here

// FIXME(jackh726): This is a hack. It's somewhat like
// `rustc_traits::normalize_after_erasing_regions`. Ideally, we'd
// like to normalize *before* inserting into `local_decls`, but
// doing so ends up causing some other trouble.
let b = self.normalize(b, Locations::All(span));

// Note: if we have to introduce new placeholders during normalization above, then we
// won't have added those universes to the universe info, which we would want in
// `relate_tys`.
Comment on lines -193 to -195
Copy link
Member Author

Choose a reason for hiding this comment

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

i am missing something here. This comment would seem to imply we do things differently to avoid calling relate_types (note taht there is no such thing as relate_tys) but eq_types is a trivial wrapper around relate_tys.

I guess the point is that there will be universes made during the normalization call which this eq_types is unaware of the origin for? I don't quite get why this is worth calling out, but maybe it's worth keeping this :>

Copy link
Member Author

Choose a reason for hiding this comment

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

at minimum I think I'd want to move this comment to the actual normalize call as that's where the source of the "weirdness" is? but also we don't have a similar comment in other places we normalize and maybe we should if we do want this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also do not get this comment. I don't think it's relevant 🤔

if let Err(terr) =
self.eq_types(a, b, Locations::All(span), ConstraintCategory::BoringNoLocation)
{
// This is a hack. `body.local_decls` are not necessarily normalized in the old
// solver due to not deeply normalizing in writeback. So we must re-normalize here.
//
// However, in most cases normalizing is unnecessary so we only do so if it may be
// necessary for type equality to hold. This leads to some (very minor) performance
// wins.
let b = match self.infcx.tcx.next_trait_solver_globally() {
true => b,
false => match self.eq_types(
a,
b,
Locations::All(span),
ConstraintCategory::BoringNoLocation,
) {
Ok(_) => return,
Err(_) => self.normalize(b, Locations::All(span)),
},
};

self.eq_types(a, b, Locations::All(span), ConstraintCategory::BoringNoLocation)
.unwrap_or_else(|terr| {
span_mirbug!(
self,
Location::START,
"equate_normalized_input_or_output: `{:?}=={:?}` failed with `{:?}`",
a,
b,
terr
"equate_normalized_input_or_output: `{a:?}=={b:?}` failed with `{terr:?}`",
);
}
}
});
}
}
23 changes: 13 additions & 10 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,19 @@ pub(crate) fn type_check<'tcx>(
known_type_outlives_obligations,
} = free_region_relations::create(infcx, universal_regions, &mut constraints);

let pre_obligations = infcx.take_registered_region_obligations();
assert!(
pre_obligations.is_empty(),
"there should be no incoming region obligations = {pre_obligations:#?}",
);
let pre_assumptions = infcx.take_registered_region_assumptions();
assert!(
pre_assumptions.is_empty(),
"there should be no incoming region assumptions = {pre_assumptions:#?}",
);
{
// Scope these variables so it's clear they're not used later
Copy link
Member Author

Choose a reason for hiding this comment

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

this seemed kinda scary to me when i was reading it :> but i think this would have made it not seem so scary

let pre_obligations = infcx.take_registered_region_obligations();
assert!(
Copy link
Contributor

Choose a reason for hiding this comment

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

why have these as variables at all, we could just do

assert_eq!(
    infcx.take_registered_region_obligations(),
    vec![],
    "there should be no incoming region obligations"
);

🤔 this assert is hard to read/noisy

Copy link
Member Author

Choose a reason for hiding this comment

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

oo smart

pre_obligations.is_empty(),
"there should be no incoming region obligations = {pre_obligations:#?}",
);
let pre_assumptions = infcx.take_registered_region_assumptions();
assert!(
pre_assumptions.is_empty(),
"there should be no incoming region assumptions = {pre_assumptions:#?}",
);
}

debug!(?normalized_inputs_and_output);

Expand Down
5 changes: 2 additions & 3 deletions compiler/rustc_middle/src/ty/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -609,9 +609,8 @@ impl<'tcx> TyCtxt<'tcx> {
/// have the same `DefKind`.
///
/// Note that closures have a `DefId`, but the closure *expression* also has a
/// `HirId` that is located within the context where the closure appears (and, sadly,
/// a corresponding `NodeId`, since those are not yet phased out). The parent of
/// the closure's `DefId` will also be the context where it appears.
/// `HirId` that is located within the context where the closure appears. The
/// parent of the closure's `DefId` will also be the context where it appears.
pub fn is_closure_like(self, def_id: DefId) -> bool {
matches!(self.def_kind(def_id), DefKind::Closure)
}
Expand Down
3 changes: 3 additions & 0 deletions tests/ui/associated-types/normalization-generality-2.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
//@ build-pass
//@ revisions: current next
Copy link
Member Author

Choose a reason for hiding this comment

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

all of these are tests that fail in the old solver if you remove the normalization in equate_normalized_input_or_output

//@ ignore-compare-mode-next-solver (explicit revisions)
//@[next] compile-flags: -Znext-solver

// Ensures that we don't regress on "implementation is not general enough" when
// normalizating under binders. Unlike `normalization-generality.rs`, this also produces
Expand Down
3 changes: 3 additions & 0 deletions tests/ui/associated-types/normalization-generality.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
//@ build-pass
//@ revisions: current next
//@ ignore-compare-mode-next-solver (explicit revisions)
//@[next] compile-flags: -Znext-solver

// Ensures that we don't regress on "implementation is not general enough" when
// normalizating under binders.
Expand Down
3 changes: 3 additions & 0 deletions tests/ui/higher-ranked/trait-bounds/issue-88446.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
//@ check-pass
//@ revisions: current next
//@ ignore-compare-mode-next-solver (explicit revisions)
//@[next] compile-flags: -Znext-solver

trait Yokeable<'a> {
type Output: 'a;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
//@ check-pass
//@ revisions: current next
//@ ignore-compare-mode-next-solver (explicit revisions)
//@[next] compile-flags: -Znext-solver

#![allow(unused)]

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
//@check-pass
//@ revisions: current next
//@ ignore-compare-mode-next-solver (explicit revisions)
//@[next] compile-flags: -Znext-solver

trait Yokeable<'a>: 'static {
type Output: 'a;
Expand Down
3 changes: 3 additions & 0 deletions tests/ui/nll/issue-112604-closure-output-normalize.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
//@check-pass
//@ revisions: current next
//@ ignore-compare-mode-next-solver (explicit revisions)
//@[next] compile-flags: -Znext-solver

use higher_kinded_types::*;
mod higher_kinded_types {
Expand Down
Loading