Rewrite 8 migration requirements

[kunli2]

Background of rewrite-8
Background for the impetus for rewrite 8. It is to support large repositories. Before 8, all LST for a repository needed to be loaded into memory and available for LST to operate. For large repos like Shelter’s e-commerce or Block’s franklin, this would not fit into memory. So we needed to redesign everything because the constraint now is that only part of LST is available in memory at a time and you don’t have random access - therefore LST needs to be scanned first to collect all info and then edit LST.

---

Rewrite 7 -> 8 change log highlights

1. All applicable test methods org.openrewrite.Recipe getSingleSourceApplicableTest() and org.openrewrite.Recipe getApplicableTest() methods are removed, introduced new Preconditions APIs as a replacement.
2. The method Recipe visit(List<SourceFile> before, ExecutionContext ctx) that is used to visit multiple sources has been removed in rewrite 8. This method is used to change files, collect information, or generate new files.
   There are 3 phases in the new rewrite 8 API design.
   - (1) Scanning phase
   - (2) generating phase
   - (3) Editing phase
   basically, the Scanning phase doesn't make any code change, but just does information collection, rewrite8 introduces a new ScanningRecipe class to support this, For the generating phase, there is a new method .
   a ScanningRecipe may first scan a repository and study it in one pass over the repository's source files before, in another pass, it decides how to transform the code. New source file generation is part of this type as well, since in almost every case we check that a file doesn't yet exist (and perhaps other conditions) before deciding to generate a file.
3. applicable tests will not be supported for yaml recipes anymore.
4. JavaTemplate upgrade, introduced context free JavaTemplate, @knutwannheden may provide more precise information.
5. All clean up recipes are moved to repository rewrite-static-analysis and the package name is changed from org.openrewrite.java.cleanup to org.openrewrite.staticanalysis
6. Method JavaSourceFile org.openrewrite.java.JavaVisitor.visitJavaSourceFile(JavaSourceFile cu, P p) is removed, we need to use J org.openrewrite.java.TreeVisitor.visit(@Nullable Tree tree, P p) instead.
7. Some other minor changes
   • Methods org.openrewrite.marker.Markers#SearchResult() and org.openrewrite.marker.Markers.searchResult(@Nullable String description are deprecated and removed, use SearchResult.found() instead.
   • method org.openrewrite.Recipe doNext(..) is removed, it needs to change to use scanning recipe if the current recipe doesn't make any change, or just simply replace to use TreeVisitor#doAfterVisit(), and in some unit test, we need to change cod like spec.recipe(X.doNext(Y)) to spec.recipes(X, Y) since spec.recipe() method is removed as well.

---

Comments from Sam (in this thread)

1. Visiting. Scanning/generating/editing
   Recipes whose changes are local to a single source file have to make only minimal changes. Recipe.getVisitor() became public where previously it was protected. Recipes which must query information from multiple different sources now need to be a ScanningRecipe. ScanningRecipe is the replacement for the old Recipe.visit(List<SourceFile>) method which is now gone as the random access it provided is incompatible with efficient operation on large source sets. A ScanningRecipe provides an accumulator and a scanner. An accumulator is a custom data structure defined by the recipe itself which stores whatever information it needs later. A scanner is a visitor which populates the accumulator with data. The accumulator is then available during an optional generation step where new sources may be added to the list, and in the edit phase where the recipe provides a visitor which makes changes to source files as usual.

2. Applicability and Preconditions
   Applicability tests have been supplanted by Preconditions. Recipe no longer exposes a getSingleSourceApplicabilityTest() or getApplicabilityTest() methods. Instead within getVisitor() a recipe author may use Preconditions.check(TreeVisitor check, TreeVisitor visitor) to conditionally apply the visitor only if the check makes a change. This is equivalent semantically to a rewrite 7 single source applicability test, so usually all a recipe written in code has to do is copy the body of the old applicability test method into the first argument of a call to Preconditions.check().
   Recipe.getApplicabilityTest() was rarely used and confusing, one of the reasons for this change, but since it had the property of checking all source files any recipe which requires those semantics will have to be a ScanningRecipe.
   There is unfortunately no way for a yaml recipe to use Preconditions. This is a reduction in functionality for yaml recipes relative to rewrite 7. We will have a way to support Preconditions or something similar eventually.

---

Rewrite 7 -> 8 migration requires below changes

• 1. getVisitor() to public
• 2. single source applicability test to Preconditions
  • Includes potentially changing return type of getVisitor() to TreeVisitor<?, ExecutionContext>, because that is what Preconditions returns
• 3. Add a comment with a link to our migration documentation (to be written) to classes which use non-single source applicability tests
• 4. Add a comment with a link to our migration documentation to classes which override Recipe.visit(List)
• 5. replace removed JavaSourceFile visitJavaSourceFile(JavaSourceFile cu, ExecutionContext ctx) to J visit(@Nullable Tree tree, ExecutionContext ctx)
• 6. Applicability.of() -> Preconditions.of(), and Applicability.and() -> Preconditions.and()
• 7. For YAML recipes using applicability tests I think we should comment the lines out and add a marker / comment which makes it clear that this is no longer supported and requires migrating the recipe to Java code
• 8. Migrate both imperative and declarative recipes referencing the recipes which were moved to rewrite-static-analysis
• 9. The JavaTemplate.Builder#javaParser(Supplier<JavaParser>) method no longer exists. Jonathan created a recipe to migrate it to JavaTemplate.Builder#javaParser(JavaParser.Builder), but I think that recipe needs to be extended a bit, in case the Supplier<JavaParser> is represented by a variable or field
• 10. TreeVisitor.doNext(..) is removed, leave a comment to replace with TreeVisitor.doAfterVisit(..) or follow the instruction.
• 11. For unit test, change spec.recipe(X.doNext(Y)) to spec.recipes(X, Y)
• 12. org.openrewrite.marker.Markers#SearchResult() and org.openrewrite.marker.Markers.searchResult(@Nullable String description are deprecated and removed, use SearchResult.found() instead.
• 13. Gradle ChangeJavaCompatibility was renamed to UpdateJavaCompatibility with slightly different arguments in Feature/gradle/enhance update java compatibility #3238
• 14. handle the conversion from gradle.RemoveGradleDependency (yet to be removed) -> gradle.RemoveDependency (argument order was flipped around to be consistent other than the rename;
• 15. TreeVisitor#doAfterVisit(Recipe) has been removed, left a comment to use doAfterVisit(Recipe.getVisitor()) instead or use "inline" with visit(), doAfterVisit(Recipe) was always more or less shorthand for doAfterVisit(Recipe.getVisitor()), which means it only runs on the current source, not all sources. So there are quite a few places where someone while visiting a java source file tried to use doAfterVisit(Recipe) to affect a dependency update that would never happen. So keep an eye out for mistaken usages of doAfterVisit(Recipe)
• 16. JavaTemplate update, Change X.withTemplate(t, coordinate, params) to t.apply(cursor, coordinate, params) and JavaTemplate.builder(this::getCursor, "xyz") -> JavaTemplate.builder("xyz").contextSensitive(), and put comments to require human review.

[knutwannheden]

@kunli2 One more thing that is coming is the refactoring of the JavaTemplate API. Let me describe it with a few Refaster templates. For the template construction:

class FactoryMethod {
  @BeforeTemplate
  JavaTemplate before(String code, Supplier<Cursor> context) {
    return JavaTemplate.builder(context, code);
  }
  @AfterTemplate
  JavaTemplate after(String code, Supplier<Cursor> context) {
    return JavaTemplate.builder(code).context(context);
  }
}

When applying the template the code changes something like this (this is a pseudo-template which isn't 100% correct):

class TemplateApplication {
  @BeforeTemplate
  J before(TreeVisitor context, J j, JavaTemplate template, JavaCoordinates coordinates, Object... parameters) {
    return j.withTemplate(template, coordinates, parameters);
  }
  @AfterTemplate
  J after(TreeVisitor context, J j, JavaTemplate template, JavaCoordinates coordinates, Object... parameters) {
    return j.withTemplate(template, context.getCursor(), coordinates, parameters);
  }
}

Please also note that the context would typically be the visitor itself in which the template is applied, so instead of context.getCursor() the code would read getCursor() as in this example:

            return literal
                    .withTemplate(
                            JavaTemplate.builder(template).context(this::getCursor).imports(owningType).build(),
                            getCursor(),
                            literal.getCoordinates().replace())
                    .withPrefix(literal.getPrefix());

[kunli2]

@kunli2 One more thing that is coming is the refactoring of the JavaTemplate API. Let me describe it with a few Refaster templates. For the template construction:

class FactoryMethod {
  @BeforeTemplate
  JavaTemplate before(String code, Supplier<Cursor> context) {
    return JavaTemplate.builder(context, code);
  }
  @AfterTemplate
  JavaTemplate after(String code, Supplier<Cursor> context) {
    return JavaTemplate.builder(code).context(context);
  }
}

When applying the template the code changes something like this (this is a pseudo-template which isn't 100% correct):

class TemplateApplication {
  @BeforeTemplate
  J before(TreeVisitor context, J j, JavaTemplate template, JavaCoordinates coordinates, Object... parameters) {
    return j.withTemplate(template, coordinates, parameters);
  }
  @AfterTemplate
  J after(TreeVisitor context, J j, JavaTemplate template, JavaCoordinates coordinates, Object... parameters) {
    return j.withTemplate(template, context.getCursor(), coordinates, parameters);
  }
}

Please also note that the context would typically be the visitor itself in which the template is applied, so instead of context.getCursor() the code would read getCursor() as in this example:

            return literal
                    .withTemplate(
                            JavaTemplate.builder(template).context(this::getCursor).imports(owningType).build(),
                            getCursor(),
                            literal.getCoordinates().replace())
                    .withPrefix(literal.getPrefix());

Yeah, we can have a separate recipe for this change and add it to the yaml recipe

[jsoref]

org.openrewrite.java.spring.boot2.MigrateHibernateContraintsToJavax to org.openrewrite.java.spring.boot2.MigrateHibernateConstraintsToJavax

[shanman190]

Unless somebody objects, it might also be nice to handle the conversion from gradle.RemoveGradleDependency (yet to be removed) -> gradle.RemoveDependency (argument order was flipped around to be consistent other than the rename; the later was accidentally what was used downstream in the recent rewrite-migrate-java changes in YAML form). I don't mind writing up the recipe -- unless somebody beats me to it -- and adding it to be migration to save @kunli2 some time.

[timtebeek]

Point 8 above for the recipes moved to rewrite-static-analysis was also identified in openrewrite/rewrite-gradle-plugin#203 and could be a nice addition to META-INF/rewrite/migrate-rewrite.yml. We'd need to update those values in custom recipes, yaml files, and build plugin configurations.

[kunli2]

Tested recipe MigrateToRewrite8 on this branch of rewrite-static-analysis (a branch before rewrite 8 and still in 7). and got this patch
rewrite.patch

verified and it looks good to me, after addressing all the comments put by the migration recipes. it can pass compile.

[timtebeek]

Unless somebody objects, it might also be nice to handle the conversion from gradle.RemoveGradleDependency (yet to be removed) -> gradle.RemoveDependency (argument order was flipped around to be consistent other than the rename; the later was accidentally what was used downstream in the recent rewrite-migrate-java changes in YAML form). I don't mind writing up the recipe -- unless somebody beats me to it -- and adding it to be migration to save @kunli2 some time.

@shanman190 we now have such a recipe in UpdateMovedRecipe which we already use in src/main/resources/META-INF/rewrite/migrate-rewrite.yml; sounds like we'd need to add one more recipe to that list?

Many thanks to @kunli2 for all the work on this!

[shanman190]

@timtebeek, yeah that should work for this case most definitely.