Stream: t-compiler

Topic: Why do we need `StructuralEq` in addition to `Eq`


ecstatic-morse (Apr 06 2020 at 17:35, on Zulip):

@pnkfelix there's multiple topics being discussed in the other thread, so I thought I'd ask in a separate one: why do we need a separate StructuralEq?

pnkfelix (Apr 06 2020 at 17:36, on Zulip):

its in the docs, isn't it?

pnkfelix (Apr 06 2020 at 17:36, on Zulip):

the issue is that there was a case where the constant in question does not implement Eq

ecstatic-morse (Apr 06 2020 at 17:36, on Zulip):

The docs say:

(The problem in the above code is that Wrap<fn(&())> does not implement PartialEq, nor Eq, because for<'a> fn(&'a _) does not implement those traits.)

ecstatic-morse (Apr 06 2020 at 17:36, on Zulip):

https://doc.rust-lang.org/nightly/std/marker/trait.StructuralEq.html

pnkfelix (Apr 06 2020 at 17:37, on Zulip):

but we needed to still encode the fact that it did derive Eq

pnkfelix (Apr 06 2020 at 17:37, on Zulip):

i.e., the derived Eq has bounds that are not satisfied

eddyb (Apr 06 2020 at 17:37, on Zulip):

@ecstatic-morse the docs sound confused

pnkfelix (Apr 06 2020 at 17:37, on Zulip):

but we still need to mark it as usuable in a pattern

ecstatic-morse (Apr 06 2020 at 17:37, on Zulip):

Do function pointers not implement PartialEq?

eddyb (Apr 06 2020 at 17:37, on Zulip):

I've already answered this: #[derive(PartialEq, Eq)] needs to be detected but the derives can't know about eachother

pnkfelix (Apr 06 2020 at 17:37, on Zulip):

function pointers that have lifetimes do not

eddyb (Apr 06 2020 at 17:38, on Zulip):

so the only thing they can do is each implement a different trait

eddyb (Apr 06 2020 at 17:38, on Zulip):

that is tested later

pnkfelix (Apr 06 2020 at 17:38, on Zulip):

@eddyb in an ideal world we would just have StrucutralPartialEq and not need the derive of Eq

pnkfelix (Apr 06 2020 at 17:38, on Zulip):

because Eq has no methods

pnkfelix (Apr 06 2020 at 17:38, on Zulip):

so we in theory should be able to just test that Eq is implemented

eddyb (Apr 06 2020 at 17:38, on Zulip):

but it could have the wrong bounds

eddyb (Apr 06 2020 at 17:38, on Zulip):

hence why I think the docs are confused

eddyb (Apr 06 2020 at 17:39, on Zulip):

the Eq impl must fundamentally be a #[derive(Eq)] impl

pnkfelix (Apr 06 2020 at 17:39, on Zulip):

well I suppose it depends on what the goals are

eddyb (Apr 06 2020 at 17:39, on Zulip):

those are special in that they test the fields

eddyb (Apr 06 2020 at 17:39, on Zulip):

the generated impl is not impl Eq for Foo {}

eddyb (Apr 06 2020 at 17:39, on Zulip):

maybe this is the point of confusion?

pnkfelix (Apr 06 2020 at 17:39, on Zulip):

I don't know.

eddyb (Apr 06 2020 at 17:39, on Zulip):

"it has no methods" is irrelevant and I think actually false

eddyb (Apr 06 2020 at 17:40, on Zulip):

/me is not sure where the code to assert Eq on fields is generated. a method would make sense

pnkfelix (Apr 06 2020 at 17:40, on Zulip):

Let me try a different tack on this question

eddyb (Apr 06 2020 at 17:40, on Zulip):

yeah Eq has a method https://github.com/rust-lang/rust/blob/master/src/libcore/cmp.rs#L271-L281

pnkfelix (Apr 06 2020 at 17:40, on Zulip):

The hypothetical check is one where we just look for an impl of StructuralPartialEq and an impl of Eq

pnkfelix (Apr 06 2020 at 17:41, on Zulip):

what is the scenario, @eddyb , where you are envisaging that causing breakage for the structural-equality analysis?

eddyb (Apr 06 2020 at 17:41, on Zulip):

you mean because the fields are recursively checked?

pnkfelix (Apr 06 2020 at 17:41, on Zulip):

apart from the one documented in the docs, which is due to the (I think we all agree) compiler bug that for <'a> fn (&'a _) does not impl Eq

pnkfelix (Apr 06 2020 at 17:42, on Zulip):

@eddyb right. What's the problem with relying on the Eq impl alone? So what if it adds that extra condition?

eddyb (Apr 06 2020 at 17:43, on Zulip):

I think requiring #[derive(PartialEq, Eq)] should remain the only opt-in and changing that is a completely different discussion from why both traits are needed right now

pnkfelix (Apr 06 2020 at 17:43, on Zulip):

I don't mind that outcome

pnkfelix (Apr 06 2020 at 17:43, on Zulip):

but I'm still confused as to why you are so militant about it

eddyb (Apr 06 2020 at 17:44, on Zulip):

that is, the example in the docs seems to ignore the fact that we were using #[structural_match] gated on both traits being auto-derived before we ever had to switch to these extra traits

pnkfelix (Apr 06 2020 at 17:44, on Zulip):

My memory is that the #[structural_match] wasn't gated on the traits. It was gated on the derives.

pnkfelix (Apr 06 2020 at 17:44, on Zulip):

or sorry

pnkfelix (Apr 06 2020 at 17:44, on Zulip):

that's what you said

eddyb (Apr 06 2020 at 17:45, on Zulip):

sorry yes I mean on the traits in the derive list

pnkfelix (Apr 06 2020 at 17:45, on Zulip):

so yes, if the goal is to preserve the exact semantcs that we had before

eddyb (Apr 06 2020 at 17:45, on Zulip):

it just feels like there's a lot of confusion here

pnkfelix (Apr 06 2020 at 17:45, on Zulip):

then sure, you'd need to have two traits

eddyb (Apr 06 2020 at 17:45, on Zulip):

and a lot of factors that don't help

pnkfelix (Apr 06 2020 at 17:45, on Zulip):

but that wasn't even the original goal when I first started working on switching to a trait

eddyb (Apr 06 2020 at 17:45, on Zulip):

mechanically, Structural{Partial,}Eq are attributes

eddyb (Apr 06 2020 at 17:46, on Zulip):

and IMO that's what the docs should describe

eddyb (Apr 06 2020 at 17:46, on Zulip):

and the names are wrong

pnkfelix (Apr 06 2020 at 17:46, on Zulip):

apart from the case where we manually implement them (for PhantomData)

pnkfelix (Apr 06 2020 at 17:46, on Zulip):

which arguably is just another instance of an attribute encoded as a trait

eddyb (Apr 06 2020 at 17:46, on Zulip):

that's just #[derive(PartialEq, Eq)] for PhantomData but w/o the bounds

eddyb (Apr 06 2020 at 17:46, on Zulip):

(because there are no fields)

pnkfelix (Apr 06 2020 at 17:47, on Zulip):

and because it would break things to have the bounds, yes

eddyb (Apr 06 2020 at 17:47, on Zulip):

it's only necessary because the derive-generated bounds don't match the fields

pnkfelix (Apr 06 2020 at 17:48, on Zulip):

as in, they use the generic parameters, not the fields, right? You're just talking about how derive works in that remark?

eddyb (Apr 06 2020 at 17:48, on Zulip):

yeah

pnkfelix (Apr 06 2020 at 17:48, on Zulip):

So okay

pnkfelix (Apr 06 2020 at 17:49, on Zulip):

I 100% agree that the traits here effectively are attributes

pnkfelix (Apr 06 2020 at 17:49, on Zulip):

and that they are encoded as traits for ... reasons ...

eddyb (Apr 06 2020 at 17:49, on Zulip):

this explains, for example, why @oli was confused about libcore not having impls of these traits

eddyb (Apr 06 2020 at 17:49, on Zulip):

for primitives

eddyb (Apr 06 2020 at 17:50, on Zulip):

and that's because they only make sense where #[structural_match] did

pnkfelix (Apr 06 2020 at 17:50, on Zulip):

but I still don't understand why you are insisting it would be wrong to check for StructuralPartialEq and Eq directly instead of StructuralEq

eddyb (Apr 06 2020 at 17:50, on Zulip):

there's no extra semantics involved

pnkfelix (Apr 06 2020 at 17:50, on Zulip):

it would be a change in the semantics for structural-match, yes

pnkfelix (Apr 06 2020 at 17:50, on Zulip):

but I don't yet see where it causes things to go wrong

pnkfelix (Apr 06 2020 at 17:51, on Zulip):

I'm sorry if you feel like we are talking in circles, but I'm just not getting it

eddyb (Apr 06 2020 at 17:51, on Zulip):

if we've value-based and allow things like None::<T> always being structurally matchable no matter, then we can't be checking Option<T>: Eq, since that might be false, whereas Option<T>: AutoDerivedEq would fit

eddyb (Apr 06 2020 at 17:51, on Zulip):

unless you're talking about looking for an impl outside of the trait system?

eddyb (Apr 06 2020 at 17:52, on Zulip):

without the bounds being met

eddyb (Apr 06 2020 at 17:52, on Zulip):

frankly it would make more sense to me to just use AutoDerivedPartialEq and that's it. no Eq check (assuming all fields are recursively checked by the analysis)

eddyb (Apr 06 2020 at 17:53, on Zulip):

if you check for Eq it should be consistent with PartialEq

pnkfelix (Apr 06 2020 at 17:53, on Zulip):

Ah, okay, so now you're looping in the ... "value-based" system (historically I've thought of it as expression-based, but I suppose this depends on whether one implements it as a traversal of the AST, or as a compile-time evaluation of a const-expression)

pnkfelix (Apr 06 2020 at 17:53, on Zulip):

and yes, that may well be an area where we are better off sticking with the system as written. I wasn't thinking about that.

eddyb (Apr 06 2020 at 17:53, on Zulip):

I mean "value" in the broad sense of "a value being constructed" (by a HIR expression or a series of MIR statements)

eddyb (Apr 06 2020 at 17:54, on Zulip):

"dataflow" might be more appropriate I suppose

pnkfelix (Apr 06 2020 at 17:55, on Zulip):

okay thank you for the clarification, both of the local terminology and of your thinking more broadly.

eddyb (Apr 06 2020 at 17:55, on Zulip):

my priority was untangling the confusion that the traits have apparently created

eddyb (Apr 06 2020 at 17:56, on Zulip):

because we weren't understanding eachother (I mean the 4 of us) very well at first

ecstatic-morse (Apr 06 2020 at 17:59, on Zulip):

I didn't realize there was a shortcoming around function pointers with lifetimes, but the second half of the conversation was also helpful.

eddyb (Apr 06 2020 at 18:01, on Zulip):

I think the fn pointer thing is a bit of a red herring, the clear breakdown for me is in something like None::<String> (which I think we allow today?)

pnkfelix (Apr 06 2020 at 18:01, on Zulip):

yeah, I probably should have called out that aspect of the example in the docs

pnkfelix (Apr 06 2020 at 18:02, on Zulip):

We allow some cases like @eddyb describes, but the indirect_structural_match lint (which is allow by default) will trigger on some that we would prefer not to...

pnkfelix (Apr 06 2020 at 18:03, on Zulip):

namely #62614 is an example I can cite

pnkfelix (Apr 06 2020 at 18:04, on Zulip):

oh and of course #65466 is the really bad one ....

centril (Apr 07 2020 at 09:42, on Zulip):

pnkfelix said:

We allow some cases like eddyb describes, but the indirect_structural_match lint (which is allow by default) will trigger on some that we would prefer not to...

The RFC states otherwise btw; It seems like "we would prefer not to" was not a decision that was made as a team

centril (Apr 07 2020 at 09:42, on Zulip):

That is, the RFC encodes a fully type-based set of rules

Vadim Petrochenkov (Apr 07 2020 at 10:44, on Zulip):

I recommended replacing #[structural_match] with traits under assumption that no dataflow analysis happens and all the analysis is based on the StructuralPartialEq traits, which are regular traits and not attributes in any way.

eddyb (Apr 07 2020 at 10:45, on Zulip):

I was calling them "attributes" because their bounds don't matter AFAIK

eddyb (Apr 07 2020 at 10:45, on Zulip):

or at least I think they use no bounds?

Vadim Petrochenkov (Apr 07 2020 at 10:45, on Zulip):

(And types of all fields in the constant must satisfy the StructuralPartialEq bound recursively.)

Vadim Petrochenkov (Apr 07 2020 at 10:45, on Zulip):

(For the constant to be usable in patterns.)

eddyb (Apr 07 2020 at 10:45, on Zulip):

so they work effectively as an attribute on the struct/enum

oli (Apr 07 2020 at 12:27, on Zulip):

how about an attribute on the impl StructuralPartialEq for TheType?

eddyb (Apr 07 2020 at 12:29, on Zulip):

do you mean on the PartialEq impl?

eddyb (Apr 07 2020 at 12:29, on Zulip):

there might already be an "automatically derived" attribute

oli (Apr 07 2020 at 12:30, on Zulip):

right... so we can just use that?

eddyb (Apr 07 2020 at 12:30, on Zulip):

the problem is that you'd have to manually search for the impl

eddyb (Apr 07 2020 at 12:30, on Zulip):

but that's not that bad

eddyb (Apr 07 2020 at 12:30, on Zulip):

you can rely on the fast_reject mechanism

oli (Apr 07 2020 at 12:30, on Zulip):

we could make automatically_derived a field on the ast/hir structures for impls

eddyb (Apr 07 2020 at 12:30, on Zulip):

that's not the interesting bit

eddyb (Apr 07 2020 at 12:31, on Zulip):

@oli Span also has the derive info and I don't think you can forge that :P

eddyb (Apr 07 2020 at 12:31, on Zulip):

although I wonder if you could make your own derives with the same names that create the same impls

eddyb (Apr 07 2020 at 12:32, on Zulip):

anyway the tricky bit is finding the impls

eddyb (Apr 07 2020 at 12:32, on Zulip):

because you can't use the trait system like you can with the Structural traits

oli (Apr 07 2020 at 12:32, on Zulip):

ah

oli (Apr 07 2020 at 12:32, on Zulip):

right

pnkfelix (Apr 07 2020 at 13:35, on Zulip):

centril said:

The RFC states otherwise btw; It seems like "we would prefer not to" was not a decision that was made as a team

The lang team has discussed the matter, at least tangentially. See for example the meeting from January of this year, where we discussed PR #67088.

pnkfelix (Apr 07 2020 at 13:36, on Zulip):

The type-based check is simply too conservative to be deployed in practice.

Last update: May 29 2020 at 16:15UTC