Skip to content

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

Merged
sambsnyd merged 3 commits into
mainfrom
tim/kotlin-dsl-matcher-param-types
Sep 10, 2026
Merged

sambsnyd merged 3 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.
@github-project-automation github-project-automation Bot moved this from In Progress to Ready to Review in OpenRewrite Sep 10, 2026
@sambsnyd
sambsnyd merged commit 726736a into main Sep 10, 2026
1 check passed
@sambsnyd
sambsnyd deleted the tim/kotlin-dsl-matcher-param-types branch September 10, 2026 20:10
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Sep 10, 2026
sambsnyd added a commit to moderneinc/moderne-docs that referenced this pull request Sep 10, 2026
* Replace Refaster module with Kotlin DSL in Fundamentals workshop

Module 3 of the Fundamentals of recipe development workshop now teaches
the Kotlin recipe DSL instead of Refaster templates. The DSL fills the
same pattern-shaped niche between declarative YAML and imperative
visitors, but it is compiler-checked Kotlin, needs no annotation
processor, and produces recipes that also rewrite Java, Groovy, and
Scala sources when the pattern names a pure-Java API.

The new module is sourced from the Kotlin recipe DSL documentation and
built around the kotlin-recipe-starter project:

* Exercise 3-1 sets up the starter, reads through the pattern-shaped
  recipes it ships with, runs their tests, and shows one recipe
  rewriting both Kotlin and Java sources.
* Exercise 3-2 has the reader write a UseKotlinMath recipe set covering
  one-, two-, and zero-parameter patterns, compose it with recipes(...),
  and drive each addition with tests including a no-change case.
* A closing section introduces the kotlin { visit... } imperative scope
  as a bridge into Module 4.

Supporting updates: sidebar entry, a redirect from the old URL, the
module list and learning objectives in both workshop overviews, and the
Refaster references in Modules 1 and 4. Module 1 keeps a pointer to the
Refaster guide so that recipe type stays discoverable.

* Correct CLI version for Kotlin active recipe support

`mod config recipes active set` began accepting Kotlin sources in CLI
4.5.2, not 4.4.2. See the "Accept Kotlin recipe sources in `mod config
recipes active set`" entry under CLI / DX v4.5.2 in cli-releases.md.

* Correct the Kotlin DSL displayName/description constraint

The warning told authors to avoid `+` concatenation, but concatenating
literals is explicitly supported and constant-folds into the generated
recipe. The real constraint is that the value must be written inline as a
compile-time constant; referencing a `val`, parameter, or `const val`
fails the build rather than silently falling back.

Verified against rewrite-kotlin 8.91.1: literal, literal concatenation,
and a text block with `trimIndent()` all compile and reach the generated
`$KtRecipe` class; `val` and `const val` references both fail with
"Recipe `displayName` must be a compile-time constant String".

* Select the composite explicitly in module 3 step 7

Without --recipe, `mod config recipes active set` takes the first
declaration in the file, so readers following step 7 verbatim ran
UseKotlinMathAbs while believing they ran the UseKotlinMath composite
they had just built in step 5. The CLI does report the alternatives, but
that line is easy to scroll past.

Add --recipe to the command and fold the reason into the existing
$KtRecipe warning, which already covers the quoting the flag requires.

* Explain where Code Genome Project credentials come from

Module 3 told readers to set codegenomeUsername/codegenomePassword but
never said where to get them, and the one page in these docs that answers
that was not linked from here. Link it, name what Moderne issues, and show
the gradle.properties entries with the token as the password.

Also drop the claim that Maven Central "may not include the DSL". Central
currently serves rewrite-kotlin 8.90.4, which ships the K2 recipe compiler
plugin, so a reader without credentials is a release or two behind rather
than blocked.

* Answer two questions the module leaves a Java reader with

Name the symptom of a missing compiler plugin. Removing the
kotlinCompilerPluginClasspath entry fails every test inside Jackson with
nothing pointing at the plugin, so quote the message that readers will
actually search for.

Close the pure-Java loop opened in exercise 3-1. Exercise 3-2 targets
java.lang.Math, which is exactly the condition that let UseIsWhitespace
reach Java sources, so a reader who took that lesson has reason to fear
step 7 will mangle Java repositories. Verified the composite leaves Java
sources unchanged: the kotlin.math replacement is not valid Java, so the
second condition fails and nothing is rewritten.

* List module 3's prerequisites, which differ from module 1's

Module 3 is the only module that builds a different starter project, but
it never said so up front. Its requirements are not the ones module 1
established: build.gradle.kts pins a Java 21 toolchain, so a JDK 21 must
be installed even when the default JDK is newer, and kotlin-recipe-starter
ships no pom.xml, so module 1's and 2's Maven commands do not apply.

* Restore the Maven Central warning dropped in fdd7aae

fdd7aae softened this to "lags by a release or two" on the strength of a
point-in-time check: Central happened to serve a rewrite-kotlin that
included the DSL. That read a diverging trend as a steady state. OpenRewrite
is moving off Maven Central, so the gap widens from here - Central is on
8.90.4 (Aug 24) while the Code Genome Project is on 8.91.1 with
8.92.0-SNAPSHOT.

Restore the substance of the original warning: the fallback is silent, the
build still succeeds, and the version it resolves can predate the DSL.

* Correct the claim that a pattern's parameter type narrows the match

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.

---------

Co-authored-by: Tim te Beek <tim@moderne.io>
Co-authored-by: Sam Snyder <sam@moderne.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants