Support Error Prone @Matches and @NotMatches on @BeforeTemplate parameters - #188
Support Error Prone @Matches and @NotMatches on @BeforeTemplate parameters#188juherr wants to merge 2 commits into
@Matches and @NotMatches on @BeforeTemplate parameters#188Conversation
Template parameters render as `#{name:any(Type)}`, so the annotations on them
are never part of the generated `JavaTemplate`, and the types those annotations
reference are not needed to parse it. Feeding them to `ClasspathJarNameDetector`
is why it already had to blacklist `rewrite-templating` and `error_prone_core`.
Scan parameter declarations without their annotations instead. Annotations in a
template *body* are still scanned, as `TemplateCodePrinter` extends javac's
`Pretty` and renders those into the template string.
Two generated recipes lose a `jspecify-1` entry that came from `@Nullable` on a
parameter and was never needed to parse the template.
Also hoist the jar name `Pattern` out of `addJarNameFor`, which recompiled it
for every resolved class symbol of every template.
… parameters Refaster rules guarding a parameter with `com.google.errorprone.refaster.annotation.@Matches` or `@NotMatches` were skipped entirely, as both annotations sat in `UNSUPPORTED_ANNOTATIONS`. Those annotations take a `com.google.errorprone.matchers.Matcher`, which operates on javac trees and a `VisitorState`. A generated recipe only ever sees an OpenRewrite LST, so the matcher class named in the annotation cannot be invoked at recipe runtime. `MatcherRegistry` therefore maps each supported Error Prone matcher onto an equivalent `org.openrewrite.java.template.Matcher` shipped here, letting both flavors of the annotations share the existing guard generation. The annotations remain unsupported on methods, which Error Prone also targets. A template naming a matcher without a known equivalent is still skipped, now with a note that names the matcher, since generating the recipe without its guard would silently widen it. The registry is seeded with `IsLambdaExpressionOrMethodReference`, the most used matcher in error-prone-support. Compiling `error-prone-contrib` against this raises the number of generated recipes from 1024 to 1040. Part of openrewrite#47
864d4d3 to
6bde58d
Compare
|
I went through what this actually unblocks upstream: all 17 rule classes on The AssertJ half is a regression fix, not new coverage. Those annotations were added to
Three things that stood out:
|
|
Thanks — that's a much better picture of the impact than I opened with. I checked your three claims and they all hold:
On Worth noting the first is already how this works: a rule whose matcher has no registered equivalent is skipped. So once upstream adds the dedicated void-compatibility matcher the What I'd mainly like to know is whether the design holds up — the registry, and skipping rather than degrading when a matcher is unknown. If it does, I'll carry on with the remaining matchers and the other unsupported annotations from #47. If you'd rather not take this layer on, say so and I'll close it. |
2 participants
Note
This is a draft to open a discussion about the approach, not a finished feature.
What I'd mainly like feedback on is the mechanism: resolving Error Prone matchers to OpenRewrite
equivalents through a registry, and skipping the rule when there is no equivalent. That seemed
easier to judge against a real diff and one working end-to-end example than described in the
abstract, so only a single matcher is mapped here.
If the shape holds up, the remaining ten matchers are mechanical follow-ups. If it doesn't, very
little work is thrown away — and I'd rather find that out now than after porting eleven of them.
Origin
This came out of PicnicSupermarket/error-prone-support#2276, where
AssertJObjectRuleswas rewritingassertThat(null != x)into non-compilingassertThat(null).isNotSameAs(x)under OpenRewrite. One of the fixes suggested there was to guard the same-reference templates so they don't match anullliteral operand — which is exactly what@Matches/@NotMatchesexpress, and what led here. openrewrite/rewrite-testing-frameworks#1067 has since resolved that particular case natively.Building this surfaced a separate and larger consequence of the missing support: a rule carrying either annotation is skipped in full, so it is silently absent from the published
error-prone-contrib:recipesjar. Reported upstream as PicnicSupermarket/error-prone-support#2286.What
Refaster rules that guard a
@BeforeTemplateparameter with Error Prone'scom.google.errorprone.refaster.annotation.@Matches/@NotMatchesare currently skipped entirely: both annotations sit inRefasterTemplateProcessor.UNSUPPORTED_ANNOTATIONS. The last count posted on #47 was@Matches=30,@NotMatches=7.This adds the mechanism to support them, plus the first matcher mapping.
Why it can't just call the Error Prone matcher
@Matchestakes aClass<? extends com.google.errorprone.matchers.Matcher<? super ExpressionTree>>. Those matchers operate on javac trees and aVisitorState; a generated recipe only ever sees an OpenRewrite LST, with no javac compilation in sight. So the matcher class named in the annotation cannot be invoked at recipe runtime.Instead, each supported Error Prone matcher is mapped onto an equivalent
org.openrewrite.java.template.Matchershipped here, and the existing guard-generation path is reused unchanged. The two flavors of the annotations now converge on one code path.How
MatcherRegistry(new, package-private) resolves either flavor of the annotation to the OpenRewrite matcher to instantiate, going through a static Error Prone → OpenRewrite table for the Error Prone flavor.TemplateDescriptor.validate()accepts the Error Prone annotations on parameters when the named matcher has a known equivalent. They stay unsupported on methods, which Error Prone also targets. A template naming an unmapped matcher is still skipped, now with a note that names it:Generating the recipe without the guard would silently widen it, so skipping remains the conservative default.
RecipeWriter.generateTemplateMatchBlockgained no new emission logic — the two near-identical existing branches were folded intoappendMatcherGuard(...), now shared by all four annotations.The registry is seeded with a single entry:
tech.picnic.errorprone.refaster.matchers.IsLambdaExpressionOrMethodReference→org.openrewrite.java.template.matchers.IsLambdaExpressionOrMethodReference.It is the most-used matcher in error-prone-support (20 occurrences, tied for first) and the most trivial to port, which makes it a good proof of the mechanism. The remaining ten are deliberately left for follow-ups so this PR stays reviewable.
Registry values are class names, not class literals, on purpose: this code runs on the annotation processor path, where
rewrite-javais usually absent, so loading aMatcherimplementation there would fail.MatcherRegistryTestcompensates by asserting every mapped name resolves, implementsMatcher, and is instantiable vianew X()— exactly what generated code does.First commit: parameter annotations no longer pollute the template classpath
TemplateCodefed whole parameter declarations toClasspathJarNameDetector, annotations included. That is why the detector already had to blacklistrewrite-templatinganderror_prone_core— the pre-existing@Matchessupport was tripping it. With the Error Prone flavor it got worse:@Matches(IsLambdaExpressionOrMethodReference.class)pullederror_prone_check_apiinto the generatedclasspathFromResources(...).Parameters render as
#{name:any(Type)}, so their annotations are never part of the template.ClasspathJarNameDetector.scanParameter(...)now skips them. The body scan is deliberately left alone:TemplateCodePrinterextends javac'sPretty, so an annotation appearing in a template body is rendered into the template string and does need its jar. A first attempt skipped annotations everywhere and broke that;detectAnnotationInTemplateBodynow pins it down.Side effect: two golden files lose a
jspecify-1entry that came from@Nullableon parameters and was never needed.Drive-by in the same method:
Pattern.compilewas being called per resolved class symbol per template. Hoisted to a constant. Happy to drop that hunk if you'd rather keep it separate.Verification
Beyond the unit tests, this was run end-to-end against error-prone-support (
0.30.1-SNAPSHOT), comparingrewrite-templating1.42.1with this branch:error-prone-contribNewly unblocked rules include
StreamAnyMatch,StreamAllMatchWithPredicate,StreamMapToIntSum,AssertThatThrownBy*andAssertThatCode*, acrossStreamRules,JUnitToAssertJRulesandTestNGToAssertJRules.Warning
Heads-up for the error-prone-support side. Their
generate-recipesexecution runs withfailOnWarning=true. Three of the newly generatedStreamRulesrecipes need a return-type widening guard, which emits the compile warning added in #184, and that fails their build. The generated code itself compiles fine — adding-Arewrite.suppressWarnings=true(also from #184) to theircompilerArgsmakes the build green again. Flagging it so it isn't a surprise on the next bump.Test coverage
refaster/ErrorProneMatching.java→ goldenErrorProneMatchingRecipes.java, covering both polarities.errorProneMatcherWithoutEquivalent— unmapped matcher produces the note and zero recipes.errorProneMatcherOnMethod— method-level@Matchesis still rejected.MatcherRegistryTest— every mapped name resolves to an instantiableMatcher.IsLambdaExpressionOrMethodReferenceTest— lambda / method reference / identifier / method call.ClasspathJarNameDetectorTest— annotations in a body still contribute jars; annotations on a parameter do not. Both fail if the skip is widened to the body.Known gap
An unmapped matcher on a parameter that the template never uses currently skips the whole rule, whereas the OpenRewrite flavor just logs
Ignoring annotationand generates the recipe. The annotation is inert in that case, so this is stricter than it needs to be. Left alone becausevalidate()would need to compute used parameters across every arity — happy to fix it here if you'd prefer.Follow-ups
The remaining ten Picnic matchers, roughly by value/effort (usage counts across the 78
*Rules.javafiles):IsIdentityOperation(20, easy) →IsArray/IsList/IsCharacter/IsMultidimensionalArray(9 total, trivial) →IsRefasterAsVarargs(3, trivial) →RequiresComputation(2, easy) →IsEmpty(16, large) →ReturnsMono(6, medium) →ThrowsCheckedException(2, hard — needs thrown-exception analysis over a lambda body and functional-method lookup withoutTypes.findDescriptorType).Part of #47