Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ModelTransformer.mapShapes ignores location changes #2508

Open
kubukoz opened this issue Feb 6, 2025 · 2 comments
Open

ModelTransformer.mapShapes ignores location changes #2508

kubukoz opened this issue Feb 6, 2025 · 2 comments

Comments

@kubukoz
Copy link
Contributor

kubukoz commented Feb 6, 2025

When you use mapShapes to update your shapes' sourceLocation, it appears to be ignored.

Demo (scala-cli snippet):

//> using scala 3.6.3
//> using dep software.amazon.smithy:smithy-model:1.54.0
import software.amazon.smithy.model.Model
import software.amazon.smithy.model.transform.ModelTransformer
import software.amazon.smithy.model.shapes.Shape
import software.amazon.smithy.model.shapes.AbstractShapeBuilder
import software.amazon.smithy.model.shapes.StructureShape

def transform(s: Shape): Shape = {
  val sb: AbstractShapeBuilder[?, ?] = Shape
    .shapeToBuilder(s)

  sb
    .source("foo", 1, 2)
    .build()
}

val struct = StructureShape
  .builder()
  .id("foo#bar")
  .build()
// struct: StructureShape = (structure: `foo#bar`)

transform(struct).getSourceLocation()
// res0: SourceLocation = foo [1, 2]

val baseModel = Model.builder().addShape(struct).build()
// baseModel: Model = software.amazon.smithy.model.Model@5eab45b

// doesn't get updated with mapShapes
ModelTransformer
  .create()
  .mapShapes(
    baseModel,
    transform(_),
  )
  .expectShape(struct.getId())
  .getSourceLocation()
// res1: SourceLocation = N/A [0, 0]

// but it works when you create a model from scratch
Model
  .builder()
  .addShapes(
    baseModel.shapes().map(transform).toList()
  )
  .build()
  .expectShape(struct.getId())
  .getSourceLocation()
// res2: SourceLocation = foo [1, 2]
@mtdowling
Copy link
Member

I don't have a solution, but I'm pretty sure this is caused by the fact that shape equality is not impacted by SourceLocation. So when mapShapes checks the return value, it sees the shape hasn't changed, and doesn't update it. This is the right behavior shape equality, but we might need something to account for this in the transform specifically.

@yefrig
Copy link
Contributor

yefrig commented Feb 10, 2025

It is unlikely we will change the behaviour of the ModelTransformer. However, a workaround is for you to map over all the shapes that need their source location to be updated and then update them directly in your existing model as such:

existingModel.toBuilder().addShapes(<>).build();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants