From dcf16ac5de59fa016f119b881deebbaf7b6f7eac Mon Sep 17 00:00:00 2001 From: Gustavo Muenz Date: Tue, 9 Jul 2024 11:38:53 +0200 Subject: [PATCH] Remove attributes annotation of Builder `struct` (#3674) 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: ```kotlin 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](https://github.com/smithy-lang/smithy-rs/blob/a76dc184c2cf52d6027115f64fe78ab0c2a429e8/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt#L188)). I believe this change was added by mistake in https://github.com/smithy-lang/smithy-rs/pull/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._ Co-authored-by: Gustavo Muenz --- .../rust/codegen/core/rustlang/RustType.kt | 1 + .../smithy/generators/BuilderGenerator.kt | 8 +- .../smithy/rust/codegen/core/testutil/Rust.kt | 7 +- .../smithy/generators/BuilderGeneratorTest.kt | 56 +++++++++++++ .../generators/ServerBuilderGeneratorTest.kt | 79 +++++++++++++++++++ 5 files changed, 148 insertions(+), 3 deletions(-) diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustType.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustType.kt index 6c57f5fd65..c5a7b76276 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustType.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustType.kt @@ -560,6 +560,7 @@ class Attribute(val inner: Writable, val isDeriveHelper: Boolean = false) { val AllowUnusedMut = Attribute(allow("unused_mut")) val AllowUnusedVariables = Attribute(allow("unused_variables")) val CfgTest = Attribute(cfg("test")) + val DenyDeprecated = Attribute(deny("deprecated")) val DenyMissingDocs = Attribute(deny("missing_docs")) val DocHidden = Attribute(doc("hidden")) val DocInline = Attribute(doc("inline")) diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt index 09c0f49e22..bf99e73790 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGenerator.kt @@ -189,6 +189,12 @@ class BuilderGenerator( metadata.derives.filter { it == RuntimeType.Debug || it == RuntimeType.PartialEq || it == RuntimeType.Clone } + RuntimeType.Default + + // Filter out attributes + private val builderAttributes = + metadata.additionalAttributes.filter { + it == Attribute.NonExhaustive + } private val builderName = symbolProvider.symbolForBuilder(shape).name fun render(writer: RustWriter) { @@ -306,8 +312,8 @@ class BuilderGenerator( private fun renderBuilder(writer: RustWriter) { writer.docs("A builder for #D.", structureSymbol) - metadata.additionalAttributes.render(writer) Attribute(derive(builderDerives)).render(writer) + this.builderAttributes.render(writer) writer.rustBlock("pub struct $builderName") { for (member in members) { val memberName = symbolProvider.toMemberName(member) diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/Rust.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/Rust.kt index 0d6d3403e2..fb86012d23 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/Rust.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/testutil/Rust.kt @@ -546,11 +546,14 @@ fun RustCrate.integrationTest( writable: Writable, ) = this.withFile("tests/$name.rs", writable) -fun TestWriterDelegator.unitTest(test: Writable): TestWriterDelegator { +fun TestWriterDelegator.unitTest( + additionalAttributes: List = emptyList(), + test: Writable, +): TestWriterDelegator { lib { val name = safeName("test") withInlineModule(RustModule.inlineTests(name), TestModuleDocProvider) { - unitTest(name) { + unitTest(name, additionalAttributes = additionalAttributes) { test(this) } } diff --git a/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGeneratorTest.kt b/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGeneratorTest.kt index 2ea5c56f59..fc5fe91b4e 100644 --- a/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGeneratorTest.kt +++ b/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGeneratorTest.kt @@ -11,6 +11,7 @@ import software.amazon.smithy.model.Model import software.amazon.smithy.model.knowledge.NullableIndex import software.amazon.smithy.model.shapes.Shape import software.amazon.smithy.model.shapes.StructureShape +import software.amazon.smithy.rust.codegen.core.rustlang.Attribute import software.amazon.smithy.rust.codegen.core.rustlang.Attribute.Companion.AllowDeprecated import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter import software.amazon.smithy.rust.codegen.core.rustlang.implBlock @@ -19,6 +20,8 @@ import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate import software.amazon.smithy.rust.codegen.core.smithy.Default import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider import software.amazon.smithy.rust.codegen.core.smithy.WrappingSymbolProvider +import software.amazon.smithy.rust.codegen.core.smithy.expectRustMetadata +import software.amazon.smithy.rust.codegen.core.smithy.meta import software.amazon.smithy.rust.codegen.core.smithy.setDefault import software.amazon.smithy.rust.codegen.core.testutil.TestWorkspace import software.amazon.smithy.rust.codegen.core.testutil.asSmithyModel @@ -240,4 +243,57 @@ internal class BuilderGeneratorTest { } project.compileAndTest() } + + @Test + fun `builder doesn't inherit attributes from struct`() { + /** + * This test checks if the generated Builder doesn't inherit the macro attributes added to the main struct. + * + * The strategy is to: + * 1) mark the `Inner` struct with `#[deprecated]` + * 2) deny use of deprecated in the test + * 3) allow use of deprecated by the Builder + * 4) Ensure that the builder can be instantiated + */ + class SymbolProviderWithExtraAnnotation(val base: RustSymbolProvider) : RustSymbolProvider by base { + override fun toSymbol(shape: Shape): Symbol { + val baseSymbol = base.toSymbol(shape) + val name = baseSymbol.name + if (name == "Inner") { + var metadata = baseSymbol.expectRustMetadata() + val attribute = Attribute.Deprecated + metadata = metadata.copy(additionalAttributes = metadata.additionalAttributes + listOf(attribute)) + return baseSymbol.toBuilder().meta(metadata).build() + } else { + return baseSymbol + } + } + } + + val provider = SymbolProviderWithExtraAnnotation(testSymbolProvider(model)) + val project = TestWorkspace.testProject(provider) + project.moduleFor(inner) { + rust("##![allow(deprecated)]") + generator(model, provider, this, inner).render() + implBlock(provider.toSymbol(inner)) { + BuilderGenerator.renderConvenienceMethod(this, provider, inner) + } + unitTest("test", additionalAttributes = listOf(Attribute.DenyDeprecated), block = { + rust( + // Notice that the builder is instantiated directly, and not through the Inner::builder() method. + // This is because Inner is marked with `deprecated`, so any usage of `Inner` inside the test will + // fail the compilation. + // + // This piece of code would fail though if the Builder inherits the attributes from Inner. + """ + let _ = test_inner::Builder::default(); + """, + ) + }) + } + project.withModule(provider.moduleForBuilder(inner)) { + BuilderGenerator(model, provider, inner, emptyList()).render(this) + } + project.compileAndTest() + } } diff --git a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorTest.kt b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorTest.kt index ef20b24fb9..2c837f3395 100644 --- a/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorTest.kt +++ b/codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorTest.kt @@ -6,10 +6,16 @@ package software.amazon.smithy.rust.codegen.server.smithy.generators import org.junit.jupiter.api.Test +import software.amazon.smithy.codegen.core.Symbol +import software.amazon.smithy.model.shapes.Shape import software.amazon.smithy.model.shapes.StructureShape +import software.amazon.smithy.rust.codegen.core.rustlang.Attribute import software.amazon.smithy.rust.codegen.core.rustlang.implBlock import software.amazon.smithy.rust.codegen.core.rustlang.rust +import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider +import software.amazon.smithy.rust.codegen.core.smithy.expectRustMetadata import software.amazon.smithy.rust.codegen.core.smithy.generators.StructureGenerator +import software.amazon.smithy.rust.codegen.core.smithy.meta import software.amazon.smithy.rust.codegen.core.testutil.TestWorkspace import software.amazon.smithy.rust.codegen.core.testutil.asSmithyModel import software.amazon.smithy.rust.codegen.core.testutil.compileAndTest @@ -79,4 +85,77 @@ class ServerBuilderGeneratorTest { } project.compileAndTest() } + + @Test + fun `builder doesn't inherit attributes from struct`() { + /** + * This test checks if the generated Builder doesn't inherit the macro attributes added to the main struct. + * + * The strategy is to: + * 1) mark the `Inner` struct with `#[deprecated]` + * 2) deny use of deprecated in the test + * 3) allow use of deprecated by the Builder + * 4) Ensure that the builder can be instantiated + */ + val model = + """ + namespace test + + structure Inner {} + """.asSmithyModel() + + class SymbolProviderWithExtraAnnotation(val base: RustSymbolProvider) : RustSymbolProvider by base { + override fun toSymbol(shape: Shape): Symbol { + val baseSymbol = base.toSymbol(shape) + val name = baseSymbol.name + if (name == "Inner") { + var metadata = baseSymbol.expectRustMetadata() + val attribute = Attribute.Deprecated + metadata = metadata.copy(additionalAttributes = metadata.additionalAttributes + listOf(attribute)) + return baseSymbol.toBuilder().meta(metadata).build() + } else { + return baseSymbol + } + } + } + + val codegenContext = serverTestCodegenContext(model) + val provider = SymbolProviderWithExtraAnnotation(codegenContext.symbolProvider) + val project = TestWorkspace.testProject(provider) + project.withModule(ServerRustModule.Model) { + val shape = model.lookup("test#Inner") + val writer = this + + rust("##![allow(deprecated)]") + StructureGenerator(model, provider, writer, shape, emptyList(), codegenContext.structSettings()).render() + val builderGenerator = + ServerBuilderGenerator( + codegenContext, + shape, + SmithyValidationExceptionConversionGenerator(codegenContext), + ServerRestJsonProtocol(codegenContext), + ) + + builderGenerator.render(project, writer) + + writer.implBlock(provider.toSymbol(shape)) { + builderGenerator.renderConvenienceMethod(this) + } + + project.renderInlineMemoryModules() + } + project.unitTest(additionalAttributes = listOf(Attribute.DenyDeprecated), test = { + rust( + // Notice that the builder is instantiated directly, and not through the Inner::builder() method. + // This is because Inner is marked with `deprecated`, so any usage of `Inner` inside the test will + // fail the compilation. + // + // This piece of code would fail though if the Builder inherits the attributes from Inner. + """ + let _ = crate::model::inner::Builder::default(); + """, + ) + }) + project.compileAndTest() + } }