Improve mixin documentation + possibly mixin bug

[mullermp]

There are a few more improvements that can be made to the mixin documentation.

1. https://smithy.io/2.0/spec/mixins.html#mixin-members-must-not-conflict

This states Members that are mixed into shapes MAY be redefined if and only if each redefined member targets the same shape. Traits applied to redefined members supersede any traits inherited from mixins. however the example below does not demonstrate that redefined members supersede any traits. The example would be stronger if it used a trait with a property that gets overridden.

2. This is a possible bug with the example from 1). If you run smithy ast on the example above, it produces a json that has an apply shape for Valid$a to apply the required trait, but the A2 structure already has the required trait applied to the member shape already.

3. Mixins can also be applied to maps, lists, and unions. Are there any other aggregate cases? It would be great to include examples of these. In my generator, I used the examples from this page to test that mixins get resolved by our model traversal logic. It did not handle these cases. A minimal example for Map might be:

@mixin
map MyMixin {
    /// Generic docs
    key: String
    /// Generic docs
    value: String
}

map MyMap with [MyMixin] {}
apply MyMap$key @documentation("Specific docs")
apply MyMap$value @documentation("Specific docs")

[JordonPhillips]

Using the following Smithy definition in a file called main.smithy:

$version: "2.0"

namespace com.example

@mixin
map MyMixin {
    /// Generic docs
    key: String
    /// Generic docs
    value: String
}

map MyMap with [MyMixin] {}
apply MyMap$key @documentation("Specific docs")
apply MyMap$value @documentation("Specific docs")

Running smithy ast main.smithy produces:

{
    "smithy": "2.0",
    "shapes": {
        "com.example#MyMap": {
            "type": "map",
            "mixins": [
                {
                    "target": "com.example#MyMixin"
                }
            ]
        },
        "com.example#MyMap$key": {
            "type": "apply",
            "traits": {
                "smithy.api#documentation": "Specific docs"
            }
        },
        "com.example#MyMap$value": {
            "type": "apply",
            "traits": {
                "smithy.api#documentation": "Specific docs"
            }
        },
        "com.example#MyMixin": {
            "type": "map",
            "key": {
                "target": "smithy.api#String",
                "traits": {
                    "smithy.api#documentation": "Generic docs"
                }
            },
            "value": {
                "target": "smithy.api#String",
                "traits": {
                    "smithy.api#documentation": "Generic docs"
                }
            },
            "traits": {
                "smithy.api#mixin": {}
            }
        }
    }
}

Running smithy ast --flatten main.smithy produces:

{
    "smithy": "2.0",
    "shapes": {
        "com.example#MyMap": {
            "type": "map",
            "key": {
                "target": "smithy.api#String",
                "traits": {
                    "smithy.api#documentation": "Specific docs"
                }
            },
            "value": {
                "target": "smithy.api#String",
                "traits": {
                    "smithy.api#documentation": "Specific docs"
                }
            }
        }
    }
}

These are the outputs I would expect and do not match with what you're reporting. What version are you using (smithy --version)? If it's not 1.54.0 or newer, try upgrading to see if that fixes your issue.

[mullermp]

I think there is a misunderstanding. The bug report is for this example:

@mixin
structure A1 {
    @private
    a: String
}

@mixin
structure A2 {
    @required
    a: String
}

structure Valid with [A1, A2] {}

It produces a json that has an apply shape for Valid$a to apply the required trait, but the A2 structure already has the required trait applied to the member shape already.

The description for the example says "Traits applied to redefined members supersede any traits inherited from mixins." but that is not really demonstrated.

[mullermp]

Map, list, and union examples should be added to the docs as an enhancement.

[JordonPhillips]

Mixins can also be applied to maps, lists, and unions. Are there any other aggregate cases? It would be great to include examples of these. In my generator, I used the examples from this page to test that mixins get resolved by our model traversal logic.

Mixins can be used with any shape, and the rules for how they work are the same across each of them for traits and members. Non-member properties are called out later on that page since they're not quite so straight-forward.

If you're writing your own AST logic that consumes models from smithy ast, I would highly recommend just adding --flatten so you don't have to worry about that. This can also be achieved through the flattenAndRemoveMixins transform if you're getting the model via smithy-build.json

The bug report is for this example [...]

Ah, I see, sorry for the mixup! That particular example is intended to showcase that the target shape is the same, and thus new traits may be applied.

Apply statements are used any time a trait is applied to an existing member, whether that's via another mixin or via a local definition. Apply statements are used here instead of redefining members on structures because apply statements are more resilient to change over time if the shapes targeted by an inherited member changes.

[mullermp]

I'm sorry, I still don't understand what you're saying.

Why is there both an apply shape for Valid$a to apply the required trait, and also the required trait on the A2 structure? I would assume one or the other but not both.

If this is to demonstrate the precedence, it's not clear either.

[JordonPhillips]

Why is there both an apply shape for Valid$a to apply the required trait, and also the required trait on the A2 structure? I would assume one or the other but not both.

the tl;dr that this is to solve a different problem, we can't tell the difference once we get to writing out the ast, and since behaviorally there's no difference there's no need to.

Apply statements are added for any introduced traits, however or wherever they were introduced. Think of this as the equivalent of the $ target ellision syntax that the IDL has. Imagine you have two Smithy packages, foo and bar:

// foo.smithy
$version: "2.0"

namespace example.foo

@mixin
structure Foo {
    spam: String
}

// bar.smithy
$version: "2.0"

namespace example.bar

use example.foo#Foo

structure Bar with [Foo] {
    @required
    $spam
}

Now imagine that on some day the target of Foo$spam is changed to Integer. This is a breaking change at the API level, but not at the IDL level because $spam doesn't declare a target. smithy-diff will complain about the backwards-incompatible change, but the model won't fail while loading. This is the layer we want the error to be at because it gives the most useful information and context.

That syntax is functionally the same thing as using the apply statement with every trait locally added.

Now back to the specific example you've cited. It's different in that the introduced trait was introduced from another mixin rather than locally, so why is that apply statement there? The answer is that it's a minor performance tradeoff. If we had to check this case, we'd have to check it for every mixin. But if we don't, then we only have extra checks in the specific case where two non-hierarchical mixins define the same member, which is ultimately not that common. If there were behavioral differences, this would be a problem we'd of course have to fix. But there isn't a distinction, there's just an effective noop apply during model loading.

If this is to demonstrate the precedence

This is not to demonstrate precedence, there's an example for that earlier on in the page.

[mullermp]

That makes sense. It sounds like there's just a documentation improvement here. Do as you will.