Migrate to EnumShape #1932

82marbag · 2022-11-01T16:53:59Z

Deprecate EnumTrait and migrate to EnumShape.

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

github-actions · 2022-11-01T17:08:58Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2022-11-01T17:21:40Z

A new generated diff is ready to view.

A new doc preview is ready to view.

jdisanti

This looks good! Just a couple minor comments.

jdisanti · 2022-11-01T17:51:12Z

.../src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/EnumGenerator.kt

-        writer.rustBlock("impl <T> #T<T> for $enumName where T: #T<str>", RuntimeType.From, RuntimeType.AsRef) {
-            rustBlock("fn from(s: T) -> Self") {
-                rust("$enumName(s.as_ref().to_owned())")
+        // TODO(https://github.com/awslabs/smithy-rs/issues/1700): handle IntEnum


Does it generate invalid code or throw an exception when encountering an integer enum? If not, we should make it do so for now.

Changed so that it throws an exception both here and when the conversion fails

...n-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/CodegenVisitor.kt

jdisanti · 2022-11-02T16:57:57Z

Projection s3 failed: software.amazon.smithy.codegen.core.CodegenException: Unnamed @enum shapes are unsupported.

Heh. We may need to upgrade the SDK models for this to pass. The current S3 model is IDLv1.

Unfortunately, we can't upgrade the models until our endpoints 2.0 implementation lands though.

82marbag · 2022-11-03T09:28:11Z

Projection s3 failed: software.amazon.smithy.codegen.core.CodegenException: Unnamed @enum shapes are unsupported.

Heh. We may need to upgrade the SDK models for this to pass. The current S3 model is IDLv1.

Unfortunately, we can't upgrade the models until our endpoints 2.0 implementation lands though.

It fails on EC2 and S3. For EC2 v2:

[ERROR] com.amazonaws.ec2#AmazonEC2: Unable to resolve trait smithy.rules#endpointTests. If this is a custom trait, then it must be defined before it can be used in a model.

For S3 v2:

[ERROR] com.amazonaws.s3#DeleteBucketWebsiteRequest$Bucket: Unable to resolve trait smithy.rules#contextParam. If this is a custom trait, then it must be defined before it can be used in a model.

@enums work. We can either remove this check until v2 or I'm ok waiting for the model upgrades (there might be some merges / refactoring to be done by then)

82marbag · 2022-11-08T09:29:46Z

This PR is waiting for the migration to v2 models; there are missing features on the client before this migration can be done. When test models are in v2, this PR can be merged. (agreed with @jdisanti )

jdisanti · 2022-11-09T21:51:06Z

I can update the models once #1972 is merged.

jdisanti · 2022-11-10T23:25:48Z

The models are updated. This should be unblocked now.

Signed-off-by: Daniele Ahmed <[email protected]>

david-perez

Looks good! Just a few comments.

Can you also rename renderBuildEnforcingRequiredAndEnumTraitsFn from ServerBuilderGeneratorWithoutPublicConstrainedTypes.kt, to, perhaps, renderBuildEnforcingRequiredTraitAndEnumValuesFn?

david-perez · 2022-11-25T10:14:35Z

...n-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/CodegenVisitor.kt

-                EnumGenerator(model, symbolProvider, this, shape, enum).render()
-            }
+        if (shape.hasTrait<EnumTrait>()) {
+            throw CodegenException("Unnamed @enum shapes are unsupported: $shape")


Is this error message accurate? My understanding from looking at this is that the model transformer might fail to convert an unnamed enum if e.g. one of its values cannot be safely synthesized into a valid name. So it's not like we don't support unnamed @enum shapes entirely, just the ones that we failed to convert when synthesizeEnumNames is set, right?. Aside: is that flag enabled by default?

If the above is correct, I'd fail here with a more informative error message, perhaps instructing the user to look in the log output above for the INFO messages that the transformer from awslabs/smithy printed indicating the exact reason why the enum could not be converted.

Correct, I've changed to a better log message, thanks!

...ient/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/CodegenVisitorTest.kt

david-perez · 2022-11-25T10:17:48Z

...ient/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/CodegenVisitorTest.kt

@@ -62,4 +63,42 @@ class CodegenVisitorTest {
        val baselineModel = visitor.baselineTransform(model)
        baselineModel.getShapesWithTrait(ShapeId.from("smithy.api#mixin")).isEmpty() shouldBe true
    }
+
+    @Test
+    fun `baseline transform verify string enum converted to EnumShape`() {


nit: I'd add a negative test too that asserts we throw an exception when an unnamed enum could not be converted to an enum shape.

.../src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/EnumGenerator.kt

...e/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/Instantiator.kt

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/TestHelpers.kt

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/Constraints.kt

...er/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCodegenVisitor.kt

Signed-off-by: Daniele Ahmed <[email protected]>

david-perez · 2022-12-16T16:59:33Z

...re/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/SymbolMetadataProvider.kt

-            is StringShape -> if (shape.hasTrait<EnumTrait>()) {
-                enumMeta(shape)
-            } else null
+            is EnumShape -> enumMeta(shape)


We can now make enumMeta accept EnumShape instead of StringShape.

jmklix · 2024-04-12T18:34:25Z

we've decided to begin closing old PRs that we don't plan to work on in the near future. Please let us know if you still think this PR is needed

82marbag requested review from a team as code owners November 1, 2022 16:54

82marbag added needs-sdk-review needs-server-review labels Nov 1, 2022

82marbag force-pushed the 1671 branch from 1df22af to 1e3f49b Compare November 1, 2022 17:09

jdisanti approved these changes Nov 1, 2022

View reviewed changes

jdisanti removed the needs-sdk-review label Nov 1, 2022

jdisanti mentioned this pull request Nov 10, 2022

Add smithy-rules-engine dependency to add endpoint traits #1976

Merged

1 task

82marbag added 3 commits November 18, 2022 16:37

ModelTransformer for EnumShape

8e10f8e

Signed-off-by: Daniele Ahmed <[email protected]>

Update EnumGenerator

5d09638

Signed-off-by: Daniele Ahmed <[email protected]>

Remove EnumTrait

bd524e8

Signed-off-by: Daniele Ahmed <[email protected]>

82marbag force-pushed the 1671 branch from 0a7f8a6 to bd524e8 Compare November 18, 2022 19:26

Fix tests

3df6ad2

Signed-off-by: Daniele Ahmed <[email protected]>

david-perez approved these changes Nov 25, 2022

View reviewed changes

Address comments

077d44d

Signed-off-by: Daniele Ahmed <[email protected]>

82marbag requested a review from a team as a code owner December 1, 2022 14:49

Merge branch 'main' into 1671

e2c0d14

82marbag mentioned this pull request Dec 2, 2022

Supress warnings for deprecated shapes #2047

Closed

82marbag mentioned this pull request Dec 16, 2022

Compile regexes for @pattern strings early #2058

Merged

david-perez reviewed Dec 16, 2022

View reviewed changes

jdisanti added server Rust server SDK client labels Dec 20, 2022

spinski added the blocked label Dec 22, 2022

david-perez mentioned this pull request Apr 21, 2023

Questions on @internal, @tags, and enum values smithy-lang/smithy#1737

Closed

82marbag removed the blocked label May 11, 2023

rcoh requested review from a team as code owners November 14, 2023 02:21

jmklix closed this Apr 12, 2024

Velfi removed the needs-server-review label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to EnumShape #1932

Migrate to EnumShape #1932

82marbag commented Nov 1, 2022 •

edited

Loading

github-actions bot commented Nov 1, 2022

github-actions bot commented Nov 1, 2022

jdisanti left a comment

jdisanti Nov 1, 2022

82marbag Nov 2, 2022

jdisanti commented Nov 2, 2022

82marbag commented Nov 3, 2022

82marbag commented Nov 8, 2022

jdisanti commented Nov 9, 2022

jdisanti commented Nov 10, 2022

david-perez left a comment

david-perez Nov 25, 2022

82marbag Dec 1, 2022

david-perez Nov 25, 2022

david-perez Dec 16, 2022

jmklix commented Apr 12, 2024

Migrate to EnumShape #1932

Migrate to EnumShape #1932

Conversation

82marbag commented Nov 1, 2022 • edited Loading

Checklist

github-actions bot commented Nov 1, 2022

github-actions bot commented Nov 1, 2022

jdisanti left a comment

Choose a reason for hiding this comment

jdisanti Nov 1, 2022

Choose a reason for hiding this comment

82marbag Nov 2, 2022

Choose a reason for hiding this comment

jdisanti commented Nov 2, 2022

82marbag commented Nov 3, 2022

82marbag commented Nov 8, 2022

jdisanti commented Nov 9, 2022

jdisanti commented Nov 10, 2022

david-perez left a comment

Choose a reason for hiding this comment

david-perez Nov 25, 2022

Choose a reason for hiding this comment

82marbag Dec 1, 2022

Choose a reason for hiding this comment

david-perez Nov 25, 2022

Choose a reason for hiding this comment

david-perez Dec 16, 2022

Choose a reason for hiding this comment

jmklix commented Apr 12, 2024

82marbag commented Nov 1, 2022 •

edited

Loading