Improve ChangedNullability diff evaluator

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

@@ -25,11 +25,13 @@
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.knowledge.NullableIndex;
 import software.amazon.smithy.model.shapes.MemberShape;
+import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.StructureShape;
 import software.amazon.smithy.model.traits.ClientOptionalTrait;
 import software.amazon.smithy.model.traits.DefaultTrait;
 import software.amazon.smithy.model.traits.InputTrait;
 import software.amazon.smithy.model.traits.RequiredTrait;
+import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidationEvent;
 
 /**
@@ -91,37 +93,61 @@ private ValidationEvent createError(
         StringJoiner joiner = new StringJoiner("; ", message, "");
         boolean oldHasInput = hasInputTrait(differences.getOldModel(), oldMember);
         boolean newHasInput = hasInputTrait(differences.getNewModel(), newMember);
+        Shape newTarget = differences.getNewModel().expectShape(newMember.getTarget());
+        Severity severity = null;
 
         if (oldHasInput && !newHasInput) {
             // If there was an input trait before, but not now, then the nullability must have
             // changed from nullable to non-nullable.
             joiner.add("The @input trait was removed from " + newMember.getContainer());
+            severity = Severity.ERROR;
         } else if (!oldHasInput && newHasInput) {
             // If there was no input trait before, but there is now, then the nullability must have
             // changed from non-nullable to nullable.
             joiner.add("The @input trait was added to " + newMember.getContainer());
+            severity = Severity.DANGER;
         } else if (!newHasInput) {
             // Can't add nullable to a preexisting required member.
             if (change.isTraitAdded(ClientOptionalTrait.ID) && change.isTraitInBoth(RequiredTrait.ID)) {
                 joiner.add("The @nullable trait was added to a @required member.");
+                severity = Severity.ERROR;
             }
             // Can't add required to a member unless the member is marked as nullable.
             if (change.isTraitAdded(RequiredTrait.ID) && !newMember.hasTrait(ClientOptionalTrait.ID)) {
                 joiner.add("The @required trait was added to a member that is not marked as @nullable.");
+                severity = Severity.ERROR;
             }
             // Can't add the default trait to a member unless the member was previously required.
             if (change.isTraitAdded(DefaultTrait.ID) && !change.isTraitRemoved(RequiredTrait.ID)) {
                 joiner.add("The @default trait was added to a member that was not previously @required.");
+                severity = Severity.ERROR;
             }
             // Can only remove the required trait if the member was nullable or replaced by the default trait.
             if (change.isTraitRemoved(RequiredTrait.ID)
                     && !newMember.hasTrait(DefaultTrait.ID)
                     && !oldMember.hasTrait(ClientOptionalTrait.ID)) {
-                joiner.add("The @required trait was removed and not replaced with the @default trait.");
+                if (newTarget.isStructureShape() || newTarget.isUnionShape()) {
+                    severity = severity == null ? Severity.WARNING : severity;
+                    joiner.add("The @required trait was removed from a member that targets a " + newTarget.getType()
+                               + ". This is backward compatible in generators that always treat structures and "
+                               + "unions as optional (e.g., AWS generators)");
+                } else {
+                    joiner.add("The @required trait was removed and not replaced with the @default trait and "
+                               + "@addedDefault trait.");
+                    severity = Severity.ERROR;
+                }
             }
         }
 
-        return error(change.getNewShape(), joiner.toString());
+        // If no explicit severity was detected, then assume an error.
+        severity = severity == null ? Severity.ERROR : severity;
+
+        return ValidationEvent.builder()
+                .id(getEventId())
+                .shapeId(change.getNewShape())
+                .severity(severity)
+                .message(joiner.toString())
+                .build();
     }
 
     private boolean hasInputTrait(Model model, MemberShape member) {

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

@@ -21,6 +21,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import software.amazon.smithy.diff.Differences;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.node.Node;
@@ -34,11 +35,13 @@
 import software.amazon.smithy.model.shapes.StructureShape;
 import software.amazon.smithy.model.shapes.UnionShape;
 import software.amazon.smithy.model.traits.BoxTrait;
+import software.amazon.smithy.model.traits.RequiredTrait;
 import software.amazon.smithy.model.traits.Trait;
 import software.amazon.smithy.model.traits.TraitDefinition;
 import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidationEvent;
 import software.amazon.smithy.utils.ListUtils;
+import software.amazon.smithy.utils.SetUtils;
 
 /**
  * Finds breaking changes related to when a trait is added, removed, or
@@ -91,6 +94,9 @@ public final class ModifiedTrait extends AbstractDiffEvaluator {
             new DiffStrategy(DiffType.UPDATE, Severity.NOTE),
             new DiffStrategy(DiffType.REMOVE, Severity.WARNING));
 
+    /** Traits in this list have special backward compatibility rules and can't be validated here. */
+    private static final Set<ShapeId> IGNORED_TRAITS = SetUtils.of(BoxTrait.ID, RequiredTrait.ID);
+
     @Override
     public List<ValidationEvent> evaluate(Differences differences) {
         // Map of trait shape ID to diff strategies to evaluate.
@@ -102,7 +108,7 @@ public List<ValidationEvent> evaluate(Differences differences) {
                 Trait oldTrait = oldTraitNewTraitPair.left;
                 Trait newTrait = oldTraitNewTraitPair.right;
                 // Do not emit for the box trait because it is added and removed for backward compatibility.
-                if (!traitId.equals(BoxTrait.ID) && strategies.containsKey(traitId)) {
+                if (!IGNORED_TRAITS.contains(traitId) && strategies.containsKey(traitId)) {
                     for (DiffStrategy strategy : strategies.get(traitId)) {
                         List<ValidationEvent> diffEvents = strategy.diffType.validate(
                                 differences.getNewModel(),

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

@@ -3,6 +3,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
 
 import java.util.List;
@@ -12,6 +13,7 @@
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.ModelSerializer;
+import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.shapes.StringShape;
 import software.amazon.smithy.model.shapes.StructureShape;
@@ -23,6 +25,7 @@
 import software.amazon.smithy.model.transform.ModelTransformer;
 import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidationEvent;
+import software.amazon.smithy.utils.ListUtils;
 
 public class ChangedNullabilityTest {
     @Test
@@ -197,7 +200,7 @@ public void detectsAdditionOfInputTrait() {
         List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
 
         assertThat(events.stream()
-                           .filter(event -> event.getSeverity() == Severity.ERROR)
+                           .filter(event -> event.getSeverity() == Severity.DANGER)
                            .filter(event -> event.getId().equals("ChangedNullability"))
                            .filter(event -> event.getMessage().contains("The @input trait was added to"))
                            .count(), equalTo(1L));
@@ -305,4 +308,53 @@ public void roundTrippedV1ModelHasNoEvents() {
 
         assertThat(events, empty());
     }
+
+    @Test
+    public void specialHandlingForRequiredStructureMembers() {
+        String originalModel =
+                "$version: \"2.0\"\n"
+                + "namespace smithy.example\n"
+                + "structure Baz {}\n"
+                + "structure Foo {\n"
+                + "    @required\n"
+                + "    baz: Baz\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(RequiredTrait.ID)
+                        .build()));
+
+        List<ValidationEvent> events = TestHelper.findEvents(
+                ModelDiff.compare(oldModel, newModel), "ChangedNullability");
+
+        assertThat(events, hasSize(1));
+        assertThat(events.get(0).getSeverity(), is(Severity.WARNING));
+    }
+
+    @Test
+    public void specialHandlingForRequiredUnionMembers() {
+        String originalModel =
+                "$version: \"2.0\"\n"
+                + "namespace smithy.example\n"
+                + "union Baz {\n"
+                + "    a: String\n"
+                + "    b: String\n"
+                + "}\n"
+                + "structure Foo {\n"
+                + "    @required\n"
+                + "    baz: Baz\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(RequiredTrait.ID)
+                        .build()));
+
+        List<ValidationEvent> events = TestHelper.findEvents(
+                ModelDiff.compare(oldModel, newModel), "ChangedNullability");
+
+        assertThat(events, hasSize(1));
+        assertThat(events.get(0).getSeverity(), is(Severity.WARNING));
+    }
 }

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

@@ -18,6 +18,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 
@@ -35,10 +36,13 @@
 import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.shapes.StringShape;
 import software.amazon.smithy.model.traits.DynamicTrait;
+import software.amazon.smithy.model.traits.RequiredTrait;
 import software.amazon.smithy.model.traits.TagsTrait;
 import software.amazon.smithy.model.traits.TraitDefinition;
+import software.amazon.smithy.model.transform.ModelTransformer;
 import software.amazon.smithy.model.validation.Severity;
 import software.amazon.smithy.model.validation.ValidationEvent;
+import software.amazon.smithy.utils.ListUtils;
 
 public class ModifiedTraitTest {
     private static final ShapeId ID = ShapeId.from("com.foo#baz");
@@ -267,4 +271,23 @@ public void findsDifferencesInTraitValuesOfAllSeverities() {
                 "Changed trait contents of `smithy.example#aTrait` at path `/l` from \"a\" to \"l\""
         ));
     }
+
+    @Test
+    public void letsOtherValidatorsHandleRequiredTrait() {
+        String originalModel =
+                "$version: \"2.0\"\n"
+                + "namespace smithy.example\n"
+                + "structure Baz {}\n"
+                + "structure Foo {\n"
+                + "    @required\n"
+                + "    baz: Baz\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(RequiredTrait.ID)
+                        .build()));
+
+        assertThat(TestHelper.findEvents(ModelDiff.compare(oldModel, newModel), "ModifiedTrait"), empty());
+    }
 }