On Fri, 2 Jun 2023 12:10:45 GMT, Pavel Rappo <[email protected]> wrote:
>> Please review this change.
>>
>> It turns out that JavaDoc's model for overriding and inheritance for methods
>> is simplistic. As a result, it fails to accommodate upcoming changes in the
>> "method comments algorithm" and the new feature called "directed
>> documentation inheritance".
>>
>> This change aligns JavaDoc's model for overriding and inheritance for
>> methods with that of the Java Language Specification. The most important
>> part of the change pertains to overridden methods. The model now recognizes
>> that "overrides" is not a transitive relation: from the fact that A
>> overrides B and B overrides C, it does not necessarily follow that A
>> overrides C. The model also recognizes that "overrides" depends on three
>> parameters, not two: the method that overrides, the method that is being
>> overridden, and the class where the override takes place. For details, see
>> the test that this change adds: `TestMethodMembers.java`.
>>
>> Additionally, the change provides a unified query for method overrides: be
>> it defined in a class or interface, be it abstract or concrete, an
>> overridden method can be queried through a single new API method in
>> `VisibleMemberTable`:
>>
>> public OverrideSequence overrideAt(ExecutableElement method)
>>
>> That query accepts a method and returns a sequence of methods that the
>> accepted method overrides from the class or interface described by the
>> instance of `VisibleMemberTable` that the query has been called on. That
>> sequence is totally ordered and can be traversed in either direction.
>>
>> It's quite remarkable that there are only 2 existing tests that required
>> some adjustment after the change. However, there are approximately 250
>> changed files in the generated JDK API Documentation (i.e. the result of
>> `make docs`). Most of them are benign or beneficial (e.g. they fix a bug).
>> However, there is at least one that will need to be fixed after the upcoming
>> but separate "directed documentation inheritance" update:
>> `java.util.concurrent.LinkedBlockingDeque`.
>>
>> With this change, it takes the same time (as measured with `TIME(1)`) to run
>> the tests and produce JDK API documentation. Proper benchmarks will be done
>> later on.
>>
>> ---
>>
>> _Meanwhile, I'll try to figure out a convenient way to attach diffs for JDK
>> API Documentation to this PR._
>
> Pavel Rappo has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Merge branch 'master' into 8304135
> - Extract getActualMethod
> - Impose (almost) legacy order on implemented methods
>
> The legacy order is generated by an application of
> Utils.overriddenMethod followed by application of
> Utils.addSuperInterfaces.
> - Fix errors reported by jcheck
> - Initial commit
It's clear that a lot of work has gone into this (well done) and that in
pursuing the original seemingly-simple goal of directed inheritDoc, you have
uncovered and at least partially addressed a **lot** of Technical Debt in this
area.
It's also clear that you are battling less-than-ideal support in other APIs,
like `javax.lang.model`, which expose some of the "quirks" in the way that
`javac` models JLS.
Various comments come to mind:
1. It's clear that it is good to have (and start with) an accurate model of the
API _as specified by JLS_, especially as it relates to all the terms like
_inheritance_, _overriding_, _hiding_ etc. I also not the complications caused
by override-equivalent signatures.
2. It's also clear that a JLS-only model is insufficient, and that we need to
augment the model as needed with additional javadoc information and
requirements. For example,
* the existence of "undocumented enclosures" (to use your term) such that
members may be treated as being "declared" in the first visible documented
enclosure(s).
* the presence of multiple comments that may all be relevant to a single
method signature, and the difficulty of choosing the "best" one
* the concept of _simple overrides_ such that even if a method is declared
in a type element, we choose to "ignore" the declaration if it appears to be
just an implementation-specific override.
3. While directed inheritDoc will be a valuable tool to assist authors to
explicitly select the super type from which to inherit documentation, it does
not help in determining the presence (or absence) of methods in the "summary
lists" below the main "summary table", which is one of the areas where you have
uncovered unexpected Technical Debt.
4. While I believe the work here will lead to more accurate documentation, I
think it is too late to go in this release. I appreciate the work you have done
to compare docs before and after this change, and for the analysis you describe
in your [earlier
comment](https://github.com/openjdk/jdk/pull/14221#issuecomment-1573670298) but
I suggest the conservative approach would to push this early in the next
release, when we have more time to address any unexpected issues that may arise.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/14221#issuecomment-1574487435
