Skip to content

Add EnumSection to allow decorators to modify enum member attributes #4039

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Dorenavant
Copy link

@Dorenavant Dorenavant commented Feb 28, 2025

Motivation and Context

Allows users to use decorators to add additional attributes to members of an enum.

N/A

Description

Adds EnumSection with named AdditionalMemberAttributes to allow decorators to modify enum codegen.

Testing

Could not find existing unit tests for customizations; please advise if you would like them to be added here. Ran ./gradlew to ensure current tests pass.

Checklist

  • For changes to the smithy-rs codegen or runtime crates, I have created a changelog entry Markdown file in the .changelog directory, specifying "client," "server," or both in the applies_to key.

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

@Dorenavant Dorenavant requested review from a team as code owners February 28, 2025 01:08
@aajtodd
Copy link
Contributor

aajtodd commented Feb 28, 2025

Can you describe what problem this solves and an example of a using it?

@Dorenavant
Copy link
Author

Dorenavant commented Feb 28, 2025

Can you describe what problem this solves and an example of a using it?

In our case, we're trying to add a #[serde(rename = "name")] to the Enum members. We add #[derive(Deserialize)] to the Enum to help deserialize the output shapes of our smithy API, but this is causing problems when an enum has a value override, e.g.

enum Foo {
  Bar = "NotBar"
}

Then the string "NotBar" isn't able to be deserialized into Foo::Bar.

A user could then implement a ServerCodegenDecorator that calls extraSections

fun extraSections(codegenContext: CodegenContext): List<AdHocCustomization> = listOf()
to add custom member attributes here.

@Dorenavant
Copy link
Author

Hey team, could we get some eyes on this please? Or if there is another way to customize attributes on enum members, can you please let us know?

@drganjoo
Copy link
Contributor

We appreciate the feature addition! I'm wondering if we could refactor this slightly to make it more consistent with our existing architecture.

It seems a bit cleaner to move this customization out of EnumMemberModel and into our abstract class pattern. Since we already have methods for adding attributes to the Enum itself, what if we extend this approach with:

abstract class EnumType {
    /** Returns a writable that implements `From<&str>` and/or `TryFrom<&str>` for the enum */
    abstract fun implFromForStr(context: EnumGeneratorContext): Writable

    /** Returns a writable that implements `FromStr` for the enum */
    abstract fun implFromStr(context: EnumGeneratorContext): Writable

   // other functions...

   /** Optionally add more attributes to each variant of the enum */
    open fun additionalEnumVariantAttributes(context: EnumMemberModel): List<Attribute> = emptyList()

We will call this function in RustWriter.renderEnum:

        context.enumMeta.render(this)
        rustBlock("enum ${context.enumName}") {
            context.sortedMembers.forEach { member ->
                enumType.additionalEnumVariantAttributes(member).forEach { attribute ->
                    attribute.render(this)
                }
                member.render(this)
            }
            enumType.additionalEnumMembers(context)(this)
        }

This would allow concrete classes to override and add custom attributes to enum variants in a way that's consistent with our current pattern.

The only trade-off would be a slight change in the output order: attributes would appear first, followed by docs and deprecated markers, and then the variant name:

enum Suit {
    #[serde(rename="diamond")]
    /// Docs go here
    /// Deprecated goes here
    Diamond
}

Would this approach work for your use case? I think it gives us the same functionality while maintaining architectural consistency.

@Dorenavant
Copy link
Author

Dorenavant commented Mar 21, 2025

This would work for us, I'll do the refactor thank you

Please see below comment

@Dorenavant
Copy link
Author

Sorry, I lost context here. After ramping myself back up to speed, I'm not sure this solves our use case. We're trying to avoid overriding the enum generation completely, but instead use a decorator to modify the existing EnumGenerator behaviour. By adding a decorator that uses the extraSections (

fun extraSections(codegenContext: CodegenContext): List<AdHocCustomization> = listOf()
), we can get this behaviour without having to override the EnumGenerator class. There is an existing pattern for this in the codebase
sealed class StructureSection(name: String) : Section(name) {
abstract val shape: StructureShape
/** Hook to add additional fields to the structure */
data class AdditionalFields(override val shape: StructureShape) : StructureSection("AdditionalFields")
/** Hook to add additional fields to the `Debug` impl */
data class AdditionalDebugFields(override val shape: StructureShape, val formatterName: String) :
StructureSection("AdditionalDebugFields")
/** Hook to add additional trait impls to the structure */
data class AdditionalTraitImpls(override val shape: StructureShape, val structName: String) :
StructureSection("AdditionalTraitImpls")
}
for the StructureGenerator to inject additional behaviour into the base StructureGenerator.

@drganjoo
Copy link
Contributor

There is an existing pattern for this in the codebase

I have a broader architectural consideration: ideally, the Context should focus on elements directly from the Smithy model (shapes and related constructs) rather than including details specific to Rust code generation. For such implementation-specific concerns, leveraging the symbol provider would be architecturally cleaner.

That said, you've made a valid observation. We've already implemented this pattern for StructureGenerator. Given that precedent, extending the same approach to EnumGenerator would follow the established design pattern.

We can consider a more comprehensive refactoring of the Context/symbol provider responsibilities as a separate improvement effort if needed at a later stage.

@Dorenavant
Copy link
Author

Thanks for the approval! I realized it would be handy to have AdditionalTraitImpls as a hook in these sections as well, going to make another commit.

@landonxjames
Copy link
Contributor

Looks like there are quite a few CI failures, all similar to:

> Task :codegen-core:jar
e: file:///home/build/workspace/smithy-rs/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerEnumGenerator.kt:169:9 No value passed for parameter 'customizations'

> Task :codegen-server:compileKotlin FAILED
e: file:///home/build/workspace/smithy-rs/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ClientEnumGenerator.kt:286:9 No value passed for parameter 'customizations'

> Task :codegen-client:compileKotlin FAILED

@Dorenavant
Copy link
Author

Looks like there are quite a few CI failures, all similar to:

> Task :codegen-core:jar
e: file:///home/build/workspace/smithy-rs/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerEnumGenerator.kt:169:9 No value passed for parameter 'customizations'

> Task :codegen-server:compileKotlin FAILED
e: file:///home/build/workspace/smithy-rs/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ClientEnumGenerator.kt:286:9 No value passed for parameter 'customizations'

> Task :codegen-client:compileKotlin FAILED

I see what's wrong, pushing a fix

@landonxjames
Copy link
Contributor

Getting closer, looks like its the tests failing now:

> Task :codegen-client:classes
e: file:///home/build/workspace/smithy-rs/codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ClientEnumGeneratorTest.kt:31:46 No value passed for parameter 'customizations'

e: file:///home/build/workspace/smithy-rs/codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ClientEnumGeneratorTest.kt:91:42 No value passed for parameter 'customizations'
e: file:///home/build/workspace/smithy-rs/codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ClientEnumGeneratorTest.kt:126:42 No value passed for parameter 'customizations'
> Task :codegen-client:compileTestKotlin FAILED
15 actionable tasks: 15 executed
e: file:///home/build/workspace/smithy-rs/codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ClientEnumGeneratorTest.kt:159:42 No value passed for parameter 'customizations'
e: file:///home/build/workspace/smithy-rs/codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ClientInstantiatorTest.kt:56:49 No value passed for parameter 'customizations'
e: file:///home/build/workspace/smithy-rs/codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ClientInstantiatorTest.kt:77:49 No value passed for parameter 'customizations'

FAILURE: Build failed with an exception.

Copy link
Contributor

@landonxjames landonxjames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of small formatting things, but otherwise looks good. In favor of shipping if we can get all the CI failures cleaned up.

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

Successfully merging this pull request may close these issues.

4 participants