Skip to content

Commit

Permalink
Remove attributes annotation of Builder struct (#3674)
Browse files Browse the repository at this point in the history
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
#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: Gustavo Muenz <muenze@amazon.com>
  • Loading branch information
edisongustavo and Gustavo Muenz authored Jul 9, 2024
1 parent 0c72716 commit dcf16ac
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
}
}

0 comments on commit dcf16ac

Please sign in to comment.