KTOR-9037 Make formData inline function #5185
Conversation
WalkthroughAdded a public no-arg Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
osipxd
force-pushed
the
osipxd/make-formData-inline
branch
from
c07f996 to
2b93efe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.kt (1)
89-90: Document the builder contract on the inline overload.We invoke
blockexactly once here. Adding the sameExperimentalContractsopt-in +contract { callsInPlace(block, InvocationKind.EXACTLY_ONCE) }used elsewhere tightens the DSL guarantees and keeps the new inline overload aligned with the rest of the API surface.- public inline fun formData(block: FormBuilder.() -> Unit): List<PartData> = - formData(*FormBuilder().apply(block).build().toTypedArray()) +@OptIn(ExperimentalContracts::class) +public inline fun formData(block: FormBuilder.() -> Unit): List<PartData> { + contract { callsInPlace(block, InvocationKind.EXACTLY_ONCE) } + return formData(*FormBuilder().apply(block).build().toTypedArray()) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ktor-client/ktor-client-core/api/ktor-client-core.api(2 hunks)ktor-client/ktor-client-core/api/ktor-client-core.klib.api(3 hunks)ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.kt(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Applied to files:
ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.ktktor-client/ktor-client-core/api/ktor-client-core.klib.api
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Binary compatibility is enforced using the binary-compatibility-validator plugin.
Applied to files:
ktor-client/ktor-client-core/api/ktor-client-core.klib.api
🔇 Additional comments (2)
ktor-client/ktor-client-core/api/ktor-client-core.klib.api (2)
891-907: The review comment is incorrect; FormBuilder is already correctly annotated.The source code in
ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.ktalready implements what the review suggests:
- Constructor:
@PublishedApi internal constructor()- build() method:
@PublishedApi internal fun build()The klib.api dump correctly exposes these members because
@PublishedApi internalmeans "internal but part of the public API for inliners." This is the intended design and causes no external compatibility issues since only the inlineformData()function usesFormBuilder()directly.Likely an incorrect or invalid review comment.
1531-1531: Kotlin/Native ABI incompatibility confirmed; verify release status before proceeding.Web search confirms your concern: changing a function from non-inline to inline is a binary-breaking change for Kotlin/Native klib consumers, as previously compiled call sites expect the original callable symbol, and making the function inline removes/changes that emitted symbol.
However, referencing Ktor team precedent: breaking changes to unreleased code are acceptable per prior PRs (#4916, #4855, #4860). If ktor-client-core hasn't been released yet, this change may be acceptable with a corresponding version bump. Confirm release status; if released, consider preserving a non-inline forwarding symbol to maintain binary compatibility.
osipxd
force-pushed
the
osipxd/make-formData-inline
branch
from
2b93efe to
c9fcb5c
Compare
osipxd
force-pushed
the
osipxd/make-formData-inline
branch
from
c9fcb5c to
b9534b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.kt (1)
89-90: Good use ofinlinemodifier with proper@PublishedApisupport.The inlining is correctly implemented with the necessary
@PublishedApiannotations on the internal APIs.Consider adding a contract to specify that
blockis called exactly once, consistent with other builder functions in this file (lines 226, 267):+@OptIn(ExperimentalContracts::class) public inline fun formData(block: FormBuilder.() -> Unit): List<PartData> = - formData(*FormBuilder().apply(block).build().toTypedArray()) + formData(*FormBuilder().apply(block).build().toTypedArray()).also { + contract { + callsInPlace(block, InvocationKind.EXACTLY_ONCE) + } + }Wait, that won't work with an expression body. Here's the correct approach:
+@OptIn(ExperimentalContracts::class) -public inline fun formData(block: FormBuilder.() -> Unit): List<PartData> = - formData(*FormBuilder().apply(block).build().toTypedArray()) +public inline fun formData(block: FormBuilder.() -> Unit): List<PartData> { + contract { + callsInPlace(block, InvocationKind.EXACTLY_ONCE) + } + return formData(*FormBuilder().apply(block).build().toTypedArray()) +}ktor-client/ktor-client-core/api/ktor-client-core.klib.api (1)
891-907: FormBuilder public ctor +build()look coherent with the builder APIExposing a no-arg constructor and
build(): List<FormPart<*>>aligns with the existingappend(...)overloads and the top-levelformData(vararg FormPart<*>)helper. This makes the builder usable directly while keeping the types consistent. Just be aware this cementsFormBuilderas a more fully public construction API that you’ll need to support going forward, but I don’t see any functional or ABI issues here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ktor-client/ktor-client-core/api/ktor-client-core.api(2 hunks)ktor-client/ktor-client-core/api/ktor-client-core.klib.api(3 hunks)ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.kt(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-client/ktor-client-core/api/ktor-client-core.api
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-14T18:05:02.321Z
Learnt from: bjhham
Repo: ktorio/ktor PR: 4855
File: ktor-server/ktor-server-plugins/ktor-server-di/api/ktor-server-di.klib.api:334-336
Timestamp: 2025-05-14T18:05:02.321Z
Learning: Breaking changes in constructor parameter order are acceptable for the ktor-server-di module when the code hasn't been released yet, as confirmed by the development team.
Applied to files:
ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.ktktor-client/ktor-client-core/api/ktor-client-core.klib.api
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Binary compatibility is enforced using the binary-compatibility-validator plugin.
Applied to files:
ktor-client/ktor-client-core/api/ktor-client-core.klib.api
🔇 Additional comments (3)
ktor-client/ktor-client-core/common/src/io/ktor/client/request/forms/formDsl.kt (2)
97-97: Verify the intended visibility of theFormBuilderconstructor.The constructor is marked
@PublishedApi internal, which allows inline functions to use it while keeping it out of the public API. However, the AI-generated summary states "Added a public no-arg FormBuilder() constructor," suggesting it should be public.Please clarify the intended API design:
- Should
FormBuilderbe publicly instantiable for direct use outside of theformDatabuilder?- Or should it remain internal and only accessible through the
formDatainline function?If users need to instantiate
FormBuilderdirectly, change the visibility:-public class FormBuilder @PublishedApi internal constructor() { +public class FormBuilder() {If it should remain internal for inline use only, the current implementation is correct.
215-216: Correct use of@PublishedApifor inline support.The
@PublishedApiannotation is properly applied to allow the inlineformDatafunction to access this internal method.ktor-client/ktor-client-core/api/ktor-client-core.klib.api (1)
1520-1532: InlineformData(FormBuilder.() -> Unit)matches the DSL goal; please just sanity‑check K/N implicationsMarking
io.ktor.client.request.forms/formData((FormBuilder) -> Unit)asinlinesatisfies KTOR-9037 and keeps the same parameter/return types as the existing non‑inline variant, so JVM/JS callers remain binary compatible and new code benefits from inlining. For Kotlin/Native this changes klib metadata but still shouldn’t break already-built binaries; the main constraint is that removinginlinelater would be a breaking change.Given the earlier question about K/N ABI, I’d recommend double‑checking this behavior against the Kotlin/Native docs or a tiny repro project for your supported compiler versions, but from an API-surface perspective this looks fine and consistent with the rest of the client DSL.
Based on learnings
Subsystem
ktor-client-core
Motivation
KTOR-9037 Multipart/form-data: Make
formData's block inlineSolution
Add the
inlinemodifier to the function