Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render a Union member of type Unit to an enum variant without inner data #1989

Merged
merged 25 commits into from
Dec 7, 2022
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
bdf7858
Avoid explicitly emitting Unit type within Union
ysaito1001 Nov 15, 2022
2b148aa
Address test failures washed out in CI
ysaito1001 Nov 16, 2022
1f702df
Update codegen-core/src/test/kotlin/software/amazon/smithy/rust/codeg…
ysaito1001 Nov 16, 2022
11caf0c
Remove commented-out code
ysaito1001 Nov 16, 2022
a7002aa
Merge branch 'ysaito/remove-unit-from-generated-rust-enum' of https:/…
ysaito1001 Nov 16, 2022
fd60317
Add a helper for comparing against ShapeId for Unit
ysaito1001 Nov 17, 2022
2455eb6
Merge branch 'main' into ysaito/remove-unit-from-generated-rust-enum
ysaito1001 Nov 17, 2022
74041ca
Move Unit type bifurcation logic to jsonObjectWriter
ysaito1001 Nov 17, 2022
5802bd5
Make QuerySerializerGenerator in sync with the change
ysaito1001 Nov 17, 2022
d48b115
Update CHANGELOG.next.toml
ysaito1001 Nov 17, 2022
ccd2e63
Merge branch 'main' into ysaito/remove-unit-from-generated-rust-enum
ysaito1001 Nov 17, 2022
de1f5cf
Refactor ofTypeUnit -> isTargetUnit
ysaito1001 Nov 30, 2022
7c3a0a8
Merge branch 'main' into ysaito/remove-unit-from-generated-rust-enum
ysaito1001 Nov 30, 2022
5e95a55
Update codegen-core/src/main/kotlin/software/amazon/smithy/rust/codeg…
ysaito1001 Dec 3, 2022
4b9a355
Simplify if-else in jsonObjectWriter
ysaito1001 Dec 3, 2022
e4f6559
Avoid the union member's reference name being empty
ysaito1001 Dec 3, 2022
0e8b621
CHANGELOG.next.toml
ysaito1001 Dec 3, 2022
45ef80d
Ensure Union with Unit target can be serialized
ysaito1001 Dec 6, 2022
884155e
Ensure Union with Unit target can be parsed
ysaito1001 Dec 6, 2022
9962214
Merge branch 'main' into ysaito/remove-unit-from-generated-rust-enum
ysaito1001 Dec 6, 2022
2269361
Ensure match arm for Unit works in custom Debug impl
ysaito1001 Dec 6, 2022
5445282
Merge branch 'main' into ysaito/remove-unit-from-generated-rust-enum
ysaito1001 Dec 6, 2022
fd8e8fc
Fix unused variables warnings in CI
ysaito1001 Dec 7, 2022
2d1f01d
Fix E0658 on unused_variables
ysaito1001 Dec 7, 2022
d0c8b0b
Merge branch 'main' into ysaito/remove-unit-from-generated-rust-enum
ysaito1001 Dec 7, 2022
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
16 changes: 16 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -637,3 +637,19 @@ message = "`SdkBody` callbacks have been removed. If you were using these, pleas
references = ["smithy-rs#2065"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client"}
author = "jdisanti"

[[aws-sdk-rust]]
message = """
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.
"""
references = ["smithy-rs#1989"]
meta = { "breaking" = true, "tada" = false, "bug" = false }
author = "ysaito1001"

[[smithy-rs]]
message = """
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.
"""
references = ["smithy-rs#1989"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "all" }
author = "ysaito1001"
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.rustType
import software.amazon.smithy.rust.codegen.core.util.dq
import software.amazon.smithy.rust.codegen.core.util.expectMember
import software.amazon.smithy.rust.codegen.core.util.hasTrait
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
import software.amazon.smithy.rust.codegen.core.util.letIf

/**
Expand Down Expand Up @@ -258,8 +259,10 @@ open class Instantiator(
val member = shape.expectMember(memberName)
writer.rust("#T::${symbolProvider.toMemberName(member)}", unionSymbol)
// Unions should specify exactly one member.
writer.withBlock("(", ")") {
renderMember(this, member, variant.second, ctx)
if (!member.isTargetUnit()) {
writer.withBlock("(", ")") {
renderMember(this, member, variant.second, ctx)
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

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

import software.amazon.smithy.codegen.core.Symbol
import software.amazon.smithy.codegen.core.SymbolProvider
import software.amazon.smithy.model.Model
import software.amazon.smithy.model.shapes.MemberShape
Expand All @@ -25,6 +26,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.renamedFrom
import software.amazon.smithy.rust.codegen.core.util.REDACTION
import software.amazon.smithy.rust.codegen.core.util.dq
import software.amazon.smithy.rust.codegen.core.util.hasTrait
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
import software.amazon.smithy.rust.codegen.core.util.shouldRedact
import software.amazon.smithy.rust.codegen.core.util.toSnakeCase

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

fun render() {
renderUnion()
renderImplBlock()
writer.documentShape(shape, model)
writer.deprecatedShape(shape)

val containerMeta = unionSymbol.expectRustMetadata()
containerMeta.render(writer)

renderUnion(unionSymbol)
renderImplBlock(unionSymbol)
if (!unionSymbol.expectRustMetadata().derives.derives.contains(RuntimeType.Debug)) {
if (shape.hasTrait<SensitiveTrait>()) {
renderFullyRedactedDebugImpl()
Expand All @@ -68,12 +76,7 @@ class UnionGenerator(
}
}

private fun renderUnion() {
writer.documentShape(shape, model)
writer.deprecatedShape(shape)

val containerMeta = unionSymbol.expectRustMetadata()
containerMeta.render(writer)
private fun renderUnion(unionSymbol: Symbol) {
writer.rustBlock("enum ${unionSymbol.name}") {
sortedMembers.forEach { member ->
val memberSymbol = symbolProvider.toSymbol(member)
Expand All @@ -82,7 +85,7 @@ class UnionGenerator(
documentShape(member, model, note = note)
deprecatedShape(member)
memberSymbol.expectRustMetadata().renderAttributes(this)
write("${symbolProvider.toMemberName(member)}(#T),", symbolProvider.toSymbol(member))
writer.renderVariant(symbolProvider, member, memberSymbol)
}
if (renderUnknownVariant) {
docs("""The `Unknown` variant represents cases where new union variant was received. Consider upgrading the SDK to the latest available version.""")
Expand All @@ -99,7 +102,7 @@ class UnionGenerator(
}
}

private fun renderImplBlock() {
private fun renderImplBlock(unionSymbol: Symbol) {
writer.rustBlock("impl ${unionSymbol.name}") {
sortedMembers.forEach { member ->
val memberSymbol = symbolProvider.toSymbol(member)
Expand All @@ -109,15 +112,7 @@ class UnionGenerator(
if (sortedMembers.size == 1) {
Attribute.Custom("allow(irrefutable_let_patterns)").render(this)
}
rust(
"/// Tries to convert the enum instance into [`$variantName`](#T::$variantName), extracting the inner #D.",
unionSymbol,
memberSymbol,
)
rust("/// Returns `Err(&Self)` if it can't be converted.")
rustBlock("pub fn as_$funcNamePart(&self) -> std::result::Result<&#T, &Self>", memberSymbol) {
rust("if let ${unionSymbol.name}::$variantName(val) = &self { Ok(val) } else { Err(self) }")
}
writer.renderAsVariant(member, variantName, funcNamePart, unionSymbol, memberSymbol)
rust("/// Returns true if this is a [`$variantName`](#T::$variantName).", unionSymbol)
rustBlock("pub fn is_$funcNamePart(&self) -> bool") {
rust("self.as_$funcNamePart().is_ok()")
Expand Down Expand Up @@ -152,10 +147,13 @@ class UnionGenerator(
rustBlock("match self") {
sortedMembers.forEach { member ->
val memberName = symbolProvider.toMemberName(member)
if (member.shouldRedact(model)) {
rust("${unionSymbol.name}::$memberName(_) => f.debug_tuple($REDACTION).finish(),")
} else {
rust("${unionSymbol.name}::$memberName(val) => f.debug_tuple(${memberName.dq()}).field(&val).finish(),")
val shouldRedact = member.shouldRedact(model)
val isTargetUnit = member.isTargetUnit()
when {
!shouldRedact && isTargetUnit -> rust("${unionSymbol.name}::$memberName => f.debug_tuple(${memberName.dq()}).finish(),")
!shouldRedact && !isTargetUnit -> rust("${unionSymbol.name}::$memberName(val) => f.debug_tuple(${memberName.dq()}).field(&val).finish(),")
// We can always render (_) because the Unit target in a Union cannot be marked as sensitive separately.
else -> rust("${unionSymbol.name}::$memberName(_) => f.debug_tuple($REDACTION).finish(),")
}
}
if (renderUnknownVariant) {
Expand All @@ -175,3 +173,39 @@ fun unknownVariantError(union: String) =
"Cannot serialize `$union::${UnionGenerator.UnknownVariantName}` for the request. " +
"The `Unknown` variant is intended for responses only. " +
"It occurs when an outdated client is used after a new enum variant was added on the server side."

private fun RustWriter.renderVariant(symbolProvider: SymbolProvider, member: MemberShape, memberSymbol: Symbol) {
if (member.isTargetUnit()) {
write("${symbolProvider.toMemberName(member)},")
} else {
write("${symbolProvider.toMemberName(member)}(#T),", memberSymbol)
}
}

private fun RustWriter.renderAsVariant(
member: MemberShape,
variantName: String,
funcNamePart: String,
unionSymbol: Symbol,
memberSymbol: Symbol,
) {
if (member.isTargetUnit()) {
rust(
"/// Tries to convert the enum instance into [`$variantName`], extracting the inner `()`.",
)
rust("/// Returns `Err(&Self)` if it can't be converted.")
rustBlock("pub fn as_$funcNamePart(&self) -> std::result::Result<(), &Self>") {
ysaito1001 marked this conversation as resolved.
Show resolved Hide resolved
rust("if let ${unionSymbol.name}::$variantName = &self { Ok(()) } else { Err(self) }")
}
} else {
rust(
"/// Tries to convert the enum instance into [`$variantName`](#T::$variantName), extracting the inner #D.",
unionSymbol,
memberSymbol,
)
rust("/// Returns `Err(&Self)` if it can't be converted.")
rustBlock("pub fn as_$funcNamePart(&self) -> std::result::Result<&#T, &Self>", memberSymbol) {
rust("if let ${unionSymbol.name}::$variantName(val) = &self { Ok(val) } else { Err(self) }")
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import software.amazon.smithy.rust.codegen.core.util.PANIC
import software.amazon.smithy.rust.codegen.core.util.dq
import software.amazon.smithy.rust.codegen.core.util.hasTrait
import software.amazon.smithy.rust.codegen.core.util.inputShape
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
import software.amazon.smithy.rust.codegen.core.util.outputShape
import software.amazon.smithy.utils.StringUtils

Expand Down Expand Up @@ -311,6 +312,7 @@ class JsonParserGenerator(
rust("#T::from(u.as_ref())", symbolProvider.toSymbol(target))
}
}

else -> rust("u.into_owned()")
}
}
Expand Down Expand Up @@ -510,9 +512,19 @@ class JsonParserGenerator(
for (member in shape.members()) {
val variantName = symbolProvider.toMemberName(member)
rustBlock("${jsonName(member).dq()} =>") {
withBlock("Some(#T::$variantName(", "))", returnSymbolToParse.symbol) {
deserializeMember(member)
unwrapOrDefaultOrError(member)
if (member.isTargetUnit()) {
rustTemplate(
"""
#{skip_value}(tokens)?;
Some(#{Union}::$variantName)
""",
"Union" to returnSymbolToParse.symbol, *codegenScope,
)
} else {
withBlock("Some(#T::$variantName(", "))", returnSymbolToParse.symbol) {
deserializeMember(member)
unwrapOrDefaultOrError(member)
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import software.amazon.smithy.rust.codegen.core.util.dq
import software.amazon.smithy.rust.codegen.core.util.expectMember
import software.amazon.smithy.rust.codegen.core.util.hasTrait
import software.amazon.smithy.rust.codegen.core.util.inputShape
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
import software.amazon.smithy.rust.codegen.core.util.outputShape

// The string argument is the name of the XML ScopedDecoder to continue parsing from
Expand Down Expand Up @@ -420,18 +421,22 @@ class XmlBindingTraitParserGenerator(
members.forEach { member ->
val variantName = symbolProvider.toMemberName(member)
case(member) {
val current =
"""
(match base.take() {
None => None,
Some(${format(symbol)}::$variantName(inner)) => Some(inner),
Some(_) => return Err(#{XmlDecodeError}::custom("mixed variants"))
})
"""
withBlock("let tmp =", ";") {
parseMember(member, ctx.copy(accum = current.trim()))
if (member.isTargetUnit()) {
rust("base = Some(#T::$variantName);", symbol)
} else {
val current =
"""
(match base.take() {
None => None,
Some(${format(symbol)}::$variantName(inner)) => Some(inner),
Some(_) => return Err(#{XmlDecodeError}::custom("mixed variants"))
})
"""
withBlock("let tmp =", ";") {
parseMember(member, ctx.copy(accum = current.trim()))
}
rust("base = Some(#T::$variantName(tmp));", symbol)
}
rust("base = Some(#T::$variantName(tmp));", symbol)
}
}
when (target.renderUnknownVariant()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticOutputTra
import software.amazon.smithy.rust.codegen.core.util.dq
import software.amazon.smithy.rust.codegen.core.util.expectTrait
import software.amazon.smithy.rust.codegen.core.util.inputShape
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
import software.amazon.smithy.rust.codegen.core.util.outputShape

/**
Expand Down Expand Up @@ -447,8 +448,22 @@ class JsonSerializerGenerator(

private fun RustWriter.jsonObjectWriter(context: MemberContext, inner: RustWriter.(String) -> Unit) {
safeName("object").also { objectName ->
rust("##[allow(unused_mut)]")
rust("let mut $objectName = ${context.writerExpression}.start_object();")
inner(objectName)
// We call inner only when context's shape is not the Unit type.
// If it were, calling inner would generate the following function:
// pub fn serialize_structure_crate_model_unit(
// object: &mut aws_smithy_json::serialize::JsonObjectWriter,
// input: &crate::model::Unit,
// ) -> Result<(), aws_smithy_http::operation::error::SerializationError> {
// let (_, _) = (object, input);
// Ok(())
// }
// However, this would cause a compilation error at a call site because it cannot
// extract data out of the Unit type that corresponds to the variable "input" above.
if (!context.shape.isTargetUnit()) {
inner(objectName)
}
rust("$objectName.finish();")
}
}
Expand Down Expand Up @@ -488,8 +503,12 @@ class JsonSerializerGenerator(
) {
rustBlock("match input") {
for (member in context.shape.members()) {
val variantName = symbolProvider.toMemberName(member)
withBlock("#T::$variantName(inner) => {", "},", unionSymbol) {
val variantName = if (member.isTargetUnit()) {
"${symbolProvider.toMemberName(member)}"
} else {
"${symbolProvider.toMemberName(member)}(inner)"
}
withBlock("#T::$variantName => {", "},", unionSymbol) {
serializeMember(MemberContext.unionMember(context, "inner", member, jsonName))
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import software.amazon.smithy.rust.codegen.core.util.dq
import software.amazon.smithy.rust.codegen.core.util.getTrait
import software.amazon.smithy.rust.codegen.core.util.hasTrait
import software.amazon.smithy.rust.codegen.core.util.inputShape
import software.amazon.smithy.rust.codegen.core.util.isTargetUnit
import software.amazon.smithy.rust.codegen.core.util.orNull

abstract class QuerySerializerGenerator(codegenContext: CodegenContext) : StructuredDataSerializerGenerator {
Expand Down Expand Up @@ -151,6 +152,21 @@ abstract class QuerySerializerGenerator(codegenContext: CodegenContext) : Struct
}

private fun RustWriter.serializeStructure(context: Context<StructureShape>) {
// We proceed with the rest of the method only when context.shape.members() is nonempty.
// If it were empty, the method would generate the following code:
// #[allow(unused_mut)]
// pub fn serialize_structure_crate_model_unit(
// mut writer: aws_smithy_query::QueryValueWriter,
// input: &crate::model::Unit,
// ) -> Result<(), aws_smithy_http::operation::error::SerializationError> {
// let (_, _) = (writer, input);
// Ok(())
// }
// However, this would cause a compilation error at a call site because it cannot
// extract data out of the Unit type that corresponds to the variable "input" above.
if (context.shape.members().isEmpty()) {
return
}
val fnName = symbolProvider.serializeFunctionName(context.shape)
val structureSymbol = symbolProvider.toSymbol(context.shape)
val structureSerializer = RuntimeType.forInlineFun(fnName, querySerModule) {
Expand All @@ -160,9 +176,6 @@ abstract class QuerySerializerGenerator(codegenContext: CodegenContext) : Struct
"Input" to structureSymbol,
*codegenScope,
) {
if (context.shape.members().isEmpty()) {
rust("let (_, _) = (writer, input);") // Suppress unused argument warnings
}
serializeStructureInner(context)
rust("Ok(())")
}
Expand Down Expand Up @@ -311,8 +324,12 @@ abstract class QuerySerializerGenerator(codegenContext: CodegenContext) : Struct
) {
rustBlock("match input") {
for (member in context.shape.members()) {
val variantName = symbolProvider.toMemberName(member)
withBlock("#T::$variantName(inner) => {", "},", unionSymbol) {
val variantName = if (member.isTargetUnit()) {
"${symbolProvider.toMemberName(member)}"
} else {
"${symbolProvider.toMemberName(member)}(inner)"
}
withBlock("#T::$variantName => {", "},", unionSymbol) {
serializeMember(
MemberContext.unionMember(
context.copy(writerExpression = "writer"),
Expand Down
Loading