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

Check for "constants we match on must be PartialEq" misses lifetime-dependent impls #121007

Open
RalfJung opened this issue Feb 13, 2024 · 3 comments
Labels
A-lifetimes Area: Lifetimes / regions A-patterns Relating to patterns and pattern matching A-traits Area: Trait system C-bug Category: This is a bug. 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

@RalfJung
Copy link
Member

RalfJung commented Feb 13, 2024

The following should trigger a warning that we can only match on PartialEq consts, but it doesn't: (example by @lcnr, then slightly tweaked)

use std::marker::PhantomData;

struct Inv<'a>(PhantomData<*mut &'a ()>);

// This type is only sometimes `PartialEq`.
impl PartialEq for Inv<'static> {
    fn eq(&self, _: &Inv<'static>) -> bool {
        true
    }
}

impl<'a> Inv<'a> {
    // The value `None` makes this have structural equality for any type `Self`.
    const NOT_STATIC: Option<Self> = None;
}

fn foo<'a>(x: Option<Inv<'a>>) {
    match x {
        Inv::<'a>::NOT_STATIC => (),
        Some(_) => panic!()
    }
    
    // Enabling the next line confirms that the type does
    // indeed not implement `PartialEq`.
    //x == Inv::<'a>::NOT_STATIC;
}

fn main() {
    foo(None)
}

Here is another example of the same issue.

The problem is that we call predicate_must_hold_modulo_regions here, but only borrowck knows the actual lifetimes and borrowck has no idea that this trait obligation even exists.

We should leave some sort of trace in MIR that there is a PartialEq obligations to ensure borrowck can check this with the right lifetimes.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 13, 2024
@RalfJung RalfJung added the A-patterns Relating to patterns and pattern matching label Feb 13, 2024
@fmease
Copy link
Member

fmease commented Feb 13, 2024

I guess we could do something similar to #105102?

@fmease fmease added 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. A-lifetimes Area: Lifetimes / regions C-bug Category: This is a bug. A-traits Area: Trait system and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 13, 2024
@RalfJung
Copy link
Member Author

Maybe? I have absolutely no idea what that PR does.^^ I don't really know anything about our trait system implementation.

@lcnr
Copy link
Contributor

lcnr commented Feb 13, 2024

I guess we could do something similar to #105102?

sadly not, the "type is copy" check can be self-contained and can therefore handle its region constraints by itself using infcx.resolve_regions. As patterns are part of MIR bodies, we have to check their region constraints together with the rest of the body during MIR borrowck. The implementation would instead be similar to https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/typeck_results/struct.CanonicalUserTypeAnnotation.html and https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.Body.html#structfield.required_consts: we store a list of requirements which have to be true for the given MIR even though they aren't directly present in the source. We then check them during mir borrowck

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lifetimes Area: Lifetimes / regions A-patterns Relating to patterns and pattern matching A-traits Area: Trait system C-bug Category: This is a bug. 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

4 participants