Address PR feedback on mixins

Author: mtdowling

designs/mixins.md

@@ -530,16 +530,15 @@ structure Invalid with
 ### Mixins in the IDL
 
 To support mixins, `structure_statement` and `union_statement` ABNF rules
-will be updated to contain an optional series of `add_mixin` productions
-that comes after the shape name and before `{`. Each `with` in the
-`add_mixin` MUST be followed by a single shape ID, and each shape MUST
-target a valid shape marked with the `@mixin` trait. Any number of mixins can
-be added to a shape.
+will be updated to contain an optional `mixins` production
+that comes after the shape name and before `{`. Each shape ID referenced in
+the `mixins` production MUST target a shape of the same type as the
+shape being defined and MUST be marked with the `@mixin` trait.
 
 ```
-structure_statement = "structure" ws `identifier` ws *add_mixin structure_members
-union_statement = "union" ws `identifier` ws *add_mixin union_members
-add_mixin = "with" `ws` `shape_id`
+structure_statement = "structure" ws identifier ws [mixins ws] structure_members
+union_statement = "union" ws identifier ws [mixins ws] union_members
+mixins = "with" 1*(ws shape_id)
 ```
 
 

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

@@ -49,7 +49,7 @@ abstract class AbstractMutableModelFile implements ModelFile {
     private final Set<ShapeId> allShapeIds = new HashSet<>();
     private final Map<ShapeId, AbstractShapeBuilder<?, ?>> shapes = new LinkedHashMap<>();
     private final Map<ShapeId, Map<String, MemberShape.Builder>> members = new HashMap<>();
-    private final Map<ShapeId, Set<ShapeId>> shapesPendings = new HashMap<>();
+    private final Map<ShapeId, Set<ShapeId>> pendingShapes = new HashMap<>();
     private final List<ValidationEvent> events = new ArrayList<>();
     private final MetadataContainer metadata = new MetadataContainer(events);
     private final TraitFactory traitFactory;
@@ -87,7 +87,7 @@ void onShape(AbstractShapeBuilder<?, ?> builder) {
     }
 
     void addPendingMixin(ShapeId shape, ShapeId mixin) {
-        shapesPendings.computeIfAbsent(shape, id -> new LinkedHashSet<>()).add(mixin);
+        pendingShapes.computeIfAbsent(shape, id -> new LinkedHashSet<>()).add(mixin);
     }
 
     private SourceException onConflict(AbstractShapeBuilder<?, ?> builder, AbstractShapeBuilder<?, ?> previous) {
@@ -153,7 +153,7 @@ public final CreatedShapes createShapes(TraitContainer resolvedTraits) {
         List<Shape> resolvedShapes = new ArrayList<>(shapes.size());
         List<PendingShape> pendingMixins = new ArrayList<>();
 
-        for (Map.Entry<ShapeId, Set<ShapeId>> entry : shapesPendings.entrySet()) {
+        for (Map.Entry<ShapeId, Set<ShapeId>> entry : pendingShapes.entrySet()) {
             ShapeId subject = entry.getKey();
             Set<ShapeId> mixins = entry.getValue();
             AbstractShapeBuilder<?, ?> builder = shapes.get(entry.getKey());
@@ -205,9 +205,9 @@ private PendingShape createPendingShape(
             // Add each mixin and ensure there are no member conflicts.
             for (ShapeId mixin : mixins) {
                 Shape mixinShape = shapeMap.get(mixin);
-                // Members cannot be redefined.
                 for (MemberShape member : mixinShape.members()) {
                     if (builderMembers.containsKey(member.getMemberName())) {
+                        // Members cannot be redefined.
                         MemberShape.Builder conflict = builderMembers.get(member.getMemberName());
                         events.add(ValidationEvent.builder()
                                 .severity(Severity.ERROR)

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

@@ -276,6 +276,15 @@ public B clearMixins() {
         return (B) this;
     }
 
+    /**
+     * Removes mixins from a shape and flattens them into the shape.
+     *
+     * <p>Flattening a mixin into a shape copies the traits and members of a
+     * mixin onto the shape, effectively resulting in the same shape but with
+     * no trace of the mixin relationship.
+     *
+     * @return Returns the updated builder.
+     */
     @SuppressWarnings("unchecked")
     public B flattenMixins() {
         if (mixins != null) {

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

@@ -70,7 +70,7 @@ private enum TraitFeature {
         OMIT_REQUIRED,
 
         /** Inline documentation traits with other traits as opposed to using /// syntax. */
-        INLINE_DOCUMENTATION;
+        NO_SPECIAL_DOCS_SYNTAX;
 
         /**
          * Checks if the current enum value is present in an array of enum values.
@@ -412,12 +412,12 @@ private void shapeWithMembers(Shape shape, List<MemberShape> members, boolean st
                         // Use short form for a single trait, and block form for multiple traits.
                         if (member.getIntroducedTraits().size() == 1) {
                             codeWriter.writeInline("apply $I ", member.getId());
-                            serializeTraits(member.getIntroducedTraits(), TraitFeature.INLINE_DOCUMENTATION);
+                            serializeTraits(member.getIntroducedTraits(), TraitFeature.NO_SPECIAL_DOCS_SYNTAX);
                             codeWriter.write("");
                         } else {
                             codeWriter.openBlock("apply $I {", "}", member.getId(), () -> {
                                 // Only serialize local traits, and don't use special documentation syntax here.
-                                serializeTraits(member.getIntroducedTraits(), TraitFeature.INLINE_DOCUMENTATION);
+                                serializeTraits(member.getIntroducedTraits(), TraitFeature.NO_SPECIAL_DOCS_SYNTAX);
                             }).write("");
                         }
                     }
@@ -430,19 +430,19 @@ private void serializeTraits(Shape shape) {
         }
 
         private void serializeTraits(Map<ShapeId, Trait> traits, TraitFeature... traitFeatures) {
-            boolean inlineDocumentation = TraitFeature.INLINE_DOCUMENTATION.hasFeature(traitFeatures);
+            boolean noSpecialDocsSyntax = TraitFeature.NO_SPECIAL_DOCS_SYNTAX.hasFeature(traitFeatures);
             boolean omitRequired = TraitFeature.OMIT_REQUIRED.hasFeature(traitFeatures);
 
             // The documentation trait always needs to be serialized first since it uses special syntax.
-            if (!inlineDocumentation && traits.containsKey(DocumentationTrait.ID)) {
+            if (!noSpecialDocsSyntax && traits.containsKey(DocumentationTrait.ID)) {
                 Trait documentation = traits.get(DocumentationTrait.ID);
                 if (traitFilter.test(documentation)) {
                     serializeDocumentation(documentation.toNode().expectStringNode().getValue());
                 }
             }
 
             traits.values().stream()
-                    .filter(trait -> inlineDocumentation || !(trait instanceof DocumentationTrait))
+                    .filter(trait -> noSpecialDocsSyntax || !(trait instanceof DocumentationTrait))
                     .filter(trait -> !(omitRequired && trait.toShapeId().equals(RequiredTrait.ID)))
                     .filter(traitFilter)
                     .sorted(Comparator.comparing(Trait::toShapeId))

smithy-model/src/main/java/software/amazon/smithy/model/traits/MixinTrait.java

@@ -93,7 +93,7 @@ public static Map<ShapeId, Trait> getNonLocalTraitsFromMap(Map<ShapeId, Trait> t
 
         Map<ShapeId, Trait> filtered = new HashMap<>(traits);
 
-        // Technically the trait could be a dynamic trait is some wacky,
+        // Technically the trait could be a dynamic trait in some wacky,
         // hand-made model that isn't sent through the assembler. That is
         // so beyond unlikely, that a hard cast here works fine.
         MixinTrait mixinTrait = (MixinTrait) traits.get(MixinTrait.ID);

smithy-model/src/main/java/software/amazon/smithy/model/transform/FlattenAndRemoveMixins.java

@@ -25,7 +25,7 @@
 import software.amazon.smithy.model.traits.MixinTrait;
 
 /**
- * Flattens out mixins out of the model.
+ * Flattens mixins out of the model.
  */
 final class FlattenAndRemoveMixins {
     Model transform(ModelTransformer transformer, Model model) {

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

@@ -32,7 +32,7 @@
 import software.amazon.smithy.utils.SmithyInternalApi;
 
 /**
- * Ensures that mixins do no introduce conflicting members across
+ * Ensures that mixins do not introduce conflicting members across
  * mixin closures.
  *
  * <p>The kinds of errors detected by this validator are not actually