Fix equals implementation to respect that anyone is allowed to implement IonElement #88
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #88 +/- ##
=========================================
Coverage ? 89.40%
Complexity ? 507
=========================================
Files ? 32
Lines ? 1038
Branches ? 140
=========================================
Hits ? 928
Misses ? 66
Partials ? 44 ☔ View full report in Codecov by Sentry. |
| type != other.type -> false | ||
| !bytes.contentEquals(other.bytes) -> false | ||
| bytesValue != other.bytesValue -> false |
There was a problem hiding this comment.
🗺️ This is now comparing the ByteArrayView rather than comparing the ByteArray directly (since we can no longer be sure that other is backed by a ByteArray).
| other as LongIntElementImpl | ||
|
|
||
| if (longValue != other.longValue) return false | ||
| when (other.integerSize) { |
There was a problem hiding this comment.
🗺️ If both IntElements are IntElementSize.LONG, then it's slightly more efficient to compare using longValue instead of bigIntegerValue.
| @@ -121,43 +122,40 @@ internal class StructElementImpl( | |||
|
|
|||
| override fun equals(other: Any?): Boolean { | |||
| if (this === other) return true | |||
| if (other !is StructElementImpl) return false | |||
| if (other !is StructElement) return false | |||
There was a problem hiding this comment.
🗺️ This change had quite a large effect. The prior implementation use a private field, so I had to rewrite it to not use that field.
There was a problem hiding this comment.
Why is anyone allowed to implement IonElement?
Why not prevent (or discourage) that?
The issue with this approach is that it is super easy for them to implement equals in a way that is not symmetric.
I suppose one could move the equals and hashcodes into the interfaces.
In no particular order:
It's also super easy for someone to create an implementation that is wrong in other ways. We can improve the documentation to make it clear. If it's really important, we could also export an We can't add default implementations of equals or hashcode to the interfaces because |
"Design and document for inheritance or else prohibit it" -- Joshua Bloch. So when I see an example, such as this, where we didn't properly design for inheritance, it makes me wonder what else we might be missing.
It's fair to say that most users would rather prefer carefully constructed abstractions and guardrails to foot-guns. Understanding why someone would want to implement their own, would guide to better abstractions for all. So I would be inclined to mark the interfaces |
I didn't mean to sound cavalier about this. We have an interface that was ostensibly designed for users to implement, and I believe it is reasonably well documented. I would also argue that this isn't really the sort of inheritance that Joshua Bloch was referring to. There is no behavior to inherit since only the interfaces are public. Interfaces are not the problem in Java, it's the fact that classes can extend other classes and override or modify all sorts of functionality of the parent class.
What exactly was not properly designed for inheritance? This PR is for a bugfix where our implementation is actually not fulfilling the expectations of the clearly specified API. That doesn't mean that the API is wrong.
My point (that I failed to state explicitly) is that we cannot predict everything that our users' will want. Yes, we can make better abstractions, and that is one of the reasons that
I think that ship has already sailed. We're at v1.2.0, and marking the interfaces as sealed (even if we were on a more recent Kotlin version) would be a backwards incompatible change. |
| override fun equals(other: Any?): Boolean = delegate == other | ||
| override fun hashCode(): Int = delegate.hashCode() | ||
| override fun toString(): String = "AnonymousProxy($delegate)" |
There was a problem hiding this comment.
Since the override logic across each anonymous class for each IonElement types is consistent, should we consider abstract the common logic?
There was a problem hiding this comment.
I don't think it's particularly important here, but if you feel strongly about it, I'll do it.
There was a problem hiding this comment.
I don't think it's particularly important here, but if you feel strongly about it, I'll do it.
Thanks for considering the feedback, I just see this could potentially enhance the maintainability. However, if there are other higher priorities at this time we can postpone this suggested change.
It just means that we didn't fully think it and work it through.
This repo has < 10 forks and stars. It strikes me as short-sighted to fear a major revision. But you seem pretty dug in here, and we're not making progress, so OK. |
|
A user might want to create their own Re. documenting clear expectations: ion-element-kotlin/src/com/amazon/ionelement/api/IonElement.kt Lines 41 to 42 in df159ac I thought that "anyone can implement IonElement" and this were pretty clear, but you made me realize that not everyone interprets it the same way. I've gone a made a big update w.r.t. to
|
| * This function is normative for equality of [IonElement]s. | ||
| * All [IonElement] implementations must override [Any.equals] in a way that is equivalent to calling this function. | ||
| */ | ||
| public fun areElementsEqual(left: IonElement, right: IonElement): Boolean { |
There was a problem hiding this comment.
Did you consider "equivalent" vs "equal" (which is clearer but also more verbose/obtuse)?
There was a problem hiding this comment.
I did, but I thought that "equal" was a little more clear about the intention that this is the authoritative function that determines if two elements are equal.
| false | ||
| } else | ||
| // Matching an enum rather than a type allows the Kotlin compiler | ||
| // to use a table switch instead of a chain of if/else comparisons. |
| thisFields === otherFields -> true | ||
| thisFields.size != otherFields.size -> false | ||
| // We've tried the inexpensive checks, now do a deep comparison | ||
| else -> thisFields.groupingBy { it }.eachCount() == otherFields.groupingBy { it }.eachCount() |
There was a problem hiding this comment.
is this comparing the occurrences of each field or am i missing something?
There was a problem hiding this comment.
It compares a Map<StructField, Int>. So for something like this Ion:
{ a: b, a: b, c: d }
It would create a map like this, which allows two structs to be compared even if their fields are in different orders, without having to impose a total ordering over all Ion values.
{
a:b = 2,
c:d = 1,
}
It's not a one-way door. If it proves to be too costly of a comparison, we could (internally) create a total order over all IonElement so that we can compare two sorted lists of StructFields.
There was a problem hiding this comment.
Right... it is the key and the value. I read it as being just the key.
| IntElementSize.LONG -> element.longValue.hashCode() | ||
| IntElementSize.BIG_INTEGER -> element.bigIntegerValue.hashCode() | ||
| } | ||
| // Adding compareTo(0.0) causes 0e0 to have a different hash code than -0e0 |
There was a problem hiding this comment.
Right—it's not incorrect for them to have the same hashcode. This is just explaining why we are using compareTo. If this ends up being a problem, we can change it.
| /** | ||
| * Calculates the hash code of an [IonElement]. | ||
| * | ||
| * Implementations of [IonElement] MAY NOT calculate their own hash codes, but MAY cache the result of this function. |
There was a problem hiding this comment.
MAY cache in-memory, but not persist. (that may be too prescriptive, just consider it).
| /** | ||
| * The size of this [IntElement]. | ||
| * | ||
| * This indicates the magnitude of the integer, but does not imply a particular in-memory representation. |
Issue #, if available:
None
Description of changes:
I discovered that all of the
equals()implementations forIonElementimplementations check that the class is the same. That would mean that if someone else implementedIonElement, two Ion-equivalent values would not be equal.However, the README says:
I've updated all of the
equals()implementations to allow other implementations of the public API be potentially equal.In some cases, I did a little more than just change the type check because some of the
equals()functions were relying on implementation details rather than fields in the public API.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.