Fix recursive constraint violations with paths over list and map shapes

There is a widespread assumption throughout the generation of constraint
violations that does not hold true all the time, namely, that a
recursive constraint violation graph has the same requirements with
regards to boxing as the regular shape graph.

Some types corresponding to recursive shapes are boxed to introduce
indirection and thus not generate an infinitely recursive type. The
algorithm however does not superfluously introduce boxes when the cycle
goes through a list shape or a map shape. Why list shapes and map
shapes? List shapes and map shapes get rendered in Rust as `Vec<T>` and
`HashMap<K, V>`, respectively, 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`. This is done in the
`RecursiveShapeBoxer` model transform.

However, the constraint violation graph needs to box types in recursive
paths more often. Since we don't collect constraint violations
(yet, see #2040), the constraint violation graph never holds
`Vec<T>`s or `HashMap<K, V>`s, only simple types. Indeed, the following simple
recursive model:

```smithy
union Recursive {
    list: List
}

@Length(min: 69)
list List {
    member: Recursive
}
```

has a cycle that goes through a list shape, so no shapes in it need
boxing in the regular shape graph. However, the constraint violation
graph is infinitely recursive if we don't introduce boxing somewhere:

```rust
pub mod model {
    pub mod list {
        pub enum ConstraintViolation {
            Length(usize),
            Member(
                usize,
                crate::model::recursive::ConstraintViolation,
            ),
        }
    }

    pub mod recursive {
        pub enum ConstraintViolation {
            List(crate::model::list::ConstraintViolation),
        }
    }
}
```

This commit fixes things by making the `RecursiveShapeBoxer` model
transform configurable so that the "cycles through lists and maps
introduce indirection" assumption can be lifted. This allows a server
model transform, `RecursiveConstraintViolationBoxer`, to tag member
shapes along recursive paths with a new trait,
`ConstraintViolationRustBoxTrait`, that the constraint violation type
generation then utilizes to ensure that no infinitely recursive
constraint violation types get generated. Likewise, places where
constraint violations are handled (like where unconstrained types are
converted to constrained types) have been updated to account for the
scenario where they now are or need to be boxed.

Parametrized tests have been added to exhaustively test combinations of
models exercising recursive paths going through (sparse and non-sparse)
list and map shapes, as well as union and structure shapes
(`RecursiveConstraintViolationsTest`). These tests even assert that the
specific member shapes along the cycles are tagged as expected
(`RecursiveConstraintViolationBoxerTest`).

Author: david-perez

codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientCodegenVisitor.kt

@@ -105,14 +105,14 @@ class ClientCodegenVisitor(
             // Add errors attached at the service level to the models
             .let { ModelTransformer.create().copyServiceErrorsToOperations(it, settings.getService(it)) }
             // Add `Box<T>` to recursive shapes as necessary
-            .let(RecursiveShapeBoxer::transform)
+            .let(RecursiveShapeBoxer()::transform)
             // Normalize the `message` field on errors when enabled in settings (default: true)
             .letIf(settings.codegenConfig.addMessageToErrors, AddErrorMessage::transform)
             // NormalizeOperations by ensuring every operation has an input & output shape
             .let(OperationNormalizer::transform)
             // Drop unsupported event stream operations from the model
             .let { RemoveEventStreamOperations.transform(it, settings) }
-            // - Normalize event stream operations
+            // Normalize event stream operations
             .let(EventStreamNormalizer::transform)
 
     /**

codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/customizations/ResiliencyConfigCustomizationTest.kt

@@ -36,7 +36,7 @@ internal class ResiliencyConfigCustomizationTest {
 
     @Test
     fun `generates a valid config`() {
-        val model = RecursiveShapeBoxer.transform(OperationNormalizer.transform(baseModel))
+        val model = RecursiveShapeBoxer().transform(OperationNormalizer.transform(baseModel))
         val project = TestWorkspace.testProject()
         val codegenContext = testCodegenContext(model, settings = project.rustSettings())
 

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

@@ -12,7 +12,8 @@ import software.amazon.smithy.model.traits.Trait
 /**
  * Trait indicating that this shape should be represented with `Box<T>` when converted into Rust
  *
- * This is used to handle recursive shapes. See RecursiveShapeBoxer.
+ * This is used to handle recursive shapes.
+ * See [software.amazon.smithy.rust.codegen.core.smithy.transformers.RecursiveShapeBoxer].
  *
  * This trait is synthetic, applied during code generation, and never used in actual models.
  */

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

@@ -7,21 +7,29 @@ package software.amazon.smithy.rust.codegen.core.smithy.transformers
 
 import software.amazon.smithy.codegen.core.TopologicalIndex
 import software.amazon.smithy.model.Model
-import software.amazon.smithy.model.shapes.ListShape
+import software.amazon.smithy.model.shapes.CollectionShape
 import software.amazon.smithy.model.shapes.MapShape
 import software.amazon.smithy.model.shapes.MemberShape
-import software.amazon.smithy.model.shapes.SetShape
 import software.amazon.smithy.model.shapes.Shape
 import software.amazon.smithy.model.transform.ModelTransformer
 import software.amazon.smithy.rust.codegen.core.smithy.traits.RustBoxTrait
 import software.amazon.smithy.rust.codegen.core.util.hasTrait
 
-object RecursiveShapeBoxer {
+class RecursiveShapeBoxer(
+    private val containsIndirectionPredicate: (Collection<Shape>) -> Boolean = ::containsIndirection,
+    private val boxShapeFn: (MemberShape) -> MemberShape = ::addRustBoxTrait,
+) {
     /**
-     * Transform a model which may contain recursive shapes into a model annotated with [RustBoxTrait]
+     * Transform a model which may contain recursive shapes into a model annotated with [RustBoxTrait].
      *
-     * When recursive shapes do NOT go through a List, Map, or Set, they must be boxed in Rust. This function will
-     * iteratively find loops & add the `RustBox` trait in a deterministic way until it reaches a fixed point.
+     * 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.
+     *
+     * Why `CollectionShape`s and `MapShape`s? Note that `CollectionShape`s get rendered in Rust as `Vec<T>`, and
+     * `MapShape`s as `HashMap<String, T>`; 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`.
      *
      * This function MUST be deterministic (always choose the same shapes to `Box`). 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
@@ -41,12 +49,12 @@ object RecursiveShapeBoxer {
      * 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
-        // 1. Find all the shapes that are part of a cycle
-        // 2. Find all the loops that those shapes are part of
-        // 3. Filter out the loops that go through a layer of indirection
-        // 3. Pick _just one_ of the remaining loops to fix
-        // 4. Select the member shape in that loop with the earliest shape id
+        // Execute 1 step of the boxing algorithm in the path to reaching a fixed point:
+        // 1. Find all the shapes that are part of a cycle.
+        // 2. Find all the loops that those shapes are part of.
+        // 3. Filter out the loops that go through a layer of indirection.
+        // 3. Pick _just one_ of the remaining loops to fix.
+        // 4. Select the member shape in that loop with the earliest shape id.
         // 5. Box it.
         // (External to this function) Go back to 1.
         val index = TopologicalIndex.of(model)
@@ -58,34 +66,32 @@ object RecursiveShapeBoxer {
             // Flatten the connections into shapes.
             loops.map { it.shapes }
         }
-        val loopToFix = loops.firstOrNull { !containsIndirection(it) }
+        val loopToFix = loops.firstOrNull { !containsIndirectionPredicate(it) }
 
         return loopToFix?.let { loop: List<Shape> ->
             check(loop.isNotEmpty())
-            // pick the shape to box in a deterministic way
+            // Pick the shape to box in a deterministic way.
             val shapeToBox = loop.filterIsInstance<MemberShape>().minByOrNull { it.id }!!
             ModelTransformer.create().mapShapes(model) { shape ->
                 if (shape == shapeToBox) {
-                    shape.asMemberShape().get().toBuilder().addTrait(RustBoxTrait()).build()
+                    boxShapeFn(shape.asMemberShape().get())
                 } else {
                     shape
                 }
             }
         }
     }
+}
 
-    /**
-     * Check if a List<Shape> contains a shape which will use a pointer when represented in Rust, avoiding the
-     * need to add more Boxes
-     */
-    private fun containsIndirection(loop: List<Shape>): Boolean {
-        return loop.find {
-            when (it) {
-                is ListShape,
-                is MapShape,
-                is SetShape, -> true
-                else -> it.hasTrait<RustBoxTrait>()
-            }
-        } != null
+/**
+ * Check if a `List<Shape>` contains a shape which will use a pointer when represented in Rust, avoiding the
+ * need to add more `Box`es.
+ */
+private fun containsIndirection(loop: Collection<Shape>): Boolean = loop.find {
+    when (it) {
+        is CollectionShape, is MapShape -> true
+        else -> it.hasTrait<RustBoxTrait>()
     }
-}
+} != null
+
+private fun addRustBoxTrait(shape: MemberShape): MemberShape = shape.toBuilder().addTrait(RustBoxTrait()).build()

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCodegenVisitor.kt

@@ -78,6 +78,7 @@ import software.amazon.smithy.rust.codegen.server.smithy.generators.protocol.Ser
 import software.amazon.smithy.rust.codegen.server.smithy.protocols.ServerProtocolLoader
 import software.amazon.smithy.rust.codegen.server.smithy.traits.isReachableFromOperationInput
 import software.amazon.smithy.rust.codegen.server.smithy.transformers.AttachValidationExceptionToConstrainedOperationInputsInAllowList
+import software.amazon.smithy.rust.codegen.server.smithy.transformers.RecursiveConstraintViolationBoxer
 import software.amazon.smithy.rust.codegen.server.smithy.transformers.RemoveEbsModelValidationException
 import software.amazon.smithy.rust.codegen.server.smithy.transformers.ShapesReachableFromOperationInputTagger
 import java.util.logging.Logger
@@ -162,7 +163,9 @@ open class ServerCodegenVisitor(
             // Add errors attached at the service level to the models
             .let { ModelTransformer.create().copyServiceErrorsToOperations(it, settings.getService(it)) }
             // Add `Box<T>` to recursive shapes as necessary
-            .let(RecursiveShapeBoxer::transform)
+            .let(RecursiveShapeBoxer()::transform)
+            // Add `Box<T>` to recursive constraint violations as necessary
+            .let(RecursiveConstraintViolationBoxer::transform)
             // Normalize operations by adding synthetic input and output shapes to every operation
             .let(OperationNormalizer::transform)
             // Remove the EBS model's own `ValidationException`, which collides with `smithy.framework#ValidationException`

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/CollectionConstraintViolationGenerator.kt

@@ -9,12 +9,15 @@ import software.amazon.smithy.model.shapes.CollectionShape
 import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
 import software.amazon.smithy.rust.codegen.core.rustlang.Visibility
 import software.amazon.smithy.rust.codegen.core.rustlang.join
-import software.amazon.smithy.rust.codegen.core.rustlang.rust
 import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
+import software.amazon.smithy.rust.codegen.core.smithy.makeRustBoxed
 import software.amazon.smithy.rust.codegen.core.smithy.module
+import software.amazon.smithy.rust.codegen.core.util.hasTrait
+import software.amazon.smithy.rust.codegen.core.util.letIf
 import software.amazon.smithy.rust.codegen.server.smithy.PubCrateConstraintViolationSymbolProvider
 import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext
 import software.amazon.smithy.rust.codegen.server.smithy.canReachConstrainedShape
+import software.amazon.smithy.rust.codegen.server.smithy.traits.ConstraintViolationRustBoxTrait
 import software.amazon.smithy.rust.codegen.server.smithy.traits.isReachableFromOperationInput
 
 class CollectionConstraintViolationGenerator(
@@ -38,16 +41,22 @@ class CollectionConstraintViolationGenerator(
     private val constraintsInfo: List<TraitInfo> = collectionConstraintsInfo.map { it.toTraitInfo() }
 
     fun render() {
-        val memberShape = model.expectShape(shape.member.target)
+        val targetShape = model.expectShape(shape.member.target)
         val constraintViolationSymbol = constraintViolationSymbolProvider.toSymbol(shape)
         val constraintViolationName = constraintViolationSymbol.name
-        val isMemberConstrained = memberShape.canReachConstrainedShape(model, symbolProvider)
+        val isMemberConstrained = targetShape.canReachConstrainedShape(model, symbolProvider)
         val constraintViolationVisibility = Visibility.publicIf(publicConstrainedTypes, Visibility.PUBCRATE)
 
         modelsModuleWriter.withInlineModule(constraintViolationSymbol.module()) {
             val constraintViolationVariants = constraintsInfo.map { it.constraintViolationVariant }.toMutableList()
             if (isMemberConstrained) {
                 constraintViolationVariants += {
+                    val memberConstraintViolationSymbol =
+                        constraintViolationSymbolProvider.toSymbol(targetShape).letIf(
+                            shape.member.hasTrait<ConstraintViolationRustBoxTrait>(),
+                        ) {
+                            it.makeRustBoxed()
+                        }
                     rustTemplate(
                         """
                         /// Constraint violation error when an element doesn't satisfy its own constraints.
@@ -56,7 +65,7 @@ class CollectionConstraintViolationGenerator(
                         ##[doc(hidden)]
                         Member(usize, #{MemberConstraintViolationSymbol})
                         """,
-                        "MemberConstraintViolationSymbol" to constraintViolationSymbolProvider.toSymbol(memberShape),
+                        "MemberConstraintViolationSymbol" to memberConstraintViolationSymbol,
                     )
                 }
             }

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/MapConstraintViolationGenerator.kt

@@ -11,10 +11,13 @@ import software.amazon.smithy.model.traits.LengthTrait
 import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
 import software.amazon.smithy.rust.codegen.core.rustlang.Visibility
 import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
+import software.amazon.smithy.rust.codegen.core.smithy.makeRustBoxed
 import software.amazon.smithy.rust.codegen.core.smithy.module
 import software.amazon.smithy.rust.codegen.core.util.hasTrait
+import software.amazon.smithy.rust.codegen.core.util.letIf
 import software.amazon.smithy.rust.codegen.server.smithy.PubCrateConstraintViolationSymbolProvider
 import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext
+import software.amazon.smithy.rust.codegen.server.smithy.traits.ConstraintViolationRustBoxTrait
 import software.amazon.smithy.rust.codegen.server.smithy.traits.isReachableFromOperationInput
 
 class MapConstraintViolationGenerator(
@@ -47,7 +50,14 @@ class MapConstraintViolationGenerator(
             constraintViolationCodegenScopeMutableList.add("KeyConstraintViolationSymbol" to constraintViolationSymbolProvider.toSymbol(keyShape))
         }
         if (isValueConstrained(valueShape, model, symbolProvider)) {
-            constraintViolationCodegenScopeMutableList.add("ValueConstraintViolationSymbol" to constraintViolationSymbolProvider.toSymbol(valueShape))
+            constraintViolationCodegenScopeMutableList.add(
+                "ValueConstraintViolationSymbol" to
+                    constraintViolationSymbolProvider.toSymbol(valueShape).letIf(
+                        shape.value.hasTrait<ConstraintViolationRustBoxTrait>(),
+                    ) {
+                        it.makeRustBoxed()
+                    },
+            )
             constraintViolationCodegenScopeMutableList.add("KeySymbol" to constrainedShapeSymbolProvider.toSymbol(keyShape))
         }
         val constraintViolationCodegenScope = constraintViolationCodegenScopeMutableList.toTypedArray()

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderConstraintViolations.kt

@@ -20,13 +20,13 @@ import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
 import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
 import software.amazon.smithy.rust.codegen.core.smithy.isOptional
 import software.amazon.smithy.rust.codegen.core.smithy.makeRustBoxed
-import software.amazon.smithy.rust.codegen.core.smithy.traits.RustBoxTrait
 import software.amazon.smithy.rust.codegen.core.util.hasTrait
 import software.amazon.smithy.rust.codegen.core.util.letIf
 import software.amazon.smithy.rust.codegen.core.util.toPascalCase
 import software.amazon.smithy.rust.codegen.server.smithy.PubCrateConstraintViolationSymbolProvider
 import software.amazon.smithy.rust.codegen.server.smithy.ServerCodegenContext
 import software.amazon.smithy.rust.codegen.server.smithy.targetCanReachConstrainedShape
+import software.amazon.smithy.rust.codegen.server.smithy.traits.ConstraintViolationRustBoxTrait
 
 /**
  * Renders constraint violation types that arise when building a structure shape builder.
@@ -138,8 +138,8 @@ class ServerBuilderConstraintViolations(
 
                     val constraintViolationSymbol =
                         constraintViolationSymbolProvider.toSymbol(targetShape)
-                            // If the corresponding structure's member is boxed, box this constraint violation symbol too.
-                            .letIf(constraintViolation.forMember.hasTrait<RustBoxTrait>()) {
+                            // Box this constraint violation symbol if necessary.
+                            .letIf(constraintViolation.forMember.hasTrait<ConstraintViolationRustBoxTrait>()) {
                                 it.makeRustBoxed()
                             }
 

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt

@@ -49,6 +49,7 @@ import software.amazon.smithy.rust.codegen.server.smithy.ServerRuntimeType
 import software.amazon.smithy.rust.codegen.server.smithy.canReachConstrainedShape
 import software.amazon.smithy.rust.codegen.server.smithy.hasConstraintTraitOrTargetHasConstraintTrait
 import software.amazon.smithy.rust.codegen.server.smithy.targetCanReachConstrainedShape
+import software.amazon.smithy.rust.codegen.server.smithy.traits.ConstraintViolationRustBoxTrait
 import software.amazon.smithy.rust.codegen.server.smithy.traits.isReachableFromOperationInput
 import software.amazon.smithy.rust.codegen.server.smithy.wouldHaveConstrainedWrapperTupleTypeWerePublicConstrainedTypesEnabled
 
@@ -541,18 +542,15 @@ class ServerBuilderGenerator(
         val hasBox = builderMemberSymbol(member)
             .mapRustType { it.stripOuter<RustType.Option>() }
             .isRustBoxed()
+        val errHasBox = member.hasTrait<ConstraintViolationRustBoxTrait>()
+
         if (hasBox) {
             writer.rustTemplate(
                 """
                 .map(|v| match *v {
                     #{MaybeConstrained}::Constrained(x) => Ok(Box::new(x)),
                     #{MaybeConstrained}::Unconstrained(x) => Ok(Box::new(x.try_into()?)),
                 })
-                .map(|res|
-                    res${ if (constrainedTypeHoldsFinalType(member)) "" else ".map(|v| v.into())" }
-                       .map_err(|err| ConstraintViolation::${constraintViolation.name()}(Box::new(err)))
-                )
-                .transpose()?
                 """,
                 *codegenScope,
             )
@@ -563,16 +561,23 @@ class ServerBuilderGenerator(
                     #{MaybeConstrained}::Constrained(x) => Ok(x),
                     #{MaybeConstrained}::Unconstrained(x) => x.try_into(),
                 })
-                .map(|res|
-                    res${if (constrainedTypeHoldsFinalType(member)) "" else ".map(|v| v.into())"}
-                       .map_err(ConstraintViolation::${constraintViolation.name()})
-                )
-                .transpose()?
                 """,
                 *codegenScope,
             )
         }
 
+        writer.rustTemplate(
+            """
+            .map(|res|
+                res${if (constrainedTypeHoldsFinalType(member)) "" else ".map(|v| v.into())"}
+                   ${if (errHasBox) ".map_err(Box::new)" else "" }
+                   .map_err(ConstraintViolation::${constraintViolation.name()})
+            )
+            .transpose()?
+            """,
+            *codegenScope,
+        )
+
         // Constrained types are not public and this is a member shape that would have generated a
         // public constrained type, were the setting to be enabled.
         // We've just checked the constraints hold by going through the non-public