Hashable::equals signature

[githubeing]

Current Hashable::equals signature is equals($obj): bool, but in php docs it's said to be

equals ( object $obj ) : bool
https://www.php.net/manual/en/ds-hashable.equals.php

which differs. This difference leads to fatal for me:

Fatal error: Declaration of MyHashableImplementation::equals(object $obj): bool must be compatible with Ds\Hashable::equals($obj): bool in ...

First, I decided that this is a docs mistake, and was going to report it there. But then I understood that the docs is right in that the type declaration has to be there, because

It's guaranteed that obj is an instance of the same class.

and how do you suppose to implement this interface in scalars? Thus the object type is guaranteed.

But it's still possible to call

$otherHashable = null;
$hashable->equals($otherHashable);

Therefore, the signature has to be: equals(?object $obj)) i.e. nullable object to prevent null errors.

I propose you to add the type declaration to your code. Or at least, if you stick to current version, could you fix it in the docs?

And secondly, I'd appreciate if you explain the meaning of the phrase

It's guaranteed that obj is an instance of the same class.

Which code will guarantee this? I've just checked it:

$myHashable::equals($scalar)

doesn't produce any error or something...
Does the author mean that this is me who must must guarantee it? Do I have to write something like the following when implementing Hashable?

if(!$obj instanceof $this) throw new InvalidArgumentException

or is it supposed that ext-ds will somehow magically check this for me? If the latter, then I'd also like to file a bug report ('cause it doesn't check this)

[githubeing]

forgot to mention: if $obj isn't guaranteed not to equal null, that means that the phrase "it's guaranteed to be of the same class" is false - because null isn't an instance of any class and null itself is a separate type in php with the only possible value (the null itself). this also has to be clarified in the docs: does the $obj has to be of the same class or it also may be === null?

[rtheunissen]

Currently the equals method won't be called (assumed false) if the value is not an instance of the same class.

[rtheunissen]

You should always be able to pass anything to an equality check, but the internal implementation does the instanceof check for you so that you can focus on what's important. A self typehint makes sense here but I think there was an issue with that internally - I'll check into that again at some stage.

[rtheunissen]

There is some bad design here because the equals method is not meant to be called directly, which is crazy. Thanks for bringing this to my attention.

[rtheunissen]

When checking equality, ext-ds takes a shortcut to avoid invoking a callback unnecessarily. But any value should be a valid argument so the docs are wrong.

A good solution here might be to assert the instanceof check.

[githubeing]

A self typehint makes sense here

not sure a self typehint fits well here. i'm not sure about this, but as i know self means __CLASS__ and in this case the static typehint is needed (which would mean get_class($this)). I mean that self stands for \Ds\Hashable while static stands for \MyProject\MyHashableImplementation. But unfortunately if i'm not mistaken the static typehint doesn't exist in php.

[githubeing]

interface Hashable
{
    public function equals(self $that);
}

class MyHashableImplementation implements Hashable
{
    public function equals(self $that)
    {
        return true;
    }
}

class ChildClass extends MyHashableImplementation
{
    public function equals(self $that)
    {
        parent::equals($that);
    }
}

new MyHashableImplementation();

leads to

Fatal error: Declaration of MyHashableImplementation::equals(MyHashableImplementation $that) must be compatible with Hashable::equals(Hashable $that) in

[githubeing]

And to state that "the $obj must implement Hashable" makes no sense. You try to state that "the $obj must be instanceof $this" and not instanceof Hashable. this corresponds to static typehint (which doesn't yet exist in php)

[rtheunissen]

That is correct. There should then be no typehint at all and the implementation should check that the object is of the same class using instanceof. If it is library code, I'd suggest explicitly checking, but assert otherwise if you can assume that you won't be trying to compare to a null or different class value.

[githubeing]

i'm sure the ?object typehint is needed. because $obj definitely MUST NOT be a scalar or a resource but MUST be either an object or null. (which class of object - is another question)

[githubeing]

ah. or do you mean that calling $var = 123; $someHashable->equals($var) MUST NOT produce an error but has to quietly return a false?

[githubeing]

or even something like

class MyIntegerHashable implements Hashable
{
    private $int;

    public function __construct($int)
    {
        $this->int = $int;
    }

    public function hashable($var)
    {
        if (is_integer($var)) {
            return $var === $this->int;
        }
    }
}

?

[rtheunissen]

It can be any value. If you use a string as a key and an object that happens to hash into the same bucket, you must be able to do an equality check. You must be able to equate any two values. The instanceof check would be false for non-objects. However instanceof might not be quite right.. surely it would have to compare the class exactly? ie $obj instanceof self && $obj::class === self::class. This is getting messy..

[rtheunissen]

equals should never fail.

[githubeing]

i understood now. ok, so this is a docs mistake. could you fix that or should i file a bug report there? (i don't know if this is you who maintains that docs or someone else)

[rtheunissen]

I think the best thing here would be to specify in the docs that object equality MUST be restricted to objects of the same class, or return false otherwise. The implementation should not assume this. The internal shortcut to not invoke equals unless two keys are objects of the same class is still valid.

We have to support calling equals directly.

[rtheunissen]

I'll update the docs 👍

[githubeing]

I'll update the docs

it'd be useful to explain there what "guaranteed" means (that you return false internally - this is important to know in which cases my code will never run!)

[githubeing]

surely it would have to compare the class exactly?

not sure about inheritance though. what if someone treats an object and an object of a child class as equal? they could have same hash, same id, but their class names differ!

[githubeing]

have i understood you correctly in the following? you think that ext-ds should make it impossible to create a class that will implement Hashable and that will treat equal (new MyClass(123))->equals(123)===true, right? even if i want to create such a thing, you want to deny me, right? (i don't want, but just curious)

[rtheunissen]

not sure about inheritance though. what if someone treats an object and an object of a child class as equal?

They should then wrap them both in something that adapts them to the same class. I believe we should be strict here and enforce that when two objects are equated, they must be instances of the same class to be considered equal. Otherwise it is a breach of the contract.

The problem is then how do we enforce that? I think we may be able to magically intercept a call to the equals function of any object that implements Equatable, but that might be counter-intuitive when I call a method and the system hijacks that call, ultimately not invoking the method if the arg is not an object of the same class.

This is a difficult problem.

[rtheunissen]

I think we should try to be as flexible as possible.

1. Do not enforce any types - the implementation has full control over the definition of equality.
2. Do not skip invoking equals internally when a type or class does not match (unless identical)

[githubeing]

agree. the less magic, the less invisible and not obvious operations - the better a developer understands how it works, the more reliable code they write, the more they're happy with the ext - imo

[designermonkey]

Sorry to weigh in on this over a year later, but I have to say as a simple PHP developer (not a C developer) this equals is really irritating to use, especially in a static analyser like psalm, as there is no type hint information.

My 2 cents are: Surely the type should be Hashable? If the instance must be of the same type, then isn't this the simplest option to ensure that the equals function exists on the provided instance?

This is basically how I implement it on most classes:

    /**
     * @param Hashable $object
     */
    public function equals($object): bool
    {
        return get_called_class() === get_class($object) && $object->hash() === $this->hash();
    }

I would like to remove the docblock as I don't like pointless docs like that, it is only there to serve the static analysis of the code. Therefore I would like to see:

public function equals(Hashable $object): bool;

[rtheunissen]

Thanks for weighing in, I would like to come up with a solution for this. Going by your example, would object instanceof this not be more idiomatic? I know that PHPStorm will then assert the type within the scope of the condition when using instanceof.

The difficulty with this method is that a hash table can store mixed keys, and if the hash is equal, we have to call equals to determine equality. I'll have to read through this thread again.

[designermonkey]

I don't use PHPStorm so I wouldn't know about it particularly.

object instanceof this

Do you mean instead of get_called_class() === get_class($object)? I guess it could be, but I am testing specifically for the exact class, not any of it's ancestry. Your mileage may vary with the internals of the classes of PHP DS.

[rtheunissen]

Good call re: specific class. So if we were to typehint equals, and a mixed key comes long, we can't pass it to equals or the type check will fail. I think the ideal is for the implementor to be able to typehint however they like, and a mixed key would then fail.

I've drafted separating equals out and have hashable extend that. I'll look into whether we can relax the extension of equals.

[githubeing]

fyi, instanceof can be used to compare strict classes like this:

$this instanceof $that and $that instanceof $this

[ZebulanStanphill]

Just noting that Hashable#equals() lacking a defined parameter type prevents the method from being used with PHPUnit's assertObjectEquals(). As stated in its documentation:

Please note:

• A method with name $method must exist on the $actual object
• The method must accept exactly one argument
• The respective parameter must have a declared type
• The $expected object must be compatible with this declared type
• The method must have a declared bool return type

If any of the aforementioned assumptions is not fulfilled or if $actual->$method($expected) returns false then the assertion fails.

I'm not sure if an explicit mixed param type would work, or if it would have to at least be object.