Apparent contradiction between aws-smithy-types and generated code in aws-sdk-ec2

[jeffparsons]

trafficstars

Describe the issue

Hello! I was reading through some of the generated code in the aws-sdk-ec2 crate and stumbled upon this:

aws-smithy-types has this enum:

pub enum Number {
    /// Unsigned 64-bit integer value.
    PosInt(u64),
    /// Signed 64-bit integer value. The wrapped value is _always_ negative.
    NegInt(i64),
    /// 64-bit floating-point value.
    Float(f64),
}

Note the remark that NegInt is supposed to always be negative (and that there is no variant for a signed integer that may be either positive or negative).

However, in the generated code in aws-sdk-ec2, in src/protocol_serde/shape_accelerator_count_request.rs (the first serializer) appears to pass in values that are unlikely to be negative.

So... what's going on here? E.g.

• NegInt is actually routinely being used for all signed integers in practice, and the name or at least docs should be updated?
• The generated code is bad?
• I've misunderstood something?

Links

N/A

[Velfi]

This is a case of unfortunate naming.

In the EC2 smithy model, AcceleratorCountRequest is defined like this:

        "com.amazonaws.ec2#AcceleratorCountRequest": {
            "type": "structure",
            "members": {
                "Min": {
                    "target": "com.amazonaws.ec2#Integer",
                    "traits": {
                        "smithy.api#documentation": "<p>The minimum number of accelerators. To specify no minimum limit, omit this\n         parameter.</p>"
                    }
                },
                "Max": {
                    "target": "com.amazonaws.ec2#Integer",
                    "traits": {
                        "smithy.api#documentation": "<p>The maximum number of accelerators. To specify no maximum limit, omit this\n         parameter. To exclude accelerator-enabled instance types, set <code>Max</code> to\n         <code>0</code>.</p>"
                    }
                }
            },
            "traits": {
                "smithy.api#documentation": "<p>The minimum and maximum number of accelerators (GPUs, FPGAs, or Amazon Web Services Inferentia chips)\n         on an instance. To exclude accelerator-enabled instance types, set <code>Max</code> to\n            <code>0</code>.</p>"
            }
        },

Min and Max both target com.amazonaws.ec2#Integer, which is defined thusly:

"com.amazonaws.ec2#Integer": {
    "type": "integer"
},

Which is just a smithy integer:

An integer is a 32-bit signed integer ranging from -2^31 to (2^31)-1 (inclusive).

We don't use PosInt anywhere in codegen afaict. Why? IDK; It's probably just an oversight. One thing I can think of though, because smithy only defines signed integer types, we generate all ints as signed to preserve forwards compatibility. That is to say, We use signed integers everywhere because services may decide to start sending negative values.

Will they? 🤷‍♀️ Still, we must be prepare for that possibility. Does that clear things up for you?

[jdisanti]

We don't use PosInt anywhere in codegen afaict. Why? IDK; It's probably just an oversight. One thing I can think of though, because smithy only defines signed integer types, we generate all ints as signed to preserve forwards compatibility. That is to say, We use signed integers everywhere because services may decide to start sending negative values.

I think it was for two reasons:

1. We wanted the flexibility to add unsigned support in the future, and
2. We could do special casing to support deserializing numbers larger than i64::MAX in the future

Code written against the Number type should handle both cases though, even if PosInt isn't currently used right now.

[jdisanti]

Leaving this open to fix the docs.

[jeffparsons]

Summarising to make sure I've understood correctly:

• NegInt should have been called SignedInt or something like that, because that's what it actually represents in the underlying Smithy model. However, the name can't be fixed now because it's exposed in a public API.
• The doc comment can and should be updated to explain what it's actually for.
• PosInt is unused, but that's no big deal; it just needs to be taken into account in generic machinery, and users in general aren't likely to care too much about its existence.

Does that sound about right?

[jdisanti]

You got it!