Fix equals implementation to respect that anyone is allowed to implement IonElement

[popematt]

Issue #, if available:

None

Description of changes:

I discovered that all of the equals() implementations for IonElement implementations check that the class is the same. That would mean that if someone else implemented IonElement, two Ion-equivalent values would not be equal.

However, the README says:

As long as the behavior is implemented correctly (see the documentation of each interface), the implementations of IonElement provided by this library are fully interoperable with user supplied implementations.

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.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 86.45833% with 13 lines in your changes are missing coverage. Please review.

:exclamation: No coverage uploaded for pull request base (master@5b9ec91). Click here to learn what that means.

Files	Patch %	Lines
src/com/amazon/ionelement/api/Equivalence.kt	89.47%	2 Missing and 4 partials :warning:
...rc/com/amazon/ionelement/impl/ByteArrayViewImpl.kt	33.33%	0 Missing and 2 partials :warning:
...rc/com/amazon/ionelement/impl/StructElementImpl.kt	83.33%	0 Missing and 2 partials :warning:
src/com/amazon/ionelement/impl/StructFieldImpl.kt	66.66%	0 Missing and 2 partials :warning:
...com/amazon/ionelement/impl/BigIntIntElementImpl.kt	66.66%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #88 +/- ## ========================================= Coverage ? 89.40% Complexity ? 507 ========================================= Files ? 32 Lines ? 1038 Branches ? 140 ========================================= Hits ? 928 Misses ? 66 Partials ? 44 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[popematt]

Why is anyone allowed to implement IonElement?

Why not prevent (or discourage) that?

In no particular order:

• This already part of the API contract, so taking it back is a breaking change.
• We have all of the *Element interfaces in order hide the implementation details. Since we have the API decoupled from the implementation, why not let users implement it?
• As we have learned with IonValue, people will create their own implementations even if we say not to. We should embrace what our users want rather than trying to fight them on it.

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.

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 ion-element-tests jar that provides some conformance tests that people can use (though I'd be inclined to let people copy/paste the existing tests for now).

We can't add default implementations of equals or hashcode to the interfaces because kotlinc complains that "An interface may not implement a method of 'Any'". However, we could provide functions that users could leverage to implement hashCode() and equals() (and toString()) in their own IonElement implementations.

[rmarrowstone]

We have all of the *Element interfaces in order hide the implementation details. Since we have the API decoupled from the implementation, why not let users implement it?

"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.

As we have learned with IonValue, people will create their own implementations even if we say not to. We should embrace what our users want rather than trying to fight them on it.

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 sealed (though it looks like this is still using Kotlin 1.4). Or at least document, document, document the expectations for users.

[popematt]

We have all of the *Element interfaces in order hide the implementation details. Since we have the API decoupled from the implementation, why not let users implement it?

"Design and document for inheritance or else prohibit it" -- Joshua Bloch.

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. IonElement does not have that problem. (It's classes are both final and internal.)

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.

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.

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.

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 IonElement exists, but I think it's naïve to say that we can design a perfect abstraction and that no users will need an escape hatch for some use case. We can plan for and design an escape hatch, or someone will eventually create their own escape hatch against our wishes, which (if we later try to close it) could then be forced upon us because of the institutional weight behind it.

So I would be inclined to mark the interfaces sealed (though it looks like this is still using Kotlin 1.4). Or at least document, document, document the expectations for users.

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.

[rmarrowstone]

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.

It just means that we didn't fully think it and work it through.

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.

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.

[popematt]

A user might want to create their own IonElement implementation in order to e.g. have a lazy implementation or to be able to create a view over data that is not Ion. If we close the IonElement interface, then for any of that to happen, it has to go through us (the Ion maintainers) in some form. That would make us a potential bottleneck for innovation, and it seems antithetical to the spirit of open source software.

Re. documenting clear expectations:

https://github.com/amazon-ion/ion-element-kotlin/blob/df159ac5d3ff855eafb8922bbd6ca4184ccc5bc6/src/com/amazon/ionelement/api/IonElement.kt#L41-L42 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 equals() and hashCode() so that it's more clear.

• Add Equivalence.kt that defines the following public functions: areElementsEqual(), hashElement(), areFieldsEqual(), hashField(). The are*Equals() methods are normative—all implementations must be equivalent to these functions—and the hash*() methods are the only allowed functions for hashing any implementations of IonElement.
• Add equals() and hashCode() to IonElement interface with extra doc comments.
• Update ByteArrayView to be clear about its equality semantics. Since it's just a thin wrapper around ByteArray, I updated the documentation to explain how that relates to equals() and hashCode().
• Update IntElementSize to make it clear that two equal Ion ints must have the same IntElementSize.
• Update all *ElementImpl to use hashElement() and areElementsEqual() (except for StructElement, which has a specialized implementation with some specific optimizations)
• Update the tests to check that an IonElement can be equal to the same Ion value from another implementation, ensure that the implementations match hashElement() and areElementsEqual(), and clean up some of the redundancy in EquivsTestCase.
• Incidental changes include fixing some typos and adding some missing copyright headers.