Forbid impossibly recursive unions

docs/source/1.0/spec/core/model.rst

@@ -967,10 +967,12 @@ Smithy allows recursive shape definitions with the following limitations:
    that shapes that are typically impossible to define in various programming
    languages are not defined in Smithy models (for example, you can't define
    a recursive list in Java ``List<List<List....``).
-2. A structure cannot contain a cyclical set of members marked with the
-   :ref:`required-trait` that refers back to itself.
-3. A union MUST contain at least one member that does not refer back to
-   itself.
+2. To ensure a value can be provided for a structure, recursive member
+   relationship from a structure back to itself MUST NOT be made up of all
+   :ref:`required <required-trait>` structure members.
+3. To ensure a value can be provided for a union, recursive unions MUST
+   contain at least one path through its members that is not recursive
+   or steps through a list, set, map, or optional structure member.
 
 The following recursive shape definition is **valid**:
 

smithy-model/src/main/java/software/amazon/smithy/model/validation/validators/ShapeRecursionValidator.java

@@ -17,7 +17,9 @@
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.StringJoiner;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.selector.PathFinder;
@@ -26,6 +28,7 @@
 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.shapes.ShapeVisitor;
 import software.amazon.smithy.model.shapes.StructureShape;
 import software.amazon.smithy.model.shapes.UnionShape;
 import software.amazon.smithy.model.traits.RequiredTrait;
@@ -113,16 +116,75 @@ private String formatPath(PathFinder.Path path) {
     }
 
     private void validateUnions(Model model, List<ValidationEvent> events) {
-        nextUnion: for (UnionShape union : model.getUnionShapes()) {
-            // Don't claim the union is recursive if it's empty (which is also invalid).
-            if (!union.getAllMembers().isEmpty()) {
-                for (MemberShape member : union.getAllMembers().values()) {
-                    if (!member.getTarget().equals(member.getContainer())) {
-                        continue nextUnion;
+        UnionTerminatesVisitor visitor = new UnionTerminatesVisitor(model);
+        for (UnionShape union : model.getUnionShapes()) {
+            // Don't evaluate empty unions since that's a different error.
+            if (!union.members().isEmpty() && !union.accept(visitor)) {
+                events.add(error(union, "It is impossible to create instances of this recursive union"));
+            }
+            visitor.reset();
+        }
+    }
+
+    private static final class UnionTerminatesVisitor extends ShapeVisitor.Default<Boolean> {
+
+        private final Set<MemberShape> visited = new HashSet<>();
+        private final Model model;
+
+        UnionTerminatesVisitor(Model model) {
+            this.model = model;
+        }
+
+        void reset() {
+            this.visited.clear();
+        }
+
+        @Override
+        protected Boolean getDefault(Shape shape) {
+            return true;
+        }
+
+        @Override
+        public Boolean structureShape(StructureShape shape) {
+            if (shape.members().isEmpty()) {
+                return true;
+            }
+
+            // If the structure has any non-required members, then it terminates.
+            for (MemberShape member : shape.members()) {
+                if (!member.isRequired()) {
+                    return true;
+                }
+            }
+
+            // Now check if any of the required members terminate.
+            for (MemberShape member : shape.members()) {
+                if (member.isRequired()) {
+                    if (memberShape(member)) {
+                        return true;
                     }
                 }
-                events.add(error(union, "Unions must contain at least one member that is not directly recursive"));
             }
+
+            return false;
+        }
+
+        @Override
+        public Boolean unionShape(UnionShape shape) {
+            for (MemberShape member : shape.members()) {
+                if (memberShape(member)) {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
+        @Override
+        public Boolean memberShape(MemberShape shape) {
+            return visited.add(shape)
+                   ? model.expectShape(shape.getTarget()).accept(this)
+                   : false;
         }
     }
 }

smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-union.errors

@@ -0,0 +1,4 @@
+[ERROR] smithy.example#RecursiveUnionA: It is impossible to create instances of this recursive union | ShapeRecursion
+[ERROR] smithy.example#RecursiveUnionB: It is impossible to create instances of this recursive union | ShapeRecursion
+[ERROR] smithy.example#RecursiveUnion: It is impossible to create instances of this recursive union | ShapeRecursion
+[ERROR] smithy.example#RecursiveNext: It is impossible to create instances of this recursive union | ShapeRecursion

smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/shape-recursion-union.smithy

@@ -0,0 +1,67 @@
+$version: "1.0"
+
+namespace smithy.example
+
+// This is impossible because there is no non-recursive branch along
+// the recursive path back to itself.
+union RecursiveUnionA {
+    b: RecursiveUnionB
+}
+
+union RecursiveUnionB {
+    a: RecursiveUnionA
+}
+
+// These unions are fine because there is a non-recursive branch along the
+// recursive path back to itself:
+// 1. {"e": {"str": "hi"}}
+// 2. {"d": {"c": {"e": {"str": "hi"}}}}
+union RecursiveUnionC {
+    d: RecursiveUnionD,
+    e: OkUnion
+}
+
+union RecursiveUnionD {
+    c: RecursiveUnionC
+}
+
+union OkUnion {
+    str: String
+}
+
+// This is fine too because on the of the recursive branches contains a list
+// which gives the type a size.
+union RecursiveUnionE {
+     f: RecursiveUnionF
+}
+
+union RecursiveUnionF {
+    e: EList
+}
+
+list EList {
+    member: RecursiveUnionE
+}
+
+// It's impossible to provide a value for this union.
+union RecursiveUnion {
+    a: RecursiveUnion,
+    b: RecursiveUnion,
+}
+
+union NotFullyRecursiveUnion {
+    a: RecursiveUnion,
+    b: RecursiveUnion,
+    c: String
+}
+
+// This structure is invalid and the union is invalid
+structure Start {
+    @required
+    foo: RecursiveNext
+}
+
+union RecursiveNext {
+    a: Start,
+    b: Start
+}

smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/unions.json

@@ -12,8 +12,8 @@
         "com.foo#B": {
             "type": "union",
             "members": {
-                "a": {
-                    "target": "com.foo#A"
+                "str": {
+                    "target": "smithy.api#String"
                 }
             }
         },

smithy-model/src/test/resources/software/amazon/smithy/model/loader/valid/unions.smithy

@@ -5,7 +5,7 @@ union A {
 }
 
 union B {
-  a: A
+  str: String
 }
 
 union C