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

PartialEq/Eq and PartialOrd/Ord impls generated by #[derive(...)] are not generic for RHS #20927

Closed
murarth opened this issue Jan 11, 2015 · 9 comments
Labels
A-syntaxext Area: Syntax extensions C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@murarth
Copy link
Contributor

murarth commented Jan 11, 2015

(Partial)Eq and (Partial)Ord traits are now generic for right-hand side expression, but derive-generated code has not caught up. That is, Option<T> is only comparable to other Option<T> instances, rather than any Option<U> where T: Eq<U>.

I'm currently working on implementing this myself; however, as I am not familiar with the code, I'm still working out the cleanest way to get it done. I suppose I'm wondering whether anyone has any advice or if anyone is already working on this or some other changes to derive that might include this.

@kmcallister kmcallister added A-libs A-syntaxext Area: Syntax extensions labels Jan 11, 2015
@murarth
Copy link
Contributor Author

murarth commented Jan 16, 2015

So, it turns out that implementing this isn't as straightforward as I thought it would be.
It runs into something like #19839:

#[derive(PartialEq)]
struct Foo<T> {
    bar: Bar<T>,
}

struct Bar<T> {}

The generated impl places a bound on T: PartialEq<U> in Foo<T> and for any U, but doesn't place a bound on Bar<T>: PartialEq<Bar<U>>, which it attempts to compare in the generated methods. This causes a compile error, stating that Bar<T> cannot be compared.

So, I suppose this will have to wait until the derive syntax extension becomes a bit more sophisticated.

@Gankra
Copy link
Contributor

Gankra commented Jan 21, 2015

@nikomatsakis would this run afoul of any of your coherence strategies?

@shepmaster
Copy link
Member

@murarth Is your branch available anywhere for perusal?

@murarth
Copy link
Contributor Author

murarth commented May 25, 2015

@shepmaster: Sure. It's severely outdated at this point and the standard library won't compile with it, but other than that, it worked. https://github.com/murarth/rust/tree/derive-generic

@huonw
Copy link
Member

huonw commented May 25, 2015

This is impossible to do by-default now, since it is backwards incompatible. At least, it is if we don't do something like Bar<T>: PartialEq<Bar<U>>. But this exposes implementation details, so probably isn't what we want to do.

However, I could definitely imagine adding an opt-in way to do this.

@shepmaster
Copy link
Member

Here's @huonw's example of the backwards incompatibility from our IRC conversation:

struct Foo<T> { ... }

/* manual impl */
impl<T> PartialEq for Foo<T> { ... }

#[derive(PartialEq)]
struct Bar<T> {
    x: Foo<T>,
}

@shepmaster
Copy link
Member

adding an opt-in way to do this

One concrete idea was extending the derive syntax:

#[derive(PartialEq(with-awesome-flexibility))]
struct Foo<T> { ... }

@steveklabnik
Copy link
Member

Triage; no change

@steveklabnik steveklabnik added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed A-libs labels Mar 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 22, 2017
@Mark-Simulacrum Mark-Simulacrum added C-feature-request Category: A feature request, i.e: not implemented / a PR. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Sep 10, 2017
@dtolnay
Copy link
Member

dtolnay commented Nov 19, 2017

I agree that this could be handled better by the derives in the standard library. Thanks for the discussion! Since this has a potential to interact in confusing ways with type inference, I would prefer to see this explored in an RFC. There are at least two promising ways to go with this, either by making the derived impls smarter out of the box (which will hinge on what happens with rust-lang/rfcs#917) or by requiring the programmer to tweak the derived impl using attributes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-syntaxext Area: Syntax extensions C-feature-request Category: A feature request, i.e: not implemented / a PR. 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

8 participants