Fix bugs with BDD codegen

codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/Util.kt

@@ -36,8 +36,26 @@ data class Context(
     val functionRegistry: FunctionRegistry,
     val runtimeConfig: RuntimeConfig,
     val isBddMode: Boolean = false,
+    /**
+     * Maps each reference's original Smithy name (`Identifier.toString()`) to the
+     * canonical rust identifier used in generated code. Non-empty only in BDD mode,
+     * and only when at least one SSA variable's snake_case form collides with a
+     * parameter name. Call sites that emit a reference in generated code should look
+     * up through this map so they receive the disambiguated name when applicable;
+     * other lookups fall back to `id.rustName()`.
+     *
+     * See `AnnotatedRefs.from` for how the disambiguation map is built.
+     */
+    val nameByOriginal: Map<String, String> = emptyMap(),
 )
 
+/**
+ * Returns the canonical rust identifier for a reference to [id] in this context.
+ * Transparent fallback to `id.rustName()` for identifiers that don't need
+ * disambiguation (the common case).
+ */
+fun Context.resolveName(id: Identifier): String = nameByOriginal[id.toString()] ?: id.rustName()
+
 /**
  * Utility function to convert an [Identifier] into a valid Rust identifier (snake case)
  */

codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/rulesgen/ExpressionGenerator.kt

@@ -18,6 +18,7 @@ import software.amazon.smithy.rulesengine.language.syntax.expressions.functions.
 import software.amazon.smithy.rulesengine.language.syntax.expressions.literal.Literal
 import software.amazon.smithy.rust.codegen.client.smithy.endpoint.Context
 import software.amazon.smithy.rust.codegen.client.smithy.endpoint.generators.EndpointResolverGenerator
+import software.amazon.smithy.rust.codegen.client.smithy.endpoint.resolveName
 import software.amazon.smithy.rust.codegen.client.smithy.endpoint.rustName
 import software.amazon.smithy.rust.codegen.core.rustlang.Writable
 import software.amazon.smithy.rust.codegen.core.rustlang.join
@@ -52,35 +53,39 @@ class ExpressionGenerator(
 
         override fun visitRef(ref: Reference) =
             writable {
+                // Resolve the name through the context so BDD-mode SSA variables that
+                // collide with parameter names get their disambiguated rust-name (e.g.
+                // `resource_arn_v`). See Context.resolveName / AnnotatedRefs.from.
+                val name = context.resolveName(ref.name)
                 if (ownership == Ownership.Owned) {
                     try {
                         when (ref.type()) {
-                            is BooleanType -> rust("*${ref.name.rustName()}")
-                            else -> rust("${ref.name.rustName()}.to_owned()")
+                            is BooleanType -> rust("*$name")
+                            else -> rust("$name.to_owned()")
                         }
                     } catch (_: RuntimeException) {
                         // Typechecking was never invoked, default to .to_owned()
-                        rust("${ref.name.rustName()}.to_owned()")
+                        rust("$name.to_owned()")
                     }
                 } else {
                     try {
                         when (ref.type()) {
-                            // This ensures we obtain a `&str`, regardless of whether `ref.name.rustName()` returns a `String` or a `&str`.
-                            // Typically, we don't know which type will be returned due to code generation.
-                            // In BDD mode, parameters are already bound as references, so the cast is unnecessary and causes errors.
+                            // This ensures we obtain a `&str`, regardless of whether the bound name
+                            // refers to a `String` or a `&str`. In BDD mode, parameters are already
+                            // bound as references, so the `as &str` cast is unnecessary and causes errors.
                             is StringType -> {
                                 if (context.isBddMode) {
-                                    rust("${ref.name.rustName()}.as_ref()")
+                                    rust("$name.as_ref()")
                                 } else {
-                                    rust("${ref.name.rustName()}.as_ref() as &str")
+                                    rust("$name.as_ref() as &str")
                                 }
                             }
-                            else -> rust(ref.name.rustName())
+                            else -> rust(name)
                         }
                     } catch (_: RuntimeException) {
-                        // Because Typechecking was never invoked upon calling `.type()` on Reference for an expression
-                        // like "{ref}: rust". See `generateLiterals2` in ExprGeneratorTest.
-                        rust(ref.name.rustName())
+                        // Typechecking was never invoked upon calling `.type()` on Reference for an
+                        // expression like "{ref}: rust". See `generateLiterals2` in ExprGeneratorTest.
+                        rust(name)
                     }
                 }
             }

codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/rulesgen/LiteralGenerator.kt

@@ -6,6 +6,7 @@
 package software.amazon.smithy.rust.codegen.client.smithy.endpoint.rulesgen
 
 import software.amazon.smithy.rulesengine.language.syntax.Identifier
+import software.amazon.smithy.rulesengine.language.syntax.expressions.Expression
 import software.amazon.smithy.rulesengine.language.syntax.expressions.Template
 import software.amazon.smithy.rulesengine.language.syntax.expressions.literal.Literal
 import software.amazon.smithy.rulesengine.language.syntax.expressions.literal.LiteralVisitor
@@ -23,8 +24,26 @@ import java.util.stream.Stream
  * Generator for literals (strings, numbers, bools, lists and objects)
  *
  * The `Document` type is used to support generating JSON-like documents
+ *
+ * [exprGenerator] is an optional callback used to render the dynamic parts of string templates
+ * (the `{Var}` / `{Var#attr}` placeholders inside a `Template`). BDD-mode callers pass a
+ * callback that delegates back to [BddExpressionGenerator] so that `getAttr` on optional
+ * assigned variables correctly generates an `if let Some(inner) = ... { inner.attr() }` unwrap.
+ * When omitted, the tree-based [ExpressionGenerator] is used, preserving existing behavior for
+ * callers that do not need BDD-specific optional-ref tracking.
+ *
+ * [multipartExprGenerator] is an optional callback used **only** for dynamic elements inside
+ * a multipart template (the `out.push_str(&...)` parts). When omitted, [exprGenerator] is used.
+ * BDD-mode callers can use this to apply optional-unwrapping (e.g. `.as_deref().unwrap_or_default()`)
+ * for `&str`-typed `push_str` arguments while keeping the single-dynamic-template case
+ * (`"{ref}"`) Option-typed for the outer caller's `if let Some(param) = ...` wrappers.
  */
-class LiteralGenerator(private val ownership: Ownership, private val context: Context) :
+class LiteralGenerator(
+    private val ownership: Ownership,
+    private val context: Context,
+    private val exprGenerator: ((Expression, Ownership) -> Writable)? = null,
+    private val multipartExprGenerator: ((Expression, Ownership) -> Writable)? = null,
+) :
     LiteralVisitor<Writable> {
     private val runtimeConfig = context.runtimeConfig
     private val codegenScope =
@@ -33,6 +52,20 @@ class LiteralGenerator(private val ownership: Ownership, private val context: Co
             "HashMap" to RuntimeType.HashMap,
         )
 
+    private fun renderTemplateExpression(
+        expr: Expression,
+        exprOwnership: Ownership,
+    ): Writable =
+        exprGenerator?.invoke(expr, exprOwnership)
+            ?: ExpressionGenerator(exprOwnership, context).generate(expr)
+
+    private fun renderMultipartExpression(
+        expr: Expression,
+        exprOwnership: Ownership,
+    ): Writable =
+        multipartExprGenerator?.invoke(expr, exprOwnership)
+            ?: renderTemplateExpression(expr, exprOwnership)
+
     override fun visitBoolean(b: Boolean) =
         writable {
             rust(b.toString())
@@ -42,9 +75,15 @@ class LiteralGenerator(private val ownership: Ownership, private val context: Co
         writable {
             val parts: Stream<Writable> =
                 value.accept(
-                    TemplateGenerator(ownership) { expr, ownership ->
-                        ExpressionGenerator(ownership, context).generate(expr)
-                    },
+                    TemplateGenerator(
+                        ownership,
+                        { expr, templateOwnership ->
+                            renderTemplateExpression(expr, templateOwnership)
+                        },
+                        { expr, templateOwnership ->
+                            renderMultipartExpression(expr, templateOwnership)
+                        },
+                    ),
                 )
             parts.forEach { part -> part(this) }
         }

codegen-client/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/rulesgen/TemplateGenerator.kt

@@ -33,6 +33,20 @@ import software.amazon.smithy.rust.codegen.core.util.dq
 class TemplateGenerator(
     private val ownership: Ownership,
     private val exprGenerator: (Expression, Ownership) -> Writable,
+    /**
+     * Optional separate generator used **only** for dynamic elements inside a multipart
+     * template (the `out.push_str(&...)` parts). Defaults to [exprGenerator].
+     *
+     * BDD codegen needs this distinction because:
+     *   - In a multipart template, an optional `&Option<String>` reference must be reduced
+     *     to `&str` (e.g. via `.as_deref().unwrap_or_default()`) so it fits `out.push_str(&...)`.
+     *   - In a single-dynamic template (`"{ref}"`), the whole template result is the inner
+     *     expression's value. If the outer caller wraps that in `if let Some(param) = ...`,
+     *     the inner expression must remain `Option`-typed, so the unwrap must NOT be applied.
+     *
+     * Tree-mode callers ignore this parameter and pass a single generator for both.
+     */
+    private val multipartElementGenerator: (Expression, Ownership) -> Writable = exprGenerator,
 ) : TemplateVisitor<Writable> {
     override fun visitStaticTemplate(value: String) =
         writable {
@@ -60,7 +74,7 @@ class TemplateGenerator(
         writable {
             // we don't need to own the argument to push_str
             Attribute.AllowClippyNeedlessBorrow.render(this)
-            rust("out.push_str(&#W);", exprGenerator(expr, Ownership.Borrowed))
+            rust("out.push_str(&#W);", multipartElementGenerator(expr, Ownership.Borrowed))
         }
 
     override fun startMultipartTemplate() =

codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/BddCodegenTest.kt

@@ -0,0 +1,109 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package software.amazon.smithy.rust.codegen.client.smithy.endpoint
+
+import org.junit.jupiter.api.Test
+import software.amazon.smithy.rust.codegen.client.testutil.clientIntegrationTest
+import software.amazon.smithy.rust.codegen.core.testutil.asSmithyModel
+
+/**
+ * Regression tests for BDD codegen bugs
+ *
+ * Bug #1 (`tryGenerateTrivialCondition.visitBoolEquals` does not gate on `ref.isOptional`) is
+ * unreachable through valid Smithy models — Smithy's own trait validator rejects
+ * `booleanEquals(OptionalBool, literal)` before codegen runs, so no test is included.
+ *
+ * Bug #3 (`isStringTypedRef` defaulting to `true` on typecheck failure) cannot be triggered
+ * through a normal Smithy model because the typechecker runs during model load. The fix makes
+ * the fallback conservative regardless; see the comment on `isStringTypedRef` in
+ * `BddExpressionGenerator.kt`.
+ */
+class BddCodegenTest {
+    /**
+     * Bug #2 regression: `BddExpressionGenerator.isOptionalArgument` previously text-matched a
+     * `StringLiteral`'s raw value against parameter names. When a static literal happened to
+     * have the same text as an optional parameter (e.g. literal `"Bucket"` while a parameter
+     * `Bucket` is also declared), `wrapDefaultArg` wrapped the literal's lowering — `&str` —
+     * in `if let Some(param) = ... { param } else { return false }`. That produces invalid
+     * Rust because `&str` does not pattern-match `Some(_)`.
+     *
+     * Fix: `isOptionalSingleDynamicReference` now inspects the [Template] structure via
+     * `TemplateVisitor.visitSingleDynamicTemplate` and only flags the argument as optional
+     * when it is genuinely a one-element `{Reference}` template wrapping an optional ref.
+     *
+     * The model below intentionally collides a literal with a parameter name and exercises a
+     * library function call (`substring("Bucket", 0, 4, false)`). After the fix, codegen
+     * compiles cleanly. Before the fix, it failed to compile with E0308
+     */
+    @Test
+    fun `string literal whose text matches an optional parameter name is not falsely treated as optional`() {
+        val nodes =
+            BddTestHelpers.encodeNodes(
+                listOf(
+                    // condition 0: high → result[1] (the endpoint), low → NoMatch terminal
+                    BddTestHelpers.Node(0, BddTestHelpers.resultRef(1), -1),
+                ),
+            )
+
+        val model =
+            """
+            namespace test
+
+            use aws.protocols#awsJson1_1
+            use smithy.rules#endpointBdd
+            use smithy.rules#clientContextParams
+
+            @awsJson1_1
+            @clientContextParams(
+                Bucket: { type: "string", documentation: "An optional bucket parameter" }
+            )
+            @endpointBdd({
+                version: "1.1",
+                "parameters": {
+                    "Bucket": {
+                        "type": "string",
+                        "required": false,
+                        "documentation": "An optional bucket parameter"
+                    }
+                },
+                "conditions": [
+                    {
+                        "fn": "substring",
+                        "argv": ["Bucket", 0, 4, false]
+                    }
+                ],
+                "results": [
+                    {
+                        "conditions": [],
+                        "endpoint": {
+                            "url": "https://parsed.example.com",
+                            "properties": {},
+                            "headers": {}
+                        },
+                        "type": "endpoint"
+                    }
+                ],
+                "root": 2,
+                "nodeCount": 2,
+                "nodes": "$nodes"
+            })
+            service TestService {
+                version: "1.0",
+                operations: [TestOp]
+            }
+
+            operation TestOp {
+                input: TestInput
+            }
+
+            structure TestInput {}
+            """.asSmithyModel()
+
+        // Should compile cleanly. Before the fix, this threw CommandError with
+        // `mismatched types: expected str, found Option<_>` from the wrap.
+        clientIntegrationTest(model)
+    }
+}

codegen-client/src/test/kotlin/software/amazon/smithy/rust/codegen/client/smithy/endpoint/BddTestHelpers.kt

@@ -0,0 +1,47 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package software.amazon.smithy.rust.codegen.client.smithy.endpoint
+
+import java.nio.ByteBuffer
+import java.nio.ByteOrder
+import java.util.Base64
+
+/**
+ * Helpers for assembling minimal `@endpointBdd` traits inline in codegen tests.
+ *
+ * The BDD trait's `nodes` field is a base64-encoded sequence of signed 32-bit
+ * **big-endian** integer triples: `[conditionIndex, highRef, lowRef]`. A reference
+ * of `1` or `-1` is the NoMatch terminal; `>=100_000_000` encodes a result at
+ * `results[ref - 100_000_000]`. The first node (index 0) **must** be the
+ * canonical terminal `[-1, 1, -1]`.
+ *
+ */
+internal object BddTestHelpers {
+    /** Triple of (conditionIndex, highRef, lowRef) for one BDD node. */
+    data class Node(val conditionIndex: Int, val high: Int, val low: Int)
+
+    /** The canonical terminal node that must occupy index 0 of every BDD. */
+    val TERMINAL = Node(-1, 1, -1)
+
+    /** Result reference for `results[index]`. Index 0 is the implicit NoMatchRule. */
+    fun resultRef(index: Int): Int = 100_000_000 + index
+
+    /**
+     * Encodes a list of nodes as the base64 string the BDD trait expects.
+     * The first node is automatically the terminal — callers pass only the
+     * non-terminal nodes.
+     */
+    fun encodeNodes(nodesAfterTerminal: List<Node>): String {
+        val all = listOf(TERMINAL) + nodesAfterTerminal
+        val buf = ByteBuffer.allocate(all.size * 12).order(ByteOrder.BIG_ENDIAN)
+        all.forEach { n ->
+            buf.putInt(n.conditionIndex)
+            buf.putInt(n.high)
+            buf.putInt(n.low)
+        }
+        return Base64.getEncoder().encodeToString(buf.array())
+    }
+}