Make structure order part of the contract

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

@@ -809,6 +809,12 @@ Traits can be applied to structure members:
             }
         }
 
+New members added to existing structures SHOULD be added to the end of the
+structure. This ensures that programming languages that require a specific
+data structure layout or alignment for code generated from Smithy models are
+able to maintain backward compatibility.
+
+
 .. _union:
 
 Union
@@ -861,6 +867,11 @@ The following example defines a union shape with several members:
             }
         }
 
+New members added to existing unions SHOULD be added to the end of the
+union. This ensures that programming languages that require a specific
+data structure layout or alignment for code generated from Smithy models are
+able to maintain backward compatibility.
+
 
 .. _default-values:
 

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

@@ -0,0 +1,71 @@
+/*
+ * Copyright 2020 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.diff.evaluators;
+
+import java.util.Collection;
+import java.util.Iterator;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import software.amazon.smithy.diff.ChangedShape;
+import software.amazon.smithy.diff.Differences;
+import software.amazon.smithy.model.shapes.MemberShape;
+import software.amazon.smithy.model.shapes.StructureShape;
+import software.amazon.smithy.model.shapes.UnionShape;
+import software.amazon.smithy.model.validation.ValidationEvent;
+
+/**
+ * Creates a DANGER event when a structure or union member is added
+ * anywhere other than the end of the previous definition or the
+ * member order is changed.
+ */
+public final class ChangedMemberOrder extends AbstractDiffEvaluator {
+    @Override
+    public List<ValidationEvent> evaluate(Differences differences) {
+        Stream<ChangedShape<?>> changes = Stream.concat(
+                differences.changedShapes(StructureShape.class),
+                differences.changedShapes(UnionShape.class));
+
+        return changes
+                .filter(diff -> isUnordered(diff.getOldShape().members(), diff.getNewShape().members()))
+                .map(diff -> danger(diff.getNewShape(), String.format(
+                        "%s shape members were reordered. This can cause ABI compatibility issues in languages "
+                        + "like C, C++, and Rust where the layout and alignment of a data structure matters.",
+                        diff.getOldShape().getType())))
+                .collect(Collectors.toList());
+    }
+
+    private static boolean isUnordered(Collection<MemberShape> a, Collection<MemberShape> b) {
+        Iterator<MemberShape> aIter = a.iterator();
+        Iterator<MemberShape> bIter = b.iterator();
+
+        while (aIter.hasNext()) {
+            // If members were removed, then this check is satisfied (though there are
+            // other backward incompatible changes that other evaluators will detect).
+            if (!bIter.hasNext()) {
+                break;
+            }
+
+            String oldMember = aIter.next().getMemberName();
+            String newMember = bIter.next().getMemberName();
+            if (!oldMember.equals(newMember)) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+}

smithy-diff/src/main/resources/META-INF/services/software.amazon.smithy.diff.DiffEvaluator

@@ -6,6 +6,7 @@ software.amazon.smithy.diff.evaluators.AddedShape
 software.amazon.smithy.diff.evaluators.AddedTraitDefinition
 software.amazon.smithy.diff.evaluators.ChangedEnumTrait
 software.amazon.smithy.diff.evaluators.ChangedLengthTrait
+software.amazon.smithy.diff.evaluators.ChangedMemberOrder
 software.amazon.smithy.diff.evaluators.ChangedMemberTarget
 software.amazon.smithy.diff.evaluators.ChangedMetadata
 software.amazon.smithy.diff.evaluators.ChangedOperationInput

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

@@ -0,0 +1,137 @@
+/*
+ * Copyright 2020 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.diff.evaluators;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.equalTo;
+
+import java.util.List;
+import org.junit.jupiter.api.Test;
+import software.amazon.smithy.diff.ModelDiff;
+import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.shapes.MemberShape;
+import software.amazon.smithy.model.shapes.Shape;
+import software.amazon.smithy.model.shapes.StringShape;
+import software.amazon.smithy.model.shapes.StructureShape;
+import software.amazon.smithy.model.validation.Severity;
+import software.amazon.smithy.model.validation.ValidationEvent;
+
+public class ChangedMemberOrderTest {
+    @Test
+    public void detectsOrderChanges() {
+        Shape shape1 = StringShape.builder().id("foo.baz#String").build();
+        MemberShape member1 = MemberShape.builder().id("foo.baz#Struct$member1").target(shape1).build();
+        MemberShape member2 = MemberShape.builder().id("foo.baz#Struct$member2").target(shape1).build();
+        StructureShape oldStruct = StructureShape.builder()
+                .id("foo.baz#Struct")
+                .addMember(member1)
+                .addMember(member2)
+                .build();
+
+        StructureShape newStruct = StructureShape.builder()
+                .id("foo.baz#Struct")
+                .addMember(member2)
+                .addMember(member1)
+                .build();
+
+        Model modelA = Model.assembler().addShapes(shape1, oldStruct).assemble().unwrap();
+        Model modelB = Model.assembler().addShapes(shape1, newStruct).assemble().unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberOrder").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, oldStruct.getId()).size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, Severity.DANGER).size(), equalTo(1));
+    }
+
+    @Test
+    public void detectsOrderInsertionChanges() {
+        Shape shape1 = StringShape.builder().id("foo.baz#String").build();
+        MemberShape member1 = MemberShape.builder().id("foo.baz#Struct$member1").target(shape1).build();
+        MemberShape member2 = MemberShape.builder().id("foo.baz#Struct$member2").target(shape1).build();
+        StructureShape oldStruct = StructureShape.builder()
+                .id("foo.baz#Struct")
+                .addMember(member1)
+                .addMember(member2)
+                .build();
+
+        MemberShape member3 = MemberShape.builder().id("foo.baz#Struct$member3").target(shape1).build();
+        StructureShape newStruct = StructureShape.builder()
+                .id("foo.baz#Struct")
+                .addMember(member1)
+                .addMember(member3)
+                .addMember(member2)
+                .build();
+
+        Model modelA = Model.assembler().addShapes(shape1, oldStruct).assemble().unwrap();
+        Model modelB = Model.assembler().addShapes(shape1, newStruct).assemble().unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberOrder").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, oldStruct.getId()).size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, Severity.DANGER).size(), equalTo(1));
+    }
+
+    @Test
+    public void detectsCompatibleChanges() {
+        Shape shape1 = StringShape.builder().id("foo.baz#String").build();
+        MemberShape member1 = MemberShape.builder().id("foo.baz#Struct$member1").target(shape1).build();
+        MemberShape member2 = MemberShape.builder().id("foo.baz#Struct$member2").target(shape1).build();
+        StructureShape oldStruct = StructureShape.builder()
+                .id("foo.baz#Struct")
+                .addMember(member1)
+                .addMember(member2)
+                .build();
+
+        MemberShape member3 = MemberShape.builder().id("foo.baz#Struct$member3").target(shape1).build();
+        StructureShape newStruct = StructureShape.builder()
+                .id("foo.baz#Struct")
+                .addMember(member1)
+                .addMember(member2)
+                .addMember(member3)
+                .build();
+
+        Model modelA = Model.assembler().addShapes(shape1, oldStruct).assemble().unwrap();
+        Model modelB = Model.assembler().addShapes(shape1, newStruct).assemble().unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberOrder"), empty());
+    }
+
+    // Ignore the fact that a member was removed.
+    @Test
+    public void ignoresOtherBreakingChanges() {
+        Shape shape1 = StringShape.builder().id("foo.baz#String").build();
+        MemberShape member1 = MemberShape.builder().id("foo.baz#Struct$member1").target(shape1).build();
+        MemberShape member2 = MemberShape.builder().id("foo.baz#Struct$member2").target(shape1).build();
+        StructureShape oldStruct = StructureShape.builder()
+                .id("foo.baz#Struct")
+                .addMember(member1)
+                .addMember(member2)
+                .build();
+
+        StructureShape newStruct = StructureShape.builder()
+                .id("foo.baz#Struct")
+                .addMember(member1)
+                .build();
+
+        Model modelA = Model.assembler().addShapes(shape1, oldStruct).assemble().unwrap();
+        Model modelB = Model.assembler().addShapes(shape1, newStruct).assemble().unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberOrder"), empty());
+    }
+}

smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderVisitor.java

@@ -21,6 +21,7 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -83,10 +84,10 @@ final class LoaderVisitor {
     private final List<ForwardReferenceResolver> forwardReferenceResolvers = new ArrayList<>();
 
     /** Shapes that have yet to be built. */
-    private final Map<ShapeId, AbstractShapeBuilder> pendingShapes = new HashMap<>();
+    private final Map<ShapeId, AbstractShapeBuilder> pendingShapes = new LinkedHashMap<>();
 
     /** Built shapes to add to the Model. Keys are not allowed to conflict with pendingShapes. */
-    private final Map<ShapeId, Shape> builtShapes = new HashMap<>();
+    private final Map<ShapeId, Shape> builtShapes = new LinkedHashMap<>();
 
     /** Built trait definitions. */
     private final Map<ShapeId, TraitDefinition> builtTraitDefinitions = new HashMap<>();

smithy-model/src/main/java/software/amazon/smithy/model/node/ObjectNode.java

@@ -19,7 +19,6 @@
 
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.Map;
@@ -221,7 +220,7 @@ public Map<String, Node> getMembersByPrefix(String prefix) {
     public Map<String, Node> getStringMap() {
         Map<String, Node> map = stringMap;
         if (map == null) {
-            map = new HashMap<>(nodeMap.size());
+            map = new LinkedHashMap<>(nodeMap.size());
             for (Map.Entry<StringNode, Node> entry : nodeMap.entrySet()) {
                 map.put(entry.getKey().getValue(), entry.getValue());
             }

smithy-model/src/main/java/software/amazon/smithy/model/shapes/ModelSerializer.java

@@ -250,8 +250,6 @@ private ObjectNode createStructureAndUnion(Shape shape, Map<String, MemberShape>
             ObjectNode result = createTypedNode(shape);
             result = result.withMember("members", members.entrySet().stream()
                     .map(entry -> Pair.of(entry.getKey(), entry.getValue().accept(this)))
-                    // Sort by key to ensure a consistent member order.
-                    .sorted(Comparator.comparing(Pair::getLeft))
                     .collect(ObjectNode.collectStringKeys(Pair::getLeft, Pair::getRight)));
             return withTraits(shape, result);
         }

smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembersShape.java

@@ -18,27 +18,30 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.TreeMap;
 import java.util.function.Consumer;
 
 /**
  * Abstract classes shared by structure and union shapes.
+ *
+ * <p>The order of members in structures and unions are the same as the
+ * order that they are defined in the model.
  */
 abstract class NamedMembersShape extends Shape {
 
     private final Map<String, MemberShape> members;
+    private volatile List<String> memberNames;
 
     NamedMembersShape(NamedMembersShape.Builder<?, ?> builder) {
         super(builder, false);
         assert builder.members != null;
 
         // Copy the members to make them immutable and ensure that each
         // member has a valid ID that is prefixed with the shape ID.
-        members = Collections.unmodifiableMap(new TreeMap<>(builder.members));
+        members = Collections.unmodifiableMap(new LinkedHashMap<>(builder.members));
 
         members.forEach((key, value) -> {
             ShapeId expected = getId().withMember(key);
@@ -60,12 +63,19 @@ public Map<String, MemberShape> getAllMembers() {
     }
 
     /**
-     * Returns a list of member names.
+     * Returns an ordered list of member names based on the order they are
+     * defined in the model.
      *
-     * @return Returns list of member names.
+     * @return Returns an immutable list of member names.
      */
     public List<String> getMemberNames() {
-        return new ArrayList<>(members.keySet());
+        List<String> names = memberNames;
+        if (names == null) {
+            names = Collections.unmodifiableList(new ArrayList<>(members.keySet()));
+            memberNames = names;
+        }
+
+        return names;
     }
 
     /**
@@ -85,7 +95,13 @@ public Collection<MemberShape> members() {
 
     @Override
     public boolean equals(Object other) {
-        return super.equals(other) && members.equals(((NamedMembersShape) other).members);
+        if (!super.equals(other)) {
+            return false;
+        }
+
+        // Members are ordered, so do a test on the ordering and their values.
+        NamedMembersShape b = (NamedMembersShape) other;
+        return getMemberNames().equals(b.getMemberNames()) && members.equals(b.members);
     }
 
     /**
@@ -95,7 +111,7 @@ public boolean equals(Object other) {
      */
     abstract static class Builder<B extends Builder, S extends NamedMembersShape> extends AbstractShapeBuilder<B, S> {
 
-        Map<String, MemberShape> members = new HashMap<>();
+        Map<String, MemberShape> members = new LinkedHashMap<>();
 
         /**
          * Replaces the members of the builder.

smithy-model/src/test/java/software/amazon/smithy/model/node/ObjectNodeTest.java

@@ -20,6 +20,7 @@
 import static org.hamcrest.CoreMatchers.instanceOf;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -98,6 +99,19 @@ public void containsMember() {
         assertTrue(node.containsMember("bam"));
     }
 
+    @Test
+    public void membersAreOrdered() {
+        ObjectNode node = ObjectNode.objectNodeBuilder()
+                .withMember("foo", "bar")
+                .withMember("baz", true)
+                .withMember("bam", false)
+                .build();
+
+        assertThat(node.getMembers().values(),
+                   contains(node.expectMember("foo"), node.expectMember("baz"), node.expectBooleanMember("bam")));
+        assertThat(node.getStringMap().keySet(), contains("foo", "baz", "bam"));
+    }
+
     @Test
     public void getMemberByType() {
         ObjectNode node = ObjectNode.objectNodeBuilder()