Akka.net: ActorRefBase.Equals(IActorRef) and ActorRefBase.CompareTo(IActorRef) do not handle null parameters properly

[RoBiK75]

https://github.com/akkadotnet/akka.net/blob/bc5cc65a3dff829cc5844f921826586f828510f5/src/core/Akka/Actor/ActorRef.cs#L347-L361
Both methods do not handle null parameter properly and if the parameter is null, System.NullReferenceException will be thrown on the other.Path property access.

[siudeks]

I just checked in Akka there are more places where Path is simply used without prechecking nullability. IMHO Path property simply can't be null by contract, what should be highlighted on IAkkaRef interfacece's documentation.

[RoBiK75]

I just checked in Akka there are more places where Path is simply used without prechecking nullability. IMHO Path property simply can't be null by contract, what should be highlighted on IAkkaRef interfacece's documentation.

Hi,
i think you misunderstood me. It is not the Path property that is null, it is the value of the "other" parameter that can be null (because any type implementing the IActorRef interface is by definition a reference type and therefore can be null) that will lead to a System.NullReferenceException.

[siudeks]

Your description was good, my misunderstanging.
Looking into interfaces: IEquatable and IComparable (which contain mentioned methods) I see the reason: both methods - Equals and CompareTo do not define what kind of values should be returned when methods' argument is null, so throwing exception seems to be an acceptable behavior (instead of returning false in Equals or whatever in CompareTo).

What, in your opinion, should be "proper handling" when an argument is null? Try to define it from method's contract perspective, not from implementation.

[RoBiK75]

I agree that the documentation for IEquatable.Equals method does not explicitly state how null should be handled.
However, there are other parts of documentation that are relevant to this case and indirectly dictate the behaviour:

From the documentation of the IEquatable interface https://docs.microsoft.com/en-us/dotnet/api/system.iequatable-1

Both your implementation of Equals(T) and your override of Equals(Object) should return consistent results.

From the documentation for object.Equals method
https://docs.microsoft.com/en-us/dotnet/api/system.object.equals

The following statements must be true for all implementations of the Equals(Object) method.
...
x.Equals(null) returns false.

Implementations of Equals must not throw exceptions; they should always return a value.

If we follow these requirements, the proper behaviour for IEquatable.Equals is that in case the “other” parameter is null, the method should return false.

In case of IComparable.CompareTo the documentation actually states what the method should do in case when the parameter is null
https://docs.microsoft.com/en-us/dotnet/api/system.icomparable-1.compareto

By definition, any object compares greater than null, and two null references compare equal to each other.

[siudeks]

@RoBiK75 good explanation. To sum up (according to docs.microsoft.com)

x.Equals(null) should returns false
x.CompareTo(null) should returns 1
for x != null

so the issue is well described