Skip to content

Commit 2a71c21

Browse files
vcjanavcjana
committed
Implement proper TDD approach for union serialization with empty structures
- Use _inner only for empty structures, inner for structures with data - Add comprehensive tests with RUSTFLAGS to verify unused variable handling - Complete fix for issue #4308 across JSON, CBOR, and Query protocols
1 parent d71bd1b commit 2a71c21

File tree

9 files changed

+94
-61
lines changed

9 files changed

+94
-61
lines changed

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

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -547,14 +547,23 @@ class CborSerializerGenerator(
547547

548548
rustBlock("match input") {
549549
for (member in context.shape.members()) {
550+
val memberShape = model.expectShape(member.target)
551+
val memberName = symbolProvider.toMemberName(member)
552+
val isEmptyStruct =
553+
memberShape.isStructureShape &&
554+
memberShape.asStructureShape().get().allMembers.isEmpty()
550555
val variantName =
551556
if (member.isTargetUnit()) {
552-
symbolProvider.toMemberName(member)
557+
memberName
558+
} else if (isEmptyStruct) {
559+
// Empty structures don't use the inner variable
560+
"$memberName(_inner)"
553561
} else {
554-
"${symbolProvider.toMemberName(member)}(inner)"
562+
"$memberName(inner)"
555563
}
556564
rustBlock("#T::$variantName =>", unionSymbol) {
557-
serializeMember(MemberContext.unionMember("inner", member))
565+
val innerRef = if (isEmptyStruct) "_inner" else "inner"
566+
serializeMember(MemberContext.unionMember(innerRef, member))
558567
}
559568
}
560569
if (codegenTarget.renderUnknownVariant()) {

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

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -556,26 +556,20 @@ class JsonSerializerGenerator(
556556
rustBlock("match input") {
557557
for (member in context.shape.members()) {
558558
val memberShape = model.expectShape(member.target)
559+
val isEmptyStruct =
560+
memberShape.isStructureShape &&
561+
memberShape.asStructureShape().get().allMembers.isEmpty()
559562
val variantName =
560563
if (member.isTargetUnit()) {
561564
"${symbolProvider.toMemberName(member)}"
562-
} else if (memberShape.isStructureShape &&
563-
memberShape.asStructureShape().get().allMembers.isEmpty()
564-
) {
565-
// Unit structs don't serialize inner, so it is never accessed
565+
} else if (isEmptyStruct) {
566+
// Empty structures don't use the inner variable
566567
"${symbolProvider.toMemberName(member)}(_inner)"
567568
} else {
568569
"${symbolProvider.toMemberName(member)}(inner)"
569570
}
570571
withBlock("#T::$variantName => {", "},", unionSymbol) {
571-
val innerRef =
572-
if (memberShape.isStructureShape &&
573-
memberShape.asStructureShape().get().allMembers.isEmpty()
574-
) {
575-
"_inner"
576-
} else {
577-
"inner"
578-
}
572+
val innerRef = if (isEmptyStruct) "_inner" else "inner"
579573
serializeMember(MemberContext.unionMember(context, innerRef, member, jsonName))
580574
}
581575
}

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

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -359,26 +359,20 @@ abstract class QuerySerializerGenerator(private val codegenContext: CodegenConte
359359
for (member in context.shape.members()) {
360360
val memberShape = model.expectShape(member.target)
361361
val memberName = symbolProvider.toMemberName(member)
362+
val isEmptyStruct =
363+
memberShape.isStructureShape &&
364+
memberShape.asStructureShape().get().allMembers.isEmpty()
362365
val variantName =
363366
if (member.isTargetUnit()) {
364367
"$memberName"
365-
} else if (memberShape.isStructureShape &&
366-
memberShape.asStructureShape().get().allMembers.isEmpty()
367-
) {
368-
// Unit structs don't serialize inner, so it is never accessed
368+
} else if (isEmptyStruct) {
369+
// Empty structures don't use the inner variable
369370
"$memberName(_inner)"
370371
} else {
371372
"$memberName(inner)"
372373
}
373374
withBlock("#T::$variantName => {", "},", unionSymbol) {
374-
val innerRef =
375-
if (memberShape.isStructureShape &&
376-
memberShape.asStructureShape().get().allMembers.isEmpty()
377-
) {
378-
"_inner"
379-
} else {
380-
"inner"
381-
}
375+
val innerRef = if (isEmptyStruct) "_inner" else "inner"
382376
serializeMember(
383377
MemberContext.unionMember(
384378
context.copy(writerExpression = "writer"),

‎codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/serialize/JsonSerializerGeneratorTest.kt‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -365,31 +365,33 @@ class JsonSerializerGeneratorTest {
365365
val operationGenerator = jsonSerializer.operationInputSerializer(model.lookup("test#TestOp"))
366366

367367
// Render all necessary structures and unions
368-
model.lookup<StructureShape>("test#Unit").renderWithModelBuilder(model, symbolProvider, project)
368+
model.lookup<StructureShape>("test#NoneFilter").renderWithModelBuilder(model, symbolProvider, project)
369+
model.lookup<StructureShape>("test#AesFilter").renderWithModelBuilder(model, symbolProvider, project)
369370
model.lookup<OperationShape>("test#TestOp").inputShape(model).renderWithModelBuilder(model, symbolProvider, project)
370371

371-
project.moduleFor(model.lookup<UnionShape>("test#TestUnion")) {
372-
UnionGenerator(model, symbolProvider, this, model.lookup("test#TestUnion")).render()
372+
project.moduleFor(model.lookup<UnionShape>("test#EncryptionFilter")) {
373+
UnionGenerator(model, symbolProvider, this, model.lookup("test#EncryptionFilter")).render()
373374
}
374375

375376
// Generate the actual protocol_serde module with union serialization
376377
project.lib {
377378
unitTest(
378379
"json_union_serialization",
379380
"""
380-
use test_model::{TestUnion, Unit};
381+
use test_model::{EncryptionFilter, NoneFilter};
381382
382-
// Create a test input to actually use the serializer
383+
// Create a test input using unit struct pattern that causes unused variable warnings
383384
let input = crate::test_input::TestOpInput::builder()
384-
.union(TestUnion::UnitMember(Unit::builder().build()))
385+
.filter(EncryptionFilter::None(NoneFilter::builder().build()))
385386
.build()
386387
.unwrap();
387388
388389
// This will generate and use the serialization code that should not have unused variable warnings
389-
let _serialized = ${format(operationGenerator!!)};
390-
let _result = _serialized(&input);
390+
let serialized = ${format(operationGenerator!!)}(&input).unwrap();
391391
392-
// Test that the code compiles and runs - this validates our fix works
392+
// Verify the serialization worked
393+
let output = std::str::from_utf8(serialized.bytes().unwrap()).unwrap();
394+
assert!(output.contains("none"));
393395
""",
394396
)
395397
}

‎codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/serialize/QuerySerializerGeneratorTest.kt‎

Lines changed: 53 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -13,33 +13,40 @@ import software.amazon.smithy.rust.codegen.core.smithy.generators.UnionGenerator
1313
import software.amazon.smithy.rust.codegen.core.smithy.transformers.OperationNormalizer
1414
import software.amazon.smithy.rust.codegen.core.smithy.transformers.RecursiveShapeBoxer
1515
import software.amazon.smithy.rust.codegen.core.testutil.TestWorkspace
16+
import software.amazon.smithy.rust.codegen.core.testutil.TestWriterDelegator
1617
import software.amazon.smithy.rust.codegen.core.testutil.asSmithyModel
17-
import software.amazon.smithy.rust.codegen.core.testutil.compileAndTest
1818
import software.amazon.smithy.rust.codegen.core.testutil.renderWithModelBuilder
19+
import software.amazon.smithy.rust.codegen.core.testutil.rustSettings
1920
import software.amazon.smithy.rust.codegen.core.testutil.testCodegenContext
2021
import software.amazon.smithy.rust.codegen.core.testutil.unitTest
2122
import software.amazon.smithy.rust.codegen.core.util.inputShape
2223
import software.amazon.smithy.rust.codegen.core.util.lookup
24+
import software.amazon.smithy.rust.codegen.core.util.runCommand
2325

2426
class QuerySerializerGeneratorTest {
2527
companion object {
2628
val unionWithUnitStructModel =
2729
"""
2830
namespace test
2931
30-
union TestUnion {
31-
unitMember: Unit
32+
union EncryptionFilter {
33+
none: NoneFilter,
34+
aes: AesFilter
3235
}
3336
34-
structure Unit {}
37+
structure NoneFilter {}
38+
39+
structure AesFilter {
40+
keyId: String
41+
}
3542
3643
@http(uri: "/test", method: "POST")
3744
operation TestOp {
38-
input: TestInput
45+
input: TestOpInput
3946
}
4047
41-
structure TestInput {
42-
union: TestUnion
48+
structure TestOpInput {
49+
filter: EncryptionFilter
4350
}
4451
""".asSmithyModel()
4552
}
@@ -57,35 +64,62 @@ class QuerySerializerGeneratorTest {
5764
val operationGenerator = querySerializer.operationInputSerializer(model.lookup("test#TestOp"))
5865

5966
// Render all necessary structures and unions
60-
model.lookup<StructureShape>("test#Unit").renderWithModelBuilder(model, symbolProvider, project)
67+
model.lookup<StructureShape>("test#NoneFilter").renderWithModelBuilder(model, symbolProvider, project)
68+
model.lookup<StructureShape>("test#AesFilter").renderWithModelBuilder(model, symbolProvider, project)
6169
model.lookup<OperationShape>("test#TestOp").inputShape(model).renderWithModelBuilder(model, symbolProvider, project)
6270

63-
project.moduleFor(model.lookup<UnionShape>("test#TestUnion")) {
64-
UnionGenerator(model, symbolProvider, this, model.lookup("test#TestUnion")).render()
71+
project.moduleFor(model.lookup<UnionShape>("test#EncryptionFilter")) {
72+
UnionGenerator(model, symbolProvider, this, model.lookup("test#EncryptionFilter")).render()
6573
}
6674

6775
// Generate the serialization module that will contain the union serialization code
6876
project.lib {
6977
unitTest(
7078
"test_query_union_serialization",
7179
"""
72-
use test_model::{TestUnion, Unit};
80+
use test_model::{EncryptionFilter, NoneFilter};
7381
74-
// Create a test input to actually use the serializer
82+
// Create a test input using unit struct pattern that causes unused variable warnings
7583
let input = crate::test_input::TestOpInput::builder()
76-
.union(TestUnion::UnitMember(Unit::builder().build()))
84+
.filter(EncryptionFilter::None(NoneFilter::builder().build()))
7785
.build()
7886
.unwrap();
7987
80-
// This will generate and use the serialization code that should not have unused variable warnings
81-
let _serialized = ${format(operationGenerator!!)};
82-
let _result = _serialized(&input);
83-
84-
// Test that the code compiles and runs - this validates our fix works
88+
let serialized = ${format(operationGenerator!!)}(&input).unwrap();
89+
let output = std::str::from_utf8(serialized.bytes().unwrap()).unwrap();
90+
assert!(!output.is_empty());
8591
""",
8692
)
8793
}
8894

89-
project.compileAndTest()
95+
// Compile with warnings as errors to ensure no unused variable warnings
96+
compileWithWarningsAsErrors(project)
97+
}
98+
99+
private fun compileWithWarningsAsErrors(project: TestWriterDelegator) {
100+
val stubModel =
101+
"""
102+
namespace fake
103+
service Fake {
104+
version: "123"
105+
}
106+
""".asSmithyModel()
107+
108+
project.finalize(
109+
project.rustSettings(),
110+
stubModel,
111+
manifestCustomizations = emptyMap(),
112+
libRsCustomizations = listOf(),
113+
)
114+
115+
try {
116+
"cargo fmt".runCommand(project.baseDir)
117+
} catch (e: Exception) {
118+
// cargo fmt errors are useless, ignore
119+
}
120+
121+
// Use RUSTFLAGS to treat unused variable warnings as errors, but allow dead code
122+
val env = mapOf("RUSTFLAGS" to "-D unused-variables -A dead-code")
123+
"cargo test".runCommand(project.baseDir, env)
90124
}
91125
}

‎rust-runtime/Cargo.lock‎

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎rust-runtime/aws-smithy-http/Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "aws-smithy-http"
3-
version = "0.62.3"
3+
version = "0.62.4"
44
authors = [
55
"AWS Rust SDK Team <aws-sdk-rust@amazon.com>",
66
"Russell Cohen <rcoh@amazon.com>",

‎rust-runtime/aws-smithy-json/Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "aws-smithy-json"
3-
version = "0.61.5"
3+
version = "0.61.6"
44
authors = ["AWS Rust SDK Team <aws-sdk-rust@amazon.com>", "John DiSanti <jdisanti@amazon.com>"]
55
description = "Token streaming JSON parser for smithy-rs."
66
edition = "2021"

‎rust-runtime/aws-smithy-types/Cargo.toml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "aws-smithy-types"
3-
version = "1.3.2"
3+
version = "1.3.3"
44
authors = [
55
"AWS Rust SDK Team <aws-sdk-rust@amazon.com>",
66
"Russell Cohen <rcoh@amazon.com>",

0 commit comments

Comments
 (0)