Handle generic associated constants in const qualification for structural match

[ecstatic-morse]

#67343 started using const qualification for more precise structural match warnings. The idea is that const qualification should be strictly more precise than the current type visitor (search_for_structural_match_violation). However, const qualification is more conservative in the face of generic associated constants than the type visitor, which typically runs after substitution. This was the case in the example in #73431.

We will need to make const qualification handle all cases that search_for_structural_match_violation does before switching away from the latter.

[lcnr]

I am not completely sure how the FlowSensitiveAnalysis works, but at least for the parts I am familiar with it should be possible to change ConstQualifs::custom_eq to Option<&'tcx [Ty<'tcx>]> where all the Ty<'tcx> are projections/opaque types which are required to be structurally match but are still too generic.

i.e. The current custom_eq: false would then be Some(&[]) and custom_eq: true would be None.

Afaik this is only a problem when dealing with generic associated constants using other
associated constants/generic methods, so I am personally fine with drawing an arbitrary line where
we stop using value based reasoning here 🤔

trait Foo {
    const A: Self;
}

impl Foo for Option<f32> {
    const A: Self = None;
}

impl Foo for usize {
    const A: Self = 7;
}

struct A<T>(T);

impl<T: Foo> A<T> {
    const VALUE: T = T::A;
}

fn main() {
    match Some(1.0f32) {
        // I do not want to use value based reasoning for `T::A`,
        // so the following should error/warn because `Option<f32>` is not structurally match.
        A::<Option<f32>>::VALUE => (),
        _ => (),
    }

    match 42 {
        // This should be accepted.
        A::<usize>::VALUE => (),
        _ => (),
    }
}

[bjorn3]

This seems to happen for ui/consts/const_in_pattern/issue-73431.rs too.

[lcnr]

@bjorn3 what exactly do you mean here? https://github.com/rust-lang/rust/blob/master/src/test/ui/consts/const_in_pattern/issue-73431.rs was the issue which first hit this problem.

#73431 is similar to this part of my code snippet:

    match 42 {
        // This should be accepted.
        A::<usize>::VALUE => (),
        _ => (),
    }

[bjorn3]

DIdn't read the top-level comment properly. I just saw the warning while running the test suite with RUSTC_LOG=warn.

[lcnr]

Assigning P-medium as described here #73431 (comment)

[ecstatic-morse]

@lcnr Sorry I missed your comment.

it should be possible to change ConstQualifs::custom_eq to Option<&'tcx [Ty<'tcx>]> where all the Ty<'tcx> are projections/opaque types which are required to be structurally match but are still too generic.

This is indeed possible, although not with the current dataflow framework, which only works on BitSets of some index type. We would need to extend it to work on arbitrary lattices (in this case the set of all projections/opaque types ordered by inclusion). I've had this on the backburner for some time, but I might get around to it soon.

If we were to go this route, const qualification will become slower and more memory hungry, so like you I would prefer to avoid it. Given that, it seems we would need to break code like this to convert fully to a const-qualification-based approach. Presumably, having two layers of associated constants in a pattern is presumably something very few users care about. However, I don't think this meets the requirements for a breaking change, since the code isn't actually unsound.

[drewkett]

Not sure if I should make a separate issue for this, but I found some surprising behavior when running into this when using arrayvec::ArrayString which has a const generic length and manually implements PartialEq, Eq.

The following compiles without complaint

use arrayvec::ArrayString;

#[derive(PartialEq,Eq)]
enum Foo {
    A,
}

#[derive(PartialEq, Eq)]
enum Bar {
    Foo(Foo),
    #[allow(dead_code)]
    Other(ArrayString<64>)
}

impl Bar {
    const A: Bar = Bar::Foo(Foo::A);
}

fn main() {
    match Bar::A {
        Bar::A => {},
        _ => {}
    }
}

But if I change Bar to Self in the const definition like below, it warns that the constant initializer is nontrivial

use arrayvec::ArrayString;

#[derive(PartialEq,Eq)]
enum Foo {
    A,
}

#[derive(PartialEq, Eq)]
enum Bar {
    Foo(Foo),
    #[allow(dead_code)]
    Other(ArrayString<64>)
}

impl Bar {
    const A: Bar = Self::Foo(Foo::A);
}

fn main() {
    match Bar::A {
        Bar::A => {},
        _ => {}
    }
}

with the following warning on both stable 1.58.1 and nightly 1.60.0-nightly (2022-02-14 c5c610aad0a012a9228e)

warning: to use a constant of type arrayvec::ArrayString in a pattern, the constant's initializer must be trivial or arrayvec::ArrayString must be annotated with #[derive(PartialEq, Eq)]

playground

[RalfJung]

That seems to be an accident in the analysis, which does not know how to handle Self and then falls back to a less precise analysis that takes into account both variants of Bar. Does const qualification have a special case for enum variant constructors that does not trigger on Self::Foo?

Bar::A isn't even generic, but it is an associated const, and AFAIK we usually treat them all the same even if we could make a special case for monomorphic associated consts.

[hrkz]

I also came across this warning while trying to match a const initialized with a const fn.
I don't know if this is an expected behavior or not, since the structs Foo and Bar are directly annotated with #[derive(PartialEq, Eq)].

Here is the playground link : https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=b8a524fa8d710581190495a481ecad60

Note that the warning is gone for Foo::D, which is not defined from the const fn.

[cmyr]

Ran into a compiler warning that referenced this issue in rustc 1.62.0. In case it's useful to anyone, here is a reproduction.

[RalfJung]

I think I'm a bit confused about the point of this custom_eq MIR qualif analysis.

Thinking about the two major possible design options for consts-in-patterns that I describe in this WIP t-lang design meeting document, I don't think we really need the MIR qualif analysis in either case?

• If we say that we always desugar the constant to a pattern, then we can simply check the actual valtree we see. If we hit any non-structural-eq ADT, we throw an error, otherwise we are fine. The MIR qualif rejects some extra cases but there's not really a good reason to reject them.
• If we say that we always treat the constant with == in the pattern, and only use structural-eq to determine whether we want to use the const value for exhaustiveness checking, then we definitely need the const value if we want to do exhaustiveness checking, so again we can just look at the valtree. Or maybe we do a completely type-based analysis first to avoid the exhaustiveness check depending on valtree construction details.

Either way I don't think I see the benefit of doing a static analysis of the MIR code before running it, with the goal of predicting whether the final value will have custom equality. We need the final value anyway in any case where we want to exploit that it doesn't have custom equality. So we can just do the check on that value.

I checked our test suite for code that gets rejected by this lint, and I don't think we have to reject any of that code:

• custom-eq-branch-warn.rs: the final value is Foo::Bar, its pattern semantics agrees with ==, so we can totally accept this.
• warn_corner_cases.rs: the final values are None for all of these, where again pattern semantics agrees with ==, so we can totally accept this.

Cc @rust-lang/wg-const-eval

[lcnr]

I was somewhat confused by your comment for a while 😅 to summarize my understanding:

1. if we simply remove the const qualification while keeping the rest of the code the same, I get a bunch of new failures for stuff like None::<NotStructurallyEq>.

2. if we instead only look for structural match violations of the enum variants which are actually used in a pattern, we would accept more code: None::<NotStructurallyEq>, but computed in a way which is not handled by the const qualification. This is what happens in the two testcases mentioned.

I think you're saying that no matter how we go forward wrt to structural match, we want to accept the new code allowed by going with option 2. here.

My only worry is that currently constants in patterns participate in exhaustiveness checking if types simply derive PartialEq which I consider to be ugly backcompat wise. Allowing more constants in patterns without warning, even if the reason why they are not allowed isn't really sensible, therefore doesn't feel great.

I don't think that the const qualification adds a lot of technical debt to the compiler, so I would personally wait until we decided when constants in patterns should participate in exhaustiveness checking before being more liberal here.

[RalfJung]

Yeah I'm not suggesting to remove it right now. I'm just saying I think the final design we go for shouldn't involve a MIR qualif.

[RalfJung]

The RFC at rust-lang/rfcs#3535, if accepted, would lead to us removing the structural match qualification, also making this issue moot.