Squash assorted Clippy and Rust doc warnings in codegen integration tests (#3684)

david-perez · web-flow · commit 41b938dfe994 · 2024-06-11T14:52:22.000Z
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). ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
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
@@ -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<EnumTrait>()) {
+                    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
@@ -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<TitleTrait>()?.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
@@ -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
@@ -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
@@ -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 `<protocol>-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
@@ -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 : AbstractCodeWriter<T>> 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 : AbstractCodeWriter<T>> 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
+ * <body>
+ *     <p>Text without brackets</p>
+ *     <div>Some text with [brackets]</div>
+ *     <span>Another [example, 1]</span>
+ * </body>
+ * ```
+ *
+ * gets converted to:
+ *
+ * ```html
+ * <body>
+ *     <p>Text without brackets</p>
+ *     <div>Some text with \[brackets\]</div>
+ *     <span>Another \[example, 1\]</span>
+ * </body>
+ * ```
+ *
+ * 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
@@ -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
@@ -332,17 +332,13 @@ class HttpBindingGenerator(
                         }
                     }
                     if (targetShape.hasTrait<EnumTrait>()) {
-                        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
@@ -99,6 +99,21 @@ class RustWriterTest {
         output shouldContain "/// Top level module"
     }
 
+    @Test
+    fun `normalize HTML`() {
+        val output =
+            normalizeHtml(
+                """
+                <a>Link without href attribute</a>
+                <div>Some text with [brackets]</div>
+                <span>] mismatched [ is escaped too</span>
+                """,
+            )
+        output shouldContain "<code>Link without href attribute</code>"
+        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
@@ -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<DefaultTrait>().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
@@ -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
@@ -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(
