Address comments

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<Shape>) -> 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<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`.
+     * 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<Shape>` 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<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`.
+ *
  */
 private fun containsIndirection(loop: Collection<Shape>): Boolean = loop.find {
     when (it) {

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].

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