Unverified Commit dcf16ac5 authored by Gustavo Muenz's avatar Gustavo Muenz Committed by GitHub
Browse files

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.

<!--- Describe your changes in detail -->

## 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.


<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [ ] 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: default avatarGustavo Muenz <muenze@amazon.com>
parent 0c727166

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustType.kt

+1 −0
Original line number Diff line number Diff line
@@ -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"))

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

+7 −1
Original line number Diff line number Diff line
@@ -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)

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

+5 −2
Original line number Diff line number Diff line
@@ -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<Attribute> = emptyList(),

    test: Writable,

): TestWriterDelegator {

    lib {

        val name = safeName("test")

        withInlineModule(RustModule.inlineTests(name), TestModuleDocProvider) {

            unitTest(name) {

            unitTest(name, additionalAttributes = additionalAttributes) {

                test(this)

            }

        }

codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/BuilderGeneratorTest.kt

+56 −0
Original line number Diff line number Diff line
@@ -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()

    }

}

codegen-server/src/test/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorTest.kt

+79 −0
Original line number Diff line number Diff line
@@ -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<StructureShape>("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()

    }

}