rewrite icon indicating copy to clipboard operation
rewrite copied to clipboard

Add ChangeTaskToTasksRegister recipe

Open jhl221123 opened this issue 7 months ago • 8 comments

What's changed?

This PR introduces a new Gradle recipe, ChangeTaskToTasksRegister, which changes eager task creations task exampleTask(type: ExampleType) to lazy registrations tasks.register("exampleTask", ExampleType).

What's your motivation?

  • Closes #5333. This change aligns with modern Gradle best practices for improved build performance by utilizing lazy task registration.

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

  • The handling of task names (both identifiers and string literals).
  • The preservation of task type expressions and configuration closures.
  • Whether any logic in the recipe could be further simplified or if any defensive checks are redundant.
  • The test suite for any duplicated test cases or any missing edge cases that should be covered.

Checklist

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

jhl221123 avatar May 25 '25 12:05 jhl221123

Thanks a lot @jhl221123 ! I've tagged @Jenson3210 to also have a look as I'm out much of the coming week. Seems like a good addition! Have you tried this against any projects already?

timtebeek avatar Jun 07 '25 15:06 timtebeek

Not necessary for this current PR, but an optimization from a code reduction standpoint would be to use the a single JavaVisitor because everything reduces down to a J.MethodInvocation. Equally, the GroovyTemplate and KotlinTemplate should work here nicely once a target code template has been selected.

shanman190 avatar Jun 07 '25 16:06 shanman190

Thanks for the feedback @timtebeek, @shanman190! I've pushed the updates you suggested.

Have you tried this against any projects already?

Not yet. If there's anything else I can do on my end, please let me know!

jhl221123 avatar Jun 09 '25 10:06 jhl221123

Hi @Jenson3210, I've updated the implementation based on the discussion and added more comprehensive tests. It should be ready for another review. Thanks!

jhl221123 avatar Jun 30 '25 06:06 jhl221123

As I am on a 3 weeks leave, responses will be a bit late. Feel free to take this further with anyone else if I fail to answer in a timely manner. Great to see the recipe progressing this way. I think, from my perspective, there are 2 things still open (of which I think, based on @shanman190 comment, only the first one is necessary before merging):

  • Printing should/could be avoided.
  • https://github.com/openrewrite/rewrite/pull/5492#issuecomment-2952739041

Jenson3210 avatar Jul 01 '25 11:07 Jenson3210

Locally this test is green, but here OOM. 🤔

Jenson3210 avatar Jul 28 '25 10:07 Jenson3210

flamegraph

This failing test is the only test that is using the static fromRuntimeClasspath method. This method scans entire RuntimeClassPath (250Mb). Together with the GradleParser (classpath taking 200Mb) and the GroovyParser (using 300Mb), I locally see a consistent usage of 850Mb in total.

Reworked the test to use a different recipe scanning part which reduces the need of scanning all other recipes also

Jenson3210 avatar Jul 28 '25 11:07 Jenson3210

Screenshot 2025-07-28 at 13 03 08

Jenson3210 avatar Jul 28 '25 11:07 Jenson3210