8300517: Refactor VisibleMemberTable (method members) by pavelrappo · Pull Request #12887 · openjdk/jdk

pavelrappo · 2023-03-06T19:48:48Z

Please review this explorative refactoring for VisibleMemberTable (VMT).

This is the first round of refactoring for VMT. This round is about method members: declared (overriding and not) and inherited.

During this work I gained some insight into internal workings of VMT, fixed what was feasible and left TODOs and FIXMEs for everything else. Leaving those comments might look untidy, but leaving them out is wasteful: they clearly mark issues that should be revisited in upcoming rounds of refactoring.

As I see it today, the main issue with VMT is that implements complex and error-prone computations from Java Language Specification (JLS) by hand. For example, VMT interprets JLS rules for relations such as inherits, overrides and hides. As one would imagine, sometimes VMT does it incorrectly. It would be better to eventually re-implement VMT using javax.lang.model as much as possible. Unlike that of jdk.javadoc, the day job of javax.lang.model is to provide JLS services.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8300517: Refactor VisibleMemberTable (method members)

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12887/head:pull/12887
$ git checkout pull/12887

Update a local copy of the PR:
$ git checkout pull/12887
$ git pull https://git.openjdk.org/jdk pull/12887/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12887

View PR using the GUI difftool:
$ git pr show -t 12887

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12887.diff

The original sentence lacked a comma that when added still didn't make the sentence clear: > if it is specified if it contains a specified This commit restructures that sentence to clarify that "is selected" relates to all selection alternatives (1), not just the last one (2): 1. (specified OR nests-specified OR nested-in-specified) AND selected 2. specified OR nests-specified OR (nested-in-specified AND selected) The following locations corroborate that the restructured sentence (1) is accurate: * jdk/javadoc/internal/tool/ElementsTable.java:1048 Covers (i) is a specified element and (ii) is enclosed in a specified element * (??? need to find what covers (iii) contains a specified element)

Does not change code for now; just comments it.

While those members are used privately anyway, making them officially private aids reasoning when reading code. This commit also fixes a few whitespace issues.

Simplification is based on jx.l.m and JLS itself: 1. Any class except for java.lang.Object has a superclass 2. An interface or the java.lang.Object class has NoType(kind=NONE) as the superclass type 3. Element of NoType is null

That method is solely used by VisibleMemberTable.

Renames some variables and methods, removes or clarifies some comments.

@see

This commit obsoletes 8186693. A static method of an interface can neither be inherited nor hidden: JLS 8.4.8, 8.4.8.2, 9.1.3. The only affected test is fixed, but the results of `make docs` slightly differ. This is because before this fix, @see tags were "inherited" from "hidden" static methods of interfaces. Here's the list of affected files in the JDK documentation: java.base/java/time LocalDate.html LocalDateTime.html ZonedDateTime.html java.base/java/time/chrono HijrahDate.html JapaneseDate.html MinguoDate.html ThaiBuddhistDate.html The "inherited" method in all of those files is: public static LocalDate from(TemporalAccessor temporal) While propagating @see information across static methods might be useful and/or desirable behaviour, it's linked to an incorrect code, which had to be fixed. If that behavior needs to be re-introduced, we should investigate it separately. FWIW, the symptoms of documentation inheritance, such as appearance of "Description copied from" and inheriting @see information, do NOT appear even for a validly hidden method: a static method of a class. So more reasons to fix it for invalidly hidden methods. Archaeologically relevant bug fixes: 8139101, 8186703.

Those methods are used only by VisibleMemberTable.

To simplify reasoning.

Trust jx.l.m to filter those out and user not to run javadoc on non-compiling source.

Tests for interface methods that override java.lang.Object methods.

Removes misleading getSuperType, which returned java.lang.Object if passed java.lang.Object.

* The removed `typeUtils.isSameType(type, superType)` condition was historically guarding from passing interfaces or reaching java.lang.Object. Both cases are handled differently now. Because the condition had no effect, it was removed. However, when removed, a couple of issues surfaced and were captured as 8302545 and 8302542. Until either 8302545 has been resolved or --ignore-source-errors has been removed, I slightly amended containingModule and containingPackage methods to account for possible RootPackageSymbol being passed. Amending Utils.containingPackage was NOT necessary, but was done for robustness. Not sure how relevant it is to run javadoc with --ignore-source-errors though. Again: people should not run javadoc on source that does not compile. But if they run, javadoc should fail-fast and not try to recover and proceed no matter what. * Fixed a typo in the IgnoreSourceErrors test. (FWIW, there used to be "ignore-errors" options in AOT and build.)

Not sure why it was checking for qualified names; perhaps, to avoid loops, which seem to be guarding against broken source. Loops are more robustly detected by maintaining a set of already seen TypeElements. Anyway, tests pass. The good news is that some classes now show better type hierarchy. For example: Class ByteVector java.lang.Object jdk.incubator.vector.Vector<Byte> ^^^^^^^^^^^^ jdk.incubator.vector.ByteVector public abstract class ByteVector extends Vector<Byte> ^^^^^^^^^^^^ This is similar to what interfaces already had: Class StringBuilder java.lang.Object java.lang.StringBuilder All Implemented Interfaces: Serializable, Appendable, CharSequence, Comparable<StringBuilder> ^^^^^^^^^^^^^^^^^^^^^^^^^

Add guarding asserts for now.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/VisibleMemberTable.java

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java

jonathan-gibbons · 2023-03-09T22:29:06Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java

@@ -1093,28 +1026,23 @@ public TypeElement getFirstVisibleSuperClassAsTypeElement(TypeElement te) {
     *          be found.


Although not edited, this comment could be improved by defining the term "closest visible".
In particular, I notice the implementation includes reference to @hidden tags.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java

jonathan-gibbons

Approved, although there are still some outstanding suggestions for comments.

Generally, this is obviously a big project, so well done on progress so far.

It is well worth aligning the functionality and terminology of this code with current JLS, and so I (generally) encourage JLS references in key places, preferably in a stylized manner (to facilitate checking and/or updates).

I also think it is worth adding notes comparing the functionality here with that of plain-old Elements.getAllMembers. That is a simple building block that returns a list-y set of the members of a TypeElement, but VMT needs to do more:

handle members of so-called hidden types
build a DAG for each method, relating that method to corresponding methods in the supertypes of the type element, for use by @link and Specified By:.
...

openjdk · 2023-03-10T20:02:48Z

@pavelrappo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8300517: Refactor VisibleMemberTable (method members)

Reviewed-by: jjg

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

5685107: 8302491: NoClassDefFoundError omits the original cause of an error
671a452: 8303606: Memory leaks in Arguments::parse_each_vm_init_arg
a95bc7a: 8294974: Convert jdk.jshell/jdk.jshell.execution.LocalExecutionControl to use the Classfile API to instrument classes

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Stores superclasses and parents in sets, not lists.

Moves comment to the record declaration.

pavelrappo · 2023-03-13T18:12:49Z

@jonathan-gibbons, while I haven't addressed all your review comments, I believe I addressed the most important ones.

Unaddressed comments will either be explicitly addressed in upcoming PRs or will become irrelevant as a result of code change in those PRs.

pavelrappo · 2023-03-13T20:52:01Z

/integrate

openjdk · 2023-03-13T20:54:10Z

Going to push as commit 7bbc5e0.
Since your change was applied there have been 4 commits pushed to the master branch:

a8f662e: 8303908: Add missing check in VTMS_transition_disable_for_all() for suspend mode
5685107: 8302491: NoClassDefFoundError omits the original cause of an error
671a452: 8303606: Memory leaks in Arguments::parse_each_vm_init_arg
a95bc7a: 8294974: Convert jdk.jshell/jdk.jshell.execution.LocalExecutionControl to use the Classfile API to instrument classes

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-13T20:54:26Z

@pavelrappo Pushed as commit 7bbc5e0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

pavelrappo added 30 commits March 6, 2023 16:50

Fix a typo in an option name

b059107

Add missing and clarify existing comments

a6537d2

Does not change code for now; just comments it.

Make some members private

656399b

While those members are used privately anyway, making them officially private aids reasoning when reading code. This commit also fixes a few whitespace issues.

Remove useless Map.getOrDefault

aaffc54

Simplify Utils.getSuperClass

1281d4e

Simplification is based on jx.l.m and JLS itself: 1. Any class except for java.lang.Object has a superclass 2. An interface or the java.lang.Object class has NoType(kind=NONE) as the superclass type 3. Element of NoType is null

Inline Utils.getSuperClass

69ea5fe

That method is solely used by VisibleMemberTable.

Rename and clarify, add temporary FIXMEs

c2a9c06

Renames some variables and methods, removes or clarifies some comments.

Incrementally clarify

8eb1551

Rename OverriddenMethodInfo

92462ea

Make isNotSimpleOverride private

21077cc

Move methods from Utils to VisibleMemberTable (1)

38543d2

Those methods are used only by VisibleMemberTable.

Inline executableMembersEqual

2c3e29f

Simplify findMethod

6a40e1f

Rename and simplify findMethod further

c198b61

Simplify ImplementedMethods

0683c9e

Refactor ImplementedMethods

5a27e17

Remove redundant data structure

60ea5db

To simplify reasoning.

Refactor ImplementedMethods

e29cb15

Inline Utils.getType

e1a3d82

Don't care about static "overrides"

e44c707

Trust jx.l.m to filter those out and user not to run javadoc on non-compiling source.

Increase test coverage

6c9c46c

Tests for interface methods that override java.lang.Object methods.

Simplify Utils.overriddenMethod

750d3e8

Removes misleading getSuperType, which returned java.lang.Object if passed java.lang.Object.

Remove now-unused Utils.isNoType

7cc61af

Simplify Utils.getFirstVisibleSuperClass

3c59145

Simplify Utils.getFirstVisibleSuperClass

5c69b2f

Add guarding asserts for now.

Consolidate Utils.{overriddenMethod,overriddenType}

eaa360b

jonathan-gibbons reviewed Mar 7, 2023

View reviewed changes

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java Show resolved Hide resolved

jonathan-gibbons reviewed Mar 7, 2023

View reviewed changes

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java Outdated Show resolved Hide resolved

jonathan-gibbons reviewed Mar 7, 2023

View reviewed changes

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java Show resolved Hide resolved

jonathan-gibbons reviewed Mar 7, 2023

View reviewed changes

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java Outdated Show resolved Hide resolved

Respond to feedback

3ad7faf