Skip to content

Commit da74b5d

Browse files
committed
Narrow setter/getter rename to meta members only
The previous approach renamed every member that the symbol provider touched, which changed `set_default`/`set_build`/`set_builder` (and the matching getters) for any struct with those fields — a breaking change flagged by @rcoh on review. The duplicate-definition bug only occurs for error-struct builders, where `ErrorGenerator` injects `set_meta` for `ErrorMetadata`. Only rename members whose raw name is `meta` and leave every other setter and getter at its pre-existing `set_<raw>` / `get_<raw>` name. Rewrite the regression test to actually reproduce the original bug: it now builds a `TestError` (an `@error` shape with a `meta` member) and asserts `set_default` stays intact, so the earlier failure mode and the guard against unrelated renames are both covered. Also drop a now-unused `toSnakeCase` import in `ServerBuilderGenerator` that surfaced after rebasing onto main.
1 parent 54890e1 commit da74b5d

4 files changed

Lines changed: 62 additions & 13 deletions

File tree

.changelog/fix-builder-accessor-naming.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,4 +6,4 @@ breaking: false
66
new_feature: false
77
bug_fix: true
88
---
9-
Fix builder accessor methods to use symbol provider for naming. This resolves conflicts when struct members are renamed due to reserved words (e.g., `meta` -> `meta_value`), ensuring setter and getter methods use the correct renamed field names.
9+
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.

codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGeneratorTest.kt

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,13 @@ class FluentClientGeneratorTest {
139139
clientIntegrationTest(model)
140140
}
141141

142+
// Regression test for https://github.com/smithy-lang/smithy-rs/issues/4338: an error shape
143+
// with a `meta` member used to produce a duplicate `set_meta` method on the generated builder
144+
// because `ErrorGenerator` injects its own `set_meta` for `ErrorMetadata`. The fix renames
145+
// only `meta` members (field, setter, getter) while leaving other members like `default`
146+
// untouched so we don't introduce unrelated breaking changes.
142147
@Test
143-
fun `meta field gets renamed to meta_value with correct setter`() {
148+
fun `meta field on error shape does not collide with injected set_meta`() {
144149
val model =
145150
"""
146151
namespace com.example
@@ -152,10 +157,21 @@ class FluentClientGeneratorTest {
152157
version: "1"
153158
}
154159
155-
operation TestOperation { input: TestInput }
160+
operation TestOperation {
161+
input: TestInput,
162+
errors: [TestError]
163+
}
164+
156165
structure TestInput {
157166
meta: String,
158-
other: String
167+
other: String,
168+
default: String
169+
}
170+
171+
@error("client")
172+
structure TestError {
173+
meta: String,
174+
message: String
159175
}
160176
""".asSmithyModel()
161177

@@ -172,24 +188,40 @@ class FluentClientGeneratorTest {
172188
.build();
173189
let client = $moduleName::Client::from_conf(config);
174190
175-
// Test the renamed field and setter
191+
// `meta` is renamed to `meta_value` for field, setter, and getter to
192+
// avoid the `ErrorMetadata` collision on error shapes.
176193
let builder = client.test_operation()
177194
.meta_value("test_meta")
178195
.set_meta_value(Some("test_meta_2".to_string()))
179-
.other("other_value");
196+
.other("other_value")
197+
// `default` keeps `set_default` / `get_default` even though its field
198+
// is renamed to `default_value` — renaming the accessors would be an
199+
// unnecessary breaking change.
200+
.set_default(Some("default_value".to_string()));
180201
181-
// Verify getter returns correct value
182202
assert_eq!(*builder.get_meta_value(), Some("test_meta_2".to_string()));
183203
assert_eq!(*builder.get_other(), Some("other_value".to_string()));
204+
assert_eq!(*builder.get_default(), Some("default_value".to_string()));
184205
185-
// Build the input and verify field is accessible
186206
let input = builder.as_input();
187207
assert_eq!(*input.get_meta_value(), Some("test_meta_2".to_string()));
208+
209+
// The error builder must compile without a duplicate `set_meta` — the
210+
// original bug — and `meta_value` must coexist with the injected error
211+
// metadata field.
212+
let _error = $moduleName::types::error::TestError::builder()
213+
.meta_value("user_meta")
214+
.message("boom")
215+
.meta(#{ErrorMetadata}::builder().code("TestError").build())
216+
.build();
188217
}
189218
""",
190219
"NeverClient" to
191220
CargoDependency.smithyHttpClientTestUtil(codegenContext.runtimeConfig).toType()
192221
.resolve("test_util::NeverClient"),
222+
"ErrorMetadata" to
223+
CargoDependency.smithyTypes(codegenContext.runtimeConfig).toType()
224+
.resolve("error::ErrorMetadata"),
193225
)
194226
}
195227
}

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,14 +134,32 @@ class OperationBuildError(private val runtimeConfig: RuntimeConfig) {
134134
}
135135
}
136136

137+
// Builder setters/getters keep the raw member name in almost all cases: setter names don't
138+
// collide with Rust reserved words, and renaming would be a breaking change for consumers of
139+
// the generated SDK. The one exception is members named `meta` on error-struct builders, where
140+
// `ErrorGenerator` injects a `set_meta` method for `ErrorMetadata` — we fall back to the
141+
// symbol-provider-renamed name (`meta_value`) there to avoid a duplicate definition.
142+
// See https://github.com/smithy-lang/smithy-rs/issues/4338.
137143
fun MemberShape.setterName(symbolProvider: SymbolProvider): String {
138-
val unescaped = symbolProvider.toMemberName(this).removePrefix("r##")
139-
return "set_$unescaped"
144+
val raw = this.memberName.toSnakeCase()
145+
val name =
146+
if (raw == "meta") {
147+
symbolProvider.toMemberName(this).removePrefix("r##")
148+
} else {
149+
raw
150+
}
151+
return "set_$name"
140152
}
141153

142154
fun MemberShape.getterName(symbolProvider: SymbolProvider): String {
143-
val unescaped = symbolProvider.toMemberName(this).removePrefix("r##")
144-
return "get_$unescaped"
155+
val raw = this.memberName.toSnakeCase()
156+
val name =
157+
if (raw == "meta") {
158+
symbolProvider.toMemberName(this).removePrefix("r##")
159+
} else {
160+
raw
161+
}
162+
return "get_$name"
145163
}
146164

147165
class BuilderGenerator(

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@ import software.amazon.smithy.rust.codegen.core.util.dq
4646
import software.amazon.smithy.rust.codegen.core.util.hasTrait
4747
import software.amazon.smithy.rust.codegen.core.util.letIf
4848
import software.amazon.smithy.rust.codegen.core.util.redactIfNecessary
49-
import software.amazon.smithy.rust.codegen.core.util.toSnakeCase
5049
import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext
5150
import software.amazon.smithy.rust.codegen.server.smithy.canReachConstrainedShape
5251
import software.amazon.smithy.rust.codegen.server.smithy.generators.protocol.ServerProtocol

0 commit comments

Comments
 (0)