Prevent bad list, set, map recursion

docs/source/spec/core.rst

@@ -415,8 +415,7 @@ list
 The :dfn:`list` type represents a homogeneous collection of values. A list is
 defined using a :token:`list_statement`. A list statement consists of the
 shape named followed by an object with a single key-value pair of "member"
-that defines the :ref:`member <member>` of the list. The member of a list
-cannot target the containing list shape.
+that defines the :ref:`member <member>` of the list.
 
 The following example defines a list with a string member from the
 :ref:`prelude <prelude>`:
@@ -514,8 +513,7 @@ set
 The :dfn:`set` type represents an unordered collection of unique homogeneous
 values. A set is defined using a :token:`set_statement` that consists of the
 shape named followed by an object with a single key-value pair of "member"
-that defines the :ref:`member <member>` of the set. The member of a set
-cannot target the containing set shape.
+that defines the :ref:`member <member>` of the set.
 
 The following example defines a set of strings:
 
@@ -947,6 +945,82 @@ with the box trait. Members that target strings, timestamps, and
 aggregate shapes are always considered boxed and have no default values.
 
 
+Recursive shape definitions
+```````````````````````````
+
+Smithy allows for recursive shape definitions with the following constraint:
+the member of a list, set, or map cannot directly or transitively target its
+containing shape unless one or more members in the path from the container
+back to itself targets a structure or union shape. This ensures 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....``).
+
+The following shape definition is invalid:
+
+.. tabs::
+
+    .. code-tab:: smithy
+
+        list RecursiveList {
+            member: RecursiveList
+        }
+
+    .. code-tab:: json
+
+        {
+            "smithy": "0.4.0",
+            "smithy.example": {
+                "shapes": {
+                    "RecursiveList": {
+                        "type": "list",
+                        "member": {
+                            "target": "RecursiveList"
+                        }
+                    }
+                }
+            }
+        }
+
+The following shape definition is valid:
+
+.. tabs::
+
+    .. code-tab:: smithy
+
+        list ValidList {
+            member: IntermediateStructure
+        }
+
+        structure IntermediateStructure {
+            foo: ValidList
+        }
+
+    .. code-tab:: json
+
+        {
+            "smithy": "0.4.0",
+            "smithy.example": {
+                "shapes": {
+                    "ValidList": {
+                        "type": "list",
+                        "member": {
+                            "target": "IntermediateStructure"
+                        }
+                    },
+                    "IntermediateStructure": {
+                        "type": "structure",
+                        "members": {
+                            "foo": {
+                                "target": "ValidList"
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+
 .. _service-types:
 
 Service types

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

@@ -0,0 +1,130 @@
+/*
+ * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package software.amazon.smithy.model.validation.validators;
+
+import java.util.ArrayDeque;
+import java.util.Deque;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Objects;
+import java.util.Set;
+import java.util.stream.Collectors;
+import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.shapes.ListShape;
+import software.amazon.smithy.model.shapes.MapShape;
+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.ShapeId;
+import software.amazon.smithy.model.shapes.ShapeIndex;
+import software.amazon.smithy.model.shapes.ShapeVisitor;
+import software.amazon.smithy.model.validation.AbstractValidator;
+import software.amazon.smithy.model.validation.ValidationEvent;
+
+/**
+ * Ensures that list, set, and map shapes are not directly recursive,
+ * meaning that if they do have a recursive reference to themselves,
+ * one or more references that form the recursive path travels through
+ * a structure or union shape.
+ *
+ * <p>This check removes an entire class of problems from things like
+ * code generators where a list of itself or a list of maps of itself
+ * is impossible to define.
+ */
+public class ShapeRecursionValidator extends AbstractValidator {
+
+    @Override
+    public List<ValidationEvent> validate(Model model) {
+        ShapeIndex index = model.getShapeIndex();
+        return index.shapes()
+                .map(shape -> validateShape(index, shape))
+                .filter(Objects::nonNull)
+                .collect(Collectors.toList());
+    }
+
+    private ValidationEvent validateShape(ShapeIndex index, Shape shape) {
+        return new RecursiveNeighborVisitor(index, shape).visit(shape);
+    }
+
+    private final class RecursiveNeighborVisitor extends ShapeVisitor.Default<ValidationEvent> {
+
+        private final ShapeIndex index;
+        private final Shape root;
+        private final Set<ShapeId> visited = new HashSet<>();
+        private final Deque<String> context = new ArrayDeque<>();
+
+        RecursiveNeighborVisitor(ShapeIndex index, Shape root) {
+            this.root = root;
+            this.index = index;
+        }
+
+        ValidationEvent visit(Shape shape) {
+            ValidationEvent event = hasShapeBeenVisited(shape);
+            return event != null ? event : shape.accept(this);
+        }
+
+        private ValidationEvent hasShapeBeenVisited(Shape shape) {
+            if (!visited.contains(shape.getId())) {
+                return null;
+            }
+
+            return error(shape, String.format(
+                    "Found invalid shape recursion: %s. A recursive list, set, or map shape is only valid if "
+                    + "an intermediate reference is through a union or structure.",
+                    String.join(" > ", context)));
+        }
+
+        @Override
+        protected ValidationEvent getDefault(Shape shape) {
+            return null;
+        }
+
+        @Override
+        public ValidationEvent listShape(ListShape shape) {
+            return validateMember(shape, shape.getMember());
+        }
+
+        @Override
+        public ValidationEvent setShape(SetShape shape) {
+            return validateMember(shape, shape.getMember());
+        }
+
+        @Override
+        public ValidationEvent mapShape(MapShape shape) {
+            return validateMember(shape, shape.getValue());
+        }
+
+        private ValidationEvent validateMember(Shape container, MemberShape member) {
+            ValidationEvent event = null;
+            Shape target = index.getShape(member.getTarget()).orElse(null);
+
+            if (target != null) {
+                // Add to the visited set and the context deque before visiting,
+                // the remove from them after done visiting this shape.
+                visited.add(container.getId());
+                // Eventually, this would look like: member-id > shape-id[ > member-id > shape-id [ > [...]]
+                context.addLast(member.getId().toString());
+                context.addLast(member.getTarget().toString());
+                event = visit(target);
+                context.removeLast();
+                context.removeLast();
+                visited.remove(container.getId());
+            }
+
+            return event;
+        }
+    }
+}

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

@@ -47,7 +47,6 @@ public class TargetValidator extends AbstractValidator {
 
     private static final Set<ShapeType> INVALID_MEMBER_TARGETS = SetUtils.of(
             ShapeType.SERVICE, ShapeType.RESOURCE, ShapeType.OPERATION, ShapeType.MEMBER);
-    private static final Set<ShapeType> INVALID_RECURSIVE_MEMBER_TARGETS = SetUtils.of(ShapeType.LIST, ShapeType.SET);
 
     @Override
     public List<ValidationEvent> validate(Model model) {
@@ -85,10 +84,6 @@ private Optional<ValidationEvent> validateTarget(ShapeIndex index, Shape shape,
                 if (INVALID_MEMBER_TARGETS.contains(target.getType())) {
                     return Optional.of(error(shape, format(
                             "Members cannot target %s shapes, but found %s", target.getType(), target)));
-                } else if (target.getId().equals(shape.getId().withoutMember())
-                           && INVALID_RECURSIVE_MEMBER_TARGETS.contains(target.getType())) {
-                    return Optional.of(error(shape, format(
-                            "%s shape members cannot target their container", target.getType())));
                 }
             case MAP_KEY:
                 return target.asMemberShape().flatMap(m -> validateMapKey(shape, m.getTarget(), index));

smithy-model/src/main/resources/META-INF/services/software.amazon.smithy.model.validation.Validator

@@ -25,6 +25,7 @@ software.amazon.smithy.model.validation.validators.ResourceIdentifierValidator
 software.amazon.smithy.model.validation.validators.ResourceLifecycleValidator
 software.amazon.smithy.model.validation.validators.ServiceValidator
 software.amazon.smithy.model.validation.validators.ShapeIdConflictValidator
+software.amazon.smithy.model.validation.validators.ShapeRecursionValidator
 software.amazon.smithy.model.validation.validators.SingleOperationBindingValidator
 software.amazon.smithy.model.validation.validators.SingleResourceBindingValidator
 software.amazon.smithy.model.validation.validators.TargetValidator

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

@@ -0,0 +1,12 @@
+[ERROR] ns.foo#IndirectRecursiveList: Found invalid shape recursion: ns.foo#IndirectRecursiveList$member > ns.foo#IndirectRecursiveListIntermediate1 > ns.foo#IndirectRecursiveListIntermediate1$member > ns.foo#IndirectRecursiveListIntermediate2 > ns.foo#IndirectRecursiveListIntermediate2$member > ns.foo#IndirectRecursiveList. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#IndirectRecursiveListIntermediate1: Found invalid shape recursion: ns.foo#IndirectRecursiveListIntermediate1$member > ns.foo#IndirectRecursiveListIntermediate2 > ns.foo#IndirectRecursiveListIntermediate2$member > ns.foo#IndirectRecursiveList > ns.foo#IndirectRecursiveList$member > ns.foo#IndirectRecursiveListIntermediate1. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#IndirectRecursiveListIntermediate2: Found invalid shape recursion: ns.foo#IndirectRecursiveListIntermediate2$member > ns.foo#IndirectRecursiveList > ns.foo#IndirectRecursiveList$member > ns.foo#IndirectRecursiveListIntermediate1 > ns.foo#IndirectRecursiveListIntermediate1$member > ns.foo#IndirectRecursiveListIntermediate2. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#IndirectRecursiveMap: Found invalid shape recursion: ns.foo#IndirectRecursiveMap$value > ns.foo#IndirectRecursiveMapIntermediate1 > ns.foo#IndirectRecursiveMapIntermediate1$value > ns.foo#IndirectRecursiveMapIntermediate2 > ns.foo#IndirectRecursiveMapIntermediate2$value > ns.foo#IndirectRecursiveMap. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#IndirectRecursiveMapIntermediate1: Found invalid shape recursion: ns.foo#IndirectRecursiveMapIntermediate1$value > ns.foo#IndirectRecursiveMapIntermediate2 > ns.foo#IndirectRecursiveMapIntermediate2$value > ns.foo#IndirectRecursiveMap > ns.foo#IndirectRecursiveMap$value > ns.foo#IndirectRecursiveMapIntermediate1. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#IndirectRecursiveMapIntermediate2: Found invalid shape recursion: ns.foo#IndirectRecursiveMapIntermediate2$value > ns.foo#IndirectRecursiveMap > ns.foo#IndirectRecursiveMap$value > ns.foo#IndirectRecursiveMapIntermediate1 > ns.foo#IndirectRecursiveMapIntermediate1$value > ns.foo#IndirectRecursiveMapIntermediate2. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#IndirectRecursiveSet: Found invalid shape recursion: ns.foo#IndirectRecursiveSet$member > ns.foo#IndirectRecursiveSetIntermediate1 > ns.foo#IndirectRecursiveSetIntermediate1$member > ns.foo#IndirectRecursiveSetIntermediate2 > ns.foo#IndirectRecursiveSetIntermediate2$member > ns.foo#IndirectRecursiveSet. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#IndirectRecursiveSetIntermediate1: Found invalid shape recursion: ns.foo#IndirectRecursiveSetIntermediate1$member > ns.foo#IndirectRecursiveSetIntermediate2 > ns.foo#IndirectRecursiveSetIntermediate2$member > ns.foo#IndirectRecursiveSet > ns.foo#IndirectRecursiveSet$member > ns.foo#IndirectRecursiveSetIntermediate1. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#IndirectRecursiveSetIntermediate2: Found invalid shape recursion: ns.foo#IndirectRecursiveSetIntermediate2$member > ns.foo#IndirectRecursiveSet > ns.foo#IndirectRecursiveSet$member > ns.foo#IndirectRecursiveSetIntermediate1 > ns.foo#IndirectRecursiveSetIntermediate1$member > ns.foo#IndirectRecursiveSetIntermediate2. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#InvalidDirectlyRecursiveList: Found invalid shape recursion: ns.foo#InvalidDirectlyRecursiveList$member > ns.foo#InvalidDirectlyRecursiveList. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#InvalidDirectlyRecursiveMap: Found invalid shape recursion: ns.foo#InvalidDirectlyRecursiveMap$value > ns.foo#InvalidDirectlyRecursiveMap. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion
+[ERROR] ns.foo#InvalidDirectlyRecursiveSet: Found invalid shape recursion: ns.foo#InvalidDirectlyRecursiveSet$member > ns.foo#InvalidDirectlyRecursiveSet. A recursive list, set, or map shape is only valid if an intermediate reference is through a union or structure. | ShapeRecursion

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

@@ -0,0 +1,112 @@
+{
+  "smithy": "0.4.0",
+  "ns.foo": {
+    "shapes": {
+      "InvalidDirectlyRecursiveList": {
+        "type": "list",
+        "member": {
+          "target": "InvalidDirectlyRecursiveList"
+        }
+      },
+      "InvalidDirectlyRecursiveSet": {
+        "type": "set",
+        "member": {
+          "target": "InvalidDirectlyRecursiveSet"
+        }
+      },
+      "InvalidDirectlyRecursiveMap": {
+        "type": "map",
+        "key": {
+          "target": "String"
+        },
+        "value": {
+          "target": "InvalidDirectlyRecursiveMap"
+        }
+      },
+
+      "IndirectRecursiveList": {
+        "type": "list",
+        "member": {
+          "target": "IndirectRecursiveListIntermediate1"
+        }
+      },
+      "IndirectRecursiveListIntermediate1": {
+        "type": "list",
+        "member": {
+          "target": "IndirectRecursiveListIntermediate2"
+        }
+      },
+      "IndirectRecursiveListIntermediate2": {
+        "type": "list",
+        "member": {
+          "target": "IndirectRecursiveList"
+        }
+      },
+
+      "IndirectRecursiveSet": {
+        "type": "set",
+        "member": {
+          "target": "IndirectRecursiveSetIntermediate1"
+        }
+      },
+      "IndirectRecursiveSetIntermediate1": {
+        "type": "set",
+        "member": {
+          "target": "IndirectRecursiveSetIntermediate2"
+        }
+      },
+      "IndirectRecursiveSetIntermediate2": {
+        "type": "set",
+        "member": {
+          "target": "IndirectRecursiveSet"
+        }
+      },
+
+      "IndirectRecursiveMap": {
+        "type": "map",
+        "key": {
+          "target": "String"
+        },
+        "value": {
+          "target": "IndirectRecursiveMapIntermediate1"
+        }
+      },
+      "IndirectRecursiveMapIntermediate1": {
+        "type": "map",
+        "key": {
+          "target": "String"
+        },
+        "value": {
+          "target": "IndirectRecursiveMapIntermediate2"
+        }
+      },
+      "IndirectRecursiveMapIntermediate2": {
+        "type": "map",
+        "key": {
+          "target": "String"
+        },
+        "value": {
+          "target": "IndirectRecursiveMap"
+        }
+      },
+
+      "ValidRecursiveShape": {
+        "type": "map",
+        "key": {
+          "target": "String"
+        },
+        "value": {
+          "target": "ValidRecursiveShapeStruct"
+        }
+      },
+      "ValidRecursiveShapeStruct": {
+        "type": "structure",
+        "members": {
+          "foo": {
+            "target": "ValidRecursiveShape"
+          }
+        }
+      }
+    }
+  }
+}

smithy-model/src/test/resources/software/amazon/smithy/model/errorfiles/validators/target-validator.errors

@@ -8,7 +8,6 @@
 [ERROR] ns.foo#InvalidListMemberReference$member: member shape targets an unresolved shape `ns.foo#NotFound` | Target
 [ERROR] ns.foo#InvalidListMemberResource$member: Members cannot target resource shapes, but found (resource: `ns.foo#MyResource`) | Target
 [ERROR] ns.foo#InvalidListMemberService$member: Members cannot target service shapes, but found (service: `ns.foo#MyService`) | Target
-[ERROR] ns.foo#InvalidRecursiveList$member: list shape members cannot target their container | Target
 [ERROR] ns.foo#InvalidMapType: Map key member targets (structure: `ns.foo#ValidInput`), but is expected to target a string | Target
 [ERROR] ns.foo#InvalidOperationBadErrorTraits: Operation error targets an invalid structure, `ns.foo#ValidInput`, that is not marked with the `error` trait. | Target
 [ERROR] ns.foo#InvalidOperationBadErrorTraits: Operation input targets an invalid structure `ns.foo#ValidError` that is marked with the `error` trait. | Target