Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changelog/fix-builder-accessor-naming.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
applies_to: ["client", "server"]
authors: ["haydenbaker", "nated0g"]
references: ["smithy-rs#4338"]
breaking: false
new_feature: false
bug_fix: true
---
Fix a duplicate `set_meta` definition when an error structure has a `meta` member. The generated struct field is already renamed to `meta_value`, so the builder's setter and getter now match (`set_meta_value`, `get_meta_value`, `meta_value`). Setters for other renamed members (e.g. `default`, `build`, `builder`) are unchanged.
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ class SmokeTestsBuilderKindBehavior(val codegenContext: CodegenContext) : Instan
override fun hasFallibleBuilder(shape: StructureShape): Boolean =
BuilderGenerator.hasFallibleBuilder(shape, codegenContext.symbolProvider)

override fun setterName(memberShape: MemberShape): String = memberShape.setterName()
override fun setterName(memberShape: MemberShape): String = memberShape.setterName(codegenContext.symbolProvider)

override fun doesSetterTakeInOption(memberShape: MemberShape): Boolean = true
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ class OperationInputTestGenerator(_ctx: ClientCodegenContext, private val test:
testOperationInput.operationParams.members.forEach { (key, value) ->
val member = operationInput.expectMember(key.value)
rustTemplate(
".${member.setterName()}(#{value})",
".${member.setterName(ctx.symbolProvider)}(#{value})",
"value" to instantiator.generate(member, value),
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
import software.amazon.smithy.rust.codegen.core.rustlang.writable
import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderInstantiator
import software.amazon.smithy.rust.codegen.core.smithy.generators.setterName

class ClientBuilderInstantiator(private val clientCodegenContext: ClientCodegenContext) : BuilderInstantiator {
override fun setField(
Expand All @@ -25,6 +26,10 @@ class ClientBuilderInstantiator(private val clientCodegenContext: ClientCodegenC
return setFieldWithSetter(builder, value, field)
}

override fun setterProvider(field: MemberShape): String {
return field.setterName(clientCodegenContext.symbolProvider)
}

/**
* For the client, we finalize builders with error correction enabled
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class ClientBuilderKindBehavior(val codegenContext: CodegenContext) : Instantiat
override fun hasFallibleBuilder(shape: StructureShape): Boolean =
BuilderGenerator.hasFallibleBuilder(shape, codegenContext.symbolProvider)

override fun setterName(memberShape: MemberShape): String = memberShape.setterName()
override fun setterName(memberShape: MemberShape): String = memberShape.setterName(codegenContext.symbolProvider)

override fun doesSetterTakeInOption(memberShape: MemberShape): Boolean = true
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -368,11 +368,11 @@ class FluentBuilderGenerator(
else -> renderInputHelper(member, memberName, coreType)
}
// pure setter
val setterName = member.setterName()
val setterName = member.setterName(symbolProvider)
val optionalInputType = outerType.asOptional()
renderInputHelper(member, setterName, optionalInputType)

val getterName = member.getterName()
val getterName = member.getterName(symbolProvider)
renderGetterHelper(member, getterName, optionalInputType)
}
}
Expand Down Expand Up @@ -438,7 +438,7 @@ class FluentBuilderGenerator(
"""
Appends an item to `${member.memberName}`.

To override the contents of this collection use [`${member.setterName()}`](Self::${member.setterName()}).
To override the contents of this collection use [`${member.setterName(symbolProvider)}`](Self::${member.setterName(symbolProvider)}).
""",
)
documentShape(member, model)
Expand Down Expand Up @@ -468,7 +468,7 @@ class FluentBuilderGenerator(
"""
Adds a key-value pair to `${member.memberName}`.

To override the contents of this collection use [`${member.setterName()}`](Self::${member.setterName()}).
To override the contents of this collection use [`${member.setterName(symbolProvider)}`](Self::${member.setterName(symbolProvider)}).
""",
)
documentShape(member, model)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ private fun generateOperationShapeDocs(
val builderInputDoc = memberShape.asFluentBuilderInputDoc(symbolProvider)
val builderInputLink = docLink("$fluentBuilderFullyQualifiedName::${symbolProvider.toMemberName(memberShape)}")
val builderSetterDoc = memberShape.asFluentBuilderSetterDoc(symbolProvider)
val builderSetterLink = docLink("$fluentBuilderFullyQualifiedName::${memberShape.setterName()}")
val builderSetterLink = docLink("$fluentBuilderFullyQualifiedName::${memberShape.setterName(symbolProvider)}")

val docTrait = memberShape.getMemberTrait(model, DocumentationTrait::class.java).orNull()
val docs =
Expand Down Expand Up @@ -449,7 +449,7 @@ internal fun MemberShape.asFluentBuilderInputDoc(symbolProvider: SymbolProvider)
* _NOTE: This function generates the setter type names that appear under **"The fluent builder is configurable:"**_
*/
private fun MemberShape.asFluentBuilderSetterDoc(symbolProvider: SymbolProvider): String {
val memberName = this.setterName()
val memberName = this.setterName(symbolProvider)
val outerType = symbolProvider.toSymbol(this).rustType()

return "$memberName(${outerType.asArgumentType(fullyQualified = false)})"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ class ProtocolParserGenerator(
val member = binding.member
val parsedValue = renderBindingParser(binding, operationShape, httpBindingGenerator, structuredDataParser)
if (parsedValue != null) {
withBlock("output = output.${member.setterName()}(", ");") {
withBlock("output = output.${member.setterName(symbolProvider)}(", ");") {
parsedValue(this)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,4 +138,92 @@ class FluentClientGeneratorTest {

clientIntegrationTest(model)
}

// Regression test for https://github.com/smithy-lang/smithy-rs/issues/4338: an error shape
// with a `meta` member used to produce a duplicate `set_meta` method on the generated builder
// because `ErrorGenerator` injects its own `set_meta` for `ErrorMetadata`. The fix renames
// only `meta` members (field, setter, getter) while leaving other members like `default`
// untouched so we don't introduce unrelated breaking changes.
@Test
fun `meta field on error shape does not collide with injected set_meta`() {
val model =
"""
namespace com.example
use aws.protocols#awsJson1_0

@awsJson1_0
service TestService {
operations: [TestOperation],
version: "1"
}

operation TestOperation {
input: TestInput,
errors: [TestError]
}

structure TestInput {
meta: String,
other: String,
default: String
}

@error("client")
structure TestError {
meta: String,
message: String
}
""".asSmithyModel()

clientIntegrationTest(model) { codegenContext, rustCrate ->
rustCrate.integrationTest("meta_field_renamed") {
val moduleName = codegenContext.moduleUseName()
rustTemplate(
"""
##[test]
fn test_meta_field_accessors() {
let config = $moduleName::Config::builder()
.endpoint_url("http://localhost:1234")
.http_client(#{NeverClient}::new())
.build();
let client = $moduleName::Client::from_conf(config);

// `meta` is renamed to `meta_value` for field, setter, and getter to
// avoid the `ErrorMetadata` collision on error shapes.
let builder = client.test_operation()
.meta_value("test_meta")
.set_meta_value(Some("test_meta_2".to_string()))
.other("other_value")
// `default` keeps `set_default` / `get_default` even though its field
// is renamed to `default_value` — renaming the accessors would be an
// unnecessary breaking change.
.set_default(Some("default_value".to_string()));

assert_eq!(*builder.get_meta_value(), Some("test_meta_2".to_string()));
assert_eq!(*builder.get_other(), Some("other_value".to_string()));
assert_eq!(*builder.get_default(), Some("default_value".to_string()));

let input = builder.as_input();
assert_eq!(*input.get_meta_value(), Some("test_meta_2".to_string()));

// The error builder must compile without a duplicate `set_meta` — the
// original bug — and `meta_value` must coexist with the injected error
// metadata field.
let _error = $moduleName::types::error::TestError::builder()
.meta_value("user_meta")
.message("boom")
.meta(#{ErrorMetadata}::builder().code("TestError").build())
.build();
}
""",
"NeverClient" to
CargoDependency.smithyHttpClientTestUtil(codegenContext.runtimeConfig).toType()
.resolve("test_util::NeverClient"),
"ErrorMetadata" to
CargoDependency.smithyTypes(codegenContext.runtimeConfig).toType()
.resolve("error::ErrorMetadata"),
)
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,33 @@ class OperationBuildError(private val runtimeConfig: RuntimeConfig) {
}
}

// Setter names will never hit a reserved word and therefore never need escaping.
fun MemberShape.setterName() = "set_${this.memberName.toSnakeCase()}"
// Builder setters/getters keep the raw member name in almost all cases: setter names don't
// collide with Rust reserved words, and renaming would be a breaking change for consumers of
// the generated SDK. The one exception is members named `meta` on error-struct builders, where
// `ErrorGenerator` injects a `set_meta` method for `ErrorMetadata` — we fall back to the
// symbol-provider-renamed name (`meta_value`) there to avoid a duplicate definition.
// See https://github.com/smithy-lang/smithy-rs/issues/4338.
fun MemberShape.setterName(symbolProvider: SymbolProvider): String {
val raw = this.memberName.toSnakeCase()
val name =
if (raw == "meta") {
symbolProvider.toMemberName(this).removePrefix("r##")
} else {
raw
}
return "set_$name"
}

// Getter names will never hit a reserved word and therefore never need escaping.
fun MemberShape.getterName() = "get_${this.memberName.toSnakeCase()}"
fun MemberShape.getterName(symbolProvider: SymbolProvider): String {
val raw = this.memberName.toSnakeCase()
val name =
if (raw == "meta") {
symbolProvider.toMemberName(this).removePrefix("r##")
} else {
raw
}
return "get_$name"
}

class BuilderGenerator(
private val model: Model,
Expand Down Expand Up @@ -192,7 +214,7 @@ class BuilderGenerator(
val memberName = member.memberName.toSnakeCase()
val setter =
if (symbolProvider.toSymbol(member).isOptional()) {
member.setterName()
member.setterName(symbolProvider)
} else {
memberName
}
Expand Down Expand Up @@ -311,7 +333,7 @@ class BuilderGenerator(

writer.documentShape(member, model)
writer.deprecatedShape(member)
writer.rustBlock("pub fn ${member.setterName()}(mut self, input: ${inputType.render(true)}) -> Self") {
writer.rustBlock("pub fn ${member.setterName(symbolProvider)}(mut self, input: ${inputType.render(true)}) -> Self") {
rust("self.$memberName = input; self")
}
}
Expand All @@ -333,7 +355,7 @@ class BuilderGenerator(

writer.documentShape(member, model)
writer.deprecatedShape(member)
writer.rustBlock("pub fn ${member.getterName()}(&self) -> &${inputType.render(true)}") {
writer.rustBlock("pub fn ${member.getterName(symbolProvider)}(&self) -> &${inputType.render(true)}") {
rust("&self.$memberName")
}
}
Expand Down Expand Up @@ -409,7 +431,13 @@ class BuilderGenerator(
) {
docs("Appends an item to `$memberName`.")
rust("///")
docs("To override the contents of this collection use [`${member.setterName()}`](Self::${member.setterName()}).")
docs(
"To override the contents of this collection use [`${member.setterName(symbolProvider)}`](Self::${
member.setterName(
symbolProvider,
)
}).",
)
rust("///")
documentShape(member, model, autoSuppressMissingDocs = false)
deprecatedShape(member)
Expand All @@ -435,7 +463,13 @@ class BuilderGenerator(
) {
docs("Adds a key-value pair to `$memberName`.")
rust("///")
docs("To override the contents of this collection use [`${member.setterName()}`](Self::${member.setterName()}).")
docs(
"To override the contents of this collection use [`${member.setterName(symbolProvider)}`](Self::${
member.setterName(
symbolProvider,
)
}).",
)
rust("///")
documentShape(member, model, autoSuppressMissingDocs = false)
deprecatedShape(member)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,14 @@ interface BuilderInstantiator {
mapErr: Writable? = null,
): Writable

fun setterProvider(field: MemberShape): String

/** Set a field on a builder using the `$setterName` method. $value will be passed directly. */
fun setFieldWithSetter(
builder: String,
value: Writable,
field: MemberShape,
) = writable {
rustTemplate("$builder = $builder.${field.setterName()}(#{value})", "value" to value)
rustTemplate("$builder = $builder.${setterProvider(field)}(#{value})", "value" to value)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ class CborParserGenerator(
rustBlock("${member.memberName.dq()} =>") {
val callBuilderSetMemberFieldWritable =
writable {
withBlock("builder.${member.setterName()}(", ")") {
withBlock("builder.${member.setterName(symbolProvider)}(", ")") {
conditionalBlock("Some(", ")", shouldWrapBuilderMemberSetterInputWithOption(member)) {
val symbol = symbolProvider.toSymbol(member)
if (symbol.isRustBoxed()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ class EventStreamUnmarshallerGenerator(
}

private fun RustWriter.renderUnmarshallEventHeader(member: MemberShape) {
withBlock("builder = builder.${member.setterName()}(", ");") {
withBlock("builder = builder.${member.setterName(symbolProvider)}(", ");") {
conditionalBlock("Some(", ")", member.isOptional) {
when (val target = model.expectShape(member.target)) {
is BooleanShape -> rustTemplate("#{expect_fns}::expect_bool(header)?", *codegenScope)
Expand Down Expand Up @@ -306,7 +306,7 @@ class EventStreamUnmarshallerGenerator(
*codegenScope,
)
}
withBlock("builder = builder.${member.setterName()}(", ");") {
withBlock("builder = builder.${member.setterName(symbolProvider)}(", ");") {
conditionalBlock("Some(", ")", member.isOptional) {
when (target) {
is BlobShape -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,14 +267,14 @@ class JsonParserGenerator(
rustBlock("${jsonName(member).dq()} =>") {
when (codegenTarget) {
CodegenTarget.CLIENT -> {
withBlock("builder = builder.${member.setterName()}(", ");") {
withBlock("builder = builder.${member.setterName(symbolProvider)}(", ");") {
deserializeMember(member)
}
}

CodegenTarget.SERVER -> {
if (symbolProvider.toSymbol(member).isOptional()) {
withBlock("builder = builder.${member.setterName()}(", ");") {
withBlock("builder = builder.${member.setterName(symbolProvider)}(", ");") {
deserializeMember(member)
}
} else {
Expand All @@ -283,7 +283,7 @@ class JsonParserGenerator(
rust(
"""
{
builder = builder.${member.setterName()}(v);
builder = builder.${member.setterName(symbolProvider)}(v);
}
""",
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ class XmlBindingTraitParserGenerator(
Ctx(tag = decoder, accum = "$builder.${symbolProvider.toMemberName(member)}.take()"),
)
}
rust("$builder = $builder.${member.setterName()}($temp);")
rust("$builder = $builder.${member.setterName(symbolProvider)}($temp);")
}
rustTemplate(
"_ => return Err(#{XmlDecodeError}::custom(\"expected ${member.xmlName()} tag\"))",
Expand Down Expand Up @@ -335,7 +335,7 @@ class XmlBindingTraitParserGenerator(
forceOptional = true,
)
}
rust("$builder = $builder.${member.setterName()}($temp);")
rust("$builder = $builder.${member.setterName(symbolProvider)}($temp);")
}
}
}
Expand Down
Loading