RFR: 8280713: Related to comment inheritance jdk.javadoc cleanup and refactoring [v4]

[Pavel Rappo]

On Fri, 25 Feb 2022 20:30:45 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> Pavel Rappo has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 28 commits:
>> 
>>  - Merge branch 'master' into 8280713
>>  - Fix outdated code
>>    
>>    Uses a set instead of list for quick method search. In a future commit we'll try to figure out why `found` are not unique.
>>  - Fix outdated code
>>    
>>    Fix outdated inline comments and names. Both the assertion and the `contains` method will be removed in a future commit.
>>  - Fix ImplementedMethods.overridingMethodFound
>>    
>>    Removes code from VisibleMemberTable.ImplementedMethods.overridingMethodFound; the removed code is dead for the reasons similar to those in the previous commit. This commit makes overridingMethodFound degrade to linear search, which could be substituted with hashing in a later commit.
>>  - Remove ImplementedMethods::removeOverriddenMethod
>>    
>>    First of all, this commit has been tested and the before and after JDK API Documentation compared.
>>    
>>    `removeOverriddenMethod` does not seem to do any visible work and only confuses the reader. Here are some problems, all of which has been confirmed while debugging. Hopefully, the bellow bullet points in conjunction with the source code will help you see these problems.
>>    
>>    Outside:
>>    
>>    1. `removeOverriddenMethod` is private and is only used in one place: VisibleMemberTable.java:1015
>>    2. `VisibleMemberTable` passes `removeOverriddenMethod` with a direct interface method, an interface method that is either defined or overridden by an interface, but not passively inherited.
>>    
>>    Inside:
>>    
>>    1. `overriddenClass` is either a class that first defined a method that the passed `method` overrides, or `null` if there's no such class.
>>    2. `overriddenClass` is searched for recursively, using only supertypes.
>>    3. Regardless of any interfaces it might extend, an interface's supertype is `java.lang.Object`; see, for example, `javax.lang.model.element.TypeElement#getSuperclass`.
>>    4. `method` belongs to an interface; see "Outside" (2).
>>    5. Given 1-4, the only class `overriddenClass` can be is `java.lang.Object`. This can happen, for example, if `method` is an interface-override of one of the methods defined in `java.lang.Object`. Typically those are `equals`, `hashCode` or `toString`.
>>    7. `isSubclassOf(a, b)` checks if `a` is a subtype of `b`.
>>    8. Since `a` is `java.lang.Object`, condition `b == a || isSubclassOf(a, b)` can be `true` only if `b` is also `java.lang.Object`. `java.lang.Object` is not a subclass of anything but itself.
>>    9. `b` can never be a class, let alone `java.lang.Object`. This is because `b` is an immediately enclosing type of an interface method (by construction of `methlist`), i.e. `b` is an interface.
>>    10. So the above check always fails, which means that `methlist.remove(i)` is never executed.
>>  - Simplify VisibleMemberTable.ImplementedMethods
>>    
>>    Also adds a TODO to remind us to investigate an important issue later; either in this or a follow-up PR.
>>  - Fix confusing doc
>>  - Merge branch 'master' into 8280713
>>  - Refine overriddenType(ExecutableElement)
>>    
>>    Makes it clear that the returned TypeMirror is DeclaredType which represents a class and never an interface.
>>  - Refactor how superinterfaces are collected
>>    
>>    Improves readability of Utils.getAllInterfaces and friends.
>>  - ... and 18 more: https://git.openjdk.java.net/jdk/compare/efd3967b...41a00ce9
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/VisibleMemberTable.java line 1012:
> 
>> 1010:             for (TypeMirror interfaceType : intfacs) {
>> 1011:                 // TODO: this method also finds static methods which are pseudo-inherited;
>> 1012:                 //  this needs to be fixed
> 
> I can't add this comment where I'd like to ... line 992, the declaration of the `ImplementedMethods` class.
> Especially given the "curious" nature of the constructor, it would help to have descriptive comments on either the class or constructor.  It seems like `ImplementedMethods` should be a collection of methods implemented by a `TypeElement` (i.e. similar to filtering `Element.getEnclosedElements`). But it seems like this collection is more like the collection of methods in a class hierarchy that "match" a given signature .. perhaps to get a candidate list of methods from which to gather documentation (e.g with `{@inheritDoc}`).
> 
> Given that, would it be a reasonable alternative rationale for removing the `removeOverriddenMethod` because by construction, nothing ever gets added that subsequently needs to be removed?
> 
> I'm still trying to understand this one. I think I know what it is trying to do, but I'm concerned that while it might be trying to do something useful, it might be implemented incorrectly ... in which case, it should be fixed, not removed.
> 
> I guess I'd be interest to hear the actual/intended operation when a collection of different possibly-unrelated interfaces all implement the "same" method according to its signature.   Think: the "graphical cowboy" issue.   Two unrelated interfaces that both implement the `draw()` method. Now create a hierarchy of subtypes of those interfaces that start overriding methods with either simple overrides on not-simple overrides.  Then see if `removeOverriddenMethod` kicks in. My guess is that this situation is uncommon and does not arise in JDK API or even our tests.

> I'm not convinced by this one. I'd like to see more investigation.

I couldn't find a case where `removeOverriddenMethod` removed an element from `methlist`. I tried to find it by reasoning and by debugging our tests, which in conjunction convinced me that `methlist.remove(i)` is currently unreachable.

Has it ever been reachable? This might depend on how `Utils.overriddenClass` and its predecessors were implemented. Had they ever been capable of returning an *interface* rather than only a class? If not, I cannot see how this could ever work.

If at some point `overriddenClass` could return an interface, then my hypothesis is that those two methods, removeOverriddenMethod and overridingMethodFound, ensured that we constructed `methlist` in such a way that it only contained the *most derived* methods. That is, that for every method m1 from `methlist` there were no method m2 in `methlist` such that m2 overrode m1.

I think that at this point `ImplementedMethods` can work with these two methods being broken because of the way override-candidates are collected by `utils.getAllInterfaces`.

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

PR: https://git.openjdk.java.net/jdk/pull/7233