stdweb icon indicating copy to clipboard operation
stdweb copied to clipboard

default implementation of Eq trait wrong?

Open NeverGivinUp opened this issue 6 years ago • 3 comments

SVG has matrices. It also has a function for inverting a matrix. For the unit matrix U U==inverse(U) should hold. However it does not.

I speculate, that this is because a) stdweb structs are really references and b) the derived Eq implementation that stdweb regularly uses is wrong (technically it is correct, but it is not what users expect), because the compiler does not know they are references and users expect equality to be semantic equality, not memory pointer equality (identity).

Take the following code:

#[derive(Debug, Clone, Eq, PartialEq)]
struct Foo {
    a: u32,
    b: Box<u32>,
}


fn main() {
    let p = Foo { a: 1, b: Box::new(99) };
    let q = Foo { a: 1, b: Box::new(99) };
    let r = Foo { a: 2, b: Box::new(99) };
    let s = Foo { a: 1, b: Box::new(100) };


    assert_eq!(p, p); // p=p, because p is identical to p
    assert_eq!(p, q); // p=q, although p and q are different instances (allocated at different
    // locations in memory), their contents is equal. 
    assert_ne!(p, r); // p!=r, because their content differs
    assert_ne!(p, s); // p!=s, because their content differs

    //comparing references works, because of automatic dereferencing
    assert_eq!(&p, &p);
    assert_eq!(&p, &q);
    assert_ne!(&p, &r);
    assert_ne!(&p, &s);

    // I believe this is what stdweb's eq implementation is actually doing -> case 2 fails
    assert_eq!(&p as *const Foo, &p as *const Foo); // assertion holds
    assert_eq!(&p as *const Foo, &q as *const Foo); // assertion fails
    assert_ne!(&p as *const Foo, &r as *const Foo); // assertion holds
    assert_ne!(&p as *const Foo, &s as *const Foo); // assertion holds
}

In the second assertion (assert_eq!(p, q)) the user cannot tell p and q apart, unless she tracks them to their respective sources of their declaration or ask for the memory locations of p and q. This is, because Rust's Eq trait compares content, not adresses and thus equality not identity.

stdweb's struct instances are really references to Javascript objects. stdweb pretends to it's users and to the compiler, that they are dealing with real structs by not declaring them as references (through Box or &). Because the compiler does not know, that stdweb's instances are actually references it thus does not automatically dereference them for the implementation of the Eq trait (also, it wouldn't know how to dereference, because the data really is behind a foreign interface).

End of speculation.

Are my speculations correct? Is there a way to clarify that stdweb structs are references and to get Eq implemented correctly if the current implementation is wrong?

NeverGivinUp avatar Jan 16 '19 14:01 NeverGivinUp

users expect equality to be semantic equality, not memory pointer equality (identity).

It is true that most things in Rust use semantic equality, but not everything:

As you can see, raw pointers (which are primarily used in FFI) use reference equality (which is correct).

JavaScript uses reference equality almost exclusively (only a handful of types use semantic equality).

So, equality in stdweb uses reference equality, because it should match what JavaScript does.

stdweb's struct instances are really references to Javascript objects.

Yes, that is correct.

stdweb pretends to it's users and to the compiler, that they are dealing with real structs by not declaring them as references (through Box or &).

There is no pretending. The stdweb structs are opaque: their contents are private, so they are like a black box.

As far as users are concerned, stdweb might be using *mut T or *const T behind the scenes. There is no way for users to know.

Consider this Rust code:

struct Foo {
    foo: Box<u32>,
}

How can users know that Foo contains a reference (Box)? They can't, because the fields on the struct are private.

So you can't just assume that because you don't see Box or & anywhere that therefore it isn't a reference. There could be a private reference inside of the struct.

In general it doesn't matter whether something is a reference or not, what matters is the public API.

Because the compiler does not know, that stdweb's instances are actually references it thus does not automatically dereference them for the implementation of the Eq trait (also, it wouldn't know how to dereference, because the data really is behind a foreign interface).

I think you are perhaps misunderstanding how all of this works.

Equality is not special, and references aren't special either. Equality is controlled by the PartialEq trait, which is just a normal trait, there is no magic.

Here is the official implementation of PartialEq for Box:

fn eq(&self, other: &Box<T>) -> bool {
    PartialEq::eq(&**self, &**other)
}

As you can see, what it does is it uses * to dereference the Box (which returns the contents inside the Box), and then it calls eq on the dereferenced contents.

So the reason why Box compares the equality of its contents is not because the Rust compiler magically knows that it's a reference... The reason is simply because that's what the PartialEq implementation does. No magic.

Since PartialEq is a normal trait, you can implement it on your own types as well (including stdweb types).

Here is an example:

use stdweb::Reference;
use stdweb::unstable::TryInto;

#[derive(Clone, PartialEq, Eq, Debug, ReferenceType)]
#[reference(instance_of = "Object")]
pub struct Foo(Reference);

impl Foo {
    pub fn new(foo: u32, bar: u32) -> Self {
        js!(
            return {
                foo: @{foo},
                bar: @{bar}
            };
        ).try_into().unwrap()
    }
}

The above code creates a Foo struct, which is a reference to a JavaScript object. This JavaScript object has two fields: foo and bar.

The Reference type has an implementation of PartialEq. That implementation uses reference equality.

So when we use derive(PartialEq) that means our Foo struct will also have reference equality (since it reuses the PartialEq implementation of Reference).

So that means that Foo::new(1, 2) == Foo::new(1, 2) will return false, since they are different objects.

However, we can simply not use derive(PartialEq) and instead define it ourself. When we do so, we can give it whatever behavior we want:

impl PartialEq for Foo {
    fn eq(&self, other: &Self) -> bool {
        js!(
            var self = @{self.as_ref()};
            var other = @{other.as_ref()};

            return self.foo === other.foo &&
                   self.bar === other.bar;
        ).try_into().unwrap()
    }
}

In this case it will return true if the foo and bar fields are equal, even if the objects are not reference equal.

So if you do Foo::new(1, 2) == Foo::new(1, 2) then it will return true, even though the objects are different!

As you can see, there's no magic: just the normal PartialEq trait.

Whether Foo is using a reference or not doesn't matter, what matters is the implementation of PartialEq.

By the way, dereferencing is also not magic: it is controlled by the Deref trait. That means you can implement Deref on your own types, with whatever behavior you want.

In fact, most of Rust's syntax can be controlled through traits. Including things like addition, multiplication, ranges, assignment, etc.

So why does stdweb use reference equality by default? Two reasons:

  1. In many situations it doesn't make sense to use semantic equality for JS.

  2. JS uses reference equality. So since stdweb is providing references to JS objects, it should match JS's behavior.

Is there a way to clarify that stdweb structs are references

Could you explain why this is important to you?

and to get Eq implemented correctly if the current implementation is wrong?

Reference equality is correct, so the current implementation isn't wrong.

Of course you are free to implement PartialEq on your own types, and you can give it whatever behavior you want (including semantic equality, if you wish).

Pauan avatar Jan 18 '19 04:01 Pauan

To really prove the point that equality isn't special, here's the implementation of equality for slices (including Vec):

impl<A, B> PartialEq<[B]> for [A] where A: PartialEq<B> {
    fn eq(&self, other: &[B]) -> bool {
        SlicePartialEq::equal(self, other)
    }
}

It calls SlicePartialEq::equal, which is defined like this:

impl<A, B> SlicePartialEq<B> for [A]
    where A: PartialEq<B>
{
    default fn equal(&self, other: &[B]) -> bool {
        if self.len() != other.len() {
            return false;
        }

        for i in 0..self.len() {
            if !self[i].eq(&other[i]) {
                return false;
            }
        }

        true
    }
}

As you can see, it first checks the length of the slices (since slices with different lengths can never be equal).

Then it simply loops over the slices from left to right, it calls eq on each element, and as soon as eq returns false then it immediately returns false.

But if eq never returns false then that means that all of the elements are equal, so it returns true.

You can write similar PartialEq implementations for your own types.

For example, maybe you have a struct where you only care about the equality of certain fields:

struct Foo {
    foo: u32,
    bar: u32,
}

impl PartialEq for Foo {
    fn eq(&self, other: &Self) -> bool {
        self.foo == other.foo
    }
}

In this case two Foo will be equal if their foo field is equal (even if the bar field is different!)

Pauan avatar Jan 18 '19 04:01 Pauan

I think there's some confusing here about the bug being reported vs speculation on the cause of the bug. @NeverGivinUp could you provide an example of code involving SVG matrixes that does not behave how you'd expect?

shelvacu avatar Aug 23 '19 08:08 shelvacu