smithy-lang · ysaito1001 · Nov 16, 2022 · Nov 2, 2022 · Nov 3, 2022 · Nov 4, 2022
@@ -103,3 +103,36 @@ message = "Upgrade to Smithy 1.26.2"
 references = ["smithy-rs#1972"]
 meta = { "breaking" = false, "tada" = false, "bug" = false }
 author = "rcoh"
+
+[[smithy-rs]]
+message = "Generate enums that guide the users to write match expressions in a forward-compatible way."
+references = ["smithy-rs#1945"]
+meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client"}
+author = "ysaito1001"
+
+[[aws-sdk-rust]]
+message = """
+Before this change, users could write a match expression against an enum in a non-forward-compatible way:
+```rust
+match some_enum {
+    SomeEnum::Variant1 => { /* ... */ },
+    SomeEnum::Variant2 => { /* ... */ },
+    Unknown(value) if value == "NewVariant" => { /* ... */ },
+    _ => { /* ... */ },
+}
+```
+This code can handle a case for "NewVariant" with a version of SDK where the enum does not yet include `SomeEnum::NewVariant`, but breaks with another version of SDK where the enum defines `SomeEnum::NewVariant` because the execution will hit a different match arm, i.e. the last one.
+After this change, users are guided to write the above match expression as follows:
+```rust
+match some_enum {
+    SomeEnum::Variant1 => { /* ... */ },
+    SomeEnum::Variant2 => { /* ... */ },
+    other @ _ if other.as_str() == "NewVariant" => { /* ... */ },
+    _ => { /* ... */ },
+}
+```
+This is forward-compatible because the execution will hit the second last match arm regardless of whether the enum defines `SomeEnum::NewVariant` or not.
+"""
+references = ["smithy-rs#1945"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "ysaito1001"
@@ -88,6 +88,7 @@ class CodegenVisitor(
             RustModule.Input,
             RustModule.Output,
             RustModule.Config,
+            RustModule.Types,
             RustModule.operation(Visibility.PUBLIC),
         ).associateBy { it.name }
         rustCrate = RustCrate(

@@ -8,15 +8,12 @@ package software.amazon.smithy.rust.codegen.client.smithy.customizations
 import software.amazon.smithy.model.Model
 import software.amazon.smithy.model.shapes.StructureShape
 import software.amazon.smithy.rust.codegen.core.rustlang.CargoDependency
+import software.amazon.smithy.rust.codegen.core.rustlang.RustModule
 import software.amazon.smithy.rust.codegen.core.rustlang.asType
-import software.amazon.smithy.rust.codegen.core.rustlang.docs
 import software.amazon.smithy.rust.codegen.core.rustlang.rust
-import software.amazon.smithy.rust.codegen.core.rustlang.rustBlock
-import software.amazon.smithy.rust.codegen.core.rustlang.writable
 import software.amazon.smithy.rust.codegen.core.smithy.RuntimeConfig
 import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
-import software.amazon.smithy.rust.codegen.core.smithy.generators.LibRsCustomization
-import software.amazon.smithy.rust.codegen.core.smithy.generators.LibRsSection
+import software.amazon.smithy.rust.codegen.core.smithy.RustCrate
 import software.amazon.smithy.rust.codegen.core.util.hasEventStreamMember
 import software.amazon.smithy.rust.codegen.core.util.hasStreamingMember
 
@@ -69,22 +66,12 @@ internal fun pubUseTypes(runtimeConfig: RuntimeConfig, model: Model): List<Runti
         ).filter { pubUseType -> pubUseType.shouldExport(model) }.map { it.type }
 }
 
-class SmithyTypesPubUseGenerator(private val runtimeConfig: RuntimeConfig) : LibRsCustomization() {
-    override fun section(section: LibRsSection) =
-        writable {
-            when (section) {
-                is LibRsSection.Body -> {
-                    val types = pubUseTypes(runtimeConfig, section.model)
-                    if (types.isNotEmpty()) {
-                        docs("Re-exported types from supporting crates.")
-                        rustBlock("pub mod types") {
-                            types.forEach { type -> rust("pub use #T;", type) }
-                        }
-                    }
-                }
-
-                else -> {
-                }
-            }
+/** Adds re-export statements in a separate file for the types module */
+fun pubUseSmithyTypes(runtimeConfig: RuntimeConfig, model: Model, rustCrate: RustCrate) {
+    rustCrate.withModule(RustModule.Types) {
+        val types = pubUseTypes(runtimeConfig, model)
+        if (types.isNotEmpty()) {
+            types.forEach { type -> rust("pub use #T;", type) }
         }
+    }
 }
@@ -15,10 +15,11 @@ import software.amazon.smithy.rust.codegen.client.smithy.customizations.HttpVers
 import software.amazon.smithy.rust.codegen.client.smithy.customizations.IdempotencyTokenGenerator
 import software.amazon.smithy.rust.codegen.client.smithy.customizations.ResiliencyConfigCustomization
 import software.amazon.smithy.rust.codegen.client.smithy.customizations.ResiliencyReExportCustomization
-import software.amazon.smithy.rust.codegen.client.smithy.customizations.SmithyTypesPubUseGenerator
+import software.amazon.smithy.rust.codegen.client.smithy.customizations.pubUseSmithyTypes
 import software.amazon.smithy.rust.codegen.client.smithy.generators.config.ConfigCustomization
 import software.amazon.smithy.rust.codegen.client.smithy.generators.protocol.ClientProtocolGenerator
 import software.amazon.smithy.rust.codegen.core.rustlang.Feature
+import software.amazon.smithy.rust.codegen.core.rustlang.rust
 import software.amazon.smithy.rust.codegen.core.smithy.CodegenContext
 import software.amazon.smithy.rust.codegen.core.smithy.RustCrate
 import software.amazon.smithy.rust.codegen.core.smithy.customize.OperationCustomization
@@ -55,7 +56,6 @@ class RequiredCustomizations : RustCodegenDecorator<ClientProtocolGenerator, Cli
         baseCustomizations: List<LibRsCustomization>,
     ): List<LibRsCustomization> =
         baseCustomizations + CrateVersionGenerator() +
-            SmithyTypesPubUseGenerator(codegenContext.runtimeConfig) +
             AllowLintsGenerator()
 
     override fun extras(codegenContext: ClientCodegenContext, rustCrate: RustCrate) {
@@ -64,6 +64,8 @@ class RequiredCustomizations : RustCodegenDecorator<ClientProtocolGenerator, Cli
 
         // Re-export resiliency types
         ResiliencyReExportCustomization(codegenContext.runtimeConfig).extras(rustCrate)
+
+        pubUseSmithyTypes(codegenContext.runtimeConfig, codegenContext.model, rustCrate)
     }
 
     override fun supportsCodegenContext(clazz: Class<out CodegenContext>): Boolean =

@@ -29,6 +29,10 @@ data class RustModule(val name: String, val rustMetadata: RustMetadata, val docu
         val Model = public("model", documentation = "Data structures used by operation inputs/outputs.")
         val Input = public("input", documentation = "Input structures for operations.")
         val Output = public("output", documentation = "Output structures for operations.")
+        val Types = public(
+            "types",
+            documentation = "Re-exported types from supporting crates and synthesized types from the service.",
+        )
 
         /**
          * Helper method to generate the `operation` Rust module.

@@ -57,6 +57,7 @@ abstract class SymbolMetadataProvider(private val base: RustSymbolProvider) : Wr
             is StringShape -> if (shape.hasTrait<EnumTrait>()) {
                 enumMeta(shape)
             } else null
+
             else -> null
         }
         return baseSymbol.toBuilder().meta(meta).build()
@@ -100,11 +101,13 @@ class BaseSymbolMetadataProvider(
                     )
                 }
             }
+
             container.isUnionShape ||
                 container.isListShape ||
                 container.isSetShape ||
                 container.isMapShape
             -> RustMetadata(visibility = Visibility.PUBLIC)
+
             else -> TODO("Unrecognized container type: $container")
         }
     }
@@ -120,9 +123,10 @@ class BaseSymbolMetadataProvider(
     override fun enumMeta(stringShape: StringShape): RustMetadata {
         return containerDefault.withDerives(
             RuntimeType.std.member("hash::Hash"),
-        ).withDerives( // enums can be eq because they can only contain strings
+        ).withDerives(
+            // enums can be eq because they can only contain ints and strings
             RuntimeType.std.member("cmp::Eq"),
-            // enums can be Ord because they can only contain strings
+            // enums can be Ord because they can only contain ints and strings
             RuntimeType.std.member("cmp::PartialOrd"),
             RuntimeType.std.member("cmp::Ord"),
         )

@@ -12,6 +12,7 @@ import software.amazon.smithy.model.traits.DocumentationTrait
 import software.amazon.smithy.model.traits.EnumDefinition
 import software.amazon.smithy.model.traits.EnumTrait
 import software.amazon.smithy.rust.codegen.core.rustlang.Attribute
+import software.amazon.smithy.rust.codegen.core.rustlang.RustModule
 import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
 import software.amazon.smithy.rust.codegen.core.rustlang.deprecatedShape
 import software.amazon.smithy.rust.codegen.core.rustlang.docs
@@ -99,6 +100,9 @@ open class EnumGenerator(
         /** Name of the generated unknown enum member name for enums with named members. */
         const val UnknownVariant = "Unknown"
 
+        /** Name of the opaque struct that is inner data for the generated [UnknownVariant]. */
+        const val UnknownVariantValue = "UnknownVariantValue"
+
         /** Name of the function on the enum impl to get a vec of value names */
         const val Values = "values"
     }
@@ -153,6 +157,10 @@ open class EnumGenerator(
     }
 
     private fun renderEnum() {
+        if (target.renderUnknownVariant()) {
+            writer.renderForwardCompatibilityNote(enumName, sortedMembers, UnknownVariant, UnknownVariantValue)
+        }
+
         val renamedWarning =
             sortedMembers.mapNotNull { it.name() }.filter { it.renamedFrom != null }.joinToString("\n") {
                 val previousName = it.renamedFrom!!
@@ -167,9 +175,9 @@ open class EnumGenerator(
         meta.render(writer)
         writer.rustBlock("enum $enumName") {
             sortedMembers.forEach { member -> member.render(writer) }
-            if (target == CodegenTarget.CLIENT) {
-                docs("$UnknownVariant contains new variants that have been added since this code was generated.")
-                rust("$UnknownVariant(String)")
+            if (target.renderUnknownVariant()) {
+                docs("`$UnknownVariant` contains new variants that have been added since this code was generated.")
+                rust("$UnknownVariant(#T)", unknownVariantValue())
             }
         }
     }
@@ -182,8 +190,8 @@ open class EnumGenerator(
                     sortedMembers.forEach { member ->
                         rust("""$enumName::${member.derivedName()} => ${member.value.dq()},""")
                     }
-                    if (target == CodegenTarget.CLIENT) {
-                        rust("$enumName::$UnknownVariant(s) => s.as_ref()")
+                    if (target.renderUnknownVariant()) {
+                        rust("$enumName::$UnknownVariant(value) => value.as_str()")
                     }
                 }
             }
@@ -198,14 +206,36 @@ open class EnumGenerator(
         }
     }
 
+    private fun unknownVariantValue(): RuntimeType {
+        return RuntimeType.forInlineFun(UnknownVariantValue, RustModule.Types) {
+            docs(
+                """
+                Opaque struct used as inner data for the `Unknown` variant defined in enums in
+                the crate
+
+                While this is not used by users directly, it is marked as `pub` because it is
+                part of the enums that are public interface.
+                """.trimIndent(),
+            )
+            meta.render(this)
+            rust("struct $UnknownVariantValue(pub(crate) String);")
+            rustBlock("impl $UnknownVariantValue") {
+                // The generated as_str is not pub as we need to prevent users from calling it on this opaque struct.
+                rustBlock("pub(crate) fn as_str(&self) -> &str") {
+                    rust("&self.0")
+                }
+            }
+        }
+    }
+
     protected open fun renderFromForStr() {
         writer.rustBlock("impl #T<&str> for $enumName", RuntimeType.From) {
             rustBlock("fn from(s: &str) -> Self") {
                 rustBlock("match s") {
                     sortedMembers.forEach { member ->
                         rust("""${member.value.dq()} => $enumName::${member.derivedName()},""")
                     }
-                    rust("other => $enumName::$UnknownVariant(other.to_owned())")
+                    rust("other => $enumName::$UnknownVariant(#T(other.to_owned()))", unknownVariantValue())
                 }
             }
         }
@@ -225,3 +255,61 @@ open class EnumGenerator(
         )
     }
 }
+
+/**
+ * Generate the rustdoc describing how to write a match expression against a generated enum in a
+ * forward-compatible way.
+ */
+private fun RustWriter.renderForwardCompatibilityNote(
+    enumName: String, sortedMembers: List<EnumMemberModel>,
+    unknownVariant: String, unknownVariantValue: String,
+) {
+    docs(
+        """
+        When writing a match expression against `$enumName`, it is important to ensure
+        your code is forward-compatible. That is, if a match arm handles a case for a
+        feature that is supported by the service but has not been represented as an enum
+        variant in a current version of SDK, your code should continue to work when you
+        upgrade SDK to a future version in which the enum does include a variant for that
+        feature.
+        """.trimIndent(),
+    )
+    docs("")
+    docs("Here is an example of how you can make a match expression forward-compatible:")
+    docs("")
+    docs("```text")
+    rust("/// ## let ${enumName.lowercase()} = unimplemented!();")
+    rust("/// match ${enumName.lowercase()} {")
+    sortedMembers.mapNotNull { it.name() }.forEach { member ->
+        rust("///     $enumName::${member.name} => { /* ... */ },")
+    }
+    rust("""///     other @ _ if other.as_str() == "NewFeature" => { /* handles a case for `NewFeature` */ },""")
+    rust("///     _ => { /* ... */ },")
+    rust("/// }")
+    docs("```")
+    docs(
+        """
+        The above code demonstrates that when `${enumName.lowercase()}` represents
+        `NewFeature`, the execution path will lead to the second last match arm,
+        even though the enum does not contain a variant `$enumName::NewFeature`
+        in the current version of SDK. The reason is that the variable `other`,
+        created by the `@` operator, is bound to
+        `$enumName::$unknownVariant($unknownVariantValue("NewFeature".to_owned()))`
+        and calling `as_str` on it yields `"NewFeature"`.
+        This match expression is forward-compatible when executed with a newer
+        version of SDK where the variant `$enumName::NewFeature` is defined.
+        Specifically, when `${enumName.lowercase()}` represents `NewFeature`,
+        the execution path will hit the second last match arm as before by virtue of
+        calling `as_str` on `$enumName::NewFeature` also yielding `"NewFeature"`.
+        """.trimIndent(),
+    )
+    docs("")
+    docs(
+        """
+        Explicitly matching on the `$unknownVariant` variant should
+        be avoided for two reasons:
+        - The inner data `$unknownVariantValue` is opaque, and no further information can be extracted.
+        - It might inadvertently shadow other intended match arms.
+        """.trimIndent(),
+    )
+}