ext-ds
ext-ds copied to clipboard
Hashable::equals signature
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)
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
?
Currently the equals method won't be called (assumed false) if the value is not an instance of the same class.
~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.~
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.
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.
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.
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
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)
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.
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)
ah. or do you mean that calling $var = 123; $someHashable->equals($var)
MUST NOT produce an error but has to quietly return a false
?
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;
}
}
}
?
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..
equals
should never fail.
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)
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.
I'll update the docs 👍
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!)
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!
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)
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.
I think we should try to be as flexible as possible.
- Do not enforce any types - the implementation has full control over the definition of equality.
- Do not skip invoking
equals
internally when a type or class does not match (unless identical)
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
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;
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.
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.
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.
fyi, instanceof
can be used to compare strict classes like this:
$this instanceof $that and $that instanceof $this
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 typeIf 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
.