Skip to content

Commit

Permalink
Auto merge of #125671 - BoxyUwU:remove_const_ty_eq, r=compiler-errors
Browse files Browse the repository at this point in the history
Do not equate `Const`'s ty in `super_combine_const`

Fixes #114456

In #125451 we started relating the `Const`'s tys outside of a probe so it was no longer simply an assertion to catch bugs.

This was done so that when we _do_ provide a wrongly typed const argument to an item if we wind up relating it with some other instantiation we'll have a `TypeError` we can bubble up and taint the resulting mir allowing const eval to skip evaluation.

In this PR I instead change `ConstArgHasType` to correctly handle checking the types of const inference variables. Previously if we had something like `impl<const N: u32> Trait for [(); N]`, when using the impl we would instantiate it with infer vars and then check that `?x: u32` is of type `u32` and succeed. Then later we would infer `?x` to some `Const` of type `usize`.

We now stall on `?x` in `ConstArgHasType` until it has a concrete value that we can determine the type of. This allows us to fail using the erroneous implementation of `Trait` which allows us to taint the mir.

Long term we intend to remove the `ty` field on `Const` so we would have no way of accessing the `ty` of a const inference variable anyway and would have to do this. I did not fully update `ConstArgHasType` to avoid using the `ty` field as it's not entirely possible right now- we would need to lookup `ConstArgHasType` candidates in the env.

---

As for _why_ I think we should do this, relating the types of const's is not necessary for soundness of the type system. Originally this check started off as a plain `==` in `super_relate_consts` and gradually has been growing in complexity as we support more complicated types. It was never actually required to ensure that const arguments are correctly typed for their parameters however.

The way we currently check that a const argument has the correct type is a little convoluted and confusing (and will hopefully be less weird as time goes on). Every const argument has an anon const with its return type set to type of the const parameter it is an argument to. When type checking the anon const regular type checking rules require that the expression is the same type as the return type. This effectively ensure that no matter what every const argument _always_ has the correct type.

An extra bit of complexity is that during `hir_ty_lowering` we do not represent everything as a `ConstKind::Unevaluated` corresponding to the anon const. For generic parameters i.e. `[(); N]` we simply represent them as `ConstKind::Param` as we do not want `ConstKind::Unevaluated` with generic substs on stable under min const generics. The anon const still gets type checked resulting in errors about type mismatches.

Eventually we intend to not create anon consts for all const arguments (for example for `ConstKind::Param`) and instead check that the argument type is correct via `ConstArgHasType` obligations (these effectively also act as a check that the anon consts have the correctly set return type).

What this all means is that the the only time we should ever have mismatched types when relating two `Const`s is if we have messed up our logic for ensuring that const arguments are of the correct type. Having this not be an assert is:
- Confusing as it may incorrectly lead people to believe this is an important check that is actually required
- Opens the possibility for bugs or behaviour reliant on this (unnecessary) check existing

---

This PR makes two tests go from pass->ICE (`generic_const_exprs/ice-125520-layout-mismatch-mulwithoverflow.rs` and `tests/crashes/121858.rs`). This is caused by the fact that we evaluate anon consts even if their where clauses do not hold and is a pre-existing issue and only affects `generic_const_exprs`. I am comfortable exposing the brokenness of `generic_const_exprs` more with this PR

This PR makes a test go from ICE->pass (`const-generics/issues/issue-105821.rs`). I have no idea why this PR affects that but I believe that ICE is an unrelated issue to do with the fact that under `generic_const_exprs`/`adt_const_params` we do not handle lifetimes in const parameter types correctly. This PR is likely just masking this bug.

Note: this PR doesn't re-introduce the assertion that the two consts' tys are equal. I'm not really sure how I feel about this but tbh it has caused more ICEs than its found lately so 🤷‍♀️

r? `@oli-obk` `@compiler-errors`
  • Loading branch information
bors committed May 30, 2024
2 parents caa187f + d5bd4e2 commit 32a3ed2
Show file tree
Hide file tree
Showing 30 changed files with 206 additions and 327 deletions.
9 changes: 1 addition & 8 deletions compiler/rustc_infer/src/infer/relate/combine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,10 @@ use super::glb::Glb;
use super::lub::Lub;
use super::type_relating::TypeRelating;
use super::StructurallyRelateAliases;
use crate::infer::{DefineOpaqueTypes, InferCtxt, InferOk, TypeTrace};
use crate::infer::{DefineOpaqueTypes, InferCtxt, TypeTrace};
use crate::traits::{Obligation, PredicateObligations};
use rustc_middle::bug;
use rustc_middle::infer::unify_key::EffectVarValue;
use rustc_middle::traits::ObligationCause;
use rustc_middle::ty::error::{ExpectedFound, TypeError};
use rustc_middle::ty::relate::{RelateResult, TypeRelation};
use rustc_middle::ty::{self, InferConst, Ty, TyCtxt, TypeVisitableExt, Upcast};
Expand Down Expand Up @@ -170,12 +169,6 @@ impl<'tcx> InferCtxt<'tcx> {
let a = self.shallow_resolve_const(a);
let b = self.shallow_resolve_const(b);

// It is always an error if the types of two constants that are related are not equal.
let InferOk { value: (), obligations } = self
.at(&ObligationCause::dummy_with_span(relation.span()), relation.param_env())
.eq(DefineOpaqueTypes::No, a.ty(), b.ty())?;
relation.register_obligations(obligations);

match (a.kind(), b.kind()) {
(
ty::ConstKind::Infer(InferConst::Var(a_vid)),
Expand Down
26 changes: 24 additions & 2 deletions compiler/rustc_trait_selection/src/solve/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,30 @@ impl<'a, 'tcx> EvalCtxt<'a, InferCtxt<'tcx>> {
goal: Goal<'tcx, (ty::Const<'tcx>, Ty<'tcx>)>,
) -> QueryResult<'tcx> {
let (ct, ty) = goal.predicate;
self.eq(goal.param_env, ct.ty(), ty)?;
self.evaluate_added_goals_and_make_canonical_response(Certainty::Yes)

// FIXME(BoxyUwU): Really we should not be calling `ct.ty()` for any variant
// other than `ConstKind::Value`. Unfortunately this would require looking in the
// env for any `ConstArgHasType` assumptions for parameters and placeholders. I
// have not yet gotten around to implementing this though.
//
// We do still stall on infer vars though as otherwise a goal like:
// `ConstArgHasType(?x: usize, usize)` can succeed even though it might later
// get unified with some const that is not of type `usize`.
match ct.kind() {
// FIXME: Ignore effect vars because canonicalization doesn't handle them correctly
// and if we stall on the var then we wind up creating ambiguity errors in a probe
// for this goal which contains an effect var. Which then ends up ICEing.
ty::ConstKind::Infer(ty::InferConst::Var(_)) => {
self.evaluate_added_goals_and_make_canonical_response(Certainty::AMBIGUOUS)
}
ty::ConstKind::Error(_) => {
self.evaluate_added_goals_and_make_canonical_response(Certainty::Yes)
}
_ => {
self.eq(goal.param_env, ct.ty(), ty)?;
self.evaluate_added_goals_and_make_canonical_response(Certainty::Yes)
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2685,6 +2685,22 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
.with_span_label(span, format!("cannot satisfy `{predicate}`"))
}
}

// Given some `ConstArgHasType(?x, usize)`, we should not emit an error such as
// "type annotations needed: cannot satisfy the constant `_` has type `usize`"
// Instead we should emit a normal error suggesting the user to turbofish the
// const parameter that is currently being inferred. Unfortunately we cannot
// nicely emit such an error so we delay an ICE incase nobody else reports it
// for us.
ty::PredicateKind::Clause(ty::ClauseKind::ConstArgHasType(ct, ty)) => {
return self.tcx.sess.dcx().span_delayed_bug(
span,
format!(
"`ambiguous ConstArgHasType({:?}, {:?}) unaccompanied by inference error`",
ct, ty
),
);
}
_ => {
if let Some(e) = self.tainted_by_errors() {
return e;
Expand Down
41 changes: 31 additions & 10 deletions compiler/rustc_trait_selection/src/traits/fulfill.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,16 +429,37 @@ impl<'a, 'tcx> ObligationProcessor for FulfillProcessor<'a, 'tcx> {
// This is because this is not ever a useful obligation to report
// as the cause of an overflow.
ty::PredicateKind::Clause(ty::ClauseKind::ConstArgHasType(ct, ty)) => {
match self.selcx.infcx.at(&obligation.cause, obligation.param_env).eq(
// Only really excercised by generic_const_exprs
DefineOpaqueTypes::Yes,
ct.ty(),
ty,
) {
Ok(inf_ok) => ProcessResult::Changed(mk_pending(inf_ok.into_obligations())),
Err(_) => ProcessResult::Error(FulfillmentErrorCode::Select(
SelectionError::Unimplemented,
)),
// FIXME(BoxyUwU): Really we should not be calling `ct.ty()` for any variant
// other than `ConstKind::Value`. Unfortunately this would require looking in the
// env for any `ConstArgHasType` assumptions for parameters and placeholders. I
// don't really want to implement this in the old solver so I haven't.
//
// We do still stall on infer vars though as otherwise a goal like:
// `ConstArgHasType(?x: usize, usize)` can succeed even though it might later
// get unified with some const that is not of type `usize`.
let ct = self.selcx.infcx.shallow_resolve_const(ct);
match ct.kind() {
ty::ConstKind::Infer(ty::InferConst::Var(vid)) => {
pending_obligation.stalled_on.clear();
pending_obligation.stalled_on.extend([TyOrConstInferVar::Const(vid)]);
ProcessResult::Unchanged
}
ty::ConstKind::Error(_) => return ProcessResult::Changed(vec![]),
_ => {
match self.selcx.infcx.at(&obligation.cause, obligation.param_env).eq(
// Only really excercised by generic_const_exprs
DefineOpaqueTypes::Yes,
ct.ty(),
ty,
) {
Ok(inf_ok) => {
ProcessResult::Changed(mk_pending(inf_ok.into_obligations()))
}
Err(_) => ProcessResult::Error(FulfillmentErrorCode::Select(
SelectionError::Unimplemented,
)),
}
}
}
}

Expand Down
19 changes: 18 additions & 1 deletion compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -995,10 +995,27 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}
ty::PredicateKind::Ambiguous => Ok(EvaluatedToAmbig),
ty::PredicateKind::Clause(ty::ClauseKind::ConstArgHasType(ct, ty)) => {
// FIXME(BoxyUwU): Really we should not be calling `ct.ty()` for any variant
// other than `ConstKind::Value`. Unfortunately this would require looking in the
// env for any `ConstArgHasType` assumptions for parameters and placeholders. I
// don't really want to implement this in the old solver so I haven't.
//
// We do still stall on infer vars though as otherwise a goal like:
// `ConstArgHasType(?x: usize, usize)` can succeed even though it might later
// get unified with some const that is not of type `usize`.
let ct = self.infcx.shallow_resolve_const(ct);
let ct_ty = match ct.kind() {
ty::ConstKind::Infer(ty::InferConst::Var(_)) => {
return Ok(EvaluatedToAmbig);
}
ty::ConstKind::Error(_) => return Ok(EvaluatedToOk),
_ => ct.ty(),
};

match self.infcx.at(&obligation.cause, obligation.param_env).eq(
// Only really excercised by generic_const_exprs
DefineOpaqueTypes::Yes,
ct.ty(),
ct_ty,
ty,
) {
Ok(inf_ok) => self.evaluate_predicates_recursively(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,14 @@
//@ known-bug: #121858
#![feature(generic_const_exprs)]
#![allow(incomplete_features)]

struct Outer<const A: i64, const B: usize>();
impl<const A: usize, const B: usize> Outer<A, B>
//~^ ERROR: `A` is not of type `i64`
//~| ERROR: mismatched types
where
[(); A + (B * 2)]:,
{
fn o() {}
fn o() -> Union {}
}

fn main() {
Outer::<1, 1>::o();
//~^ ERROR: no function or associated item named `o` found
}
12 changes: 0 additions & 12 deletions tests/crashes/122638.rs

This file was deleted.

23 changes: 0 additions & 23 deletions tests/crashes/123141-2.rs

This file was deleted.

27 changes: 14 additions & 13 deletions tests/crashes/123141.rs
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
//@ known-bug: #123141
trait ConstChunksExactTrait<T> {
fn const_chunks_exact<const N: usize>(&self) -> ConstChunksExact<'_, T, { N }>;

trait Trait {
fn next(self) -> Self::Item;
type Item;
}

impl<T> ConstChunksExactTrait<T> for [T] {}
struct Foo<T: ?Sized>(T);

struct ConstChunksExact<'a, T: 'a, const N: usize> {}
impl<T: ?Sized, U> Trait for Foo<U> {
type Item = Foo<T>;
fn next(self) -> Self::Item {
loop {}
}
}

impl<'a, T, const N: usize> Iterator for ConstChunksExact<'a, T, { rem }> {
type Item = &'a [T; N];
fn opaque() -> impl Trait {
Foo::<_>(10_u32)
}

fn main() {
let slice = &[1i32, 2, 3, 4, 5, 6, 7, 7, 9, 1i32];

let mut iter = [[1, 2, 3], [4, 5, 6], [7, 8, 9]].iter();

for a in slice.const_chunks_exact::<3>() {
assert_eq!(a, iter.next().unwrap());
}
opaque().next();
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ impl<const N: u8> Trait for [(); N] {}
//~^ ERROR: mismatched types
impl<const N: i8> Trait for [(); N] {}
//~^ ERROR: mismatched types
//~| ERROR: conflicting implementations of trait `Trait`

fn main() {}
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
error[E0119]: conflicting implementations of trait `Trait` for type `[(); _]`
--> $DIR/generic_const_type_mismatch.rs:9:1
|
LL | impl<const N: u8> Trait for [(); N] {}
| ----------------------------------- first implementation here
LL |
LL | impl<const N: i8> Trait for [(); N] {}
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `[(); _]`

error[E0308]: mismatched types
--> $DIR/generic_const_type_mismatch.rs:7:34
|
Expand All @@ -10,6 +19,7 @@ error[E0308]: mismatched types
LL | impl<const N: i8> Trait for [(); N] {}
| ^ expected `usize`, found `i8`

error: aborting due to 2 previous errors
error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0308`.
Some errors have detailed explanations: E0119, E0308.
For more information about an error, try `rustc --explain E0119`.
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//@ known-bug: #114456
//@ check-pass
#![feature(adt_const_params, lazy_type_alias)]
//~^ WARN: the feature `adt_const_params` is incomplete
//~| WARN: the feature `lazy_type_alias` is incomplete

pub type Matrix = [usize; 1];
const EMPTY_MATRIX: Matrix = [0; 1];
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
warning: the feature `adt_const_params` is incomplete and may not be safe to use and/or cause compiler crashes
--> $DIR/alias_const_param_ty-1.rs:2:12
|
LL | #![feature(adt_const_params, lazy_type_alias)]
| ^^^^^^^^^^^^^^^^
|
= note: see issue #95174 <https://github.com/rust-lang/rust/issues/95174> for more information
= note: `#[warn(incomplete_features)]` on by default

warning: the feature `lazy_type_alias` is incomplete and may not be safe to use and/or cause compiler crashes
--> $DIR/alias_const_param_ty-1.rs:2:30
|
LL | #![feature(adt_const_params, lazy_type_alias)]
| ^^^^^^^^^^^^^^^
|
= note: see issue #112792 <https://github.com/rust-lang/rust/issues/112792> for more information

warning: 2 warnings emitted

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//@ known-bug: #114456
//@ check-pass
#![feature(adt_const_params)]
//~^ WARN: the feature `adt_const_params` is incomplete

const EMPTY_MATRIX: <Type as Trait>::Matrix = [0; 1];

Expand All @@ -12,8 +13,12 @@ impl Walk<EMPTY_MATRIX> {
}

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

fn main() {
let _ = Walk::new();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
warning: the feature `adt_const_params` is incomplete and may not be safe to use and/or cause compiler crashes
--> $DIR/alias_const_param_ty-2.rs:2:12
|
LL | #![feature(adt_const_params)]
| ^^^^^^^^^^^^^^^^
|
= note: see issue #95174 <https://github.com/rust-lang/rust/issues/95174> for more information
= note: `#[warn(incomplete_features)]` on by default

warning: 1 warning emitted

6 changes: 4 additions & 2 deletions tests/ui/const-generics/bad-subst-const-kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,7 @@ impl<const N: u64> Q for [u8; N] {
const ASSOC: usize = 1;
}

pub fn test() -> [u8; <[u8; 13] as Q>::ASSOC] { todo!() }
//~^ ERROR: `[u8; 13]: Q` is not satisfied
pub fn test() -> [u8; <[u8; 13] as Q>::ASSOC] {
//~^ ERROR: the constant `13` is not of type `u64`
todo!()
}
17 changes: 11 additions & 6 deletions tests/ui/const-generics/bad-subst-const-kind.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
error[E0277]: the trait bound `[u8; 13]: Q` is not satisfied
error: the constant `13` is not of type `u64`
--> $DIR/bad-subst-const-kind.rs:13:24
|
LL | pub fn test() -> [u8; <[u8; 13] as Q>::ASSOC] { todo!() }
| ^^^^^^^^ the trait `Q` is not implemented for `[u8; 13]`
LL | pub fn test() -> [u8; <[u8; 13] as Q>::ASSOC] {
| ^^^^^^^^ expected `u64`, found `usize`
|
= help: the trait `Q` is implemented for `[u8; N]`
note: required for `[u8; 13]` to implement `Q`
--> $DIR/bad-subst-const-kind.rs:8:20
|
LL | impl<const N: u64> Q for [u8; N] {
| ------------ ^ ^^^^^^^
| |
| unsatisfied trait bound introduced here

error[E0308]: mismatched types
--> $DIR/bad-subst-const-kind.rs:8:31
Expand All @@ -14,5 +20,4 @@ LL | impl<const N: u64> Q for [u8; N] {

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.
For more information about this error, try `rustc --explain E0308`.
4 changes: 3 additions & 1 deletion tests/ui/const-generics/defaults/doesnt_infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
struct Foo<const N: u32 = 2>;

impl<const N: u32> Foo<N> {
fn foo() -> Self { loop {} }
fn foo() -> Self {
loop {}
}
}

fn main() {
Expand Down
Loading

0 comments on commit 32a3ed2

Please sign in to comment.