Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make modules in codegen-core configurable #2336

Merged
merged 11 commits into from
Feb 10, 2023
Merged

Make modules in codegen-core configurable #2336

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bf531c3
Refactor modules to be configurable in `codegen-core`
jdisanti Feb 6, 2023
9515458
Fix `codegen-client`
jdisanti Feb 6, 2023
ede5f87
Fix `codegen-server`
jdisanti Feb 6, 2023
473a0bc
Fix `codegen-server:python`
jdisanti Feb 8, 2023
24315bc
Fix `aws:sdk-codegen`
jdisanti Feb 8, 2023
f421475
Remove panicking default test symbol provider
jdisanti Feb 8, 2023
cc0dde9
Remove as many references to Error/Types as possible
jdisanti Feb 8, 2023
4fe8994
Rename module constants
jdisanti Feb 9, 2023
74204fa
Merge remote-tracking branch 'origin/main' into jdisanti-core-configu…
jdisanti Feb 9, 2023
ad51645
Incorporate feedback
jdisanti Feb 10, 2023
94a9784
Merge branch 'main' into jdisanti-core-configurable-modules
jdisanti Feb 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsFluentClientDecorator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ private class AwsClientGenerics(private val types: Types) : FluentClientGenerics
override fun sendBounds(
operation: Symbol,
operationOutput: Symbol,
operationError: RuntimeType,
operationError: Symbol,
retryClassifier: RuntimeType,
): Writable =
writable { }
Expand Down
3 changes: 1 addition & 2 deletions aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsPresigningDecorator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ 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.customize.OperationCustomization
import software.amazon.smithy.rust.codegen.core.smithy.customize.OperationSection
import software.amazon.smithy.rust.codegen.core.smithy.generators.error.errorSymbol
import software.amazon.smithy.rust.codegen.core.smithy.protocols.HttpBoundProtocolPayloadGenerator
import software.amazon.smithy.rust.codegen.core.util.cloneOperation
import software.amazon.smithy.rust.codegen.core.util.expectTrait
Expand Down Expand Up @@ -155,7 +154,7 @@ class AwsInputPresignedMethod(
}

private fun RustWriter.writeInputPresignedMethod(section: OperationSection.InputImpl) {
val operationError = operationShape.errorSymbol(symbolProvider)
val operationError = symbolProvider.symbolForOperationError(operationShape)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about our symbol provider, I dislike this relationship of having to pass it around and then passing things into it.

When I think about operations, inputs, outputs, etc. I see them as the chief concepts to organize around and I'm thinking my mental model doesn't fit well with smithy's model of codegen.

In my mind, an operation is a core concept and has several relationships and representations.

Relationships include:

  • an input
  • an output
  • an error
  • members
  • stuff specific to an operation's protocol, like a special event stream representation (not sure about this one)
  • more stuff I haven't thought of

and representations include:

  • as a Rust type name
  • as a Rust struct declaration (and possibly an impl too)
  • as a smithy shape
  • as a smithy identifier

When I see code like this were we transform the operation by passing it into something else, I realize that my preference would be to decide the relation I want, and then the representation of that related concept i.e.

// Symbols feel like an implementation detail to me so I could
// also see the final method being called toWritable, toRuntimeType, or similar
val operationErrorSymbol = operation.error().toSymbol()

// render the symbol alone
operationErrorSymbol.render(writer)

// or render it within a writer
writer.rust("let err = #T::new();", operationErrorSymbol)

My question then is: Should I be changing my mental model or should we be changing codegen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem arises that Smithy doesn't really have a concept of an operation error. The operation has errors, but the fact that there is an operation error type is an implementation detail of smithy-rs and the SDK. So that operation.error() would need to return something non-Smithy if we were to take that approach. Other than that, things like toSymbol() will need to take a symbol provider as an argument, or alternatively we delve into some questionable global variable practices. There are certain Smithy patterns that will get more strict over time rather than less strict.

Something we can consider is making an abstraction layer on top of Smithy, but it would be expensive to refactor to that, and it would make Smithy upgrades more expensive in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spoke with John about this outside of GitHub. Including some notes here for posterity:

My assumptions

Smithy is a modelling language for defining a service that responds to requests.

  • A model defines data structures representing requests, responses, and errors.
  • An operation is a data structure that associates a request, response, and a zero or more errors.
  • Requests, responses, and errors are defined as objects containing key-value pairs. Values may be aggregate types (like maps and lists) or primitive types. Collectively, objects and types are referred to as "shapes."
  • Shapes may be annotated with one or more traits that influence codegen.
  • During codegen, we may create synthetic shapes and traits and attach them to the model. We must do this in cases where expected behavior isn't modeled. Additionally, we may do this in cases where we need to share information between individual codegen components or where modeled data must be transformed or augmented before it can be converted into Rust code.
  • To generate code, shapes must be converted into symbols by a symbol provider. The symbol provider ensures no naming or keyword collisions occur when generating code.
  • We process smithy models to generate a client. The client allows users to construct operation input structs, which are then converted into an HTTP request and sent. When an HTTP response is received, it is converted into either an error struct or an operation output struct.
  • Structs are a Rust concept, not a Smithy one. Because Rust does not support union types, and because an operation in Smithy may be associated with 0 or more possible errors, we aggregate those errors into an opaque error struct which must be assigned a name by the symbol provider.

Notes from our discussion

  • RuntimeType shouldn't really exist. We should instead merge the good parts of it into the Symbol class.
  • smithy-rs is not a general-purpose library for writing Kotlin programs that generate Rust. Rather, it's a tool for converting smithy models in Rust code that's usable by our runtime crates.
  • Symbols represent types, but we shouldn't extend them to represent syntactic things like "a block of code" or "a variable name." Instead, we should decide whether we need an abstraction to represent those things or should stick to formatting strings.
  • We should devote a small amount of time to investigating cases where pushing back the "writing a symbol into a string" phase of codegen would make codegen simpler to extend and maintain. We already do this with the "section" customization APIs. Do we need anything more?

val presignableOp = PRESIGNABLE_OPERATIONS.getValue(operationShape.id)

val makeOperationOp = if (presignableOp.hasModelTransforms()) {
Expand Down
4 changes: 2 additions & 2 deletions aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/SdkConfigDecorator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
package software.amazon.smithy.rustsdk

import software.amazon.smithy.rust.codegen.client.smithy.ClientCodegenContext
import software.amazon.smithy.rust.codegen.client.smithy.ClientRustModule
import software.amazon.smithy.rust.codegen.client.smithy.customize.ClientCodegenDecorator
import software.amazon.smithy.rust.codegen.client.smithy.generators.config.ConfigCustomization
import software.amazon.smithy.rust.codegen.client.smithy.generators.config.ServiceConfig
import software.amazon.smithy.rust.codegen.core.rustlang.RustModule
import software.amazon.smithy.rust.codegen.core.rustlang.Writable
import software.amazon.smithy.rust.codegen.core.rustlang.rust
import software.amazon.smithy.rust.codegen.core.rustlang.rustTemplate
Expand Down Expand Up @@ -105,7 +105,7 @@ class SdkConfigDecorator : ClientCodegenDecorator {
val codegenScope = arrayOf(
"SdkConfig" to AwsRuntimeType.awsTypes(codegenContext.runtimeConfig).resolve("sdk_config::SdkConfig"),
)
rustCrate.withModule(RustModule.Config) {
rustCrate.withModule(ClientRustModule.Config) {
rustTemplate(
"""
impl From<&#{SdkConfig}> for Builder {
Expand Down
36 changes: 10 additions & 26 deletions ...src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientCodegenVisitor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import software.amazon.smithy.rust.codegen.client.smithy.generators.protocol.Cli
import software.amazon.smithy.rust.codegen.client.smithy.protocols.ClientProtocolLoader
import software.amazon.smithy.rust.codegen.client.smithy.transformers.AddErrorMessage
import software.amazon.smithy.rust.codegen.client.smithy.transformers.RemoveEventStreamOperations
import software.amazon.smithy.rust.codegen.core.rustlang.RustModule
import software.amazon.smithy.rust.codegen.core.smithy.DirectedWalker
import software.amazon.smithy.rust.codegen.core.smithy.RustCrate
import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider
Expand All @@ -33,15 +32,12 @@ import software.amazon.smithy.rust.codegen.core.smithy.generators.BuilderGenerat
import software.amazon.smithy.rust.codegen.core.smithy.generators.StructureGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.UnionGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.error.OperationErrorGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.error.eventStreamErrorSymbol
import software.amazon.smithy.rust.codegen.core.smithy.generators.implBlock
import software.amazon.smithy.rust.codegen.core.smithy.protocols.ProtocolGeneratorFactory
import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticInputTrait
import software.amazon.smithy.rust.codegen.core.smithy.transformers.EventStreamNormalizer
import software.amazon.smithy.rust.codegen.core.smithy.transformers.OperationNormalizer
import software.amazon.smithy.rust.codegen.core.smithy.transformers.RecursiveShapeBoxer
import software.amazon.smithy.rust.codegen.core.smithy.transformers.eventStreamErrors
import software.amazon.smithy.rust.codegen.core.smithy.transformers.operationErrors
import software.amazon.smithy.rust.codegen.core.util.CommandFailed
import software.amazon.smithy.rust.codegen.core.util.hasTrait
import software.amazon.smithy.rust.codegen.core.util.isEventStream
Expand All @@ -56,7 +52,6 @@ class ClientCodegenVisitor(
context: PluginContext,
private val codegenDecorator: ClientCodegenDecorator,
) : ShapeVisitor.Default<Unit>() {

private val logger = Logger.getLogger(javaClass.name)
private val settings = ClientRustSettings.from(context.model, context.settings)

Expand All @@ -69,12 +64,12 @@ class ClientCodegenVisitor(
private val protocolGenerator: ClientProtocolGenerator

init {
val symbolVisitorConfig =
SymbolVisitorConfig(
runtimeConfig = settings.runtimeConfig,
renameExceptions = settings.codegenConfig.renameExceptions,
nullabilityCheckMode = NullableIndex.CheckMode.CLIENT_ZERO_VALUE_V1,
)
val symbolVisitorConfig = SymbolVisitorConfig(
runtimeConfig = settings.runtimeConfig,
renameExceptions = settings.codegenConfig.renameExceptions,
nullabilityCheckMode = NullableIndex.CheckMode.CLIENT_ZERO_VALUE_V1,
moduleProvider = ClientModuleProvider,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module provider is a cool idea

)
val baseModel = baselineTransform(context.model)
val untransformedService = settings.getService(baseModel)
val (protocol, generator) = ClientProtocolLoader(
Expand Down Expand Up @@ -224,13 +219,8 @@ class ClientCodegenVisitor(
UnionGenerator(model, symbolProvider, this, shape, renderUnknownVariant = true).render()
}
if (shape.isEventStream()) {
rustCrate.withModule(RustModule.Error) {
val symbol = symbolProvider.toSymbol(shape)
val errors = shape.eventStreamErrors()
.map { model.expectShape(it.asMemberShape().get().target, StructureShape::class.java) }
val errorSymbol = shape.eventStreamErrorSymbol(symbolProvider)
OperationErrorGenerator(model, symbolProvider, symbol, errors)
.renderErrors(this, errorSymbol, symbol)
rustCrate.withModule(ClientRustModule.Error) {
OperationErrorGenerator(model, symbolProvider, shape).render(this)
}
}
}
Expand All @@ -239,14 +229,8 @@ class ClientCodegenVisitor(
* Generate errors for operation shapes
*/
override fun operationShape(shape: OperationShape) {
rustCrate.withModule(RustModule.Error) {
val operationSymbol = symbolProvider.toSymbol(shape)
OperationErrorGenerator(
model,
symbolProvider,
operationSymbol,
shape.operationErrors(model).map { it.asStructureShape().get() },
).render(this)
rustCrate.withModule(ClientRustModule.Error) {
OperationErrorGenerator(model, symbolProvider, shape).render(this)
}
Comment on lines +232 to 234
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my mental model comment, I'm wondering if this generator functionality provided by OperationErrorGenerator could be encapsulated within some operation class. Then we'd render stuff returned by the class methods to make it slightly clearer at this call-site what's getting written out:

rustCrate.withModule(ClientRustModule.Error) {
    operation.structDeclaration().render(this)
    operation.structImpl().render(this)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep generation separate from the modeling still, in case structs need to be generated completely differently for other use cases.

}
}
59 changes: 59 additions & 0 deletions ...ent/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientRustModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

package software.amazon.smithy.rust.codegen.client.smithy

import software.amazon.smithy.model.shapes.OperationShape
import software.amazon.smithy.model.shapes.Shape
import software.amazon.smithy.model.shapes.StructureShape
import software.amazon.smithy.model.shapes.UnionShape
import software.amazon.smithy.model.traits.ErrorTrait
import software.amazon.smithy.rust.codegen.core.rustlang.RustModule
import software.amazon.smithy.rust.codegen.core.smithy.ModuleProvider
import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticInputTrait
import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticOutputTrait
import software.amazon.smithy.rust.codegen.core.util.hasTrait

/**
* Modules for code generated client crates.
*/
object ClientRustModule {
/** crate::client */
val client = Client.self
object Client {
/** crate::client */
val self = RustModule.public("client", "Client and fluent builders for calling the service.")

/** crate::client::customize */
val customize = RustModule.public("customize", "Operation customization and supporting types", parent = self)
}

val Config = RustModule.public("config", documentation = "Configuration for the service.")
val Error = RustModule.public("error", documentation = "All error types that operations can return. Documentation on these types is copied from the model.")
val Operation = RustModule.public("operation", documentation = "All operations that this crate can perform.")
val Model = RustModule.public("model", documentation = "Data structures used by operation inputs/outputs. Documentation on these types is copied from the model.")
val Input = RustModule.public("input", documentation = "Input structures for operations. Documentation on these types is copied from the model.")
val Output = RustModule.public("output", documentation = "Output structures for operations. Documentation on these types is copied from the model.")
val Types = RustModule.public("types", documentation = "Data primitives referenced by other data types.")
}

object ClientModuleProvider : ModuleProvider {
override fun moduleForShape(shape: Shape): RustModule.LeafModule = when (shape) {
is OperationShape -> ClientRustModule.Operation
is StructureShape -> when {
shape.hasTrait<ErrorTrait>() -> ClientRustModule.Error
shape.hasTrait<SyntheticInputTrait>() -> ClientRustModule.Input
shape.hasTrait<SyntheticOutputTrait>() -> ClientRustModule.Output
else -> ClientRustModule.Model
}
else -> ClientRustModule.Model
}
Comment on lines +43 to +52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this declarative approach! It's so clear and gives you a great idea of the resulting Rust crate's structure


override fun moduleForOperationError(operation: OperationShape): RustModule.LeafModule =
ClientRustModule.Error

override fun moduleForEventStreamError(eventStream: UnionShape): RustModule.LeafModule =
ClientRustModule.Error
}
4 changes: 2 additions & 2 deletions .../amazon/smithy/rust/codegen/client/smithy/customizations/ResiliencyConfigCustomization.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@

package software.amazon.smithy.rust.codegen.client.smithy.customizations

import software.amazon.smithy.rust.codegen.client.smithy.ClientRustModule
import software.amazon.smithy.rust.codegen.client.smithy.generators.config.ConfigCustomization
import software.amazon.smithy.rust.codegen.client.smithy.generators.config.ServiceConfig
import software.amazon.smithy.rust.codegen.core.rustlang.RustModule
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.CodegenContext
Expand Down Expand Up @@ -235,7 +235,7 @@ class ResiliencyConfigCustomization(codegenContext: CodegenContext) : ConfigCust

class ResiliencyReExportCustomization(private val runtimeConfig: RuntimeConfig) {
fun extras(rustCrate: RustCrate) {
rustCrate.withModule(RustModule.Config) {
rustCrate.withModule(ClientRustModule.Config) {
rustTemplate(
"""
pub use #{sleep}::{AsyncSleep, Sleep};
Expand Down
5 changes: 4 additions & 1 deletion ...lin/software/amazon/smithy/rust/codegen/client/smithy/customize/RequiredCustomizations.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package software.amazon.smithy.rust.codegen.client.smithy.customize

import software.amazon.smithy.model.shapes.OperationShape
import software.amazon.smithy.rust.codegen.client.smithy.ClientCodegenContext
import software.amazon.smithy.rust.codegen.client.smithy.ClientRustModule
import software.amazon.smithy.rust.codegen.client.smithy.customizations.EndpointPrefixGenerator
import software.amazon.smithy.rust.codegen.client.smithy.customizations.HttpChecksumRequiredGenerator
import software.amazon.smithy.rust.codegen.client.smithy.customizations.HttpVersionListCustomization
Expand Down Expand Up @@ -65,6 +66,8 @@ class RequiredCustomizations : ClientCodegenDecorator {
// Re-export resiliency types
ResiliencyReExportCustomization(codegenContext.runtimeConfig).extras(rustCrate)

pubUseSmithyTypes(codegenContext.runtimeConfig, codegenContext.model, rustCrate)
rustCrate.withModule(ClientRustModule.Types) {
pubUseSmithyTypes(codegenContext.runtimeConfig, codegenContext.model)(this)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer if we chose one convention for rendering things outside of a rust writer template:
a. writableThing(params).render(this)
b. writableThing(params)(this)

I like seeing render because it's very explicit what's happening.

It only now occurs to me that we're calling a function to render things here so I guess what I'm really saying is:

"should we render from free functions, or only by calling methods?"

Perhaps this is a nitpick.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been wanting to refactor Writable from a typealias RustWriter.() -> Unit to an interface for a long time... and doing so would enable this to be more readable.

}
}
}
3 changes: 1 addition & 2 deletions ...kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/PaginatorGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import software.amazon.smithy.rust.codegen.core.smithy.CodegenContext
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider
import software.amazon.smithy.rust.codegen.core.smithy.generators.builderSymbol
import software.amazon.smithy.rust.codegen.core.smithy.generators.error.errorSymbol
import software.amazon.smithy.rust.codegen.core.smithy.rustType
import software.amazon.smithy.rust.codegen.core.util.PANIC
import software.amazon.smithy.rust.codegen.core.util.findMemberWithTrait
Expand Down Expand Up @@ -79,7 +78,7 @@ class PaginatorGenerator private constructor(
private val inputType = symbolProvider.toSymbol(operation.inputShape(model))
private val outputShape = operation.outputShape(model)
private val outputType = symbolProvider.toSymbol(outputShape)
private val errorType = operation.errorSymbol(symbolProvider)
private val errorType = symbolProvider.symbolForOperationError(operation)

private fun paginatorType(): RuntimeType = RuntimeType.forInlineFun(
paginatorName,
Expand Down
4 changes: 2 additions & 2 deletions ...n/kotlin/software/amazon/smithy/rust/codegen/client/smithy/generators/ServiceGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ package software.amazon.smithy.rust.codegen.client.smithy.generators

import software.amazon.smithy.model.knowledge.TopDownIndex
import software.amazon.smithy.rust.codegen.client.smithy.ClientCodegenContext
import software.amazon.smithy.rust.codegen.client.smithy.ClientRustModule
import software.amazon.smithy.rust.codegen.client.smithy.customize.ClientCodegenDecorator
import software.amazon.smithy.rust.codegen.client.smithy.generators.config.ServiceConfigGenerator
import software.amazon.smithy.rust.codegen.client.smithy.generators.protocol.ClientProtocolGenerator
import software.amazon.smithy.rust.codegen.client.smithy.generators.protocol.ProtocolTestGenerator
import software.amazon.smithy.rust.codegen.core.rustlang.Attribute
import software.amazon.smithy.rust.codegen.core.rustlang.RustModule
import software.amazon.smithy.rust.codegen.core.smithy.RustCrate
import software.amazon.smithy.rust.codegen.core.smithy.generators.error.ServiceErrorGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.protocol.ProtocolSupport
Expand Down Expand Up @@ -58,7 +58,7 @@ class ServiceGenerator(

ServiceErrorGenerator(clientCodegenContext, operations).render(rustCrate)

rustCrate.withModule(RustModule.Config) {
rustCrate.withModule(ClientRustModule.Config) {
ServiceConfigGenerator.withBaseBehavior(
clientCodegenContext,
extraCustomizations = decorator.configCustomizations(clientCodegenContext, listOf()),
Expand Down
3 changes: 2 additions & 1 deletion ...tware/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientDecorator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

package software.amazon.smithy.rust.codegen.client.smithy.generators.client

import software.amazon.smithy.codegen.core.Symbol
import software.amazon.smithy.model.shapes.OperationShape
import software.amazon.smithy.model.shapes.ServiceShape
import software.amazon.smithy.rust.codegen.client.smithy.ClientCodegenContext
Expand Down Expand Up @@ -65,7 +66,7 @@ sealed class FluentClientSection(name: String) : Section(name) {
/** Write custom code into an operation fluent builder's impl block */
data class FluentBuilderImpl(
val operationShape: OperationShape,
val operationErrorType: RuntimeType,
val operationErrorType: Symbol,
Comment on lines -68 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer using a type we control in these cases. What if we updated the RustType writable impl to magically work with the symbol provider? Do we ever have more than one symbol provider in use during codegen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem is, we don't have a RuntimeType when we move to determining modules based on the shape and symbol provider, so it isn't really feasible to preserve this being a RuntimeType. Symbol is kind of the lowest common denominator since everything can convert into it, and it is ultimately the renderable type.

I don't think we really need a RuntimeType here though. We only use this for rendering.

) : FluentClientSection("FluentBuilderImpl")

/** Write custom code into the docs */
Expand Down
7 changes: 3 additions & 4 deletions ...tware/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider
import software.amazon.smithy.rust.codegen.core.smithy.customize.writeCustomizations
import software.amazon.smithy.rust.codegen.core.smithy.expectRustMetadata
import software.amazon.smithy.rust.codegen.core.smithy.generators.builderSymbol
import software.amazon.smithy.rust.codegen.core.smithy.generators.error.errorSymbol
import software.amazon.smithy.rust.codegen.core.smithy.generators.setterName
import software.amazon.smithy.rust.codegen.core.smithy.rustType
import software.amazon.smithy.rust.codegen.core.util.inputShape
Expand Down Expand Up @@ -168,7 +167,7 @@ class FluentClientGenerator(

val output = operation.outputShape(model)
val operationOk = symbolProvider.toSymbol(output)
val operationErr = operation.errorSymbol(symbolProvider).toSymbol()
val operationErr = symbolProvider.symbolForOperationError(operation)

val inputFieldsBody =
generateOperationShapeDocs(writer, symbolProvider, operation, model).joinToString("\n") {
Expand Down Expand Up @@ -263,7 +262,7 @@ class FluentClientGenerator(
"bounds" to generics.bounds,
) {
val outputType = symbolProvider.toSymbol(operation.outputShape(model))
val errorType = operation.errorSymbol(symbolProvider)
val errorType = symbolProvider.symbolForOperationError(operation)

// Have to use fully-qualified result here or else it could conflict with an op named Result
rustTemplate(
Expand Down Expand Up @@ -333,7 +332,7 @@ class FluentClientGenerator(
customizations,
FluentClientSection.FluentBuilderImpl(
operation,
operation.errorSymbol(symbolProvider),
symbolProvider.symbolForOperationError(operation),
),
)
input.members().forEach { member ->
Expand Down
4 changes: 2 additions & 2 deletions ...ftware/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerics.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ interface FluentClientGenerics {
val bounds: Writable

/** Bounds for generated `send()` functions */
fun sendBounds(operation: Symbol, operationOutput: Symbol, operationError: RuntimeType, retryClassifier: RuntimeType): Writable
fun sendBounds(operation: Symbol, operationOutput: Symbol, operationError: Symbol, retryClassifier: RuntimeType): Writable

/** Convert this `FluentClientGenerics` into the more general `RustGenerics` */
fun toRustGenerics(): RustGenerics
Expand Down Expand Up @@ -70,7 +70,7 @@ data class FlexibleClientGenerics(
}

/** Bounds for generated `send()` functions */
override fun sendBounds(operation: Symbol, operationOutput: Symbol, operationError: RuntimeType, retryClassifier: RuntimeType): Writable = writable {
override fun sendBounds(operation: Symbol, operationOutput: Symbol, operationError: Symbol, retryClassifier: RuntimeType): Writable = writable {
rustTemplate(
"""
where
Expand Down
8 changes: 2 additions & 6 deletions ...ware/amazon/smithy/rust/codegen/client/smithy/generators/http/ResponseBindingGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,7 @@ class ResponseBindingGenerator(

fun generateDeserializePayloadFn(
binding: HttpBindingDescriptor,
errorT: RuntimeType,
errorSymbol: Symbol,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we sometimes suffix these param names with Type or Symbol. I think we should pick one style and make sure we use it everywhere.

Alternatively, we could discourage the use of suffixes that note types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. We should use -Symbol for symbols and -Type for RuntimeTypes and RustTypes. It's easy to miss these when refactoring though.

payloadParser: RustWriter.(String) -> Unit,
): RuntimeType = httpBindingGenerator.generateDeserializePayloadFn(
binding,
errorT,
payloadParser,
)
): RuntimeType = httpBindingGenerator.generateDeserializePayloadFn(binding, errorSymbol, payloadParser)
}