Skip to content

Commit 3538d39

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 3538d39

File tree

5 files changed

+88
-51
lines changed

5 files changed

+88
-51
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+
// Unit structs don't serialize inner, so it is never accessed
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: 5 additions & 11 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+
} else if (isEmptyStruct) {
565566
// Unit structs don't serialize inner, so it is never accessed
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: 5 additions & 11 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+
} else if (isEmptyStruct) {
368369
// Unit structs don't serialize inner, so it is never accessed
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: 55 additions & 17 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,66 @@ 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
8088
// 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);
89+
let serialized = ${format(operationGenerator!!)}(&input).unwrap();
8390
84-
// Test that the code compiles and runs - this validates our fix works
91+
// Verify the serialization worked - just check that it doesn't panic
92+
let output = std::str::from_utf8(serialized.bytes().unwrap()).unwrap();
93+
// For query serialization, the output format may be different
94+
assert!(!output.is_empty());
8595
""",
8696
)
8797
}
8898

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

0 commit comments

Comments
 (0)