RFR: 8320716: ResolvedModule::reads includes self when configuration contains two or more automatic modules

[Mandy Chung]

On Sun, 26 Nov 2023 18:18:10 GMT, Alan Bateman <alanb at openjdk.org> wrote:

> This is update to the specification of the j.l.module.ResolvedModule.reads method to clarify that the set of resolved modules returned does not include itself. There is a small implementation change to align with the specification and fix an anomaly that arises with configurations that contain two or more automatic modules.
> 
> Every module reads itself but the intent with ResolvedModule.reads is that it returns a set that doesn't include self. As things stand right now, the returned set does not include self when all modules in the configuration are explicit modules.  However, if the configuration contains two or more automatic modules then the set includes self, a side effect of augmenting the readability graph due to implied readability.
> 
> The specification of the "reads" method is updated. The implementation is also changed to skip automatic modules when augmenting the graph due to implied readability. It is skipped as each automatic module already reads all other modules. Note that it is not goal here to change the algorithm for handling implied readability, this may be a topic for a follow on PR.
> 
> The existing ConfigurationTest already includes several tests for configurations consisting solely of explicit modules, no updates are needed. For configurations that include automatic modules, the existing AutomaticModulesTest is updated to add new asserts to each of the testConfigurationXXX methods. I decided to migrate this test to JUnit while visiting, most of it is just migrating the TestNG data providers to be parameterized tests. If needed then we could separate this but it's a simple migration so I left it in.

I read through the resolver implementation and the test changes.   Skipping the automatic modules in the process of propagating requires transitive is a simple fix to this issue.  Looks good.

src/java.base/share/classes/java/lang/module/Resolver.java line 628:

> 626:             for (Map.Entry<ResolvedModule, Set<ResolvedModule>> e : g1.entrySet()) {
> 627:                 ResolvedModule m1 = e.getKey();
> 628:                 if (!m1.descriptor().isAutomatic()) {

May be useful to add a comment to explain why automatic modules can be skipped as requires transitive edges for automatic modules already propagated in g1.

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

Marked as reviewed by mchung (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16818#pullrequestreview-1756073701
PR Review Comment: https://git.openjdk.org/jdk/pull/16818#discussion_r1409779490