Create builders for all recipes (especially lombok ones)

[crankydillo]

#4480 ultimately roots in using constructors. I think since lombok is used so heavily it might not be glaringly obvious when a breaking change is created. While we try to use declarative recipes as much as possible, we still need to instantiate some in the code. However, we are concerned about doing that and having breaking changes come in (just by adding a new @Nullable field).

WDYT about adding builders for all the recipes? At least when easy. Can't you just add @Builder on the recipe when lombok is involved?

So instead of doing new SomeRecipe("a", 1234, null), I can do SomeRecipe.Builder.newBuilder().string("a").num(123).build()? That would make our code nicer and hopefully shield us from field changes that could be backward compatible.

[timtebeek]

Thanks for the suggestion @crankydillo . I do understand the frustration when a new nullable argument is added that you might need to update constructors. Internally we tend to use AddNullMethodArgument to get around this problem when it's particularly widespread, but have so far not provided any recipes for such upgrades.

The suggestion of using a builder is interesting, but I'm somewhat hesitant as it doubles the effective exposed API, which would then also have to be supported going forward. Additionally there is some serialization involved with recipes, and loading them from declarative yaml files, which means there's potential for this to trip up in unexpected ways.

We've relatively recently added the option to build a recipe list, which also hints at potentially using builders in the future:

rewrite/rewrite-core/src/test/java/org/openrewrite/RecipeListTest.java

Lines 86 to 93 in dd67fe6

	public void buildRecipeList(RecipeList list) {
	list
	// TODO would these large option-set recipes
	// benefit from builders?
	.recipe(new FindAndReplace(
	"hi", "hello", null, false, null,
	null, null, null)
	)

For now we'll aim to have few such breaking changes, until we figure out the best path forward. The buildRecipeList above would be another reason to do so.

[jkschneider]

I agree that we essentially needs @crankydillo's suggestion because of buildRecipeList, and I imagined us going in this direction when we had the chance to do so. It wouldn't have to be on absolutely all recipes, but I think we could take a crack a recipe that adds Builder to every recipe that contains a mixture of required and optional options.

@Override
public void buildRecipeList(RecipeList list) {
    list.recipe(new ChangeMethodName(
            "java.util.List#add(..)", "plus",
            null, null));
}

Would be much better as:

@Override
public void buildRecipeList(RecipeList list) {
    list.recipe(ChangeMethodName.builder("java.util.List#add(..)", "plus") // required args only
         .matchOverrides(true)                                             // when optional args needed
    ); 
}