Repository: smithy-lang/smithy
Pull request #2053 (2023-11-26 17:31:42)

Add synthetic box to root intEnums w/o a default
Primitive root shapes without the default trait have a synthetic box trait appplied when the shape is built in the loader (see https://github.com/smithy-lang/smithy/blob/d457aabb80feb4088caa3ac27d337b84e3ebc43d/smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderShapeMap.java#L366) if the shape would have a default of 0 in Smithy 1.0. Previously, this didn't include intEnum, so if you convert to 1.0, the intEnum which previously had no default in 2.0 will have a default of 0.

This commit updates ModelInteropTransformer to consider intEnum as a shape that has a default value in 1.0, so the loader applies the synthetic box trait to root intEnums.

Tests were updated to make sure box is added to root intEnums, and to fix some incorrect assertions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelInteropTransformer.java b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelInteropTransformer.java
index 907904bbd..15ec4f750 100644
--- a/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelInteropTransformer.java
+++ b/smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelInteropTransformer.java
@@ -66,7 +66,8 @@ final class ModelInteropTransformer {
             ShapeType.LONG,
             ShapeType.FLOAT,
             ShapeType.DOUBLE,
-            ShapeType.BOOLEAN);
+            ShapeType.BOOLEAN,
+            ShapeType.INT_ENUM); // intEnum is actually an integer in v1, but the ShapeType is different.
 
     private final Model model;
     private final List<ValidationEvent> events;
@@ -167,10 +168,9 @@ final class ModelInteropTransformer {
                && memberAndTargetAreNotAlreadyExplicitlyBoxed(member, target);
     }
 
-    // Only apply box to members where the trait can be applied. Note that intEnum is treated
-    // like a normal integer in v1 implementations, so it is allowed to be synthetically boxed.
+    // Only apply box to members where the trait can be applied.
     private boolean canBoxTargetThisKindOfShape(Shape target) {
-        return HAD_DEFAULT_VALUE_IN_1_0.contains(target.getType()) || target.isIntEnumShape();
+        return HAD_DEFAULT_VALUE_IN_1_0.contains(target.getType());
     }
 
     private boolean memberAndTargetAreNotAlreadyExplicitlyBoxed(MemberShape member, Shape target) {


Test patch:
diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java
index d8c4bbc3c..c55f8a902 100644
--- a/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java
+++ b/smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelInteropTransformerTest.java
@@ -360,11 +360,28 @@ public class ModelInteropTransformerTest {
                                                 + "@default(0)\n"
                                                 + "integer PrimitiveInteger\n"
                                                 + "\n"
+                                                + "intEnum BoxedIntEnum {\n"
+                                                + "    ONE = 1\n"
+                                                + "}\n"
+                                                + "\n"
+                                                + "@default(1)\n"
+                                                + "intEnum BoxedIntEnumWithDefault {\n"
+                                                + "    ONE = 1\n"
+                                                + "}\n"
+                                                + "\n"
+                                                + "@default(0)\n"
+                                                + "intEnum PrimitiveIntEnum {\n"
+                                                + "    ZERO = 0\n"
+                                                + "}\n"
+                                                + "\n"
                                                 + "structure Foo {\n"
                                                 + "    DefaultString: DefaultString = \"\"\n"
                                                 + "    BoxedInteger: BoxedInteger\n"
                                                 + "    PrimitiveInteger: PrimitiveInteger = 0\n"
                                                 + "    BoxedIntegerWithDefault: BoxedIntegerWithDefault = 1\n"
+                                                + "    BoxedIntEnum: BoxedIntEnum\n"
+                                                + "    BoxedIntEnumWithDefault: BoxedIntEnumWithDefault = 1\n"
+                                                + "    PrimitiveIntEnum: PrimitiveIntEnum = 0\n"
                                                 +"}\n")
                 .assemble()
                 .unwrap();
@@ -385,11 +402,20 @@ public class ModelInteropTransformerTest {
         ShapeId fooBoxedInteger = ShapeId.from("smithy.example#Foo$BoxedInteger");
 
         ShapeId boxedIntegerWithDefault = ShapeId.from("smithy.example#BoxedIntegerWithDefault");
-        ShapeId fooBoxedIntegerWithDefault = ShapeId.from("smithy.example#BoxedIntegerWithDefault");
+        ShapeId fooBoxedIntegerWithDefault = ShapeId.from("smithy.example#Foo$BoxedIntegerWithDefault");
 
         ShapeId primitiveInteger = ShapeId.from("smithy.example#PrimitiveInteger");
         ShapeId fooPrimitiveInteger = ShapeId.from("smithy.example#Foo$PrimitiveInteger");
 
+        ShapeId boxedIntEnum = ShapeId.from("smithy.example#BoxedIntEnum");
+        ShapeId fooBoxedIntEnum = ShapeId.from("smithy.example#Foo$BoxedIntEnum");
+
+        ShapeId boxedIntEnumWithDefault = ShapeId.from("smithy.example#BoxedIntEnumWithDefault");
+        ShapeId fooBoxedIntEnumWithDefault = ShapeId.from("smithy.example#Foo$BoxedIntEnumWithDefault");
+
+        ShapeId primitiveIntEnum = ShapeId.from("smithy.example#PrimitiveIntEnum");
+        ShapeId fooPrimitiveIntEnum = ShapeId.from("smithy.example#Foo$PrimitiveIntEnum");
+
         // Do not box strings for v1 compatibility.
         assertThat(model.expectShape(defaultString).hasTrait(BoxTrait.class), is(false));
         assertThat(model.expectShape(fooDefaultString).hasTrait(BoxTrait.class), is(false));
@@ -404,7 +430,7 @@ public class ModelInteropTransformerTest {
 
         // Add box to BoxedIntegerWithDefault because it has a default that isn't the v1 zero value.
         assertThat(model.expectShape(boxedIntegerWithDefault).hasTrait(BoxTrait.class), is(true));
-        assertThat(model.expectShape(fooBoxedIntegerWithDefault).hasTrait(BoxTrait.class), is(true)); // no need to box the member too
+        assertThat(model.expectShape(fooBoxedIntegerWithDefault).hasTrait(BoxTrait.class), is(false)); // no need to box the member too
         assertThat(model.expectShape(boxedIntegerWithDefault).hasTrait(DefaultTrait.class), is(true));
         assertThat(model.expectShape(fooBoxedIntegerWithDefault).hasTrait(DefaultTrait.class), is(true));
 
@@ -413,6 +439,24 @@ public class ModelInteropTransformerTest {
         assertThat(model.expectShape(fooPrimitiveInteger).hasTrait(BoxTrait.class), is(false));
         assertThat(model.expectShape(primitiveInteger).hasTrait(DefaultTrait.class), is(true));
         assertThat(model.expectShape(fooPrimitiveInteger).hasTrait(DefaultTrait.class), is(true));
+
+        // Add box to BoxedIntEnum because it has no default trait.
+        assertThat(model.expectShape(boxedIntEnum).hasTrait(BoxTrait.class), is(true));
+        assertThat(model.expectShape(fooBoxedIntEnum).hasTrait(BoxTrait.class), is(false)); // no need to box the member too
+        assertThat(model.expectShape(boxedIntEnum).hasTrait(DefaultTrait.class), is(false));
+        assertThat(model.expectShape(fooBoxedIntEnum).hasTrait(DefaultTrait.class), is(false));
+
+        // Add box to BoxedIntEnumWithDefault because it has a default that isn't the v1 zero value.
+        assertThat(model.expectShape(boxedIntEnumWithDefault).hasTrait(BoxTrait.class), is(true));
+        assertThat(model.expectShape(fooBoxedIntEnumWithDefault).hasTrait(BoxTrait.class), is(false)); // no need to box the member too
+        assertThat(model.expectShape(boxedIntEnumWithDefault).hasTrait(DefaultTrait.class), is(true));
+        assertThat(model.expectShape(fooBoxedIntEnumWithDefault).hasTrait(DefaultTrait.class), is(true));
+
+        // No box trait on PrimitiveIntEnum because it has a zero value default.
+        assertThat(model.expectShape(primitiveIntEnum).hasTrait(BoxTrait.class), is(false));
+        assertThat(model.expectShape(fooPrimitiveIntEnum).hasTrait(BoxTrait.class), is(false));
+        assertThat(model.expectShape(primitiveIntEnum).hasTrait(DefaultTrait.class), is(true));
+        assertThat(model.expectShape(fooPrimitiveIntEnum).hasTrait(DefaultTrait.class), is(true));
     }
 
     @Test
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/upgrade-box/2-to-1/intEnumSetToZeroValueToNonNullable.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/upgrade-box/2-to-1/intEnumSetToZeroValueToNonNullable.smithy
index 4a0d7660f..8b5d72b76 100644
--- a/smithy-model/src/test/resources/software/amazon/smithy/model/loader/upgrade-box/2-to-1/intEnumSetToZeroValueToNonNullable.smithy
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/loader/upgrade-box/2-to-1/intEnumSetToZeroValueToNonNullable.smithy
@@ -1,11 +1,11 @@
-// {"v1-box": false, "v1-client-zero-value": false, "v2": false}
+// {"v1-box": true, "v1-client-zero-value": false, "v2": false}
+// V1 style box checks will think this member is nullable because it
+// targets a shape with the box trait.
 $version: "2.0"
 
 namespace smithy.example
 
 structure Foo {
-    // V1 models treat intEnum as normal integers, so they just see a default zero value, hence this is
-    // non-nullable in v1 and v2.
     intEnumSetToZeroValueToNonNullable: MyIntEnum = 0
 }
 
diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/shapes/ast-serialization/cases/v2/enums.1.0.json b/smithy-model/src/test/resources/software/amazon/smithy/model/shapes/ast-serialization/cases/v2/enums.1.0.json
index c02810a98..f294cb927 100644
--- a/smithy-model/src/test/resources/software/amazon/smithy/model/shapes/ast-serialization/cases/v2/enums.1.0.json
+++ b/smithy-model/src/test/resources/software/amazon/smithy/model/shapes/ast-serialization/cases/v2/enums.1.0.json
@@ -2,7 +2,10 @@
     "smithy": "1.0",
     "shapes": {
         "smithy.example#IntEnum": {
-            "type": "integer"
+            "type": "integer",
+            "traits": {
+                "smithy.api#box": {}
+            }
         },
         "smithy.example#StringEnum": {
             "type": "string",