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

Verify equality of template signatures #108

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Bananeweizen
Copy link
Contributor

@Bananeweizen Bananeweizen commented Aug 26, 2024

Make sure that all templates belonging to a recipe have the same number, order and type for all the method arguments. This avoids accidental copy/paste errors which otherwise can lead to compiling recipe code, but wrong results at runtime.

Checklist

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

Make sure that all templates belonging to a recipe have the same number, order and type for all the method arguments. This avoids accidental copy/paste errors which otherwise can lead to compiling recipe code, but wrong results at runtime.

Fixes openrewrite#107.
@Stephan202
Copy link
Contributor

Heads up: this may quite significantly reduce the number of Error Prone Support Refaster rules for which a corresponding OpenRewrite recipe is generated.

Stock Refaster requires that @AfterTemplate method parameter names are also present for each @BeforeTemplate method, but not vice versa, and does not require that the types match. We rely on latter to keep rules compact, while the former is valid in case a rule drops an unused subexpression.

@Bananeweizen
Copy link
Contributor Author

Then the only thing remaining to raise an error for would be after.args.size() > before.args.size(). However, that would have found just 1 out of the 3 or 4 copy/paste errors I made in recent weeks that all motivated me to have the strict check. :/

@Stephan202
Copy link
Contributor

Yeah, that's painful :(. I recognize that it's soo easy to make copy-paste errors when creating a bunch of similar Refaster rules.

This motivated us to write a test framework (where each rule has before- and after-example code), but as-is said framework isn't maximally user friendly, requiring changes in three files (example: PicnicSupermarket/error-prone-support#1290). At some point I looked into creating a "DSL" that would have example code be "nested" inside the relevant Refaster rule. Something to return to, one of these days :)

@knutwannheden
Copy link
Contributor

It seems that we can at least also verify that the parameters of the after templates correspond to a parameter in each before template, where we perform the check by name.

@timtebeek timtebeek added the enhancement New feature or request label Aug 26, 2024
@timtebeek timtebeek marked this pull request as draft October 27, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Mismatch in parameters of before/after not recognized
5 participants