smithy-lang · david-perez · Jun 11, 2024 · Jun 6, 2024 · drganjoo · Jun 11, 2024
@@ -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,

@@ -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

@@ -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`

@@ -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);

@@ -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 {

@@ -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")

@@ -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) {

@@ -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())")
                     }

@@ -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 =

@@ -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,
                     )
                 }
 

@@ -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

@@ -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(