rewrite-migrate-java icon indicating copy to clipboard operation
rewrite-migrate-java copied to clipboard

Add NoGuavaByteStreams Refaster recipe using Java 9+

Open timtebeek opened this issue 1 year ago • 7 comments

What's your motivation?

Fixes https://github.com/openrewrite/rewrite-migrate-java/issues/390

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+

  • https://github.com/openrewrite/rewrite-templating/pull/57
  • https://github.com/openrewrite/rewrite-migrate-java/issues/390
  • https://github.com/PicnicSupermarket/error-prone-support/pull/963

timtebeek avatar Jan 21 '24 15:01 timtebeek

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 avatar Jan 21 '24 19:01 timtebeek

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

timtebeek avatar Jan 21 '24 21:01 timtebeek

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.

  • https://github.com/openrewrite/rewrite-templating/pull/57

timtebeek avatar Feb 02 '24 10:02 timtebeek

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 avatar Mar 04 '24 09:03 timtebeek

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. 🤔

timtebeek avatar Mar 04 '24 10:03 timtebeek

Can we use source and target?

knutwannheden avatar Mar 04 '24 12:03 knutwannheden

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?

Bananeweizen avatar Mar 04 '24 16:03 Bananeweizen

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 avatar Jul 28 '24 21:07 timtebeek