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

Add NoGuavaByteStreams Refaster recipe using Java 9+ #399

Closed
wants to merge 9 commits into from

Conversation

timtebeek
Copy link
Contributor

@timtebeek timtebeek commented Jan 21, 2024

What's your motivation?

Anything in particular you'd like reviewers to focus on?

Added a new source set for that Java 9+ classes needed for these replacements;
The input refaster templates should not make it into the jar, but the generated recipes should.

Any additional context

Possible requires this PR such that there's no recipes produced with lambdas using Java 9+

@timtebeek
Copy link
Contributor Author

We currently get mismatches in the recipe execution because the ByteStreams class is attributed to error_prone_core in the find and replace templates we generate.
image

@timtebeek
Copy link
Contributor Author

Still to do:

  • Verify Jar is usable from Java 8 when running org.openrewrite.java.migrate.guava.NoGuava
  • Optionally check if usable ffrom Java 8 for org.openrewrite.java.migrate.guava.NoGuavaByteStreamsRecipes

@@ -16,7 +16,7 @@ dependencies {
testImplementation("org.projectlombok:lombok:latest.release")

annotationProcessor("org.openrewrite:rewrite-templating:$rewriteVersion")
compileOnly("com.google.errorprone:error_prone_core:2.19.1:with-dependencies") {
compileOnly("com.google.errorprone:error_prone_core:2.19.1") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise the recipe will pick error_prone_support as the lib that provides the Guava classes, and put that in the generated template classpath leading to mismatches.

@timtebeek
Copy link
Contributor Author

timtebeek commented Feb 2, 2024

Right now generated recipes use a lambda containing the input and output code, which is used to lookup the associated before/after template classes. But when the before or after template uses code that can only run on Java 11+ that might not work on Java 8 anymore.

As an example; the generated build/generated/sources/annotationProcessor/java/refaster/org/openrewrite/java/migrate/guava/NoGuavaByteStreamsRecipes.java contains the Java 9+ transferTo method.

final JavaTemplate before = Semantics.expression(this, "before", (java.io.InputStream in, java.io.OutputStream out) -> com.google.common.io.ByteStreams.copy(in, out)).build();
final JavaTemplate after = Semantics.expression(this, "after", (java.io.InputStream in, java.io.OutputStream out) -> in.transferTo(out)).build();

There's some rough work in rewrite-templating to no longer use those lamdbas for lookup, but instead inline the generated JavaTemplates such that it all compiles on Java 8; that likely needs to land first before we can merge this PR.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek
Copy link
Contributor Author

Still a minor challenge to solve with the generated classes now compiled using Java 17:

tim@tim-xps-15-9520:~/openrewrite/rewrite-migrate-java$ javap -v build/classes/java/refaster/org/openrewrite/java/migrate/guava/NoGuavaByteStreams.class | head
Classfile /home/tim/Documents/workspace/openrewrite/rewrite-migrate-java/build/classes/java/refaster/org/openrewrite/java/migrate/guava/NoGuavaByteStreams.class
  Last modified Mar 4, 2024; size 858 bytes
  SHA-256 checksum 186250a0b6a092ef5d14ac6ba833fb1a2bfbc711725d1cf722e8e3ee3ba5182e
  Compiled from "NoGuavaByteStreams.java"
public class org.openrewrite.java.migrate.guava.NoGuavaByteStreams
  minor version: 0
  major version: 61
  flags: (0x0021) ACC_PUBLIC, ACC_SUPER
  this_class: #7                          // org/openrewrite/java/migrate/guava/NoGuavaByteStreams
  super_class: #2                         // java/lang/Object

tim@tim-xps-15-9520:~/openrewrite/rewrite-migrate-java$ javap -v build/classes/java/refaster/org/openrewrite/java/migrate/guava/NoGuavaByteStreamsRecipes.class | head
Classfile /home/tim/Documents/workspace/openrewrite/rewrite-migrate-java/build/classes/java/refaster/org/openrewrite/java/migrate/guava/NoGuavaByteStreamsRecipes.class
  Last modified Mar 4, 2024; size 1684 bytes
  SHA-256 checksum 9885851bc1d92a4a04560b72168602cde0757747098d451a7c71958e88c1b487
  Compiled from "NoGuavaByteStreamsRecipes.java"
public class org.openrewrite.java.migrate.guava.NoGuavaByteStreamsRecipes extends org.openrewrite.Recipe
  minor version: 0
  major version: 61
  flags: (0x0021) ACC_PUBLIC, ACC_SUPER
  this_class: #31                         // org/openrewrite/java/migrate/guava/NoGuavaByteStreamsRecipes
  super_class: #2                         // org/openrewrite/Recipe

At the very least we want the generated sources to be compiled with Java 8, now that there's no references to Java 9+ classes in there.

@timtebeek
Copy link
Contributor Author

Predictably adding something like this then breaks on the Java 9+ constructs

tasks.named<JavaCompile>("compileRefasterJava").configure {
    options.release = 8
}

But it's not immediately clear to me how to use Java 17 only for the RefasterTemplateProcessor annotation processor, and then use Java 8 for the generated sources. 🤔

@knutwannheden
Copy link
Contributor

Can we use source and target?

@Bananeweizen
Copy link
Contributor

Bananeweizen commented Mar 4, 2024

But it's not immediately clear to me how to use Java 17 only for the RefasterTemplateProcessor annotation processor, and then use Java 8 for the generated sources. 🤔

To me the question is rather why it's an annotation processor at all (except for historical reasons in how refaster templates have evolved). If it were just normal "application code" being called from the maven or gradle plugin, it would still be able to scan the classpath and to find all the annotated classes and then to process them. Being called at the right time (i.e. before the compilation of the recipes) would just be a matter of the maven and gradle plugin attaching to the right phases. And without the annotation processor, there would be no overlap in what levels of Java are needed for compilation of generated code. Compilation of everything could happen with 8, just the JVM running this would need to be 17 to use the template processor bytecode.

Or am I completely missing something in this picture?

@timtebeek
Copy link
Contributor Author

Closing this PR for now, as we're unlikely to figure out a way to include recipes generated for Refaster templates that require Java 9+

@timtebeek timtebeek closed this Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support ByteStreams.toByteArray -> inputStream.readAllBytes()
3 participants