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.

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

Don't probe InferConst in fold_const if self.infcx is None, deeply_normalize tys in check_tys_might_be_eq #124526

Closed
wants to merge 2 commits into from
Closed
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
33 changes: 30 additions & 3 deletions compiler/rustc_infer/src/infer/canonical/canonicalizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,17 +456,32 @@ impl<'cx, 'tcx> TypeFolder<TyCtxt<'tcx>> for Canonicalizer<'cx, 'tcx> {
fn fold_const(&mut self, mut ct: ty::Const<'tcx>) -> ty::Const<'tcx> {
match ct.kind() {
ty::ConstKind::Infer(InferConst::Var(mut vid)) => {
let Some(infcx) = self.infcx else {
// FIXME(with_negative_coherence): the infcx has constraints from equating
// the impl headers in `impl_intersection_has_negative_obligation`.
// We should use these constraints as assumptions.
assert!(self.tcx.features().with_negative_coherence);
debug!("canonicalizing `ConstKind::Infer` without an `infcx`?");
assert!(!self.canonicalize_mode.preserve_universes());
return self.canonicalize_const_var(
CanonicalVarInfo {
kind: CanonicalVarKind::Const(ty::UniverseIndex::ROOT, ct.ty()),
},
ct,
);
};

// We need to canonicalize the *root* of our const var.
// This is so that our canonical response correctly reflects
// any equated inference vars correctly!
let root_vid = self.infcx.unwrap().root_const_var(vid);
let root_vid = infcx.root_const_var(vid);
if root_vid != vid {
ct = ty::Const::new_var(self.tcx, root_vid, ct.ty());
vid = root_vid;
}

debug!("canonical: const var found with vid {:?}", vid);
match self.infcx.unwrap().probe_const_var(vid) {
match infcx.probe_const_var(vid) {
Ok(c) => {
debug!("(resolved to {:?})", c);
return self.fold_const(c);
Expand All @@ -487,7 +502,19 @@ impl<'cx, 'tcx> TypeFolder<TyCtxt<'tcx>> for Canonicalizer<'cx, 'tcx> {
}
}
ty::ConstKind::Infer(InferConst::EffectVar(vid)) => {
match self.infcx.unwrap().probe_effect_var(vid) {
let Some(infcx) = self.infcx else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've avoided to review this PR for a while as this feels very suspicious to me but I wasn't able to explain why without a significant time investment. sorry 😅

The ICE occurs in equate_impl_headers which is used to compute the constraints we can assume. We cannot eagerly replace infer vars with placeholders at this point

equate_impl_headers(infcx, param_env, &impl1_header, &impl2_header)

We should use an infcx when encountering inference varialbes to make sure these infer vars do not resolve to any other types. I would personally prefer to only do the deeply_normalize change in this PR and fix the negative_coherence issue by cleaning up our const handling in general.

While your approach does work and may at worst slightly worsen type inference here, this change does cause the code to be "conceptionally broken"1, so I would prefer to not land it, esp given that it's an ICE which only occurs with a nightly feature enabled.

Footnotes

  1. this is mostly just vibe based 🤔 using it here as the way to rationalize this change is "yes, this may cause unexpected and wrong results in rare cases, but it's not unsound, so it's fine". It's somewhat difficult to explain exactly what I mean here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For context, I tried to add a subdiagnostic “if there's a conflicting implementation and if using negative coherence solves it, suggests adding #![feature(with_negative_coherence)]” but it ICEd some tests, and I may have been too hasty in my attempt to fix it. I’ll try to come up with a nicer approach when I have time.

// FIXME(with_negative_coherence): the infcx has constraints from equating
// the impl headers in `impl_intersection_has_negative_obligation`.
// We should use these constraints as assumptions.
assert!(self.tcx.features().with_negative_coherence);
debug!("canonicalizing `ConstKind::Infer` without an `infcx`?");
return self.canonicalize_const_var(
CanonicalVarInfo { kind: CanonicalVarKind::Effect },
ct,
);
};

match infcx.probe_effect_var(vid) {
Some(value) => return self.fold_const(value),
None => {
return self.canonicalize_const_var(
Expand Down
6 changes: 5 additions & 1 deletion compiler/rustc_trait_selection/src/traits/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,12 @@ pub fn check_tys_might_be_eq<'tcx>(
let (infcx, key, _) = tcx.infer_ctxt().build_with_canonical(DUMMY_SP, &canonical);
let (param_env, (ty_a, ty_b)) = key.into_parts();
let ocx = ObligationCtxt::new(&infcx);
let cause = ObligationCause::dummy();

let ty_a = ocx.deeply_normalize(&cause, param_env, ty_a).unwrap_or(ty_a);
let ty_b = ocx.deeply_normalize(&cause, param_env, ty_b).unwrap_or(ty_b);
let result = ocx.eq(&cause, param_env, ty_a, ty_b);

let result = ocx.eq(&ObligationCause::dummy(), param_env, ty_a, ty_b);
// use `select_where_possible` instead of `select_all_or_error` so that
// we don't get errors from obligations being ambiguous.
let errors = ocx.select_where_possible();
Expand Down
2 changes: 1 addition & 1 deletion src/tools/compiletest/src/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ impl<'test> TestCx<'test> {
// if a test does not crash, consider it an error
if proc_res.status.success() || matches!(proc_res.status.code(), Some(1 | 0)) {
self.fatal(&format!(
"test no longer crashes/triggers ICE! Please give it a mearningful name, \
"test no longer crashes/triggers ICE! Please give it a meaningful name, \
add a doc-comment to the start of the test explaining why it exists and \
move it to tests/ui or wherever you see fit."
));
Expand Down
20 changes: 0 additions & 20 deletions tests/crashes/114456-2.rs

This file was deleted.

17 changes: 0 additions & 17 deletions tests/crashes/114456.rs

This file was deleted.

6 changes: 0 additions & 6 deletions tests/crashes/119381.rs

This file was deleted.

11 changes: 11 additions & 0 deletions tests/ui/inference/infer-const-in-param-env.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//! Issue #119381: encountering `ty::ConstKind::Infer(InferConst::Var(_))` inside a `ParamEnv`

#![feature(with_negative_coherence)]
trait Trait {}
impl<const N: u8> Trait for [(); N] {}
impl<const N: i8> Trait for [(); N] {}
//~^ conflicting implementations of trait `Trait` for type `[(); _]`
//~| mismatched types
//~^^^^ mismatched types

fn main() {}
24 changes: 24 additions & 0 deletions tests/ui/inference/infer-const-in-param-env.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
error[E0119]: conflicting implementations of trait `Trait` for type `[(); _]`
--> $DIR/infer-const-in-param-env.rs:6:1
|
LL | impl<const N: u8> Trait for [(); N] {}
| ----------------------------------- first implementation here
LL | impl<const N: i8> Trait for [(); N] {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `[(); _]`

error[E0308]: mismatched types
--> $DIR/infer-const-in-param-env.rs:5:34
|
LL | impl<const N: u8> Trait for [(); N] {}
| ^ expected `usize`, found `u8`

error[E0308]: mismatched types
--> $DIR/infer-const-in-param-env.rs:6:34
|
LL | impl<const N: i8> Trait for [(); N] {}
| ^ expected `usize`, found `i8`

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0119, E0308.
For more information about an error, try `rustc --explain E0119`.
20 changes: 20 additions & 0 deletions tests/ui/type-alias/normalize-const-tys-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
//! Issue #114456: `deeply_normalize` tys in `check_tys_might_be_eq`
//@ check-pass
#![feature(adt_const_params)]
#![allow(incomplete_features)]

enum Type {}
trait Trait { type Matrix; }
impl Trait for Type { type Matrix = [usize; 1]; }

struct Walk<const REMAINING: <Type as Trait>::Matrix> {}

impl Walk<{ [0; 1] }> {
pub const fn new() -> Self {
Self {}
}
}

fn main() {
let _ = Walk::new();
}
17 changes: 17 additions & 0 deletions tests/ui/type-alias/normalize-const-tys.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//! Issue #114456: `deeply_normalize` tys in `check_tys_might_be_eq`
//@ check-pass
#![feature(adt_const_params, lazy_type_alias)]
#![allow(incomplete_features)]

type Matrix = [usize; 1];
struct Walk<const REMAINING: Matrix> {}

impl Walk<{ [0; 1] }> {
pub const fn new() -> Self {
Self {}
}
}

fn main() {
let _ = Walk::new();
}
Loading