Check associated const binding types

[YUZHEthefool]

View all comments

For situations such as fn() that cannot be used as type system constant types, normal E0741 is generated in advance and the error type is returned to avoid continuing to enter constant evaluation and trigger ICE.
Fix: #161100

[rustbot]

HIR ty lowering was modified

cc @fmease

[rustbot]

Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @mu001999 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, types
• compiler, types expanded to 75 candidates
• Random selection from 19 candidates

[mu001999]

I don't think E0741 is suitable to be emitted here. In this case, F is not even a const generic parameter or type const, it is a associated const instead. 🤔

[YUZHEthefool]

So what diagnosis should be generated here?and whether associated constant equality of non-ConstParamTy types should be rejected here

[mu001999]

I think maybe we could emit something like what we did for the following:

const f1: fn() = || {};
const f2: fn() = || {};

const r: bool = f1 == f2;

And for now we will get:

error: pointers cannot be reliably compared during const eval
 --> src/lib.rs:4:17
  |
4 | const r: bool = f1 == f2;
  |                 ^^^^^^^^
  |
  = note: see issue #53020 <https://github.com/rust-lang/rust/issues/53020> for more information

[YUZHEthefool]

Ok, I will make changes based on this direction

[mu001999]

and whether associated constant equality of non-ConstParamTy types should be rejected here

IIUC, ConstParamTy is used for adt_const_params and type const. For GCA, we don't need it:

#![feature(generic_const_args)]
#![feature(min_generic_const_args)]
#![allow(incomplete_features)]

enum Foo {
    A,
    B,
    C,
}

trait Trait {
    const X: Foo;
}

fn foo(x: impl Trait<X = { Foo::A }>) {}

This could compile successfully, and we don't need to mark ConstParamTy.

---

For adt_const_params/type const, we will need ConstParamTy:

#![feature(adt_const_params)]
#![allow(incomplete_features)]

enum Foo {
    A,
    B,
    C,
}

struct Bar<const X: Foo>();

will produce:

error[E0741]: `Foo` must implement `ConstParamTy` to be used as the type of a const generic parameter
  --> main.rs:10:21
   |
10 | struct Bar<const X: Foo>();
   |                     ^^^
   |
help: add `#[derive(ConstParamTy, PartialEq, Eq)]` to the enum
   |
 4 + #[derive(ConstParamTy, PartialEq, Eq)]
 5 | enum Foo {
   |

[YUZHEthefool]

Ok,so the correct boundary is not "whether the type implements ConstParamTy", but "whether this specific constant can form a stable value for type system equality",its right?

[mu001999]

I'm not sure what's the appropriate solution. At least for myself, I don't think checking in lower_assoc_item_constraint is good enough.

r? BoxyUwU, do you have time to have a look?

[rustbot]

BoxyUwU is currently at their maximum review capacity.
They may take a while to respond.

[YUZHEthefool]

Ok, I will stop my current repair work.I have almost implemented the usage context based on:nonSupportedType passed to associated equality, and then generate the pointer comparison diagnosis required by the maintainer for the function pointer.

[rustbot]

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

[YUZHEthefool]

Now,NonValTree has been separated from the generic ambiguity.
Diagnostics only occur when specific values ​​need to be compared for non-type const associated equality

[mu001999]

I hope you don't mind me asking, but did you use an LLM for this? If so, please make sure to disclose its use in accordance with our policy.

[YUZHEthefool]

Nope.I have contributed to the analyzer before and know the ai guidelines of the rust community.
Maybe I may really need LLM to help me translate some content, because I am not very good at some compiler terminology. :(

[YUZHEthefool]

just add //@ normalize-stderr: "\n\n$" -> "\n" for test,and test again.

[BoxyUwU]

FYI havent forgotten about this and am doing some thinky think about this approach

[YUZHEthefool]

FYI havent forgotten about this and am doing some thinky think about this approach

take your time :)
If you have any related questions (on gh/zulip) and tag me, I'll reply as soon as possible.

[bit-aloo]

Changes makes sense. Some suggestions. Thanks

View changes since this review

[bit-aloo]

We can add check for const type here, because we already check for type const in check_type_const

Suggested change

	if ct.kind.is_type_const(self.tcx()) {
	return;
	}

[YUZHEthefool]

emm... seems this method no name is_type_const
I took a look, and it seems it should now be called is_direct_const.

[YUZHEthefool]

All right, I'll go fix it right away.
done

[BoxyUwU]

Hi okay I've thought about this a bit now. The stuff about struggling to get a test case for free and inherent consts is interesting but also makes sense. I think that there probably actually is no need to check ConstParamTy for them 🤔

Every const argument is an argument to some const parameter which at its definition site has its type checked to implement ConstParamTy/be valid for use in const generics. The only exception to this is const arguments used in associated const bindings (T: Trait<Assoc = { ... }>) because the assoc const never gets checked. For free/inherent consts we don't support where FREE = { .. } or sth equivalent to that.

So I think the fix here is really less of a "const items in the type system should have their types checked to implement ConstParamTy" and more of a "associated const binding clauses should have the type of the const item checked". Which means that we care about two cases:

• where T: Trait<ASSOC = ...>
• dyn Trait<ASSOC = ...>

Both of those are handled in the wf.rs file that you've already looked into for your existing set of changes (see clause_obligations and visit_ty's handling of TyKind::Object).

Though I think you already handle the second case with your existing set of changes (nice! :3) I think you also handle the first case by having updated add_wf_preds_for_projection_term too 🤔 I think the main next step here is to actually emit these goals in less cases, and only do it in visit_ty and clause_obligations. There'll probably be some small behavioural differences there but that should be okay and only matter due to other type system bugs which I'm not too concerned about working around :3

@rustbot author

thanks for being patient and waiting for me to get around to this

[YUZHEthefool]

oh it‘s update to gca!

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[YUZHEthefool]

all right!
@rustbot review