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

ConstParamTy should be split into two traits #112221

Closed
BoxyUwU opened this issue Jun 2, 2023 · 4 comments
Closed

ConstParamTy should be split into two traits #112221

BoxyUwU opened this issue Jun 2, 2023 · 4 comments
Labels
A-const-generics Area: const generics (parameters and arguments) A-patterns Relating to patterns and pattern matching F-adt_const_params `#![feature(adt_const_params)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@BoxyUwU
Copy link
Member

BoxyUwU commented Jun 2, 2023

ConstParamTy is currently used to require people to opt-in to having a type used as a const parameter under feature(adt_const_params) as not all types are legal to have as a const parameter type. One of the restrictions impls of this trait have is that the type has Eq, PartialEq recursively derived. This is also something that pattern matching is interested so we should find some way of sharing the impl between ConstParamTy and the trait used for allowing constants to be used as patterns.

The current ConstParamTy trait should be split out into trait ConstParamTy: StructuralMatch and trait StructuralMatch: PartialEq + StructuralPartialEq with StructuralMatch also having a builtin check that all its fields are StructuralMatch.

It's possible that the requirements for const parameter types may end up being stricter than the requirements to use a constant as a pattern so we should have two separate traits rather than renaming ConstParamTy to something that makes sense in the context of pattern matching.

cc @oli-obk

@BoxyUwU BoxyUwU added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-generics Area: const generics (parameters and arguments) A-patterns Relating to patterns and pattern matching F-adt_const_params `#![feature(adt_const_params)]` T-types Relevant to the types team, which will review and decide on the PR/issue. labels Jun 2, 2023
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jun 3, 2023

One thing to note about this is that in order to use a type as a const param ty you would have to drive 4 traits instead of 3- derive(Eq, PartialEq, StructuralMatch, ConstParamTy)

@RalfJung
Copy link
Member

One of the restrictions impls of this trait have is that the type has Eq, PartialEq recursively derived.

Pattern matching doesn't really care about this property any more since #67343. We care that the value being matched has the property that PartialEq behaves like the recursively derived version. However when there is an enum where some variants have types that cannot be matched on, then it would be a breaking change to reject matching with constants on the other variants, like in the following example:

struct NotStructuralEq;

impl PartialEq for NotStructuralEq {
    fn eq(&self, other: &Self) -> bool { true }
}
impl Eq for NotStructuralEq {}

#[derive(PartialEq, Eq)]
enum MyEnum {
    V1(i32),
    V2(NotStructuralEq),
}

const C: MyEnum = MyEnum::V1(0);

fn test(x: MyEnum) {
    match x {
        C => (),
        _ => (),
    }
}

So I don't really see who would be served by adding yet another trait? I think we should reduce the number of traits, e.g. by getting rid of StructuralEq (keeping only StructuralPartialEq).

@RalfJung
Copy link
Member

I could see one reason to introduce a new trait: to replace the search_for_structural_match_violation test by a trait query instead.

But given that we currently use a value-aware MIR qualif to determine whether a constant can be structurally matched, I am not sure if that would have any benefits.

@RalfJung RalfJung changed the title const param ty should be split into two traits ConstParamTy should be split into two traits Sep 24, 2023
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jul 2, 2024

ConstParamTy only really cares about the type being shallowly structural eq since we recursively require all fields to be ConstParamTy which effectively requires it to be deep. Going to close this since I see no reason to have more traits here.

@BoxyUwU BoxyUwU closed this as completed Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) A-patterns Relating to patterns and pattern matching F-adt_const_params `#![feature(adt_const_params)]` T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants