Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue parsing string with Java8ReloadableParser #4914

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

rlsanders4
Copy link
Contributor

@rlsanders4 rlsanders4 commented Jan 16, 2025

What's changed?

Revert the Java8ReloadableParser to use JavaCompiler.parse() instead of JavaCompiler.parseFiles() to support parsing strings as input. This issue does not occur with other versions of the Java parser.

What's your motivation?

Recent changes to Java8ReloadableParser caused the following error in our custom recipes:

SEVERE: java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
	at com.sun.tools.javac.util.List.get(List.java:476)
	at org.openrewrite.java.ReloadableJava8Parser.parseInputsToCompilerAst(ReloadableJava8Parser.java:260)
	at org.openrewrite.java.ReloadableJava8Parser.parseInputs(ReloadableJava8Parser.java:211)
	at org.openrewrite.java.Java8Parser.parseInputs(Java8Parser.java:44)
	at org.openrewrite.java.JavaParser.parse(JavaParser.java:285)
	at org.openrewrite.java.JavaParser.parse(JavaParser.java:300)

This occurs when we attempt to parse a file from a string input:

                        Path targetFile = Paths.get("/path/to/file.java")

                        Stream<SourceFile> sourceJavaFile = JavaParser.fromJavaVersion()
                                .build()
                                .parse(new String(Files.readAllBytes(targetFile)));

Anyone you would like to review specifically?

@timtebeek

Any additional context

I would like to add a unit test for this, but I have not been able to figure out how to make calls to JavaParser.fromJavaVersion().build().parse() using Java 8 within the unit testing framework. Any assistance with this would be appreciated. I have confirmed this change fixes our recipes through end-to-end testing.

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@rlsanders4 rlsanders4 requested a review from timtebeek January 16, 2025 21:29
@rlsanders4 rlsanders4 added bug Something isn't working parser-java labels Jan 16, 2025
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok from my end when compared to the line removed previously. I wouldn't necessarily know how to best test this, but I would be fine merging this without another test.
/cc @knutwannheden in case I missed anything

@rlsanders4 rlsanders4 marked this pull request as ready for review January 16, 2025 22:45
@knutwannheden
Copy link
Contributor

I added a test case to rewrite-java-tck which I think should have been able to reproduce the issue as this will be executed for rewrite-java-8 which uses a Java 1.8 VM. I was not able to reproduce the issue, but I don't see any harm with this PR either. Possibly we can get the test case working at some point.

@knutwannheden knutwannheden merged commit c1b258e into openrewrite:main Jan 17, 2025
2 checks passed
@rlsanders4 rlsanders4 deleted the java8 branch January 17, 2025 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working parser-java
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants