Unverified Commit 577c90b7 authored by ysaito1001's avatar ysaito1001 Committed by GitHub
Browse files

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: default avatarJohn 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 https://github.com/awslabs/smithy-rs/pull/1989#discussion_r1035372417

.
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: default avatarJohn DiSanti <jdisanti@amazon.com>

* Simplify if-else in jsonObjectWriter

This commit addresses https://github.com/awslabs/smithy-rs/pull/1989#discussion_r1037651893

* Avoid the union member's reference name being empty

This commit addresses https://github.com/awslabs/smithy-rs/pull/1989#discussion_r1037654601


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: default avatarYuki Saito <awsaito@amazon.com>
Co-authored-by: default avatarJohn DiSanti <jdisanti@amazon.com>
parent 31f1d35b

CHANGELOG.next.toml

+17 −1
Original line number Diff line number Diff line
@@ -661,3 +661,19 @@ message = "Clients now default max idle connections to 70 (previously unlimited) 
references = ["smithy-rs#2064", "aws-sdk-rust#632"]

meta = { "breaking" = false, "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"

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

+5 −2
Original line number Diff line number Diff line
@@ -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



/**
@@ -258,10 +259,12 @@ open class Instantiator( 
        val member = shape.expectMember(memberName)

        writer.rust("#T::${symbolProvider.toMemberName(member)}", unionSymbol)

        // Unions should specify exactly one member.

        if (!member.isTargetUnit()) {

            writer.withBlock("(", ")") {

                renderMember(this, member, variant.second, ctx)

            }

        }

    }



    /**

     * ```rust

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

+57 −23
Original line number Diff line number Diff line
@@ -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
@@ -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
@@ -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()
@@ -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)
@@ -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.""")
@@ -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)
@@ -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()")
@@ -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) {
@@ -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>") {

            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) }")

        }

    }

}

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

+15 −3
Original line number Diff line number Diff line
@@ -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
@@ -311,6 +312,7 @@ class JsonParserGenerator( 
                        rust("#T::from(u.as_ref())", symbolProvider.toSymbol(target))

                    }

                }



                else -> rust("u.into_owned()")

            }

        }
@@ -510,12 +512,22 @@ class JsonParserGenerator( 
                                for (member in shape.members()) {

                                    val variantName = symbolProvider.toMemberName(member)

                                    rustBlock("${jsonName(member).dq()} =>") {

                                        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)

                                            }

                                        }

                                    }

                                }

                                when (codegenTarget.renderUnknownVariant()) {

                                    // In client mode, resolve an unknown union variant to the unknown variant.

                                    true -> rustTemplate(

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

+16 −11
Original line number Diff line number Diff line
@@ -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
@@ -420,6 +421,9 @@ class XmlBindingTraitParserGenerator( 
                    members.forEach { member ->

                        val variantName = symbolProvider.toMemberName(member)

                        case(member) {

                            if (member.isTargetUnit()) {

                                rust("base = Some(#T::$variantName);", symbol)

                            } else {

                                val current =

                                    """

                                    (match base.take() {
@@ -434,6 +438,7 @@ class XmlBindingTraitParserGenerator( 
                                rust("base = Some(#T::$variantName(tmp));", symbol)

                            }

                        }

                    }

                    when (target.renderUnknownVariant()) {

                        true -> rust("_unknown => base = Some(#T::${UnionGenerator.UnknownVariantName}),", symbol)

                        false -> rustTemplate("""variant => return Err(#{XmlDecodeError}::custom(format!("unexpected union variant: {:?}", variant)))""", *codegenScope)