jdk
jdk copied to clipboard
openjdk
8332497: javac crashes when annotation processing runs on program with module imports
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)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Warning
⚠️ Found leading lowercase letter in issue title for 8332497: javac crashes when annotation processing runs on program with module imports
Issue
- JDK-8332497: javac crashes when annotation processing runs on program with module imports (Bug - P3)
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
:wave: 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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:
- 08d51003d142e89b9d2f66187a4ea50e12b94fbb: 8332724: x86 MacroAssembler may over-align code
- 97ee2ffb89257a37a178b70c8fee96a1d831deb6: 8332416: Add more font selection options to Font2DTest
- 985b9ce79a2d620a8b8675d1ae6c9730d72a757f: 8330694: Rename 'HeapRegion' to 'G1HeapRegion'
- 05f13e75ee4407ba9213c69b33c6032aa87c9e95: 8329667: [macos] Issue with JTree related fix for JDK-8317771
- 7bf1989f59695c3d08b4bd116fb4c022cf9661f4: 8320575: generic type information lost on mandated parameters of record's compact constructors
- 253508b03a3de4dab00ed7fb57e9f345d8aed1a4: 8332303: Better JMX interoperability with older JDKs, after removing Subject Delegation
- ebc520e83f503eeb4e5af6d5aef62df9227af4f7: 8332841: GenShen: Pull shared members from control thread into common base class
- 236432dbdb9bab4aece54c2fea08f055e5dbf97e: 8332084: Ensure JdkConsoleImpl.restoreEcho visibility in a shutdown hook
- b3b33667ad3bdb7be868fb165a1ea53054947cd0: 8332631: Update nsk.share.jpda.BindServer to don't use finalization
- f66a58661459bf64212ec332540c12d5d691270f: 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).
@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.
Webrevs
- 18: Full - Incremental (ba360ec5)
- 17: Full - Incremental (da9095d1)
- 16: Full - Incremental (a1eb82a2)
- 15: Full - Incremental (3e4ec686)
- 14: Full - Incremental (cf813cc4)
- 13: Full - Incremental (64280b42)
- 12: Full - Incremental (0cfe4ac8)
- 11: Full - Incremental (7bc8b3ec)
- 10: Full - Incremental (c6293004)
- 09: Full - Incremental (1a07801e)
- 08: Full - Incremental (c066d841)
- 07: Full - Incremental (a129ec0b)
- 06: Full - Incremental (a38583af)
- 05: Full - Incremental (b642af26)
- 04: Full - Incremental (7b554e0f)
- 03: Full - Incremental (c6defccc)
- 02: Full - Incremental (f15c446c)
- 01: Full - Incremental (08bce566)
- 00: Full (9c7796ad)
I recommend adding a minimal test case for this bug, like the one from the JBS issue.
@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
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).
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.TreeTranslatorcould 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
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.
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?
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
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.
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.
I think most Java programmers call a program crash with a stacktrace a "crash" and hs_err_pids as a "VM crash".
what I figured is that -J-Dtest.enable.preview=true option is added to @compile/process. But even when --enable-preview --source ${jdk.version} is explictily specified, and is present in command line, for some reason, java.lang.UnsupportedClassVersionError: Preview features are not enabled for ModuleImportProcessingTest (class file version 67.65535). Try running with '--enable-preview' si still thrown. I guess it is some sor of a bug in a jtreg, I will file it later. For now, I will rollback to toolbox-based test, because it, at least, works fine.
I guess the test takes unreasonably more time then trivial fix itself, so its kind of time to stop on something that at least works. If I some time later come up with more sophisticated solution, I'll make sure to revisit this code
PS: I noticed IDE added package statement at the beginning of the file, but I removed it and issue is still present
PPS: Also, as I figured, the fact that code parses fine in compile/process means that --enable-preview is recognized, but for some reason is ignored when classloader loads the processor
⚠️ @Evemose This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).
Do I have to somehow request review or just wait untill someone eventually does it?
Do I have to somehow request review or just wait untill someone eventually does it?
I think @lahodaj would review as Jan's assigned to the JBS issue. This patch is ready to review so let's wait for him to review and sponsor (process might lapse to next Monday)
@vicente-romero-oracle I have addressed your review comments in last commit. Hope I got everything correctly and now everything is fine!
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
+}
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)
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 Your change (at version ba360ec5d6e9b25bac7e58316d09de6505dfa7cf) is now ready to be sponsored by a Committer.
@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?
Looks good to me. Thanks for working on this!
I am running tests now.
@Evemose, please note you'll need to type:
/integratein a separate comment, so that a committer can sponsor.
Thanks!
Hi! I have already typed integrate a bit earlier in the conversation (https://github.com/openjdk/jdk/pull/19292#issuecomment-2131213489), so I am waiting for sponsor for now
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.
Going to push as commit 617edf3f0dea2b73e4b444e085de2ad282826e31.
Since your change was applied there have been 92 commits pushed to the master branch:
- ffa4badb78118d154e47e41073e467c0e0e4273c: 8332527: ZGC: generalize object cloning logic
- a3a367ef5d6c462ebca40104d05c11219e84a64f: 8332871: Parallel: Remove public bits APIs in ParMarkBitMap
- 61db2f5b90cd40ce104cb55bf9fd52d6e141161d: 8079167: Fix documentation for G1SATBBufferEnqueueingThresholdPercent == 0
- a083364520ab75cb5596f103b2fa51d7f7a8a706: 8321292: SerialGC: NewSize vs InitialHeapSize check has an off-by-one error
- 16dba04e8dfa871f8056480a42a9baeb24a2fb24: 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
- 4e8deb396e38c69de22b6348dca637d814d73aef: 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist
- 08d51003d142e89b9d2f66187a4ea50e12b94fbb: 8332724: x86 MacroAssembler may over-align code
- 97ee2ffb89257a37a178b70c8fee96a1d831deb6: 8332416: Add more font selection options to Font2DTest
- 985b9ce79a2d620a8b8675d1ae6c9730d72a757f: 8330694: Rename 'HeapRegion' to 'G1HeapRegion'
- 05f13e75ee4407ba9213c69b33c6032aa87c9e95: 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.
@lahodaj @Evemose Pushed as commit 617edf3f0dea2b73e4b444e085de2ad282826e31.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.