RFR: 8338981: Access to private classes should be permitted inside the permits clause of the enclosing top-level class [v7]

[Jan Lahoda]

On Sat, 28 Sep 2024 22:31:14 GMT, Evemose <duke at openjdk.org> wrote:

>> Fix involves adding new flag to after context that indicates that currently resolved symbol is in permits clause
>
> Evemose has updated the pull request incrementally with one additional commit since the last revision:
> 
>   remove trailing line

Overall, I think this is good. Left some comments to make the test and style similar to what is generally used in javac, at least in my opinion. Thanks for doing this change!

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 427:

> 425:                 (env.enclClass.sym == sym.owner // fast special case
> 426:                  ||
> 427:                  env.info.isPermitsClause

I would suggest to have parentheses around the new test, i.e. around `env.info.isPermitsClause && ((JCClassDecl) env.tree).sym.outermostClass() == sym.owner.outermostClass()`, to make it clearer what is happening.

I would also suggest to reorder the tests, so that this new test is after the existing `env.enclClass.sym.outermostClass() == sym.owner.outermostClass()` (even at the cost of modifying the existing line to remove the `)`. It seems unnecessary to make almost all code go through the `env.info.isPermitsClause` check. I don't think there will be any real runtime difference, but still better to do the more common tests first. It is also, I think, better for reading the code to have the common cases earlier, unless there's another reason for the ordering.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Resolve.java line 465:

> 463:         }
> 464:     }
> 465: 

Nit: please revert this change.

test/langtools/tools/javac/sealed/PrivateMembersInPermitClause.java line 24:

> 22:  */
> 23: 
> 24: import java.nio.file.Path;

Nit: we usually (try to) put the jtreg tags/header right after the license header (if there's any), and put imports after that. The import is part of the test, the jtreg header is test configuration.

test/langtools/tools/javac/sealed/PrivateMembersInPermitClause.java line 33:

> 31:  * @modules jdk.compiler/com.sun.tools.javac.api
> 32:  * jdk.compiler/com.sun.tools.javac.main
> 33:  * jdk.compiler/com.sun.tools.javac.util

Nit:
Suggestion:

 * @modules jdk.compiler/com.sun.tools.javac.api
 *                     jdk.compiler/com.sun.tools.javac.main
 *                     jdk.compiler/com.sun.tools.javac.util

test/langtools/tools/javac/sealed/PrivateMembersInPermitClause.java line 74:

> 72:     public void givenPrivateClassOfOtherTopLevelClassInPermitsClause_whenCompiling_thenShouldFail() throws Exception {
> 73:         var root = Path.of("src");
> 74:         tb.writeJavaFiles(root,

Nit, up to you if you want to make the change: the `writeJavaFile` takes multiple `String`s, and can generate any number of files under the given directory. So the two calls to `writeJavaFile` can be coalesced to one.

test/langtools/tools/javac/sealed/PrivateMembersInPermitClause.java line 92:

> 90:         new toolbox.JavacTask(tb)
> 91:             .files(root.resolve("S.java"), root.resolve("T.java"))
> 92:             .run(toolbox.Task.Expect.FAIL);

Negative tests should, unless there is a very good reason not to, verify the exact errors printed. Use `-XDrawDiagnostics` option, and `.getOutputLines(Task.OutputKind.DIRECT)` on the result to get the output lines. (And, preferably also `.writeAll()` on all `toolbox.JavacTask` runs, passing or failing.) See for example here:

https://github.com/openjdk/jdk/blob/855c8a7def21025bc2fc47594f7285a55924c213/test/langtools/tools/javac/ImportModule.java#L175

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

PR Review: https://git.openjdk.org/jdk/pull/20718#pullrequestreview-2342652009
PR Review Comment: https://git.openjdk.org/jdk/pull/20718#discussion_r1784351154
PR Review Comment: https://git.openjdk.org/jdk/pull/20718#discussion_r1784327802
PR Review Comment: https://git.openjdk.org/jdk/pull/20718#discussion_r1784355593
PR Review Comment: https://git.openjdk.org/jdk/pull/20718#discussion_r1784356470
PR Review Comment: https://git.openjdk.org/jdk/pull/20718#discussion_r1784353437
PR Review Comment: https://git.openjdk.org/jdk/pull/20718#discussion_r1784332032