Skip to content

Commit 42d082f

Browse files
authored
Fix clippy warnings in generated server Rust code (#4425)
## Summary Updates the Rust server codegen to generate more idiomatic code that passes clippy lints without warnings. ## Existing Clippy Warnings Currently `:codegen-server-test:test` results in the following: ``` error: redundant closure --> rest_json/rust-server-codegen/src/output.rs:2533:21 | 2533 | || ::std::collections::HashMap::new(), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `::std::collections::HashMap::new` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure ``` and: ``` error: redundant closure --> json_rpc10/rust-server-codegen/src/input.rs:1005:17 | 1005 | |v| crate::constrained::MaybeConstrained::Constrained(v), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace the closure with the function itself: `crate::constrained::MaybeConstrained::Constrained` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure ``` ## Changes - Use `unwrap_or_default` instead of `unwrap_or_else(Vec::new)` and `unwrap_or_else(HashMap::new)` - Generate closures only when transformation is actually needed (avoid redundant closures) ## Fixes This resolves clippy warnings in generated code: - `clippy::redundant-closure` - Unnecessary closures that can be replaced with function references - Suboptimal use of `unwrap_or_else` with default constructors
1 parent bfe50e2 commit 42d082f

2 files changed

Lines changed: 51 additions & 29 deletions

File tree

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGenerator.kt

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -347,12 +347,28 @@ class ServerBuilderGenerator(
347347
"${symbol.makeMaybeConstrained().rustType().namespace}::MaybeConstrained::Constrained"
348348

349349
var varExpr = if (symbol.isOptional()) "v" else "input"
350-
if (hasBox) varExpr = "*$varExpr"
351-
if (!constrainedTypeHoldsFinalType(member)) varExpr = "($varExpr).into()"
350+
var needsTransformation = false
351+
352+
if (hasBox) {
353+
varExpr = "*$varExpr"
354+
needsTransformation = true
355+
}
356+
if (!constrainedTypeHoldsFinalType(member)) {
357+
varExpr = "($varExpr).into()"
358+
needsTransformation = true
359+
}
352360

353361
if (wrapInMaybeConstrained) {
354-
conditionalBlock("input.map(##[allow(clippy::redundant_closure)] |v| ", ")", conditional = symbol.isOptional()) {
355-
conditionalBlock("Box::new(", ")", conditional = hasBox) {
362+
if (needsTransformation) {
363+
conditionalBlock("input.map(|v| ", ")", conditional = symbol.isOptional()) {
364+
conditionalBlock("Box::new(", ")", conditional = hasBox) {
365+
rust("$maybeConstrainedVariant($varExpr)")
366+
}
367+
}
368+
} else {
369+
if (symbol.isOptional()) {
370+
rust("input.map($maybeConstrainedVariant)")
371+
} else {
356372
rust("$maybeConstrainedVariant($varExpr)")
357373
}
358374
}

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt

Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -84,34 +84,40 @@ fun generateFallbackCodeToDefaultValue(
8484
symbolProvider: RustSymbolProvider,
8585
publicConstrainedTypes: Boolean,
8686
) {
87-
var defaultValue = defaultValue(model, runtimeConfig, symbolProvider, member)
87+
val node = member.expectTrait<DefaultTrait>().toNode()!!
8888
val targetShape = model.expectShape(member.target)
89-
val targetSymbol = symbolProvider.toSymbol(targetShape)
90-
// We need an .into() conversion to create defaults for the server types. A larger scale refactoring could store this information in the
91-
// symbol, however, retrieving it in this manner works for the moment.
92-
if (targetSymbol.rustType().qualifiedName().startsWith("::aws_smithy_http_server_python")) {
93-
defaultValue = defaultValue.map { rust("#T.into()", it) }
94-
}
9589

96-
if (member.isStreaming(model)) {
90+
val useUnwrapOrDefault =
91+
(targetShape is ListShape && node is ArrayNode && node.isEmpty) ||
92+
(targetShape is MapShape && node is ObjectNode && node.isEmpty)
93+
94+
if (member.isStreaming(model) || useUnwrapOrDefault) {
9795
writer.rust(".unwrap_or_default()")
98-
} else if (targetShape.hasPublicConstrainedWrapperTupleType(model, publicConstrainedTypes)) {
99-
// TODO(https://github.com/smithy-lang/smithy-rs/issues/2134): Instead of panicking here, which will ungracefully
100-
// shut down the service, perform the `try_into()` check _once_ at service startup time, perhaps
101-
// storing the result in a `OnceCell` that could be reused.
102-
writer.rustTemplate(
103-
"""
104-
.unwrap_or_else(||
105-
#{DefaultValue:W}
106-
.try_into()
107-
.expect("this check should have failed at generation time; please file a bug report under https://github.com/smithy-lang/smithy-rs/issues")
108-
)
109-
""",
110-
"DefaultValue" to defaultValue,
111-
)
11296
} else {
113-
val node = member.expectTrait<DefaultTrait>().toNode()!!
114-
if ((targetShape is DocumentShape && (node is BooleanNode || node is NumberNode)) ||
97+
// Compute defaultValue only when we actually need it
98+
var defaultValue = defaultValue(model, runtimeConfig, symbolProvider, member)
99+
val targetSymbol = symbolProvider.toSymbol(targetShape)
100+
// We need an .into() conversion to create defaults for the server types. A larger scale refactoring could store this information in the
101+
// symbol, however, retrieving it in this manner works for the moment.
102+
if (targetSymbol.rustType().qualifiedName().startsWith("::aws_smithy_http_server_python")) {
103+
defaultValue = defaultValue.map { rust("#T.into()", it) }
104+
}
105+
106+
if (targetShape.hasPublicConstrainedWrapperTupleType(model, publicConstrainedTypes)) {
107+
// TODO(https://github.com/smithy-lang/smithy-rs/issues/2134): Instead of panicking here, which will ungracefully
108+
// shut down the service, perform the `try_into()` check _once_ at service startup time, perhaps
109+
// storing the result in a `OnceCell` that could be reused.
110+
writer.rustTemplate(
111+
"""
112+
.unwrap_or_else(||
113+
#{DefaultValue:W}
114+
.try_into()
115+
.expect("this check should have failed at generation time; please file a bug report under https://github.com/smithy-lang/smithy-rs/issues")
116+
)
117+
""",
118+
"DefaultValue" to defaultValue,
119+
)
120+
} else if ((targetShape is DocumentShape && (node is BooleanNode || node is NumberNode)) ||
115121
targetShape is BooleanShape ||
116122
targetShape is NumberShape ||
117123
targetShape is EnumShape
@@ -120,7 +126,7 @@ fun generateFallbackCodeToDefaultValue(
120126
} else {
121127
// Values for the Rust types of the rest of the shapes might require heap allocations,
122128
// so we calculate them in a (lazily-executed) closure for minimal performance gains.
123-
writer.rustTemplate(".unwrap_or_else(##[allow(clippy::redundant_closure)] || #{DefaultValue:W})", "DefaultValue" to defaultValue)
129+
writer.rustTemplate(".unwrap_or_else(|| #{DefaultValue:W})", "DefaultValue" to defaultValue)
124130
}
125131
}
126132
}

0 commit comments

Comments
 (0)