Skip to content
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

Remove attributes annotation of Builder struct #3674

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

edisongustavo
Copy link
Contributor

The struct created for a Builder should not have attributes because these structs only derive from Debug, PartialEq, Clone and Default. None of these support attributes.

This becomes a problem if a plugin tries to add attributes to the metadata field to configure the generated code for the struct. In this case, the attributes will also be added to the Builder struct; which is wrong.

Motivation and Context

I'm customizing client code generation by overriding the method:

override fun symbolProvider(base: RustSymbolProvider): RustSymbolProvider

I override the symbol.expectRustMetadata().additionalAttributes value and add extra values there. The generated code adds the correct attributes for the struct, however, it also adds these attributes to the Builder.

The Builder is restricted to deriving from Debug, PartialEq, Clone and Default (see code).

I believe this change was added by mistake in #2222.

Description

To solve the issue, I simply remove the offending line.

Testing

I don't expect any existing code to break, given that this is a bug. It only manifests when someone is customizing the generated code.

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.

@edisongustavo edisongustavo marked this pull request as ready for review May 30, 2024 17:08
@edisongustavo edisongustavo requested review from a team as code owners May 30, 2024 17:08
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@edisongustavo
Copy link
Contributor Author

Ok, I see that we actually need Builders to have the non_exhaustive attribute. I'll work on fixing this.

Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

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

This LGTM but we'd want this change too to apply to the server. Make the change to these files too:

  • ServerBuilderGenerator
  • ServerBuilderGeneratorWithoutPublicConstrainedTypes

Bonus points if you make things DRYer and/or document the motivation for this change with either a comment or a small test.

@edisongustavo
Copy link
Contributor Author

Thanks @david-perez! I will change those files too, as you requested.

@edisongustavo edisongustavo force-pushed the main branch 3 times, most recently from 7d662a4 to 2b2a04b Compare June 6, 2024 19:30
@edisongustavo
Copy link
Contributor Author

Hello @david-perez , I have added the tests as you requested, to both client and server generators.

Notice that I didn't need to make any changes to the server classes because they don't have the same bug as the client.

Please let me know if you'd like me to do anything else in this PR.

Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

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

Thanks for the tests!

@edisongustavo
Copy link
Contributor Author

Hello, how could we expedite this to be merged and available in the next release?

@david-perez david-perez enabled auto-merge June 11, 2024 13:29
@edisongustavo
Copy link
Contributor Author

I see some linting checks failed. I'll resolve these and update the branch

The `struct` created for a Builder should not have attributes because these structs only derive from `Debug`,
`PartialEq`, `Clone` and `Default`. None of these support attributes.

This becomes a problem if a plugin tries to add attributes to the `metadata` field to configure the generated code for
the `struct`. In this case, the attributes will also be added to the `Builder` struct; which is wrong.
auto-merge was automatically disabled June 11, 2024 14:49

Head branch was pushed to by a user without write access

@edisongustavo
Copy link
Contributor Author

I'm sorry I didn't see the ./gradlew ktlint --format option. I can run everything locally now, so the next set of tests should pass.

@edisongustavo
Copy link
Contributor Author

edisongustavo commented Jun 12, 2024

Hello, the failing check is ci / Ask maintainer to run the PR bot and canary workflows.

@david-perez david-perez added this pull request to the merge queue Jul 9, 2024
Merged via the queue into smithy-lang:main with commit dcf16ac Jul 9, 2024
41 of 42 checks passed
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.

3 participants