On Fri, 25 Feb 2022 20:30:45 GMT, Jonathan Gibbons <j...@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
