Skip to content

Commit

Permalink
Forbid impossibly recursive unions
Browse files Browse the repository at this point in the history
Closes #1247
  • Loading branch information
mtdowling authored and Michael Dowling committed Jun 13, 2022
1 parent b59f9f8 commit 358fd45
Show file tree
Hide file tree
Showing 8 changed files with 149 additions and 30 deletions.
10 changes: 6 additions & 4 deletions docs/source/1.0/spec/core/model.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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**:

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
}
}
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
}

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
"com.foo#B": {
"type": "union",
"members": {
"a": {
"target": "com.foo#A"
"str": {
"target": "smithy.api#String"
}
}
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ union A {
}

union B {
a: A
str: String
}

union C
Expand Down

0 comments on commit 358fd45

Please sign in to comment.