fix(compiler-plugin): apply the template rules to string concatenation - #368
Merged
Conversation
Inside a TemplateBuilder lambda, `+` on strings now means what `$` means:
literal operands are SQL text, every other operand is interpolated through
t(), and nested concatenations are spliced into the parent so fragments and
values keep their order.
An interpolation concatenated with a literal is folded by the compiler into a
single concatenation node whose first argument is the left-hand template. That
argument is now told apart from a `${...}` expression by its source range: the
arguments of a string literal all start after its opening quote, while the
first operand of a `+` chain starts where the chain itself starts.
A concatenated value of a type other than String is compiled to a String.plus
call, which the transformer now rewrites into a concatenation and processes by
the same rules, so the operand is parameterized rather than concatenated into
the SQL text.
An expression inside `${...}` yields a value, so its own interpolations and
concatenations are left to Kotlin. This also covers `"... LIKE ${"%$name%"}"`,
which resolved the inner interpolation and then interpolated the result,
producing two values for one placeholder.
There was a problem hiding this comment.
Pull request overview
This PR updates the Storm Kotlin compiler plugin so that +-based string concatenation inside a TemplateBuilder lambda follows the same interpolation/parameterization rules as $ string templates, fixing incorrect operand handling described in #367.
Changes:
- Rewrite
String.pluscalls in template “text position” intoIrStringConcatenationand apply the existing template-wrapping rules to operands. - Detect whether an
IrStringConcatenationrepresents a+chain vs a single string literal using source offsets, and splice nested concatenations to preserve fragment/value ordering. - Add plugin-unit tests and end-to-end tests covering concatenation shapes and “value-position” behavior inside
${...}.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| storm-kotlin/src/test/kotlin/st/orm/template/CompilerPluginTest.kt | Adds end-to-end H2 tests exercising concatenated templates in real ORM queries. |
| storm-compiler-plugin/src/test/kotlin/st/orm/kotlin/plugin/StormTemplatePluginTest.kt | Adds compiler-plugin tests validating fragments/values output for various concatenation forms. |
| storm-compiler-plugin/src/main/kotlin-transformer-2.2/st/orm/kotlin/plugin/StormTemplateIrTransformer.kt | Implements concatenation handling (offset-based detection, String.plus rewrite, splicing, value-position guard). |
| storm-compiler-plugin/src/main/kotlin-transformer-2.1/st/orm/kotlin/plugin/StormTemplateIrTransformer.kt | Same transformer changes for Kotlin 2.1 API surface. |
| storm-compiler-plugin/src/main/kotlin-transformer-2.0/st/orm/kotlin/plugin/StormTemplateIrTransformer.kt | Same transformer changes for Kotlin 2.0 API surface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| transformed is IrConst && hasMergedConstant(transformed) -> | ||
| splitMergedConstant(transformed, receiver, tFunction) | ||
| // A literal operand of a `+` chain is SQL text, like the literal part of a string template. | ||
| transformed is IrConst && operatorConcatenation -> listOf(transformed) |
| transformed is IrConst && hasMergedConstant(transformed) -> | ||
| splitMergedConstant(transformed, receiver, tFunction) | ||
| // A literal operand of a `+` chain is SQL text, like the literal part of a string template. | ||
| transformed is IrConst && operatorConcatenation -> listOf(transformed) |
| transformed is IrConst<*> && hasMergedConstant(transformed) -> | ||
| splitMergedConstant(transformed, receiver, tFunction) | ||
| // A literal operand of a `+` chain is SQL text, like the literal part of a string template. | ||
| transformed is IrConst<*> && operatorConcatenation -> listOf(transformed) |
Comment on lines
+679
to
+682
| fun main() { | ||
| val id = 42 | ||
| val builder: TemplateBuilder = { "SELECT * FROM users WHERE id = " + id } | ||
| val result = builder.build() |
…tion
A `+` chain treated every constant operand as SQL text, so `"WHERE id = " + 42`
inlined the number while `"WHERE id = ${42}"` produced a bind value. Only a
string literal is text; any other constant is interpolated, so both forms agree.
Collaborator
Author
|
Adopted the constant-operand finding: the One correction to the framing: the constants that reached that branch are compile-time literals, so nothing attacker-controlled was being inlined. The defect was the inconsistency with the interpolated form, not injection. The operand shapes that do carry outside values ( Verified: plugin tests 38 per Kotlin variant across all five modules, and |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
storm-compiler-plugin/src/main/kotlin-transformer-2.2/st/orm/kotlin/plugin/StormTemplateIrTransformer.kt:227
isStringPlusonly recognizes member calls with adispatchReceiver. Nullable-string concatenation (String? + x) uses the stdlibString?.plusextension and will have anextensionReceiverinstead, so it won't be rewritten in template text position.
private fun isStringPlus(expression: IrCall): Boolean {
if (expression.symbol.owner.name.asString() != "plus") return false
val receiverType = expression.dispatchReceiver?.type ?: return false
return receiverType.classOrNull?.owner?.fqNameWhenAvailable == STRING_FQN
}
storm-compiler-plugin/src/main/kotlin-transformer-2.1/st/orm/kotlin/plugin/StormTemplateIrTransformer.kt:222
isStringPlusonly recognizes member calls with adispatchReceiver. Nullable-string concatenation (String? + x) uses the stdlibString?.plusextension and will have anextensionReceiverinstead, so it won't be rewritten in template text position.
private fun isStringPlus(expression: IrCall): Boolean {
if (expression.symbol.owner.name.asString() != "plus") return false
val receiverType = expression.dispatchReceiver?.type ?: return false
return receiverType.classFqName == STRING_FQN
}
storm-compiler-plugin/src/main/kotlin-transformer-2.0/st/orm/kotlin/plugin/StormTemplateIrTransformer.kt:222
isStringPlusonly recognizes member calls with adispatchReceiver. Nullable-string concatenation (String? + x) uses the stdlibString?.plusextension and will have anextensionReceiverinstead, so it won't be rewritten in template text position.
private fun isStringPlus(expression: IrCall): Boolean {
if (expression.symbol.owner.name.asString() != "plus") return false
val receiverType = expression.dispatchReceiver?.type ?: return false
return receiverType.classFqName == STRING_FQN
}
| } | ||
| // `a + b` in text position means the same as "$a$b", so rewrite it into a concatenation and apply the | ||
| // template rules to its operands. Nested `+` calls become concatenations in turn and are flattened below. | ||
| val operands = listOfNotNull(expression.dispatchReceiver, valueArgument(expression)) |
| } | ||
| // `a + b` in text position means the same as "$a$b", so rewrite it into a concatenation and apply the | ||
| // template rules to its operands. Nested `+` calls become concatenations in turn and are flattened below. | ||
| val operands = listOfNotNull(expression.dispatchReceiver, expression.getValueArgument(0)) |
| } | ||
| // `a + b` in text position means the same as "$a$b", so rewrite it into a concatenation and apply the | ||
| // template rules to its operands. Nested `+` calls become concatenations in turn and are flattened below. | ||
| val operands = listOfNotNull(expression.dispatchReceiver, expression.getValueArgument(0)) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #367.
Inside a
TemplateBuilderlambda,+on strings now means what$means: literal operands are SQL text, every other operand is interpolated throught(), and nested concatenations are spliced into the parent so fragments and values keep their order."SELECT ${City::class}, " + "COUNT(*) FROM city"previously interpolated the whole left-hand string as a bind parameter, because the compiler folds the two operands into one concatenation node and the plugin could not tell the first operand apart from a${...}expression. The source range tells them apart: the arguments of a string literal all start after its opening quote, while the first operand of a+chain starts where the chain itself starts."SELECT * FROM city WHERE id = " + cityIdpreviously concatenated the operand'stoString()into the SQL text, because an operand of a type other thanStringcompiles to aString.pluscall rather than a concatenation node. Those calls are now rewritten into a concatenation and processed by the same rules, so the operand is parameterized.An expression inside
${...}yields a value, so its own interpolations and concatenations are left to Kotlin. That also covers"... LIKE ${"%$name%"}", which resolved the inner interpolation and then interpolated the result, producing two values for one placeholder.The transformer is versioned per compiler API, so the change is applied to the 2.0, 2.1 and 2.2 sources (the last shared by the 2.2, 2.3 and 2.4 modules). The concatenation node is built with
irConcat():IrStringConcatenationImplcompiles against 2.0 but is absent from the 2.0.21 compiler runtime.Verification
storm-kotlin'sCompilerPluginTestrunning concatenated templates against H2; the module's 1682 tests are green.Known limitation
Accumulating SQL in a local (
var sql = "SELECT "; sql += City::class) reads the local as an operand, so it is interpolated as a value rather than spliced as text. This matches what"$sql ..."does today and needs assignment tracking to resolve, which is left out of this change.