From dfa18d6315042ea1c2fee77f96735da8b46935eb Mon Sep 17 00:00:00 2001 From: david-perez Date: Wed, 15 Feb 2023 19:16:51 +0100 Subject: [PATCH] Address comments --- .../transformers/RecursiveShapeBoxer.kt | 48 ++++++++++++++----- .../traits/ConstraintViolationRustBoxTrait.kt | 3 +- .../RecursiveConstraintViolationsTest.kt | 2 +- 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/transformers/RecursiveShapeBoxer.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/transformers/RecursiveShapeBoxer.kt index 77905ef580..d53751829f 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/transformers/RecursiveShapeBoxer.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/transformers/RecursiveShapeBoxer.kt @@ -16,24 +16,41 @@ import software.amazon.smithy.rust.codegen.core.smithy.traits.RustBoxTrait import software.amazon.smithy.rust.codegen.core.util.hasTrait class RecursiveShapeBoxer( + /** + * A predicate that determines when a cycle in the shape graph contains "indirection". If a cycle contains + * indirection, no shape needs to be tagged. What constitutes indirection is up to the caller to decide. + */ private val containsIndirectionPredicate: (Collection) -> Boolean = ::containsIndirection, + /** + * A closure that gets called on one member shape of a cycle that does not contain indirection for "fixing". For + * example, the [RustBoxTrait] trait can be used to tag the member shape. + */ private val boxShapeFn: (MemberShape) -> MemberShape = ::addRustBoxTrait, ) { /** - * Transform a model which may contain recursive shapes into a model annotated with [RustBoxTrait]. - * - * When recursive shapes do NOT go through a `CollectionShape` or a `MapShape` shape, they must be boxed in Rust. - * This function will iteratively find loops and add the [RustBoxTrait] trait in a deterministic way until it - * reaches a fixed point. + * Transform a model which may contain recursive shapes. * - * Why `CollectionShape`s and `MapShape`s? Note that `CollectionShape`s get rendered in Rust as `Vec`, and - * `MapShape`s as `HashMap`; they're the only Smithy shapes that "organically" introduce indirection - * (via a pointer to the heap) in the recursive path. For other recursive paths, we thus have to introduce the - * indirection artificially ourselves using `Box`. + * For example, when recursive shapes do NOT go through a `CollectionShape` or a `MapShape` shape, they must be + * boxed in Rust. This function will iteratively find cycles and call [boxShapeFn] on a member shape in the + * cycle to act on it. This is done in a deterministic way until it reaches a fixed point. * - * This function MUST be deterministic (always choose the same shapes to `Box`). If it is not, that is a bug. Even so + * This function MUST be deterministic (always choose the same shapes to fix). If it is not, that is a bug. Even so * this function may cause backward compatibility issues in certain pathological cases where a changes to recursive * structures cause different members to be boxed. We may need to address these via customizations. + * + * For example, given the following model, + * + * ```smithy + * namespace com.example + * + * structure Recursive { + * recursiveStruct: Recursive + * anotherField: Boolean + * } + * ``` + * + * The `com.example#Recursive$recursiveStruct` member shape is part of a cycle, but the + * `com.example#Recursive$anotherField` member shape is not. */ fun transform(model: Model): Model { val next = transformInner(model) @@ -45,8 +62,9 @@ class RecursiveShapeBoxer( } /** - * If [model] contains a recursive loop that must be boxed, apply one instance of [RustBoxTrait] return the new model. - * If [model] contains no loops, return null. + * If [model] contains a recursive loop that must be boxed, return the transformed model resulting form a call to + * [boxShapeFn]. + * If [model] contains no loops, return `null`. */ private fun transformInner(model: Model): Model? { // Execute 1 step of the boxing algorithm in the path to reaching a fixed point: @@ -86,6 +104,12 @@ class RecursiveShapeBoxer( /** * Check if a `List` contains a shape which will use a pointer when represented in Rust, avoiding the * need to add more `Box`es. + * + * Why `CollectionShape`s and `MapShape`s? Note that `CollectionShape`s get rendered in Rust as `Vec`, and + * `MapShape`s as `HashMap`; they're the only Smithy shapes that "organically" introduce indirection + * (via a pointer to the heap) in the recursive path. For other recursive paths, we thus have to introduce the + * indirection artificially ourselves using `Box`. + * */ private fun containsIndirection(loop: Collection): Boolean = loop.find { when (it) { diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/traits/ConstraintViolationRustBoxTrait.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/traits/ConstraintViolationRustBoxTrait.kt index 5c4f489b0a..9aee2b884e 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/traits/ConstraintViolationRustBoxTrait.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/traits/ConstraintViolationRustBoxTrait.kt @@ -11,7 +11,8 @@ import software.amazon.smithy.model.traits.Trait /** * This shape is analogous to [software.amazon.smithy.rust.codegen.core.smithy.traits.RustBoxTrait], but for the - * constraint violation graph. + * constraint violation graph. The sets of shapes we tag are different, and they are interpreted by the code generator + * differently, so we need a separate tag. * * This is used to handle recursive constraint violations. * See [software.amazon.smithy.rust.codegen.server.smithy.transformers.RecursiveConstraintViolationBoxer]. diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/RecursiveConstraintViolationsTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/RecursiveConstraintViolationsTest.kt index f214ac1086..8285d20c8b 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/RecursiveConstraintViolationsTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/RecursiveConstraintViolationsTest.kt @@ -96,7 +96,7 @@ internal class RecursiveConstraintViolationsTest { attributeValue: ${unionPrefix}AttributeValue } - // Named `${unionPrefix}AttributeValue` to have reminiscenses of DynamoDB's famous `AttributeValue`. + // Named `${unionPrefix}AttributeValue` in honor of DynamoDB's famous `AttributeValue`. // https://docs.rs/aws-sdk-dynamodb/latest/aws_sdk_dynamodb/model/enum.AttributeValue.html union ${unionPrefix}AttributeValue { set: SetAttribute