Skip to content

Kotlin recipe DSL: type the matcher's parameters, not just their count - #8694

Open
timtebeek wants to merge 2 commits into
mainfrom
tim/kotlin-dsl-matcher-param-types
Open

Kotlin recipe DSL: type the matcher's parameters, not just their count#8694
timtebeek wants to merge 2 commits into
mainfrom
tim/kotlin-dsl-matcher-param-types

Conversation

@timtebeek

@timtebeek timtebeek commented Aug 28, 2026

Copy link
Copy Markdown
Member

Naming types outright is unsafe because KotlinTypeMapping only remaps Kotlin builtins to their JVM FQN for Java-declared methods — the same String parameter reads java.lang.String on a Java-declared callee and kotlin.String on a Kotlin-declared one, and authors routinely write Kotlin stand-in classes for Java targets — so reference builtins emit a package-wildcard token (*..String) that names both spellings, primitives emit the JVM keyword, and anything unpredictable (type parameters, nullable primitives, value classes, nested classes, the kotlin. package, the lifted extension-receiver slot) keeps the old *.

Four new tests in RecipePluginRewriteTest cover the narrowed match, an untargeted sibling overload left alone, the Math.round miscompile, a Kotlin-declared callee, and the kotlin-recipe-starter recipes still firing; all four fail without the change, and one existing spec assertion moves from substring(*, *, *) to substring(*, int, int).

Verified downstream against moderneinc/recipes-kotlin (964 tests, 244 DSL recipes): green, and its UseDoubleRoundToLong/UseFloatRoundToInt pair plus three same-arity Math.floorMod recipes were silently cross-matching before this.

A `rewrite { x: Double -> Math.abs(x) } to { x -> kotlin.math.abs(x) }`
recipe matched every same-arity `abs` overload. The K2 plugin's
`computeArgsPattern` emitted `List(jvmArgCount) { "*" }`, so the declared
`Double` reached the after-template (`#{any(kotlin.Double)}`) but never the
`MethodMatcher` spec (`java.lang.Math abs(*)`).

That is a correctness bug, not just over-eager matching. `Math.round(double)`
returns `Long` while `Math.round(float)` returns `Int`, so a recipe targeting
the `Double` overload with `to { x -> x.roundToLong() }` also fired on
`fun r(x: Float): Int = Math.round(x)` and produced a `Long` assigned to an
`Int`. #7737 deliberately scoped itself to arity; the varargs branch added by
#7895 already types its fixed prefix, and this brings the non-varargs branch
in line.

Each value parameter is now rendered by `matcherParamType`. Naming a type
outright is unsafe, because `KotlinTypeMapping` only remaps Kotlin builtins to
their JVM FQN for methods declared in Java: the same `String` parameter reads
`java.lang.String` on a Java-declared callee and `kotlin.String` on a
Kotlin-declared one, and recipe authors routinely write Kotlin stand-in classes
for Java targets. Reference builtins therefore emit a package-wildcard token
(`*..String`) that names both spellings, primitives emit the JVM keyword, and
anything whose `JavaType` spelling isn't predictable keeps the old `*` —
type parameters, nullable primitives (`Int?` boxes), value classes, nested
classes, the `kotlin.` package (collections, arrays, `Function1`), and the
lifted extension-receiver slot.

Verified against moderneinc/recipes-kotlin (964 tests, 244 DSL recipes): green.
Its `UseDoubleRoundToLong` / `UseFloatRoundToInt` pair and the three
same-arity `Math.floorMod` recipes were cross-matching before this change.
@timtebeek

timtebeek commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

Downstream check: moderneinc/recipes-kotlin

Measured against recipes-kotlin@origin/main (9ba907e) by publishing both builds to mavenLocal and running the full suite against each. The only harness edit is pinning rewriteVersion to the locally published 8.92.0-SNAPSHOT; nothing else in the repo is touched.

No regressions:

tests failures
baseline 964 0
with this change 964 0

Green both ways — but that's because each recipe's test only exercises its own overload, so the over-match is invisible to them. Dumping the generated MethodMatcher specs out of the compiled $KtRecipe classes from each run shows what actually changed:

recipe baseline with this change
UseDoubleRoundToLong Math round(*) Math round(double)
UseFloatRoundToInt Math round(*) Math round(float)
UseIntMod / UseLongMod / UseLongModInt Math floorMod(*,*) ×3 (int,int) / (long,long) / (long,int)
UseIntFloorDiv / UseLongFloorDiv Math floorDiv(*,*) ×2 (int,int) / (long,long)
UseAppendLineChar / …CharSequence / …WithValue StringsKt appendln(*,*) ×3 (*,char) / (*,*..CharSequence) / (*,*..String)
UseCharLowercaseCharForCharacter Character toLowerCase(*) (char)
UseLowercaseWithLocale StringsKt toLowerCase(*,*) (*,java.util.Locale)

The round pair is the type-unsafe collision this PR is about: two recipes sharing one matcher but with incompatible after-templates (roundToLong returns Long, roundToInt returns Int), so whichever ran first miscompiled the other's call sites.

One collision survives by design. UseAppendLineAny stays appendln(*,*) and still overlaps the three appendln siblings — kotlin.Any and java.lang.Object share no simple name, so no single token matches both parsers' spellings.

Suggested follow-up for that repo: a test that runs the composite over a source holding several overloads at once. The per-overload tests they have now pass whether or not the matchers collide.

@timtebeek
timtebeek marked this pull request as ready for review August 28, 2026 11:35
bryanfriedman added a commit to moderneinc/moderne-docs that referenced this pull request Aug 28, 2026
The K2 plugin builds its matcher spec with a wildcard per argument, so
the type declared on the before lambda reaches the after-template but
not the MethodMatcher. UseKotlinMathAbs therefore also rewrites
Math.abs on Int and Long receivers, which openrewrite/rewrite#8694 will
change once it ships.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

1 participant