Skip to content

Switch #[derive(PartialEq)] to have separate lhs and rhs lifetimes #83957

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

Open
programmerjake opened this issue Apr 7, 2021 · 8 comments
Open
Labels
A-lifetimes Area: Lifetimes / regions A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@programmerjake
Copy link
Member

As far as I can tell the following code shouldn't cause a lifetime error but it does. If Iter::Item is changed to &'a () instead of PhantomData<&'a ()> the error goes away.

use core::marker::PhantomData;

fn g<A: Iterator, B: Iterator>(a: A, b: B) -> A
where
    A::Item: PartialEq<B::Item>,
{
    todo!()
}

pub struct Iter<'a>(&'a ());

impl<'a> Iterator for Iter<'a> {
    type Item = PhantomData<&'a ()>;
    fn next(&mut self) -> Option<Self::Item> {
        todo!()
    }
}

pub fn f<'a, B: AsRef<()>>(a: &'a (), b: B) -> &'a () {
    g(Iter(a), Iter(b.as_ref())).0
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0309]: the parameter type `B` may not live long enough
  --> src/lib.rs:20:21
   |
19 | pub fn f<'a, B: AsRef<()>>(a: &'a (), b: B) -> &'a () {
   |              -- help: consider adding an explicit lifetime bound...: `B: 'a +`
20 |     g(Iter(a), Iter(b.as_ref())).0
   |                     ^ ...so that the type `B` is not borrowed for too long

error[E0309]: the parameter type `B` may not live long enough
  --> src/lib.rs:20:23
   |
19 | pub fn f<'a, B: AsRef<()>>(a: &'a (), b: B) -> &'a () {
   |              -- help: consider adding an explicit lifetime bound...: `B: 'a +`
20 |     g(Iter(a), Iter(b.as_ref())).0
   |                       ^^^^^^ ...so that the reference type `&B` does not outlive the data it points at

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0309`.
error: could not compile `playground`

To learn more, run the command again with --verbose.

@programmerjake
Copy link
Member Author

@rustbot label: +C-bug

@rustbot rustbot added the C-bug Category: This is a bug. label Apr 7, 2021
@eggyal
Copy link
Contributor

eggyal commented Apr 7, 2021

The sole implementation of PartialEq for PhantomData<T> is where Rhs is also PhantomData<T>, so the call to g requires the lifetimes to match.

@programmerjake
Copy link
Member Author

hmm, I had changed it to PhantomData in an attempt to reduce the problem, it had before been an enum like so:

#[derive(PartialEq)]
pub enum MyItem<'a> {
    Empty,
    Thing(&'a [u8]),
}

I'd expect to be able to compare two different instances of MyItem with differing lifetimes...

@programmerjake
Copy link
Member Author

That produces

#[automatically_derived]
#[allow(unused_qualifications)]
impl <'a> ::core::cmp::PartialEq for MyItem<'a> {
    // ...
}

So, I think #[derive(PartialEq)] should instead let the lifetimes be unequal when comparing, producing:

#[automatically_derived]
#[allow(unused_qualifications)]
impl<'a, 'b> ::core::cmp::PartialEq<MyItem<'b>> for MyItem<'a> {
    // ...
}

@eggyal
Copy link
Contributor

eggyal commented Apr 7, 2021

I think whether two values that have different lifetimes can be considered equal or not is application-dependent. If, in your case, they are equal then you merely need implement PartialEq yourself rather than use the default derive implementation.

@programmerjake
Copy link
Member Author

Hmm, I'd argue that you'd have some field's PartialEq that requires matching lifetimes if you wanted to have derive(PartialEq) require matching lifetimes...I think the default should be to just match the PartialEq implementations of all fields:

impl<'lhs1, 'rhs1> PartialEq<Type<'rhs1>> for Type<'lhs1>
where
    Field1<'lhs1>: PartialEq<Field1<'rhs1>>,
    Field2<'lhs1>: PartialEq<Field2<'rhs1>>,
    Field3<'lhs1>: PartialEq<Field3<'rhs1>>,
{
    // ...
}

@programmerjake programmerjake changed the title Incorrect lifetime bug Switch #[derive(PartialEq)] to have separate lhs and rhs lifetimes Apr 7, 2021
@eggyal
Copy link
Contributor

eggyal commented Apr 7, 2021

@rustbot label: +A-lifetimes +A-macros -C-bug +C-enhancement +T-libs

@rustbot rustbot added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-lifetimes Area: Lifetimes / regions C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed C-bug Category: This is a bug. labels Apr 7, 2021
@eggyal
Copy link
Contributor

eggyal commented Apr 8, 2021

See also #27950 and, more generally, #26925.

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-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants