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)