RFR: 8332497: javac prints an AssertionError when annotation processing runs on program with module imports [v17]

[Evemose]

On Fri, 24 May 2024 21:31:45 GMT, Vicente Romero <vromero at openjdk.org> wrote:

> ok this is what I would do, please feel free to use it, it is applicable on top of your current code. As you can see I have reformatted the test to align it better with the rest of similar tests in our code base. Also as Jan suggested I have added an equivalent method to TreeTranslator for completeness.
> 
> ```
> diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeTranslator.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeTranslator.java
> index 59a7457e6d0..63778fb42ff 100644
> --- a/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeTranslator.java
> +++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeTranslator.java
> @@ -131,6 +131,11 @@ public void visitImport(JCImport tree) {
>          result = tree;
>      }
>  
> +    public void visitModuleImport(JCModuleImport tree) {
> +        tree.module = translate(tree.module);
> +        result = tree;
> +    }
> +
>      public void visitClassDef(JCClassDecl tree) {
>          tree.mods = translate(tree.mods);
>          tree.typarams = translateTypeParams(tree.typarams);
> diff --git a/test/langtools/tools/javac/processing/ModuleImportProcessingTest.java b/test/langtools/tools/javac/processing/ModuleImportProcessingTest.java
> index 88c7974a26e..760ec30820e 100644
> --- a/test/langtools/tools/javac/processing/ModuleImportProcessingTest.java
> +++ b/test/langtools/tools/javac/processing/ModuleImportProcessingTest.java
> @@ -21,7 +21,16 @@
>   * questions.
>   */
>  
> -import toolbox.*;
> +/**
> + * @test
> + * @bug 8332497
> + * @summary javac prints an AssertionError when annotation processing runs on program with module imports
> + * @library /tools/lib
> + * @modules jdk.compiler/com.sun.tools.javac.api
> + *          jdk.compiler/com.sun.tools.javac.main
> + * @build toolbox.JavacTask toolbox.ToolBox toolbox.Task
> + * @run main ModuleImportProcessingTest
> + */
>  
>  import javax.annotation.processing.AbstractProcessor;
>  import javax.annotation.processing.RoundEnvironment;
> @@ -32,40 +41,49 @@
>  import java.nio.file.Paths;
>  import java.util.Set;
>  
> -/**
> - * @test
> - * @bug 8332497
> - * @summary error: javac prints an AssertionError when annotation processing runs on program with module imports
> - * @library /tools/lib
> - * @modules jdk.compiler/com.sun.tools.javac.api
> - *          jdk.compiler/com.sun.tools.javac.main
> - * @build toolbox.JavacTask toolbox.ToolBox toolbox.Task
> - * @run main ModuleImportProcessingTest
> - */
> -public class ModuleImportProcessingTest {
> -    final toolbox.ToolBox tb = new ToolBox();
> -    final Path base = Paths.get(".");
> -    final String processedSource = """
> -        import module java.base;
> -        import java.lang.annotation.*;
> -        public class Main {
> -          public static void main(String[] args) {
> -            List.of();
> -          }
> -          @Ann
> -          private void test() {}
> -          @Retention(RetentionPolicy.RUNTIME)
> -          @Target(ElementType.METHOD)
> -          public @interface Ann {}
> -        }
> -        """;
> +import toolbox.TestRunner;
> +import toolbox.ToolBox;
> +import toolbox.JavacTask;
> +import toolbox.Task;
> +
> +public class ModuleImportProcessingTest extends TestRunner {
> +    ToolBox tb = new ToolBox();
> +
> +    public ModuleImportProcessingTest() {
> +        super(System.err);
> +    }
> +
> +    protected void runTests() throws Exception {
> +        runTests(m -> new Object[] { Paths.get(m.getName()) });
> +    }
>  
> -    public static void main(String[] args) throws Exception {
> -        new ModuleImportProcessingTest().test();
> +    Path[] findJavaFiles(Path... paths) throws Exception {
> +        return tb.findJavaFiles(paths);
>      }
>  
> -    public void test() throws Exception {
> -        tb.writeJavaFiles(base, processedSource);
> +    public static void main(String... args) throws Exception {
> +        new ModuleImportProcessingTest().runTests();
> +    }
> +
> +    @Test
> +    public void test(Path base) throws Exception {
> +        Path src = base.resolve("src");
> +        tb.writeJavaFiles(src,
> +                """
> +                import module java.base;
> +                import java.lang.annotation.*;
> +                public class Main {
> +                    public static void main(String[] args) {
> +                        List.of();
> +                    }
> +                    @Ann
> +                    private void test() {}
> +
> +                    @Retention(RetentionPolicy.RUNTIME)
> +                    @Target(ElementType.METHOD)
> +                    public @interface Ann {}
> +                }
> +                """);
>          new toolbox.JavacTask(tb)
>                  .options(
>                          "-processor", AP.class.getName(),
> @@ -73,24 +91,15 @@ public void test() throws Exception {
>                          "-source", Integer.toString(Runtime.version().feature()),
>                          "-proc:only"
>                  )
> -                .outdir(base.toString())
> -                .files(base.resolve("Main.java"))
> -                .run(Task.Expect.SUCCESS)
> -                .writeAll();
> +                .files(findJavaFiles(src))
> +                .run();
>      }
>  
>      @SupportedAnnotationTypes("*")
>      public static final class AP extends AbstractProcessor {
> -
>          @Override
>          public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment roundEnv) {
>              return false;
>          }
> -
> -        @Override
> -        public SourceVersion getSupportedSourceVersion() {
> -            return SourceVersion.latest();
> -        }
> -
>      }
> -}
> \ No newline at end of file
> +}
> ```

Thanks for suggestions. I have sliglty changed the test code you have provided. I saw that people use base.resolve("src") for source file path, but as I understand, the context of base is flushed after each test so effectively it doesnt make much of a difference but adds some noise in the code making harder to distinguish the flow of test. Same goes for test source string. I thought that this way test may be more readable.

As for TreeTranslator, I also have thought about adding visit method in this issue, but I figured that it doesnt really relate to the issue title so this change could kind of subtle. Still, If you think it should be fixed in the same issue, than I dont see anything against it as long as such expromt changes are usual there. Maybe its worth renaming the issue to something like "Some visitors dont override visitModuleImport resulting in AssertionError" though.

I also guess maybe if we add visitModuleImport for TreeTranslator here then we should also add a test for it just in case, but I didnt find mockito in jtreg suite so im not sure that it is possible to verify how many times exactly method has been called and writing full translation test just to verify simple 2-line methods works correctly seems like overkill. (I didnt even find any tests for TreeTranslator, seems like its not covered by tests at all)

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

PR Comment: https://git.openjdk.org/jdk/pull/19292#issuecomment-2130793688