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]

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!

[YUZHEthefool]

all right!
@rustbot review

[khyperia]

sorry for taking so long to take a look at this when I said I'd take a look soon a while ago 😅

a few nits on this specific approach, but tbh I'm generally happy with the technical impl as-is too, addressing nits not totally required :P

I haven't done as many thinky thinks as boxy on this yet though, so I'm not as sure of the high-level approach - this generally makes sense though (tbh my main uncertainty is whether we should be checking more things, but, at the very least, that's purely additive and can be done later, I'm fairly certain that what is being checked here is good and correct)

View changes since this review

[khyperia]

nit: could you rename this method? it's no longer called for just const params

edit: ah, the phrasing is also a bit wonky... there are technically no const generic parameters in some of your test files, but this method is yapping about const generic parameters. maybe that's fine though, eugh, idk, diagnostics are hard. (on the same line of logic, one could argue requiring ConstParamTy for assoc const projection clauses is also conceptually wonky, since again, not a parameter, but, like, whatever, I guess)

[YUZHEthefool]

hum... To be honest, I'm not sure what this should be renamed to either...🤔
But this really isn't quite appropriate right now.

[YUZHEthefool]

I did not change the original diagnosis;simply renamed the current method to report_const_param_ty_error...
I'm realy not sure how should this new diagnosis written

[YUZHEthefool]

I seem to have thought of a good way to do related diagnosis. It may take a while... (or not put it in this pr, and do it together with gca!'s diagnosis :)

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

[khyperia]

nice, looks good to me! @BoxyUwU do you wanna take another look?