Correct precedence in NullableIndex and fix diffs

smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedDefault.java

@@ -59,10 +59,11 @@ private void validateChange(
             DefaultTrait oldTrait,
             DefaultTrait newTrait
     ) {
-        // Removing the default trait is always a breaking change.
         if (newTrait == null) {
-            events.add(error(change.getNewShape(),
-                             "@default trait was removed. This will break previously generated code."));
+            if (!isInconsequentialRemovalOfDefaultTrait(model, oldTrait, change.getNewShape())) {
+                events.add(error(change.getNewShape(),
+                                 "@default trait was removed. This will break previously generated code."));
+            }
         } else if (oldTrait == null) {
             if (change.getNewShape().getType() != ShapeType.MEMBER) {
                 events.add(error(change.getNewShape(), newTrait,
@@ -88,6 +89,25 @@ private void validateChange(
         }
     }
 
+    private boolean isInconsequentialRemovalOfDefaultTrait(Model model, DefaultTrait trait, Shape removedFrom) {
+        // Removing a default of null if the target is nullable is not an issue.
+        return removedFrom.asMemberShape()
+                .map(member -> {
+                    if (trait.toNode().isNullNode()) {
+                        // If the target has no defined default, then removing a default(null) trait is fine.
+                        Node targetDefault = model.expectShape(member.getTarget())
+                                .getTrait(DefaultTrait.class)
+                                .map(DefaultTrait::toNode)
+                                .orElse(Node.nullNode()); // If no default, then assume target has a default of null.
+                        return targetDefault.isNullNode();
+                    } else {
+                        // Removing a non-null trait is always an issue.
+                        return false;
+                    }
+                })
+                .orElse(false);
+    }
+
     private void evaluateChangedTrait(Model model, MemberShape member, DefaultTrait oldTrait,
             DefaultTrait newTrait, List<ValidationEvent> events) {
         Shape target = model.expectShape(member.getTarget());

smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedNullabilityTest.java

@@ -5,6 +5,7 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
 
 import java.util.List;
 import org.junit.jupiter.api.Test;
@@ -357,4 +358,27 @@ public void specialHandlingForRequiredUnionMembers() {
         assertThat(events, hasSize(1));
         assertThat(events.get(0).getSeverity(), is(Severity.WARNING));
     }
+
+    @Test
+    public void doesNotWarnWhenExtraneousDefaultNullTraitRemoved() {
+        String originalModel =
+                "$version: \"2.0\"\n"
+                + "namespace smithy.example\n"
+                + "structure Foo {\n"
+                + "    @required\n"
+                + "    baz: Integer = null\n"
+                + "}\n";
+        Model oldModel = Model.assembler().addUnparsedModel("foo.smithy", originalModel).assemble().unwrap();
+        Model newModel = ModelTransformer.create().replaceShapes(oldModel, ListUtils.of(
+                Shape.shapeToBuilder(oldModel.expectShape(ShapeId.from("smithy.example#Foo$baz")))
+                        .removeTrait(DefaultTrait.ID)
+                        .build()));
+
+        // The only emitted even should be a warning about the removal of the default trait, which can be ignored
+        // given the effective nullability of the member is unchanged.
+        List<ValidationEvent> events = ModelDiff.compare(oldModel, newModel);
+        assertThat(TestHelper.findEvents(events, "ChangedNullability"), empty());
+        assertThat(TestHelper.findEvents(events, "ChangedDefault"), empty());
+        assertThat(TestHelper.findEvents(events, "ModifiedTrait"), not(empty()));
+    }
 }

smithy-model/src/main/java/software/amazon/smithy/model/knowledge/NullableIndex.java

@@ -135,15 +135,11 @@ boolean isStructureMemberOptional(StructureShape container, MemberShape member,
         SERVER {
             @Override
             boolean isStructureMemberOptional(StructureShape container, MemberShape member, Shape target) {
-                // Defaults set to null on a member make the member optional.
-                if (member.hasNullDefault()) {
-                    return true;
-                } else if (member.hasNonNullDefault()) {
-                    return false;
-                } else {
-                    // A 2.0 member with the required trait is never nullable.
-                    return !member.hasTrait(RequiredTrait.class);
-                }
+                // Evaluated in this order.
+                // 1. Does the member have the required trait? Stop further checks, it's non-optional.
+                // 2. Does the member have a default trait set to null? Stop further checks, it's optional.
+                // 3. Does the member have a default trait not set to null? Stop further checks, it's non-optional.
+                return !member.hasTrait(RequiredTrait.class) && !member.hasNonNullDefault();
             }
         };
 

smithy-model/src/test/java/software/amazon/smithy/model/knowledge/NullableIndexTest.java

@@ -523,4 +523,29 @@ private void addedDefaultMakesMemberNullableInV1NotV2Assertions(Model model) {
         assertThat(index.isMemberNullable(model.expectShape(ShapeId.from("smithy.example#Foo$baz"), MemberShape.class)),
                    is(false));
     }
+
+    // The required trait makes this non-nullable. The default(null) trait just means that there's no default value,
+    // it doesn't make the member nullable. A value is still required to be present. Therefore, it's non-nullable
+    // in code generated types. "default(null)" just means the framework does not provide a default value.
+    @Test
+    public void requiredMembersAreNonNullableEvenIfDefaultNullTraitIsPresent() {
+        String modelText =
+                "$version: \"2.0\"\n"
+                + "namespace smithy.example\n"
+                + "structure Foo {\n"
+                + "    @required\n"
+                + "    baz: Integer = null\n"
+                + "}\n";
+        Model model = Model.assembler()
+                .addUnparsedModel("foo.smithy", modelText)
+                .assemble()
+                .unwrap();
+
+        NullableIndex index = NullableIndex.of(model);
+
+        assertThat(
+            index.isMemberNullable(model.expectShape(ShapeId.from("smithy.example#Foo$baz"), MemberShape.class)),
+            is(false)
+        );
+    }
 }