On Sun, 26 Nov 2023 18:18:10 GMT, Alan Bateman <al...@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
