Inconsistent behaviour between getShapesWithTrait(T.class) and getShapesWithTrait(id) for "dynamic" trait values.

[Baccata]

If a model is constructed programmatically using dynamic trait values (ie shape ids + nodes), it is not possible to query the value using the class-parameterised variant of getShapesWithTrait. This prevents a bunch of tools from working properly.

I think it's somewhat understandable for this behaviour to happen on a model that comes directly from Model.builder(), but after taking this model through an assembler, my expectation would be that any "dynamic" trait value would be taken through the trait factory, which doesn't appear to be the case, and querying the model using the class instance of traits doesn't work on those values.

Here's a reproduction of the problem in Scala (it can be run with scala-cli)

//> using dep "software.amazon.smithy:smithy-model:1.47.0"

import software.amazon.smithy.model.Model
import software.amazon.smithy.model.shapes.StructureShape
import software.amazon.smithy.model.traits.Trait
import software.amazon.smithy.model.node.Node
import software.amazon.smithy.model.shapes.ShapeId
import software.amazon.smithy.model.traits.ErrorTrait

@main
def run() = {
  val structure = StructureShape
    .builder()
    .id("foo#Foo")
    .addTrait(new Trait {
      def toShapeId(): ShapeId = ErrorTrait.ID
      def toNode(): Node = Node.from("client")
    })
    .build()

  val unvalidatedModel = Model.builder().addShape(structure).build()
  val model = Model.assembler().addModel(unvalidatedModel).assemble().unwrap()

  // prints [(structure: `foo#Foo`)]
  println(unvalidatedModel.getShapesWithTrait(ErrorTrait.ID))
  // prints []
  println(unvalidatedModel.getShapesWithTrait(classOf[ErrorTrait]))

  // prints [(structure: `foo#Foo`)]
  println(model.getShapesWithTrait(ErrorTrait.ID))
  // prints []
  println(model.getShapesWithTrait(classOf[ErrorTrait]))

}

[mtdowling]

We should better document the current behavior, but it essentially uses built shapes and models as is. Models are built and rebuilt a lot, so being able to use existing shapes as-is results is much faster transformations. It also allows an assembler to take in an existing model, shapes can refer to shapes from that model, but the assembler doesn’t neee to have any responsibility to build traits. To change this now would likely be a breaking change. We could potentially add a flag to rebuild built shapes or models though, as an opt-in.

[Baccata]

From an optimisation perspective, it does make sense. The addition of a flag would be appreciated. Or maybe a bespoke subclass of trait that the assembler would have special understanding of, as something it'd need to rebuild ?

For the record : right now, I'm using the workaround of serialising the model to Node and loading it back via the assembler.