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]

Could you remove this? I think it's okay to track the stderr for this case.

View changes since the review

[YUZHEthefool]

Ok, I will remove it, and fix the current ci problem and submit it together.

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

[YUZHEthefool]

@rustbot review

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

[BoxyUwU]

This overlaps with some other ongoing work and generally needs me to sit down for a bit and think through what the best way of checking that we don't have stuff in the type system which doesn't implement ConstParamTy_ is. I will get round to reviewing this but just want to set expectations that it might take a bit.

I think the correct solution is something along the lines of "const items in the type system must have a type which implements ConstParamTy for the use to be well formed"

[BoxyUwU]

some comments on your solution. generally though very cool that you got this far and i think it's actually very close to being the right thing.

I might recommend you to read the rustc-dev-guide section on WellFormedness: https://rustc-dev-guide.rust-lang.org/analysis/well-formed.html. It covers a lot of topics which are very directly related to what you're working on here (though I don't expect you to have been able to know that)

I haven't thought too deeply about this but what I would suggest for moving forwards with this PR is to try and change the wellformedness requirements for ty::Consts which are paths to const items to include a Ty: ConstParamTy goal where Ty is the type of the const item

I'm not entirely sure if that would actually handle the case of where T: Trait<ASSOC = ...>, you'd have to give it a try and see if it works or not 😅 I think it should work though..

View changes since this review

[BoxyUwU]

I think that this probably isn't the right way of going about this. Though I understand how you arrived at this solution and it makes sense that it would work :3

There are two main things about this that feel off to me:

1. we're re-doing "figure out what associated item the path resolves to"
2. we're just calling try_evaluate_const instead of registering a typeof(item): ConstParamTy_ obligation

the first point feels off to me partially because we're redoing work, but mostly because it means that there's somewhere else in the compiler that's resolving the associated item and isn't checking this but probably could be instead.

the second point feels off to me because not all things that aren't valid for use in const generics are invalid because they crash the compiler. some are just invalid because the user hasn't said they want their type to be useable. those types should also not be allowed.

[BoxyUwU]

this feels off to me too because it means we're only checking this when the associated const binding is equal to an anon const, rather than somethign else. for example T: Trait<ASSOC = direct_const_arg!(CONST_ITEM)> should be illegal if the type of ASSOC isn't valid in const generics, but there's no anon const there.

[BoxyUwU]

Oh also if you get stuck please do come say hi on the rust-lang zulip's #project-const-generics channel and there'll be people (myself) included who can chat to you about your PR and probably faster than back and forth via github reviews :)

[BoxyUwU]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[YUZHEthefool]

I think the correct solution is something along the lines of "const items in the type system must have a type which implements ConstParamTy for the use to be well formed"

I will continue to try based on this direction,maybe I need some time to think about it...

[rust-bors]

☔ The latest upstream changes (presumably #161505) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.