Skip to content

Commit 577c90b

Browse files
ysaito1001jdisanti
ysaito1001
and
jdisanti
authored
Render a Union member of type Unit to an enum variant without inner data (#1989)
* Avoid explicitly emitting Unit type within Union This commit addresses #1546. Previously, the Unit type in a Union was rendered as an enum variant whose inner data was crate::model::Unit. The way such a variant appears in Rust code feels a bit odd as it usually does not carry inner data for `()`. We now render a Union member of type Unit to an enum variant without inner data. * Address test failures washed out in CI This commit updates places that need to be aware of the Unit type attached to a Union member. Those places will render the Union member in a way that the generated Rust code does not include inner data of type Unit. It should help pass `codegen-client-test:test` and `codegen-server-test:test`. Further refactoring is required because each place we updated performs an explicit if-else check for special casing, which we should avoid. * Update codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/UnionGeneratorTest.kt Co-authored-by: John DiSanti <jdisanti@amazon.com> * Remove commented-out code * Add a helper for comparing against ShapeId for Unit This commit adds a helper for checking whether MemberShape targets the ShapeId of the Unit type. The benefit of the helper is that each call site does not have to construct a ShapeId for the Unit type every time comparison is made. * Move Unit type bifurcation logic to jsonObjectWriter This commit moves the if-else expression hard-coded in the StructureShape matching case within RustWriter.serializeMemberValue to jsonObjectWriter. The previous approach of inlining the if-else expression out of jsonObjectWriter to the StructureShape case and tailoring it to the call site violated the DRY principle. * Make QuerySerializerGenerator in sync with the change This commit brings the Unit type related change we've made so far to QuerySerializerGenerator. All tests in CI passed even without this commit, but noticing how similar QuerySerializerGenerator is to JsonSerializerGenerator, we should make the former in sync with the latter. * Update CHANGELOG.next.toml * Refactor ofTypeUnit -> isTargetUnit This commit addresses #1989 (comment). The extension should be renamed to isTargetUnit and moved to Smithy.kt. * Update codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/serialize/JsonSerializerGenerator.kt Co-authored-by: John DiSanti <jdisanti@amazon.com> * Simplify if-else in jsonObjectWriter This commit addresses #1989 (comment) * Avoid the union member's reference name being empty This commit addresses #1989 (comment) Even if member's reference name "inner" is present, it will not be an issue when the member is the Unit type where the reference name "inner" cannot be extracted. The reason is jsonObjectWriter won't render the serialization function if the member is the Unit type. That said, the same change in QuerySerializerGenerator may not be the case so we'll run the tests in CI and see what breaks. * Ensure Union with Unit target can be serialized This commit updates serialization of a Union with a Unit target in different types of serializers. We first updated protocol tests by adding a new field of type Unit and some of them failed as expected. We worked our way back from those failed tests and fixed the said implementation for serialization accordingly. * Ensure Union with Unit target can be parsed This commit handles deserialization of a Union with a Unit target in XmlBindingTraitParserGenerator. Prior to the commit, we already handled the said deserialization correctly in JsonParserGenerator but missed it in XmlBindingTraitParserGenerator. We added a new field of type Unit to a Union in parser protocols tests, ran them, and worked our way back from the failed tests. * Ensure match arm for Unit works in custom Debug impl This commit handles a use case that came up as a result of combining two updates, implementing a custom Debug impl for a Union and avoid rendering the Unit type in a Union. In the custom Debug impl, we now make sure that if the target is of type Unit, we render the corresponding match arm without the inner data. We add a new unit test in UnionGeneratorTest. * Fix unused variables warnings in CI This commit adds the #[allow(unused_variables)] annotation to a block of code generated for a member being the Unit type. The annotation is put before we call the handleOptional function so that it covers the whole block that the function generates. * Fix E0658 on unused_variables This commit fixes an error where attributes on expressions are experimental. It does so by rearranging code in a way that achieves the same effect but without #[allow(unused_variables)] on an expression. Co-authored-by: Yuki Saito <awsaito@amazon.com> Co-authored-by: John DiSanti <jdisanti@amazon.com>
1 parent 31f1d35 commit 577c90b

File tree

16 files changed

+248
-59
lines changed

16 files changed

+248
-59
lines changed

‎CHANGELOG.next.toml‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -660,4 +660,20 @@ author = "jdisanti"
660660
message = "Clients now default max idle connections to 70 (previously unlimited) to reduce the likelihood of hitting max file handles in AWS Lambda."
661661
references = ["smithy-rs#2064", "aws-sdk-rust#632"]
662662
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client"}
663-
author = "jdisanti"
663+
author = "jdisanti"
664+
665+
[[aws-sdk-rust]]
666+
message = """
667+
The Unit type for a Union member is no longer rendered. The serializers and parsers generated now function accordingly in the absence of the inner data associated with the Unit type.
668+
"""
669+
references = ["smithy-rs#1989"]
670+
meta = { "breaking" = true, "tada" = false, "bug" = false }
671+
author = "ysaito1001"
672+
673+
[[smithy-rs]]
674+
message = """
675+
The Unit type for a Union member is no longer rendered. The serializers and parsers generated now function accordingly in the absence of the inner data associated with the Unit type.
676+
"""
677+
references = ["smithy-rs#1989"]
678+
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "all" }
679+
author = "ysaito1001"

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.rustType
5050
import software.amazon.smithy.rust.codegen.core.util.dq
5151
import software.amazon.smithy.rust.codegen.core.util.expectMember
5252
import software.amazon.smithy.rust.codegen.core.util.hasTrait
53+
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
5354
import software.amazon.smithy.rust.codegen.core.util.letIf
5455

5556
/**
@@ -258,8 +259,10 @@ open class Instantiator(
258259
val member = shape.expectMember(memberName)
259260
writer.rust("#T::${symbolProvider.toMemberName(member)}", unionSymbol)
260261
// Unions should specify exactly one member.
261-
writer.withBlock("(", ")") {
262-
renderMember(this, member, variant.second, ctx)
262+
if (!member.isTargetUnit()) {
263+
writer.withBlock("(", ")") {
264+
renderMember(this, member, variant.second, ctx)
265+
}
263266
}
264267
}
265268

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

Lines changed: 57 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
package software.amazon.smithy.rust.codegen.core.smithy.generators
77

8+
import software.amazon.smithy.codegen.core.Symbol
89
import software.amazon.smithy.codegen.core.SymbolProvider
910
import software.amazon.smithy.model.Model
1011
import software.amazon.smithy.model.shapes.MemberShape
@@ -25,6 +26,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.renamedFrom
2526
import software.amazon.smithy.rust.codegen.core.util.REDACTION
2627
import software.amazon.smithy.rust.codegen.core.util.dq
2728
import software.amazon.smithy.rust.codegen.core.util.hasTrait
29+
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
2830
import software.amazon.smithy.rust.codegen.core.util.shouldRedact
2931
import software.amazon.smithy.rust.codegen.core.util.toSnakeCase
3032

@@ -57,8 +59,14 @@ class UnionGenerator(
5759
private val unionSymbol = symbolProvider.toSymbol(shape)
5860

5961
fun render() {
60-
renderUnion()
61-
renderImplBlock()
62+
writer.documentShape(shape, model)
63+
writer.deprecatedShape(shape)
64+
65+
val containerMeta = unionSymbol.expectRustMetadata()
66+
containerMeta.render(writer)
67+
68+
renderUnion(unionSymbol)
69+
renderImplBlock(unionSymbol)
6270
if (!unionSymbol.expectRustMetadata().derives.derives.contains(RuntimeType.Debug)) {
6371
if (shape.hasTrait<SensitiveTrait>()) {
6472
renderFullyRedactedDebugImpl()
@@ -68,12 +76,7 @@ class UnionGenerator(
6876
}
6977
}
7078

71-
private fun renderUnion() {
72-
writer.documentShape(shape, model)
73-
writer.deprecatedShape(shape)
74-
75-
val containerMeta = unionSymbol.expectRustMetadata()
76-
containerMeta.render(writer)
79+
private fun renderUnion(unionSymbol: Symbol) {
7780
writer.rustBlock("enum ${unionSymbol.name}") {
7881
sortedMembers.forEach { member ->
7982
val memberSymbol = symbolProvider.toSymbol(member)
@@ -82,7 +85,7 @@ class UnionGenerator(
8285
documentShape(member, model, note = note)
8386
deprecatedShape(member)
8487
memberSymbol.expectRustMetadata().renderAttributes(this)
85-
write("${symbolProvider.toMemberName(member)}(#T),", symbolProvider.toSymbol(member))
88+
writer.renderVariant(symbolProvider, member, memberSymbol)
8689
}
8790
if (renderUnknownVariant) {
8891
docs("""The `Unknown` variant represents cases where new union variant was received. Consider upgrading the SDK to the latest available version.""")
@@ -99,7 +102,7 @@ class UnionGenerator(
99102
}
100103
}
101104

102-
private fun renderImplBlock() {
105+
private fun renderImplBlock(unionSymbol: Symbol) {
103106
writer.rustBlock("impl ${unionSymbol.name}") {
104107
sortedMembers.forEach { member ->
105108
val memberSymbol = symbolProvider.toSymbol(member)
@@ -109,15 +112,7 @@ class UnionGenerator(
109112
if (sortedMembers.size == 1) {
110113
Attribute.Custom("allow(irrefutable_let_patterns)").render(this)
111114
}
112-
rust(
113-
"/// Tries to convert the enum instance into [`$variantName`](#T::$variantName), extracting the inner #D.",
114-
unionSymbol,
115-
memberSymbol,
116-
)
117-
rust("/// Returns `Err(&Self)` if it can't be converted.")
118-
rustBlock("pub fn as_$funcNamePart(&self) -> std::result::Result<&#T, &Self>", memberSymbol) {
119-
rust("if let ${unionSymbol.name}::$variantName(val) = &self { Ok(val) } else { Err(self) }")
120-
}
115+
writer.renderAsVariant(member, variantName, funcNamePart, unionSymbol, memberSymbol)
121116
rust("/// Returns true if this is a [`$variantName`](#T::$variantName).", unionSymbol)
122117
rustBlock("pub fn is_$funcNamePart(&self) -> bool") {
123118
rust("self.as_$funcNamePart().is_ok()")
@@ -152,10 +147,13 @@ class UnionGenerator(
152147
rustBlock("match self") {
153148
sortedMembers.forEach { member ->
154149
val memberName = symbolProvider.toMemberName(member)
155-
if (member.shouldRedact(model)) {
156-
rust("${unionSymbol.name}::$memberName(_) => f.debug_tuple($REDACTION).finish(),")
157-
} else {
158-
rust("${unionSymbol.name}::$memberName(val) => f.debug_tuple(${memberName.dq()}).field(&val).finish(),")
150+
val shouldRedact = member.shouldRedact(model)
151+
val isTargetUnit = member.isTargetUnit()
152+
when {
153+
!shouldRedact && isTargetUnit -> rust("${unionSymbol.name}::$memberName => f.debug_tuple(${memberName.dq()}).finish(),")
154+
!shouldRedact && !isTargetUnit -> rust("${unionSymbol.name}::$memberName(val) => f.debug_tuple(${memberName.dq()}).field(&val).finish(),")
155+
// We can always render (_) because the Unit target in a Union cannot be marked as sensitive separately.
156+
else -> rust("${unionSymbol.name}::$memberName(_) => f.debug_tuple($REDACTION).finish(),")
159157
}
160158
}
161159
if (renderUnknownVariant) {
@@ -175,3 +173,39 @@ fun unknownVariantError(union: String) =
175173
"Cannot serialize `$union::${UnionGenerator.UnknownVariantName}` for the request. " +
176174
"The `Unknown` variant is intended for responses only. " +
177175
"It occurs when an outdated client is used after a new enum variant was added on the server side."
176+
177+
private fun RustWriter.renderVariant(symbolProvider: SymbolProvider, member: MemberShape, memberSymbol: Symbol) {
178+
if (member.isTargetUnit()) {
179+
write("${symbolProvider.toMemberName(member)},")
180+
} else {
181+
write("${symbolProvider.toMemberName(member)}(#T),", memberSymbol)
182+
}
183+
}
184+
185+
private fun RustWriter.renderAsVariant(
186+
member: MemberShape,
187+
variantName: String,
188+
funcNamePart: String,
189+
unionSymbol: Symbol,
190+
memberSymbol: Symbol,
191+
) {
192+
if (member.isTargetUnit()) {
193+
rust(
194+
"/// Tries to convert the enum instance into [`$variantName`], extracting the inner `()`.",
195+
)
196+
rust("/// Returns `Err(&Self)` if it can't be converted.")
197+
rustBlock("pub fn as_$funcNamePart(&self) -> std::result::Result<(), &Self>") {
198+
rust("if let ${unionSymbol.name}::$variantName = &self { Ok(()) } else { Err(self) }")
199+
}
200+
} else {
201+
rust(
202+
"/// Tries to convert the enum instance into [`$variantName`](#T::$variantName), extracting the inner #D.",
203+
unionSymbol,
204+
memberSymbol,
205+
)
206+
rust("/// Returns `Err(&Self)` if it can't be converted.")
207+
rustBlock("pub fn as_$funcNamePart(&self) -> std::result::Result<&#T, &Self>", memberSymbol) {
208+
rust("if let ${unionSymbol.name}::$variantName(val) = &self { Ok(val) } else { Err(self) }")
209+
}
210+
}
211+
}

‎codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ import software.amazon.smithy.rust.codegen.core.util.PANIC
5454
import software.amazon.smithy.rust.codegen.core.util.dq
5555
import software.amazon.smithy.rust.codegen.core.util.hasTrait
5656
import software.amazon.smithy.rust.codegen.core.util.inputShape
57+
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
5758
import software.amazon.smithy.rust.codegen.core.util.outputShape
5859
import software.amazon.smithy.utils.StringUtils
5960

@@ -311,6 +312,7 @@ class JsonParserGenerator(
311312
rust("#T::from(u.as_ref())", symbolProvider.toSymbol(target))
312313
}
313314
}
315+
314316
else -> rust("u.into_owned()")
315317
}
316318
}
@@ -510,9 +512,19 @@ class JsonParserGenerator(
510512
for (member in shape.members()) {
511513
val variantName = symbolProvider.toMemberName(member)
512514
rustBlock("${jsonName(member).dq()} =>") {
513-
withBlock("Some(#T::$variantName(", "))", returnSymbolToParse.symbol) {
514-
deserializeMember(member)
515-
unwrapOrDefaultOrError(member)
515+
if (member.isTargetUnit()) {
516+
rustTemplate(
517+
"""
518+
#{skip_value}(tokens)?;
519+
Some(#{Union}::$variantName)
520+
""",
521+
"Union" to returnSymbolToParse.symbol, *codegenScope,
522+
)
523+
} else {
524+
withBlock("Some(#T::$variantName(", "))", returnSymbolToParse.symbol) {
525+
deserializeMember(member)
526+
unwrapOrDefaultOrError(member)
527+
}
516528
}
517529
}
518530
}

‎codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/XmlBindingTraitParserGenerator.kt‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ import software.amazon.smithy.rust.codegen.core.util.dq
5555
import software.amazon.smithy.rust.codegen.core.util.expectMember
5656
import software.amazon.smithy.rust.codegen.core.util.hasTrait
5757
import software.amazon.smithy.rust.codegen.core.util.inputShape
58+
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
5859
import software.amazon.smithy.rust.codegen.core.util.outputShape
5960

6061
// The string argument is the name of the XML ScopedDecoder to continue parsing from
@@ -420,18 +421,22 @@ class XmlBindingTraitParserGenerator(
420421
members.forEach { member ->
421422
val variantName = symbolProvider.toMemberName(member)
422423
case(member) {
423-
val current =
424-
"""
425-
(match base.take() {
426-
None => None,
427-
Some(${format(symbol)}::$variantName(inner)) => Some(inner),
428-
Some(_) => return Err(#{XmlDecodeError}::custom("mixed variants"))
429-
})
430-
"""
431-
withBlock("let tmp =", ";") {
432-
parseMember(member, ctx.copy(accum = current.trim()))
424+
if (member.isTargetUnit()) {
425+
rust("base = Some(#T::$variantName);", symbol)
426+
} else {
427+
val current =
428+
"""
429+
(match base.take() {
430+
None => None,
431+
Some(${format(symbol)}::$variantName(inner)) => Some(inner),
432+
Some(_) => return Err(#{XmlDecodeError}::custom("mixed variants"))
433+
})
434+
"""
435+
withBlock("let tmp =", ";") {
436+
parseMember(member, ctx.copy(accum = current.trim()))
437+
}
438+
rust("base = Some(#T::$variantName(tmp));", symbol)
433439
}
434-
rust("base = Some(#T::$variantName(tmp));", symbol)
435440
}
436441
}
437442
when (target.renderUnknownVariant()) {

‎codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/serialize/JsonSerializerGenerator.kt‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticOutputTra
5151
import software.amazon.smithy.rust.codegen.core.util.dq
5252
import software.amazon.smithy.rust.codegen.core.util.expectTrait
5353
import software.amazon.smithy.rust.codegen.core.util.inputShape
54+
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
5455
import software.amazon.smithy.rust.codegen.core.util.outputShape
5556

5657
/**
@@ -447,8 +448,22 @@ class JsonSerializerGenerator(
447448

448449
private fun RustWriter.jsonObjectWriter(context: MemberContext, inner: RustWriter.(String) -> Unit) {
449450
safeName("object").also { objectName ->
451+
rust("##[allow(unused_mut)]")
450452
rust("let mut $objectName = ${context.writerExpression}.start_object();")
451-
inner(objectName)
453+
// We call inner only when context's shape is not the Unit type.
454+
// If it were, calling inner would generate the following function:
455+
// pub fn serialize_structure_crate_model_unit(
456+
// object: &mut aws_smithy_json::serialize::JsonObjectWriter,
457+
// input: &crate::model::Unit,
458+
// ) -> Result<(), aws_smithy_http::operation::error::SerializationError> {
459+
// let (_, _) = (object, input);
460+
// Ok(())
461+
// }
462+
// However, this would cause a compilation error at a call site because it cannot
463+
// extract data out of the Unit type that corresponds to the variable "input" above.
464+
if (!context.shape.isTargetUnit()) {
465+
inner(objectName)
466+
}
452467
rust("$objectName.finish();")
453468
}
454469
}
@@ -488,8 +503,12 @@ class JsonSerializerGenerator(
488503
) {
489504
rustBlock("match input") {
490505
for (member in context.shape.members()) {
491-
val variantName = symbolProvider.toMemberName(member)
492-
withBlock("#T::$variantName(inner) => {", "},", unionSymbol) {
506+
val variantName = if (member.isTargetUnit()) {
507+
"${symbolProvider.toMemberName(member)}"
508+
} else {
509+
"${symbolProvider.toMemberName(member)}(inner)"
510+
}
511+
withBlock("#T::$variantName => {", "},", unionSymbol) {
493512
serializeMember(MemberContext.unionMember(context, "inner", member, jsonName))
494513
}
495514
}

‎codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/serialize/QuerySerializerGenerator.kt‎

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ import software.amazon.smithy.rust.codegen.core.util.dq
4343
import software.amazon.smithy.rust.codegen.core.util.getTrait
4444
import software.amazon.smithy.rust.codegen.core.util.hasTrait
4545
import software.amazon.smithy.rust.codegen.core.util.inputShape
46+
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
4647
import software.amazon.smithy.rust.codegen.core.util.orNull
4748

4849
abstract class QuerySerializerGenerator(codegenContext: CodegenContext) : StructuredDataSerializerGenerator {
@@ -151,6 +152,21 @@ abstract class QuerySerializerGenerator(codegenContext: CodegenContext) : Struct
151152
}
152153

153154
private fun RustWriter.serializeStructure(context: Context<StructureShape>) {
155+
// We proceed with the rest of the method only when context.shape.members() is nonempty.
156+
// If it were empty, the method would generate the following code:
157+
// #[allow(unused_mut)]
158+
// pub fn serialize_structure_crate_model_unit(
159+
// mut writer: aws_smithy_query::QueryValueWriter,
160+
// input: &crate::model::Unit,
161+
// ) -> Result<(), aws_smithy_http::operation::error::SerializationError> {
162+
// let (_, _) = (writer, input);
163+
// Ok(())
164+
// }
165+
// However, this would cause a compilation error at a call site because it cannot
166+
// extract data out of the Unit type that corresponds to the variable "input" above.
167+
if (context.shape.members().isEmpty()) {
168+
return
169+
}
154170
val fnName = symbolProvider.serializeFunctionName(context.shape)
155171
val structureSymbol = symbolProvider.toSymbol(context.shape)
156172
val structureSerializer = RuntimeType.forInlineFun(fnName, querySerModule) {
@@ -160,9 +176,6 @@ abstract class QuerySerializerGenerator(codegenContext: CodegenContext) : Struct
160176
"Input" to structureSymbol,
161177
*codegenScope,
162178
) {
163-
if (context.shape.members().isEmpty()) {
164-
rust("let (_, _) = (writer, input);") // Suppress unused argument warnings
165-
}
166179
serializeStructureInner(context)
167180
rust("Ok(())")
168181
}
@@ -311,8 +324,12 @@ abstract class QuerySerializerGenerator(codegenContext: CodegenContext) : Struct
311324
) {
312325
rustBlock("match input") {
313326
for (member in context.shape.members()) {
314-
val variantName = symbolProvider.toMemberName(member)
315-
withBlock("#T::$variantName(inner) => {", "},", unionSymbol) {
327+
val variantName = if (member.isTargetUnit()) {
328+
"${symbolProvider.toMemberName(member)}"
329+
} else {
330+
"${symbolProvider.toMemberName(member)}(inner)"
331+
}
332+
withBlock("#T::$variantName => {", "},", unionSymbol) {
316333
serializeMember(
317334
MemberContext.unionMember(
318335
context.copy(writerExpression = "writer"),

0 commit comments

Comments
 (0)