8332497: javac prints an AssertionError when annotation processing runs on program with module imports #19292

Evemose · 2024-05-19T00:18:40Z

Fix is pretty simple: visitModuleImport in com.sun.tools.javac.tree.TreeScanner has notbeen overriden, so defaulted to Visitor::visitModuleImport, which forwards to Visitor::visitTree, which is also not overriden, and, therefore, threw AssertionError.

PS: Im not even sure how it worked before without crashing, seems like there is some intermidiate implementation between this TreeScanner and actual scanners because otherwise it should have resultedin compile error the moment it encounter module importin any visitor

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

Warning

⚠️ Found leading lowercase letter in issue title for 8332497: javac prints an AssertionError when annotation processing runs on program with module imports

Issue

JDK-8332497: javac prints an AssertionError when annotation processing runs on program with module imports (Bug - P3)

Reviewers

Chen Liang (@liach - Author)
Vicente Romero (@vicente-romero-oracle - Reviewer)
Jan Lahoda (@lahodaj - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19292

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-19T00:19:14Z

👋 Welcome back Evemose! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-05-19T00:19:55Z

@Evemose 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:

8332497: javac prints an AssertionError when annotation processing runs on program with module imports

Reviewed-by: liach, vromero, jlahoda

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 86 new commits pushed to the master branch:

08d5100: 8332724: x86 MacroAssembler may over-align code
97ee2ff: 8332416: Add more font selection options to Font2DTest
985b9ce: 8330694: Rename 'HeapRegion' to 'G1HeapRegion'
05f13e7: 8329667: [macos] Issue with JTree related fix for JDK-8317771
7bf1989: 8320575: generic type information lost on mandated parameters of record's compact constructors
253508b: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
ebc520e: 8332841: GenShen: Pull shared members from control thread into common base class
236432d: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook
b3b3366: 8332631: Update nsk.share.jpda.BindServer to don't use finalization
f66a586: 8332641: Update nsk.share.jpda.Jdb to don't use finalization
... and 76 more: https://git.openjdk.org/jdk/compare/b92bd671835c37cff58e2cdcecd0fe4277557d7f...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@vicente-romero-oracle, @lahodaj) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-05-19T00:20:08Z

@Evemose The following label will be automatically applied to this pull request:

compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-05-19T11:42:22Z

Webrevs

liach · 2024-05-19T12:19:25Z

I recommend adding a minimal test case for this bug, like the one from the JBS issue.

Evemose · 2024-05-19T21:13:23Z

@liach been done. It's my first time writing javac tests so I am not sure I have done everything in the best way, but I've tried my best

lahodaj · 2024-05-20T07:38:38Z

The test is not wrong, but could be made shorter like here:
https://github.com/lahodaj/jdk/blob/8e4c9f4091f443766e5f009317661260154a33a8/test/langtools/tools/javac/ImportModule.java#L750

Also, the similar com.sun.tools.javac.tree.TreeTranslator could also be made consistent and handle module imports. Although I doubt there is an end-user way to see the difference (I am not aware about any whole-AST translation in javac; only per-class translations; for the internal scanner, it is also more common to scan only classes, I think).

Evemose · 2024-05-20T07:49:05Z

The test is not wrong, but could be made shorter like here: https://github.com/lahodaj/jdk/blob/8e4c9f4091f443766e5f009317661260154a33a8/test/langtools/tools/javac/ImportModule.java#L750

Also, the similar com.sun.tools.javac.tree.TreeTranslator could also be made consistent and handle module imports. Although I doubt there is an end-user way to see the difference (I am not aware about any whole-AST translation in javac; only per-class translations; for the internal scanner, it is also more common to scan only classes, I think).

Thanks for suggestion. Is it ok to just out bug-related test into some pre-existing test class? I just saw that issue-related tests are all in separate folders so figured its how it should be. Also will annotaion processing run if there is no annotated elemnts at all? If i remeber correctly, it will not.

Also im not really sure why it fails in ci workflow cause on my local machine test passes. Im not sure where is the issue, so I guess I will just use code you have provided instead

PS: i found the issue. For some reason, when I ran in local, tests did not require --enable-preview, but in ci it does

liach · 2024-05-21T03:20:56Z

Is it ok to just out bug-related test into some pre-existing test class? I just saw that issue-related tests are all in separate folders so figured its how it should be.

Yes. If you look at git blame (available from Intellij Idea annotations) you will find those Txxxxxxxx tests are from long ago; they are legacy and no longer encouraged in newer tests, which usually group by their subjects.

test/langtools/tools/javac/annotations/ModuleImportProcessingTest.java

tstuefe · 2024-05-22T11:12:26Z

Drive-by comment: I was curious about the "crashes" in the bug description, but this is no crash. It is an AssertionError. Could we please improve JBS issue title and PR title to say that?

Evemose · 2024-05-22T11:35:19Z

Drive-by comment: I was curious about the "crashes" in the bug description, but this is no crash. It is an AssertionError. Could we please improve JBS issue title and PR title to say that?

I would be happy to, but I am not the one who created the issue. I filed mine report too, but I guess it has been discarded as duplicate. I was just the one to do the fix. @lahodaj I guess this request should be addressed to you

lahodaj · 2024-05-22T11:41:53Z

I've changed the issue title to: "javac prints an AssertionError when annotation processing runs on program with module imports".

OTOH, not sure what's wrong with "crash". It crashes (the program stops working) with an exception.

tstuefe · 2024-05-22T11:42:44Z

I've changed the issue title to: "javac prints an AssertionError when annotation processing runs on program with module imports".

OTOH, not sure what's wrong with "crash". It crashes (the program stops working) with an exception.

Maybe its just me. When I read "crash", I think segfault, or core dump. A javac core'ing would be interesting, that's why I looked.

test/langtools/tools/javac/processing/ModuleImportProcessingTest.java

Evemose · 2024-05-24T18:22:23Z

@vicente-romero-oracle I have addressed your review comments in last commit. Hope I got everything correctly and now everything is fine!

vicente-romero-oracle · 2024-05-24T21:31:45Z

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
+}

Evemose · 2024-05-25T05:35:03Z

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)

test/langtools/tools/javac/processing/ModuleImportProcessingTest.java

vicente-romero-oracle

lgtm

vicente-romero-oracle · 2024-05-25T10:06:50Z

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)

yes we don't have tests for TreeTranslator, at least not targeting it explicitly, so it is OK not to provide one now. I think the title of the bug is OK as it is

Evemose · 2024-05-25T11:01:08Z

/integrate

openjdk · 2024-05-25T11:01:32Z

@Evemose
Your change (at version ba360ec) is now ready to be sponsored by a Committer.

lahodaj

Looks good to me. Thanks for working on this!

I am running tests now.

@Evemose, please note you'll need to type:

/integrate

in a separate comment, so that a committer can sponsor.

Thanks!

openjdk · 2024-05-27T05:57:08Z

@lahodaj Only the author (@Evemose) is allowed to issue the integrate command. As this pull request is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the /sponsor command?

Evemose · 2024-05-27T06:10:24Z

Looks good to me. Thanks for working on this!

I am running tests now.

@Evemose, please note you'll need to type:
/integrate
in a separate comment, so that a committer can sponsor.

Thanks!

Hi!
I have already typed integrate a bit earlier in the conversation (#19292 (comment)), so I am waiting for sponsor for now

lahodaj · 2024-05-27T06:13:16Z

Hi! I have already typed integrate a bit earlier in the conversation (#19292 (comment)), so I am waiting for sponsor for now

Right. Sorry, I missed the label.

lahodaj · 2024-05-27T08:54:06Z

/sponsor

openjdk · 2024-05-27T08:54:22Z

Going to push as commit 617edf3.
Since your change was applied there have been 92 commits pushed to the master branch:

ffa4bad: 8332527: ZGC: generalize object cloning logic
a3a367e: 8332871: Parallel: Remove public bits APIs in ParMarkBitMap
61db2f5: 8079167: Fix documentation for G1SATBBufferEnqueueingThresholdPercent == 0
a083364: 8321292: SerialGC: NewSize vs InitialHeapSize check has an off-by-one error
16dba04: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null
4e8deb3: 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist
08d5100: 8332724: x86 MacroAssembler may over-align code
97ee2ff: 8332416: Add more font selection options to Font2DTest
985b9ce: 8330694: Rename 'HeapRegion' to 'G1HeapRegion'
05f13e7: 8329667: [macos] Issue with JTree related fix for JDK-8317771
... and 82 more: https://git.openjdk.org/jdk/compare/b92bd671835c37cff58e2cdcecd0fe4277557d7f...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-27T08:54:28Z

@lahodaj @Evemose Pushed as commit 617edf3.

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

Add visitModuleImport override to com.sun.tools.javac.tree.TreeScanner

9c7796a

openjdk bot added the compiler compiler-dev@openjdk.org label May 19, 2024

Evemose changed the title ~~Fix issue described in mail to compiler dev with failing compilation in APT (will change title when issue in JBD published)~~ Fix issue described in mail to compiler dev with failing compilation in APT (will change title when issue in JBD is published) May 19, 2024

Evemose changed the title ~~Fix issue described in mail to compiler dev with failing compilation in APT (will change title when issue in JBD is published)~~ Fix issue described in mail to compiler dev with failing compilation in APT (will change title when issue in JBS is published) May 19, 2024

Evemose changed the title ~~Fix issue described in mail to compiler dev with failing compilation in APT (will change title when issue in JBS is published)~~ 8332497: javac crashes when annotation processing runs on program with module imports May 19, 2024

openjdk bot added the rfr Pull request is ready for review label May 19, 2024

added testcase

08bce56

openjdk bot removed the rfr Pull request is ready for review label May 19, 2024

remove ^M chars in test

f15c446

openjdk bot added the rfr Pull request is ready for review label May 19, 2024

Added getSupportedSourceVersion() to AP

c6defcc

Evemose added 2 commits May 20, 2024 11:05

Added --enable-preview param

7b554e0

Added --source param and moved annotation into Main class

b642af2

Added legal header and renamed test

a38583a

jddarcy reviewed May 21, 2024

View reviewed changes

test/langtools/tools/javac/annotations/ModuleImportProcessingTest.java Outdated Show resolved Hide resolved

Converted test to extend JavacTestingAbstractProcessor

a129ec0

Evemose requested a review from jddarcy May 22, 2024 11:32

vicente-romero-oracle reviewed May 24, 2024

View reviewed changes

test/langtools/tools/javac/processing/ModuleImportProcessingTest.java Outdated Show resolved Hide resolved

Addressed review comments

a1eb82a

Evemose requested a review from vicente-romero-oracle May 24, 2024 18:22

Evemose added 2 commits May 25, 2024 08:29

Overridden visitModuleImport in TreeTranslator

4084375

Refactored test to use TestRunner

da9095d

liach reviewed May 25, 2024

View reviewed changes

test/langtools/tools/javac/processing/ModuleImportProcessingTest.java Outdated Show resolved Hide resolved

test/langtools/tools/javac/processing/ModuleImportProcessingTest.java Outdated Show resolved Hide resolved

Evemose added 2 commits May 25, 2024 08:47

Addressed review

4846e8f

Addressed review (2)

ba360ec

vicente-romero-oracle approved these changes May 25, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 25, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label May 25, 2024

liach approved these changes May 25, 2024

View reviewed changes

lahodaj approved these changes May 27, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 27, 2024

openjdk bot closed this May 27, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8332497: javac prints an AssertionError when annotation processing runs on program with module imports #19292

8332497: javac prints an AssertionError when annotation processing runs on program with module imports #19292

Evemose commented May 19, 2024 •

edited by openjdk bot

bridgekeeper bot commented May 19, 2024

openjdk bot commented May 19, 2024 •

edited

openjdk bot commented May 19, 2024

mlbridge bot commented May 19, 2024 •

edited

liach commented May 19, 2024

Evemose commented May 19, 2024 •

edited

lahodaj commented May 20, 2024

Evemose commented May 20, 2024 •

edited

liach commented May 21, 2024

tstuefe commented May 22, 2024

Evemose commented May 22, 2024 •

edited

lahodaj commented May 22, 2024

tstuefe commented May 22, 2024

Evemose commented May 24, 2024

vicente-romero-oracle commented May 24, 2024

Evemose commented May 25, 2024 •

edited

vicente-romero-oracle left a comment

vicente-romero-oracle commented May 25, 2024

Evemose commented May 25, 2024

openjdk bot commented May 25, 2024

lahodaj left a comment

openjdk bot commented May 27, 2024

Evemose commented May 27, 2024

lahodaj commented May 27, 2024

lahodaj commented May 27, 2024

openjdk bot commented May 27, 2024

openjdk bot commented May 27, 2024

8332497: javac prints an AssertionError when annotation processing runs on program with module imports #19292

8332497: javac prints an AssertionError when annotation processing runs on program with module imports #19292

Conversation

Evemose commented May 19, 2024 • edited by openjdk bot

Progress

Warning

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 19, 2024

openjdk bot commented May 19, 2024 • edited

openjdk bot commented May 19, 2024

mlbridge bot commented May 19, 2024 • edited

Webrevs

liach commented May 19, 2024

Evemose commented May 19, 2024 • edited

lahodaj commented May 20, 2024

Evemose commented May 20, 2024 • edited

liach commented May 21, 2024

tstuefe commented May 22, 2024

Evemose commented May 22, 2024 • edited

lahodaj commented May 22, 2024

tstuefe commented May 22, 2024

Evemose commented May 24, 2024

vicente-romero-oracle commented May 24, 2024

Evemose commented May 25, 2024 • edited

vicente-romero-oracle left a comment

Choose a reason for hiding this comment

vicente-romero-oracle commented May 25, 2024

Evemose commented May 25, 2024

openjdk bot commented May 25, 2024

lahodaj left a comment

Choose a reason for hiding this comment

openjdk bot commented May 27, 2024

Evemose commented May 27, 2024

lahodaj commented May 27, 2024

lahodaj commented May 27, 2024

openjdk bot commented May 27, 2024

openjdk bot commented May 27, 2024

Evemose commented May 19, 2024 •

edited by openjdk bot

openjdk bot commented May 19, 2024 •

edited

mlbridge bot commented May 19, 2024 •

edited

Evemose commented May 19, 2024 •

edited

Evemose commented May 20, 2024 •

edited

Evemose commented May 22, 2024 •

edited

Evemose commented May 25, 2024 •

edited