From 2b05df93e9a311b6201a2119ca16ffbd98737377 Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 6 Jun 2024 12:39:26 +0200 Subject: [PATCH] Squash assorted Clippy and Rust doc warnings in codegen integration tests This commit gets rid of some Clippy and Rust doc warnings we produce in generated code, which are surfaced in our codegen integration tests. The aim is that eventually we will be able to deny Clippy and Rust doc warnings in #3678 (despite us baking in `RUSTDOCFLAGS="-D warnings"` and `RUSTFLAGS="-D warnings"` in our CI Docker image, we curently clear these in codegen integration tests). Note that denying compiler warnings is separately tracked in #3194. The commit contains fixes for the following: - Unconditionally referring to the `crate::types` module in Rust docs when the Smithy model is such that it is not generated. - Broken intra-crate doc links for client documentation. - Incorrectly escaping Rust reserved keywords when referring to operation names in Rust docs. - An unnecessary semi-colon when rendering additional client plugins. - Not escaping brackets in text when rendering Rust docs containing HTML, that are interpreted as broken links. - A broken link to unit variants of unions. - Using `TryFrom` instead of `From` when infallibly converting from `&str` to `String` when deserializing an `@httpPayload` in the server. - Using a redundant closure with `unwrap_or_else` when falling back to a `@default` boolean or number value for a `document` shape, instead of `unwrap_or`. - Not opting into `clippy::enum_variant_names` when rendering the `Operation` enum in the server (since we render the operation names that were modeled as-is). --- .../codegen/client/smithy/ClientRustModule.kt | 20 ++++++-- .../customizations/ClientDocsGenerator.kt | 20 +++++++- .../generators/client/FluentClientDocs.kt | 6 +-- .../client/FluentClientGenerator.kt | 10 +++- codegen-core/common-test-models/misc.smithy | 2 +- .../rust/codegen/core/rustlang/RustWriter.kt | 49 +++++++++++++++++-- .../core/smithy/generators/UnionGenerator.kt | 3 +- .../generators/http/HttpBindingGenerator.kt | 18 +++---- .../codegen/core/rustlang/RustWriterTest.kt | 15 ++++++ .../ServerBuilderGeneratorCommon.kt | 40 +++++++++------ .../generators/ServerServiceGenerator.kt | 1 + .../http/ServerRequestBindingGenerator.kt | 2 + 12 files changed, 145 insertions(+), 41 deletions(-) diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientRustModule.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientRustModule.kt index 2da355cb5c..ccd21edc6e 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientRustModule.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientRustModule.kt @@ -7,10 +7,13 @@ package software.amazon.smithy.rust.codegen.client.smithy import software.amazon.smithy.codegen.core.Symbol import software.amazon.smithy.model.Model +import software.amazon.smithy.model.shapes.EnumShape import software.amazon.smithy.model.shapes.OperationShape import software.amazon.smithy.model.shapes.Shape +import software.amazon.smithy.model.shapes.StringShape import software.amazon.smithy.model.shapes.StructureShape import software.amazon.smithy.model.shapes.UnionShape +import software.amazon.smithy.model.traits.EnumTrait import software.amazon.smithy.model.traits.ErrorTrait import software.amazon.smithy.rust.codegen.client.smithy.generators.client.FluentClientDocs import software.amazon.smithy.rust.codegen.client.smithy.generators.client.FluentClientGenerator @@ -195,8 +198,9 @@ object ClientModuleProvider : ModuleProvider { override fun moduleForShape( context: ModuleProviderContext, shape: Shape, - ): RustModule.LeafModule = - when (shape) { + ): RustModule.LeafModule { + fun shouldNotBeRendered(): Nothing = PANIC("Shape ${shape.id} should not be rendered in any module") + return when (shape) { is OperationShape -> perOperationModule(context, shape) is StructureShape -> when { @@ -206,8 +210,18 @@ object ClientModuleProvider : ModuleProvider { else -> ClientRustModule.types } - else -> ClientRustModule.types + is UnionShape, is EnumShape -> ClientRustModule.types + is StringShape -> { + if (shape.hasTrait()) { + ClientRustModule.types + } else { + shouldNotBeRendered() + } + } + + else -> shouldNotBeRendered() } + } override fun moduleForOperationError( context: ModuleProviderContext, diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/customizations/ClientDocsGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/customizations/ClientDocsGenerator.kt index fb709ac61e..d90b04c1fe 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/customizations/ClientDocsGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/customizations/ClientDocsGenerator.kt @@ -7,9 +7,11 @@ package software.amazon.smithy.rust.codegen.client.smithy.customizations import software.amazon.smithy.model.traits.TitleTrait import software.amazon.smithy.rust.codegen.client.smithy.ClientCodegenContext +import software.amazon.smithy.rust.codegen.client.smithy.ClientRustModule import software.amazon.smithy.rust.codegen.core.rustlang.Writable import software.amazon.smithy.rust.codegen.core.rustlang.containerDocs import software.amazon.smithy.rust.codegen.core.rustlang.writable +import software.amazon.smithy.rust.codegen.core.smithy.DirectedWalker import software.amazon.smithy.rust.codegen.core.smithy.generators.LibRsCustomization import software.amazon.smithy.rust.codegen.core.smithy.generators.LibRsSection import software.amazon.smithy.rust.codegen.core.smithy.generators.ModuleDocSection @@ -30,6 +32,22 @@ class ClientDocsGenerator(private val codegenContext: ClientCodegenContext) : Li private fun crateLayout(): Writable = writable { + val hasTypesModule = + DirectedWalker(codegenContext.model).walkShapes(codegenContext.serviceShape) + .any { + try { + codegenContext.symbolProvider.moduleForShape(it).name == ClientRustModule.types.name + } catch (ex: RuntimeException) { + // The shape should not be rendered in any module. + false + } + } + val typesModuleSentence = + if (hasTypesModule) { + "These structs and enums live in [`types`](crate::types). " + } else { + "" + } val serviceName = codegenContext.serviceShape.getTrait()?.value ?: "the service" containerDocs( """ @@ -40,7 +58,7 @@ class ClientDocsGenerator(private val codegenContext: ClientCodegenContext) : Li either a successful output or a [`SdkError`](crate::error::SdkError). Some of these API inputs may be structs or enums to provide more complex structured information. - These structs and enums live in [`types`](crate::types). There are some simpler types for + ${typesModuleSentence}There are some simpler types for representing data such as date times or binary blobs that live in [`primitives`](crate::primitives). All types required to configure a client via the [`Config`](crate::Config) struct live diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientDocs.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientDocs.kt index 1ceaf043cc..7570a7ce3a 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientDocs.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientDocs.kt @@ -34,8 +34,8 @@ object FluentClientDocs { or identity resolver to be configured. The config is used to customize various aspects of the client, such as: - - [HTTP Connector](crate::config::Builder::http_connector) - - [Retry](crate::config::Builder::retry_config) + - [The underlying HTTP client](crate::config::Builder::http_client) + - [Retries](crate::config::Builder::retry_config) - [Timeouts](crate::config::Builder::timeout_config) - [... and more](crate::config::Builder) @@ -76,7 +76,7 @@ object FluentClientDocs { if (operation != null && member != null) { val operationSymbol = symbolProvider.toSymbol(operation) val memberSymbol = symbolProvider.toSymbol(member) - val operationFnName = FluentClientGenerator.clientOperationFnName(operation, symbolProvider) + val operationFnName = FluentClientGenerator.clientOperationFnDocsName(operation, symbolProvider) docsTemplate( """ ## Using the `Client` diff --git a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt index a39849d59c..f7a7ff3b07 100644 --- a/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt +++ b/codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt @@ -59,7 +59,13 @@ class FluentClientGenerator( fun clientOperationFnName( operationShape: OperationShape, symbolProvider: RustSymbolProvider, - ): String = RustReservedWords.escapeIfNeeded(symbolProvider.toSymbol(operationShape).name.toSnakeCase()) + ): String = RustReservedWords.escapeIfNeeded(clientOperationFnDocsName(operationShape, symbolProvider)) + + /** When using the function name in Rust docs, there's no need to escape Rust reserved words. **/ + fun clientOperationFnDocsName( + operationShape: OperationShape, + symbolProvider: RustSymbolProvider, + ): String = symbolProvider.toSymbol(operationShape).name.toSnakeCase() fun clientOperationModuleName( operationShape: OperationShape, @@ -277,7 +283,7 @@ private fun baseClientRuntimePluginsFn( .with_client_plugin(crate::config::ServiceRuntimePlugin::new(config.clone())) .with_client_plugin(#{NoAuthRuntimePlugin}::new()); - #{additional_client_plugins:W}; + #{additional_client_plugins:W} for plugin in configured_plugins { plugins = plugins.with_client_plugin(plugin); diff --git a/codegen-core/common-test-models/misc.smithy b/codegen-core/common-test-models/misc.smithy index 10072543c9..42329ba1d2 100644 --- a/codegen-core/common-test-models/misc.smithy +++ b/codegen-core/common-test-models/misc.smithy @@ -9,7 +9,7 @@ use smithy.framework#ValidationException /// A service to test miscellaneous aspects of code generation where protocol /// selection is not relevant. If you want to test something protocol-specific, -/// add it to a separate `-extras.smithy`. +/// add it to a separate `[protocol]-extras.smithy`. @restJson1 @title("MiscService") service MiscService { diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustWriter.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustWriter.kt index c0922525b0..94b3dc67c6 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustWriter.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustWriter.kt @@ -8,6 +8,7 @@ package software.amazon.smithy.rust.codegen.core.rustlang import org.intellij.lang.annotations.Language import org.jsoup.Jsoup import org.jsoup.nodes.Element +import org.jsoup.select.Elements import software.amazon.smithy.codegen.core.CodegenException import software.amazon.smithy.codegen.core.Symbol import software.amazon.smithy.codegen.core.SymbolDependencyContainer @@ -336,10 +337,10 @@ fun > T.docsOrFallback( docs("_Note: ${it}_") } } else if (autoSuppressMissingDocs) { + // Otherwise, suppress the missing docs lint for this shape since + // the lack of documentation is a modeling issue rather than a codegen issue. rust("##[allow(missing_docs)] // documentation missing in model") } - // Otherwise, suppress the missing docs lint for this shape since - // the lack of documentation is a modeling issue rather than a codegen issue. return this } @@ -439,16 +440,56 @@ fun RustWriter.deprecatedShape(shape: Shape): RustWriter { fun > T.escape(text: String): String = text.replace("$expressionStart", "$expressionStart$expressionStart") -/** Parse input as HTML and normalize it */ +/** Parse input as HTML and normalize it, for suitable use within Rust docs. */ fun normalizeHtml(input: String): String { val doc = Jsoup.parse(input) doc.body().apply { - normalizeAnchors() // Convert anchor tags missing href attribute into pre tags + normalizeAnchors() + escapeBracketsInText() } return doc.body().html() } +/** + * Escape brackets in elements' inner text. + * + * For example: + * + * ```html + * + *

Text without brackets

+ *
Some text with [brackets]
+ * Another [example, 1] + * + * ``` + * + * gets converted to: + * + * ```html + * + *

Text without brackets

+ *
Some text with \[brackets\]
+ * Another \[example, 1\] + * + * ``` + * + * This is useful when rendering Rust docs, since text within brackets might get misinterpreted as broken Markdown doc + * links (`[link text](https://example.com)`). + **/ +private fun Element.escapeBracketsInText() { + // Select all elements that directly contain text with opening or closing brackets. + val elements: Elements = select("*:containsOwn([), *:containsOwn(])") + + // Loop through each element and escape the brackets in the text. + for (element: Element in elements) { + val originalText = element.text() + val escapedText = originalText.replace("[", "\\[").replace("]", "\\]") + element.text(escapedText) + } +} + +/** Convert anchor tags missing `href` attribute into `code` tags. **/ private fun Element.normalizeAnchors() { getElementsByTag("a").forEach { val link = it.attr("href") diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/UnionGenerator.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/UnionGenerator.kt index e25fccdeb6..5ec29d8f17 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/UnionGenerator.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/UnionGenerator.kt @@ -200,7 +200,8 @@ private fun RustWriter.renderAsVariant( ) { if (member.isTargetUnit()) { rust( - "/// Tries to convert the enum instance into [`$variantName`], extracting the inner `()`.", + "/// Tries to convert the enum instance into [`$variantName`](#T::$variantName), extracting the inner `()`.", + unionSymbol, ) rust("/// Returns `Err(&Self)` if it can't be converted.") rustBlockTemplate("pub fn as_$funcNamePart(&self) -> #{Result}<(), &Self>", *preludeScope) { diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt index 9687bb2492..c9997ba67a 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt @@ -332,17 +332,13 @@ class HttpBindingGenerator( } } if (targetShape.hasTrait()) { - if (codegenTarget == CodegenTarget.SERVER) { - rust( - "Ok(#T::try_from(body_str)?)", - symbolProvider.toSymbol(targetShape), - ) - } else { - rust( - "Ok(#T::from(body_str))", - symbolProvider.toSymbol(targetShape), - ) - } + // - In servers, `T` is an unconstrained `String` that will be constrained when building the + // builder. + // - In clients, `T` will directly be the target generated enum type. + rust( + "Ok(#T::from(body_str))", + symbolProvider.toSymbol(targetShape), + ) } else { rust("Ok(body_str.to_string())") } diff --git a/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustWriterTest.kt b/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustWriterTest.kt index 18f8cbfadc..861288041e 100644 --- a/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustWriterTest.kt +++ b/codegen-core/src/test/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustWriterTest.kt @@ -99,6 +99,21 @@ class RustWriterTest { output shouldContain "/// Top level module" } + @Test + fun `normalize HTML`() { + val output = + normalizeHtml( + """ + Link without href attribute +
Some text with [brackets]
+ ] mismatched [ is escaped too + """, + ) + output shouldContain "Link without href attribute" + output shouldContain "Some text with \\[brackets\\]" + output shouldContain "\\] mismatched \\[ is escaped too" + } + @Test fun `generate doc links`() { val model = diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt index 9c80cd1258..1035928204 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerBuilderGeneratorCommon.kt @@ -38,6 +38,8 @@ import software.amazon.smithy.rust.codegen.core.rustlang.rust import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate import software.amazon.smithy.rust.codegen.core.rustlang.writable import software.amazon.smithy.rust.codegen.core.smithy.RuntimeConfig +import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType +import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType.Companion.preludeScope import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider import software.amazon.smithy.rust.codegen.core.smithy.generators.PrimitiveInstantiator import software.amazon.smithy.rust.codegen.core.smithy.rustType @@ -108,15 +110,17 @@ fun generateFallbackCodeToDefaultValue( "DefaultValue" to defaultValue, ) } else { - when (targetShape) { - is NumberShape, is EnumShape, is BooleanShape -> { - writer.rustTemplate(".unwrap_or(#{DefaultValue:W})", "DefaultValue" to defaultValue) - } - // Values for the Rust types of the rest of the shapes require heap allocations, so we calculate them - // in a (lazily-executed) closure for slight performance gains. - else -> { - writer.rustTemplate(".unwrap_or_else(|| #{DefaultValue:W})", "DefaultValue" to defaultValue) - } + val node = member.expectTrait().toNode()!! + if ((targetShape is DocumentShape && (node is BooleanNode || node is NumberNode)) || + targetShape is BooleanShape || + targetShape is NumberShape || + targetShape is EnumShape + ) { + writer.rustTemplate(".unwrap_or(#{DefaultValue:W})", "DefaultValue" to defaultValue) + } else { + // Values for the Rust types of the rest of the shapes might require heap allocations, + // so we calculate them in a (lazily-executed) closure for minimal performance gains. + writer.rustTemplate(".unwrap_or_else(##[allow(clippy::redundant_closure)] || #{DefaultValue:W})", "DefaultValue" to defaultValue) } } } @@ -125,7 +129,7 @@ fun generateFallbackCodeToDefaultValue( * Returns a writable to construct a Rust value of the correct type holding the modeled `@default` value on the * [member] shape. */ -fun defaultValue( +private fun defaultValue( model: Model, runtimeConfig: RuntimeConfig, symbolProvider: RustSymbolProvider, @@ -164,12 +168,12 @@ fun defaultValue( } is ListShape -> { check(node is ArrayNode && node.isEmpty) - rust("Vec::new()") + rustTemplate("#{Vec}::new()", *preludeScope) } is MapShape -> { check(node is ObjectNode && node.isEmpty) - rust("std::collections::HashMap::new()") + rustTemplate("#{HashMap}::new()", "HashMap" to RuntimeType.HashMap) } is DocumentShape -> { @@ -188,7 +192,8 @@ fun defaultValue( is StringNode -> rustTemplate( - "#{SmithyTypes}::Document::String(String::from(${node.value.dq()}))", + "#{SmithyTypes}::Document::String(#{String}::from(${node.value.dq()}))", + *preludeScope, "SmithyTypes" to types, ) @@ -207,14 +212,19 @@ fun defaultValue( is ArrayNode -> { check(node.isEmpty) - rustTemplate("""#{SmithyTypes}::Document::Array(Vec::new())""", "SmithyTypes" to types) + rustTemplate( + """#{SmithyTypes}::Document::Array(#{Vec}::new())""", + *preludeScope, + "SmithyTypes" to types, + ) } is ObjectNode -> { check(node.isEmpty) rustTemplate( - "#{SmithyTypes}::Document::Object(std::collections::HashMap::new())", + "#{SmithyTypes}::Document::Object(#{HashMap}::new())", "SmithyTypes" to types, + "HashMap" to RuntimeType.HashMap, ) } diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerServiceGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerServiceGenerator.kt index 1898c6c3db..db95531f71 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerServiceGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerServiceGenerator.kt @@ -756,6 +756,7 @@ class ServerServiceGenerator( rustTemplate( """ /// An enumeration of all [operations](https://smithy.io/2.0/spec/service-types.html##operation) in $serviceName. + ##[allow(clippy::enum_variant_names)] ##[derive(Debug, PartialEq, Eq, Clone, Copy)] pub enum Operation { $operations diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/http/ServerRequestBindingGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/http/ServerRequestBindingGenerator.kt index ce02e2c99b..12ecad9b4b 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/http/ServerRequestBindingGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/http/ServerRequestBindingGenerator.kt @@ -34,6 +34,8 @@ class ServerRequestBindingGenerator( HttpBindingGenerator( protocol, codegenContext, + // Note how we parse the HTTP-bound values into _unconstrained_ types; they will be constrained when + // building the builder. codegenContext.unconstrainedShapeSymbolProvider, operationShape, listOf(