RFR: 8301618: Compare elements and type mirrors properly

[Pavel Rappo]

On Wed, 1 Feb 2023 18:11:30 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> This makes jdk.javadoc compare javax.lang.model primitives correctly: elements with `Object.equals` and type mirrors with `Types.isSameType`.
>> 
>> jdk.javadoc sometimes compares elements using identity comparison (`==`) and type mirrors using equality comparison (`equals`). While this is generally wrong, we likely get away with it because of the implementation specifics. I've heard that javac is very forgiving in the sense that there's no difference between `==` and `equals` for elements. In addition to that, I noticed that more often than not there's no difference between `equals` and `isSameType` for type mirrors in the context of jdk.javadoc.
>> 
>> Since there's no guarantee that a different compiler implementation or the future javac itself will be implemented similarly, it's better to abandon these bad practices while we are still lucky.
>> 
>> ----
>> 
>> With a change like this, there are two main concerns. The first is nullity. When it's not immediately obvious if `a`, `b`, or both can be null, it's not clear how to check for equality. To avoid introducing bugs to a code that I don't know that much, I use the `java.util.Objects.equals` method, which takes care of nullity the same way the existing code does: if any of `a` or `b` are null, `Objects.equals(a, b)` behaves exactly as `a == b`, provided `equals` is well-defined and returns false for the null argument.
>> 
>> The second concern is identity. If equality does not imply identity, it might be important to distinguish between equal elements and identical elements. There are a few cases in jdk.javadoc where it's identity that is required; for example, consider:
>> 
>>     SortedSet<ExecutableElement> members = utils.serializationMethods(currentTypeElement);
>>     for (ExecutableElement member : members) {
>>         currentMember = member;
>>         Content methodsContent = methodWriter.getMethodsContentHeader(
>>                 currentMember == members.last());
>>     ...
>> 
>> Here's the loop computes a boolean indicating the last iteration. If `members` contained identical elements, then that check could misjudge the last iteration and exit prematurely. Thankfully, in that and the similar cases, iteration happens on a (sorted) set, which by definition cannot have duplicates, provided the comparator used by the set is consistent with `equals`, which is also consistent with `==`.  So in this case we can safely change `==` to `equals`.
>> 
>> However, when I fixed those iterations to use `equals`, it didn't feel right; it felt marginally less brittle than what there was before. I then restructured the loops to avoid equality comparison altogether, which had a side-benefit of widening abstractions from `SortedSet` to just `Iterable`.
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/builders/SerializedFormBuilder.java line 269:
> 
>> 267:         Iterator<ExecutableElement> members = utils.serializationMethods(currentTypeElement).iterator();
>> 268:         if (members.hasNext()) {
>> 269:             while (members.hasNext()) {
> 
> This double `hasNext` looks odd. I understand why you are doing it, so I'm not saying it's wrong,
> but to my eyes it would be nicer to get the `members` set and check `isEmpty` as before, while
> keeping the use of the iterator within the `then` block.

I'm glad you asked. Unlike that in SerializedFormBuilder.java:402, here, the first `hasNext` can be dropped. It was useless before, it is useless now. I left it initially, because it was unrelated.

-------------

PR: https://git.openjdk.org/jdk/pull/12369