Merge pull request #1701 from disneystreaming/fix-1700

Add cache for matching mixins, solve some mixin bugs

Author: kubukoz

CHANGELOG.md

@@ -8,6 +8,7 @@ Thank you!
 # 0.18.34
 
 * codegen-cli: Ensure the command returns a failing exit code if command line arguments aren't valid in [#1694](https://github.com/disneystreaming/smithy4s/pull/1694).
+* codegen: Mixins - fix several bugs and a performance regression in [#1701](https://github.com/disneystreaming/smithy4s/pull/1701).
 
 # 0.18.33
 

modules/bootstrapped/src/generated/smithy4s/example/MixinRequiredMember.scala

@@ -0,0 +1,6 @@
+package smithy4s.example
+
+
+trait MixinRequiredMember {
+  def description: String
+}

modules/bootstrapped/src/generated/smithy4s/example/MixinRequiredMemberDefaultAdded.scala

@@ -0,0 +1,23 @@
+package smithy4s.example
+
+import smithy4s.Hints
+import smithy4s.Schema
+import smithy4s.ShapeId
+import smithy4s.ShapeTag
+import smithy4s.schema.Schema.string
+import smithy4s.schema.Schema.struct
+
+final case class MixinRequiredMemberDefaultAdded(description: String = "different description") extends MixinRequiredMember
+
+object MixinRequiredMemberDefaultAdded extends ShapeTag.Companion[MixinRequiredMemberDefaultAdded] {
+  val id: ShapeId = ShapeId("smithy4s.example", "MixinRequiredMemberDefaultAdded")
+
+  val hints: Hints = Hints.empty
+
+  // constructor using the original order from the spec
+  private def make(description: String): MixinRequiredMemberDefaultAdded = MixinRequiredMemberDefaultAdded(description)
+
+  implicit val schema: Schema[MixinRequiredMemberDefaultAdded] = struct(
+    string.required[MixinRequiredMemberDefaultAdded]("description", _.description).addHints(smithy.api.Default(smithy4s.Document.fromString("different description"))),
+  )(make).withId(id).addHints(hints)
+}

modules/bootstrapped/src/generated/smithy4s/example/MixinRequiredMemberIntermediate.scala

@@ -0,0 +1,7 @@
+package smithy4s.example
+
+
+trait MixinRequiredMemberIntermediate extends MixinRequiredMember {
+  def description: String
+  def extraField: Option[String]
+}

modules/bootstrapped/src/generated/smithy4s/example/StructUsingMixinRequiredMember.scala

@@ -0,0 +1,24 @@
+package smithy4s.example
+
+import smithy4s.Hints
+import smithy4s.Schema
+import smithy4s.ShapeId
+import smithy4s.ShapeTag
+import smithy4s.schema.Schema.string
+import smithy4s.schema.Schema.struct
+
+final case class StructUsingMixinRequiredMember(description: String, extraField: String) extends MixinRequiredMember
+
+object StructUsingMixinRequiredMember extends ShapeTag.Companion[StructUsingMixinRequiredMember] {
+  val id: ShapeId = ShapeId("smithy4s.example", "StructUsingMixinRequiredMember")
+
+  val hints: Hints = Hints.empty
+
+  // constructor using the original order from the spec
+  private def make(description: String, extraField: String): StructUsingMixinRequiredMember = StructUsingMixinRequiredMember(description, extraField)
+
+  implicit val schema: Schema[StructUsingMixinRequiredMember] = struct(
+    string.required[StructUsingMixinRequiredMember]("description", _.description),
+    string.required[StructUsingMixinRequiredMember]("extraField", _.extraField),
+  )(make).withId(id).addHints(hints)
+}

modules/codegen/src/smithy4s/codegen/internals/SmithyToIR.scala

@@ -53,6 +53,7 @@ import java.time.temporal.ChronoField
 import java.time.format.DateTimeFormatterBuilder
 import scala.util.Try
 import java.util.UUID
+import java.util.concurrent.ConcurrentHashMap
 
 private[codegen] object SmithyToIR {
 
@@ -82,7 +83,10 @@ private[codegen] class SmithyToIR(
     namespace: String
 ) {
 
-  val finder = PathFinder.create(model)
+  private val finder = PathFinder.create(model)
+
+  // Contains mixins of the given shape that have matching fields.
+  private val mixinsOfCache = new ConcurrentHashMap[ShapeId, Set[ShapeId]]()
 
   val allShapes =
     model
@@ -213,7 +217,7 @@ private[codegen] class SmithyToIR(
           fields
             .find(_.name == memberName)
             .forall { field =>
-              field.modifier == fieldModifier(member)
+              field.modifier.typeMod == fieldModifier(member).typeMod
             }
         }
       }
@@ -227,10 +231,7 @@ private[codegen] class SmithyToIR(
         val rec = isRecursive(shape.getId()) || isTrait
 
         val fields = shape.fields
-        val filteredMixins = shape
-          .getMixins()
-          .asScala
-          .filter(mixinId => doFieldsMatch(mixinId, fields))
+        val filteredMixins = getMixinsMatchingFields(shape)
         val mixins = filterMixinsExistOnParentAdt(filteredMixins.toSet, shape)
           .flatMap(_.tpe)
           .toList
@@ -263,22 +264,30 @@ private[codegen] class SmithyToIR(
           .map(mem => model.expectShape(mem.getTarget))
 
         val mixins: List[Set[ShapeId]] = memberTargets
-          .map { targetShape =>
-            def allMixinsOf(s: Shape): Set[ShapeId] =
-              s.getMixins.asScala.toSet[ShapeId].flatMap { m =>
-                allMixinsOf(model.expectShape(m)) + m
-              }
-
-            allMixinsOf(targetShape)
-              .filter(mixinId => doFieldsMatch(mixinId, targetShape.fields))
-          }
+          .map(getMixinsMatchingFields)
 
         val result =
           if (mixins.isEmpty) Set.empty else mixins.reduce(_ intersect _)
 
         result.toList
       }
 
+      private def getMixinsMatchingFields(shape: Shape): Set[ShapeId] = {
+        def allMixinsOf(s: Shape): Set[ShapeId] =
+          s.getMixins.asScala.toSet[ShapeId].flatMap { m =>
+            allMixinsOf(model.expectShape(m)) + m
+          }
+
+        mixinsOfCache
+          .computeIfAbsent(
+            shape.getId,
+            _ =>
+              allMixinsOf(shape)
+                // This filter is the more intensive part worth caching
+                .filter(doFieldsMatch(_, shape.fields))
+          )
+      }
+
       // Filters out any mixins which exist on the parent ADT (if it is part of an ADT)
       // This is so the case classes in the ADT won't also extend the same mixins
       // as the parent sealed trait. This leads to cleaner generated code (no redundancy).

sampleSpecs/mixins.smithy

@@ -54,3 +54,24 @@ structure MixinOptionalMemberOverride with [MixinOptionalMember] {
 structure MixinOptionalMemberDefaultAdded with [MixinOptionalMember] {
   a: String = "test"
 }
+
+@mixin
+structure MixinRequiredMember {
+  @required description: String
+}
+
+@mixin
+structure MixinRequiredMemberIntermediate with [MixinRequiredMember] {
+  extraField: String
+}
+
+// regression test for https://github.com/disneystreaming/smithy4s/issues/1702
+structure StructUsingMixinRequiredMember with [MixinRequiredMemberIntermediate] {
+    @required
+    $extraField
+}
+
+// regression test for https://github.com/disneystreaming/smithy4s/issues/1699
+structure MixinRequiredMemberDefaultAdded with [MixinRequiredMember] {
+  $description = "different description"
+}