Rework enum for forward-compatibility

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

@@ -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 the inner data also implements Eq
             RuntimeType.std.member("cmp::Eq"),
-            // enums can be Ord because they can only contain strings
+            // enums can be Ord because the inner data also implements Ord
             RuntimeType.std.member("cmp::PartialOrd"),
             RuntimeType.std.member("cmp::Ord"),
         )

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

@@ -99,6 +99,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"
     }
@@ -108,6 +111,10 @@ open class EnumGenerator(
             // pub enum Blah { V1, V2, .. }
             renderEnum()
             writer.insertTrailingNewline()
+            if (target == CodegenTarget.CLIENT) {
+                renderUnknownVariantValue()
+            }
+            writer.insertTrailingNewline()
             // impl From<str> for Blah { ... }
             renderFromForStr()
             // impl FromStr for Blah { ... }
@@ -153,6 +160,10 @@ open class EnumGenerator(
     }
 
     private fun renderEnum() {
+        if (target == CodegenTarget.CLIENT) {
+            writer.renderForwardCompatibilityNote(enumName, sortedMembers, UnknownVariant, UnknownVariantValue)
+        }
+
         val renamedWarning =
             sortedMembers.mapNotNull { it.name() }.filter { it.renamedFrom != null }.joinToString("\n") {
                 val previousName = it.renamedFrom!!
@@ -168,8 +179,8 @@ open class EnumGenerator(
         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)")
+                docs("`$UnknownVariant` contains new variants that have been added since this code was generated.")
+                rust("$UnknownVariant($UnknownVariantValue)")
             }
         }
     }
@@ -183,7 +194,7 @@ open class EnumGenerator(
                         rust("""$enumName::${member.derivedName()} => ${member.value.dq()},""")
                     }
                     if (target == CodegenTarget.CLIENT) {
-                        rust("$enumName::$UnknownVariant(s) => s.as_ref()")
+                        rust("$enumName::$UnknownVariant(value) => value.as_str()")
                     }
                 }
             }
@@ -198,14 +209,28 @@ open class EnumGenerator(
         }
     }
 
+    private fun renderUnknownVariantValue() {
+        // No doc or note comes with this inner opaque struct.
+        // We do want to mark it as #[allow(missing_docs)] to suppress the missing docs lint.
+        writer.docWithNote(null, null)
+        meta.render(writer)
+        writer.write("struct $UnknownVariantValue(String);")
+        writer.rustBlock("impl $UnknownVariantValue") {
+            // The generated as_str is not pub as we need to prevent users from calling it on this opaque struct.
+            rustBlock("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($UnknownVariantValue(other.to_owned()))")
                 }
             }
         }
@@ -225,3 +250,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(
+        """
+        It is worth pointing out that 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(),
+    )
+}

codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/EnumGeneratorTest.kt

@@ -9,6 +9,7 @@ import io.kotest.matchers.shouldBe
 import io.kotest.matchers.string.shouldContain
 import org.junit.jupiter.api.Nested
 import org.junit.jupiter.api.Test
+import software.amazon.smithy.model.Model
 import software.amazon.smithy.model.shapes.StringShape
 import software.amazon.smithy.model.traits.EnumTrait
 import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
@@ -117,7 +118,7 @@ class EnumGeneratorTest {
                 let instance = InstanceType::T2Micro;
                 assert_eq!(instance.as_str(), "t2.micro");
                 assert_eq!(InstanceType::from("t2.nano"), InstanceType::T2Nano);
-                assert_eq!(InstanceType::from("other"), InstanceType::Unknown("other".to_owned()));
+                assert_eq!(InstanceType::from("other"), InstanceType::Unknown(UnknownVariantValue("other".to_owned())));
                 // round trip unknown variants:
                 assert_eq!(InstanceType::from("other").as_str(), "other");
                 """,
@@ -250,7 +251,7 @@ class EnumGeneratorTest {
                 """
                 assert_eq!(SomeEnum::from("Unknown"), SomeEnum::UnknownValue);
                 assert_eq!(SomeEnum::from("UnknownValue"), SomeEnum::UnknownValue_);
-                assert_eq!(SomeEnum::from("SomethingNew"), SomeEnum::Unknown("SomethingNew".into()));
+                assert_eq!(SomeEnum::from("SomethingNew"), SomeEnum::Unknown(UnknownVariantValue("SomethingNew".to_owned())));
                 """,
             )
         }
@@ -271,7 +272,9 @@ class EnumGeneratorTest {
             val shape: StringShape = model.lookup("test#SomeEnum")
             val trait = shape.expectTrait<EnumTrait>()
             val provider = testSymbolProvider(model)
-            val rendered = RustWriter.forModule("model").also { EnumGenerator(model, provider, it, shape, trait).render() }.toString()
+            val rendered =
+                RustWriter.forModule("model").also { EnumGenerator(model, provider, it, shape, trait).render() }
+                    .toString()
 
             rendered shouldContain
                 """
@@ -297,7 +300,9 @@ class EnumGeneratorTest {
             val shape: StringShape = model.lookup("test#SomeEnum")
             val trait = shape.expectTrait<EnumTrait>()
             val provider = testSymbolProvider(model)
-            val rendered = RustWriter.forModule("model").also { EnumGenerator(model, provider, it, shape, trait).render() }.toString()
+            val rendered =
+                RustWriter.forModule("model").also { EnumGenerator(model, provider, it, shape, trait).render() }
+                    .toString()
 
             rendered shouldContain
                 """
@@ -326,8 +331,54 @@ class EnumGeneratorTest {
         writer.compileAndTest(
             """
             assert_eq!(SomeEnum::from("other"), SomeEnum::SelfValue);
-            assert_eq!(SomeEnum::from("SomethingNew"), SomeEnum::Unknown("SomethingNew".into()));
+            assert_eq!(SomeEnum::from("SomethingNew"), SomeEnum::Unknown(UnknownVariantValue("SomethingNew".to_owned())));
             """,
         )
     }
+
+    @Test
+    fun `matching on enum should be forward-compatible`() {
+        fun expectMatchExpressionCompiles(model: Model, shapeId: String, enumToMatchOn: String) {
+            val shape: StringShape = model.lookup(shapeId)
+            val trait = shape.expectTrait<EnumTrait>()
+            val provider = testSymbolProvider(model)
+            val writer = RustWriter.forModule("model")
+            EnumGenerator(model, provider, writer, shape, trait).render()
+
+            val matchExpressionUnderTest = """
+                match $enumToMatchOn {
+                    SomeEnum::Variant1 => assert!(false, "expected `Variant3` but got `Variant1`"),
+                    SomeEnum::Variant2 => assert!(false, "expected `Variant3` but got `Variant2`"),
+                    other @ _ if other.as_str() == "Variant3" => assert!(true),
+                    _ => assert!(false, "expected `Variant3` but got `_`"),
+                }
+            """
+            writer.compileAndTest(matchExpressionUnderTest)
+        }
+
+        val modelV1 = """
+            namespace test
+
+            @enum([
+                { name: "Variant1", value: "Variant1" },
+                { name: "Variant2", value: "Variant2" },
+            ])
+            string SomeEnum
+        """.asSmithyModel()
+        val variant3AsUnknown = """SomeEnum::Unknown(UnknownVariantValue("Variant3".to_owned()))"""
+        expectMatchExpressionCompiles(modelV1, "test#SomeEnum", variant3AsUnknown)
+
+        val modelV2 = """
+            namespace test
+
+            @enum([
+                { name: "Variant1", value: "Variant1" },
+                { name: "Variant2", value: "Variant2" },
+                { name: "Variant3", value: "Variant3" },
+            ])
+            string SomeEnum
+        """.asSmithyModel()
+        val variant3AsVariant3 = "SomeEnum::Variant3"
+        expectMatchExpressionCompiles(modelV2, "test#SomeEnum", variant3AsVariant3)
+    }
 }