Skip to content

Commit

Permalink
Rework enum for forward-compatibility (#1945)
Browse files Browse the repository at this point in the history
* Make enum forward-compatible

This commit implements the suggested approach described in
#627 (comment)

The idea is that once the user writes a match expression against an enum
and assumes that an execution path comes to a particular match arm, we
should guarantee that when the user upgrades a version of SDK, the
execution path should come to the same match arm as before.

* Add unit test to ensure enums are forward-compatible

The test first mimics the user's interaction with the enum generated from
a model where the user writes a match expression on the enum, wishing to
hit the match arm corresponding to Variant3, which is not yet supported
by the model. The test then simulates a scenario where the user now has
access to the updated enum generated from the next version of the model
that does support Variant3.
The user's code should compile in both of the use cases.

* Generate rustdoc for enum's forward-compatibility

This commits generates rustdoc explaining to the users how they might
write a match expression against a generated enum so their code is
forward-compatible. While it is explained in
#627 (comment),
it can be helpful if the users can read it in rustdoc right below where
the enum's definition is shown.

* Make snippet in rustdoc text

This commit updates a rustdoc code snippet generated for an enum from
`rust,no_run` to `text`. We observed that the snippet fails to compile
in CI because the name of the enum was not fully qualified; code in
rustdoc is treated in a similar way that code in integration tests is
treated as being outside of a crate, but not part of the crate, which
means the name of the crate needs to be spelled out for a `use` statement
if we want to import the enum to the code in rustdoc. However, the name
of the crate is usually one-off, generated on the fly for testing, making
it not obvious to hard-code it or obtain it programmatically from within
Kotlin code.

* Suppress missing doc lint for UnknownVariantValue

This commit marks UnknownVariantValue as `#[allow(missing_docs)]` because
failing to do so will cause tests in CI to fail.

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

Co-authored-by: Russell Cohen <[email protected]>

* Generate UnknownVariantValue via forInlineFun

This commit attempts to create UnknownVariantValue using
RuntimeType.forInlineFun. Prior to this commit, it was generated per enum
but that caused multiple definitions of UnknownVariantValue in a single
file as there can be multiple enums in the file.
By using RuntimeType.forInlineFun, we can generate a single UnknownVariantValue
per module and the enums within the module can refer to the same instance.

This commit, however, fails to pass the unit tests in EnumGeneratorTest.
The issue seems to be that a RustWriter used in the tests does not end up
calling the finalize method, failing to render inline functions.

* Replace "target == CodegenTarget.CLIENT" with a helper

This commit addresses #1945 (comment)
but uses an existing helper CodegenTarget.renderUnknownVariant.

* Update EnumGeneratorTests to use TestWorkspace

This commit addresses failures for unit tests in EnumGeneratorTests.
Now that we use RuntimeType.forInlineFcn in EnumGenerator, we need
to ensure that the inline functions should be rendered to a Rust
source file during testing. RustWriter, used in EnumGeneratorTests
prior to this commit, was not capable of doing so. We thus update
EnumGeneratorTests to use TestWorkspace by which we ensure that the
inline functions are rendered during testing.

* Make sure to use the passed-in variable for shapeId

This commit fixes a copy-and-paste error introduced in 712c983. The
function should use the passed-in variable rather than the hard-coded
literal "test#SomeEnum".

* Address https://github.com/awslabs/smithy-rs/pull/1945\#discussion_r1014389852

* Update CHANGELOG.next.toml

* Update CHANGELOG.next.toml

This commit addresses #1945 (comment)

* Avoid potential name collisions by UnknownVariantValue

This commit addresses #1945 (comment).
It changes the module in which the `UnknownVariantValue` is rendered.
Before the commit, it was emitted to the `model` module but that might cause
name collisions in the future when a Smithy model has a shape named
`UnknownVariantValue`. After this commit, we render it to the `types` module.

* Move re-exports from lib.rs to types.rs

This commit moves re-export statements in client crates from `lib.rs` to
`types.rs`. The motivation is that now that we render the struct
`UnknownVariantValue` in a separate file for the `types` module, we can
no longer have a `pub mod types {...}` block in `lib.rs` as it cannot
coexist with `pub mod types;`.

* Add docs on UnknownVariantValue

This commit adds public docs on `UnknownVariantValue`. Since it is
rendered in `types.rs`, the users may not find the `Unknown` variant
of an enum in the same file and may wonder what this struct is for when
seeing it in isolation.

* Update CHANGELOG.next.toml

This commit addresses #1945 (comment)

* Update the module documentation for types

This commit updates rustdoc for the types module to reflect that fact
that the module now contains not only re-exports but also an auxiliary
struct `UnknownVariantValue`.

* Add extensions to run code block depending on the target

This commit introduces extensions on CodegenTarget that execute the block
of code depending on the target is for client or for server. These
extensions are intended to replace if-else expressions throughout the
codebase explicitly checking whether the target is for client or for server.
We do not update such if-else expressions all at once in this commit.
Rather, we are planning to make incremental changes to update them
opportunistically.

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

Co-authored-by: John DiSanti <[email protected]>

* Move extensions into CodegenTarget as methods

This commit addresses #1945 (comment).
By moving extensions into the CodegenTarget enum, no separate import is
required to use them.

Co-authored-by: Saito <[email protected]>
Co-authored-by: Russell Cohen <[email protected]>
Co-authored-by: Yuki Saito <[email protected]>
Co-authored-by: John DiSanti <[email protected]>
  • Loading branch information
5 people authored Nov 16, 2022
1 parent 8ef985e commit cdb4a3f
Show file tree
Hide file tree
Showing 10 changed files with 381 additions and 125 deletions.
56 changes: 56 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -339,3 +339,59 @@ Server SDKs now correctly reject operation inputs that don't set values for `req
references = ["smithy-rs#1714", "smithy-rs#1342", "smithy-rs#1860"]
meta = { "breaking" = true, "tada" = false, "bug" = true, "target" = "server" }
author = "david-perez"

[[smithy-rs]]
message = """
Generate enums that guide the users to write match expressions in a forward-compatible way.
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, "target" = "client"}
author = "ysaito1001"

[[aws-sdk-rust]]
message = """
Generate enums that guide the users to write match expressions in a forward-compatible way.
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"
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ class CodegenVisitor(
RustModule.Input,
RustModule.Output,
RustModule.Config,
RustModule.Types,
RustModule.operation(Visibility.PUBLIC),
).associateBy { it.name }
rustCrate = RustCrate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -71,22 +68,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) }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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 =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ 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 = "Data primitives referenced by other data types.")

/**
* Helper method to generate the `operation` Rust module.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,23 @@ package software.amazon.smithy.rust.codegen.core.smithy
* Code generation mode: In some situations, codegen has different behavior for client vs. server (eg. required fields)
*/
enum class CodegenTarget {
CLIENT, SERVER
CLIENT, SERVER;

/**
* Convenience method to execute thunk if the target is for CLIENT
*/
fun <B> ifClient(thunk: () -> B): B? = if (this == CLIENT) {
thunk()
} else {
null
}

/**
* Convenience method to execute thunk if the target is for SERVER
*/
fun <B> ifServer(thunk: () -> B): B? = if (this == SERVER) {
thunk()
} else {
null
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -100,11 +101,13 @@ class BaseSymbolMetadataProvider(
)
}
}

container.isUnionShape ||
container.isListShape ||
container.isSetShape ||
container.isMapShape
-> RustMetadata(visibility = Visibility.PUBLIC)

else -> TODO("Unrecognized container type: $container")
}
}
Expand All @@ -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"),
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
}
Expand Down Expand Up @@ -153,6 +157,10 @@ open class EnumGenerator(
}

private fun renderEnum() {
target.ifClient {
writer.renderForwardCompatibilityNote(enumName, sortedMembers, UnknownVariant, UnknownVariantValue)
}

val renamedWarning =
sortedMembers.mapNotNull { it.name() }.filter { it.renamedFrom != null }.joinToString("\n") {
val previousName = it.renamedFrom!!
Expand All @@ -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)")
target.ifClient {
docs("`$UnknownVariant` contains new variants that have been added since this code was generated.")
rust("$UnknownVariant(#T)", unknownVariantValue())
}
}
}
Expand All @@ -182,8 +190,9 @@ open class EnumGenerator(
sortedMembers.forEach { member ->
rust("""$enumName::${member.derivedName()} => ${member.value.dq()},""")
}
if (target == CodegenTarget.CLIENT) {
rust("$enumName::$UnknownVariant(s) => s.as_ref()")

target.ifClient {
rust("$enumName::$UnknownVariant(value) => value.as_str()")
}
}
}
Expand All @@ -198,14 +207,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 intended to be used 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())
}
}
}
Expand All @@ -225,3 +256,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(),
)
}
Loading

0 comments on commit cdb4a3f

Please sign in to comment.