`TraitDiffRule$Severity` type change in 1.46.0

[kubukoz]

Hi!

I was trying to update our version of Smithy from 1.45.0 to 1.47.0 in smithy4s, but bumped into an issue: our generated code for smithy.api breaks binary compatibility against its previous version.

One of such breakages, probably the most obvious one is this:

method severity()smithy.api.TraitChangeSeverity in class smithy.api.TraitDiffRule has a different result type in current version, where it is smithy.api.Severity rather than smithy.api.TraitChangeSeverity

I recognize this may an "us" problem more than Smithy's problem, but is this kind of breakage expected in 1.x? Even smithy-diff considers an enum shape rename an ERROR:

//> using dep software.amazon.smithy:smithy-model:1.47.0
//> using dep software.amazon.smithy:smithy-diff:1.47.0
import software.amazon.smithy.model.Model
import software.amazon.smithy.diff.ModelDiff
import scala.jdk.CollectionConverters._

def parse(s: String) = Model
  .assembler()
  .addUnparsedModel("test.smithy", s)
  .assemble()
  .unwrap()

@main def run = {
  val m1 = parse("""$version: "2"
  |namespace demo
  |
  |@private
  |structure Hello {
  |  greeting: Greeting
  |}
  |
  |@private
  |enum Greeting {
  |  FOO
  |  BAR
  |}
  |""".stripMargin)
  val m2 = parse("""$version: "2"
  |namespace demo
  |
  |@private
  |structure Hello {
  |  greeting: GreetingNew
  |}
  |
  |@private
  |enum GreetingNew {
  |  FOO
  |  BAR
  |}
  |""".stripMargin)

  ModelDiff.builder().oldModel(m1).newModel(m2).compare().getDiffEvents().asScala.foreach(println)
// [NOTE] demo#GreetingNew: Added enum `demo#GreetingNew` | AddedShape test.smithy:9:1
// [ERROR] demo#Hello$greeting: The shape targeted by the member `demo#Hello$greeting` changed from `demo#Greeting` (enum) to `demo#GreetingNew` (enum). The `smithy.api#enum` trait was found on the target, so the name of the targeted shape matters for codegen. | ChangedMemberTarget test.smithy:5:3
}

[mtdowling]

Sorry to hear that impacted you. This was a private shape in the prelude, so it seemed like fair game and not a breaking change to change it given we needed to reuse this shape in other parts of the prelude for less specific purposes. I'm surprised something would codegen private shapes from smithy.api like this. If that's a requirement of smithy4s, we'll be more careful of changes to the prelude in the future.

[kubukoz]

Here's some context:

1. Smithy4s-codegen generates protocol-agnostic code for all shapes it's told to (this is configurable by users on each use-site of smithy4s, by default you get basically everything that wasn't generated already)
2. The generated code has a dependency on types available in smithy4s-core.
3. Smithy4s-core contains a mix of: a. Generated code for the smithy.api namespace, as well as some others (e.g. alloy) b. Base datatypes that represent the original Smithy model in the generated code, as well as functionality to manipulate them, some of which looks at specific types generated in part A
4. Additional runtime functionality, such as creating HTTP servers with http4s, is provided via additional runtime libraries, which are also looking at some standard and non-standard (e.g. alloy) traits. These libraries always depend on smithy4s-core (point 3) because of the core datatypes, and often because of the standard traits generated from smithy.api etc.

Now, I don't think smithy4s-core, or any of the other (runtime, not codegen-time) libraries here, actually care about traits such as @trait and its fields much. We generate and compile these sources only because they're in the smithy.api namespace, and because they're applied to many shapes, so the fact that they end up in smithy4s-core is a bit incidental.

More checks would be appreciated in the future to prevent cases like this... maybe it's worth considering something like a "community build" (inspiration: Scala's community build) where Smithy changes could be tested against wider tooling via snapshots, e.g. on a nightly/bi-weekly basis?

Also, maybe we should just adjust our expectations on private shapes. @Baccata, what do you think? Maybe these should be exempt from code generation? (especially in smithy.api)

[Baccata]

You can close the ticket imho, let's talk about it in another channel, clearly the fact that it's private makes it fair game

[Baccata]

@mtdowling, apologies but I'm compelled to ask @kubukoz re-open the issue, after giving it some further thoughts. I'm happy to open a new issue if you think it'd be preferable.

The concept carried by @private in smithy prevents the direct referencing of a shape from other namespaces. However, it doesn't prevent their indirect referencing. It is the case, for instance, for the smithy.api#Example shape that is used in the smithy.api#examples trait.

If we make the parallel with Java, it'd be akin to having a public class carry an accessor, the return type of which would be private. This would simply not pass compilation.

Now the thing is that code-generating traits can be useful. As a matter of fact, the official smithy tooling is now facilitating this usecase. Smithy4s generates code for all shapes from all models it encounters, trait or not, and for better or worse, this generated code is used in applications (as opposed to tooling).

If we avoided the generation of private trait shapes from the prelude, we'd find ourselves in a conundrum with regards to "what should we do with members of public shapes that target private shapes ?". Omitting them is hardly a solution : in the case of @examples, the smithy.api#Example is the valuable bit of information, despite being tagged as private in the smithy prelude.

So my point is that, the smithy @private doesn't make the renaming of shapes fair-game after all, because if end users can "instantiate" traits and provide values for the private shapes, then users should be in the right to want to query/manipulate those values using generated code, which implies that @private shapes from the prelude should be subject to a similar degree of strict-ness/principle-ness than other shapes when it comes to evolving them.

If that's a requirement of smithy4s, we'll be more careful of changes to the prelude in the future.

Therefore, it'd be greatly appreciated if you folks could indeed be more careful of changes to the prelude in the future.

[mtdowling]

That makes sense. We use protocol and validation traits at runtime too in some code bases. Those code bases don’t generate documentation traits and model specific traits like what’s mentioned in the issue, so it wasn’t an issue there. If something is generating them all, then I could see how it’d be impacted.

I don’t think it’s worth rolling back now that it’s deployed. To prevent an unintended break of smithy4s, we could potentially add a chain build or something via GitHub actions from this repo to smithy4s (contributions here are very welcome).

[Baccata]

Yeah no need to rollback this time around, we'll manage. I'm more interested to avoid a future mis-hap

@kubukoz and I will discuss contributing a "chain build", see what'd be possible.

Thank you for your understanding 👍