Make modules in codegen-core configurable

[jdisanti]

Motivation and Context

This PR is a prerequisite for implementing RFC-26. After these changes, every core module except for types and error are fully configurable. Making types and error fully configurable is blocked on #2129 and #2334.

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

A new generated diff is ready to view.

• AWS SDK (ignoring whitespace)
• No codegen difference in the Server Test
• No codegen difference in the Server Test Python

A new doc preview is ready to view.

[crisidev]

I really enjoyed going over this PR. Nice work, it promotes better abstractions!

The PR is pretty long and while it doesn't basically have effect on the generated code, it will requires me to go over it again to be sure I did not miss anything.

[crisidev]

This is not something I would expect to see in this PR, but in the server we generate other modules and I am wondering if it would make sense to add them here rather than what we are doing today..

[jdisanti]

At least on the client side, I'm intending to make this a hierarchical "database" of code generated modules. I think it's a good idea to centralize these so that you don't get conflicting documentation. I'm not sure which documentation would win in that case.

[github-actions]

A new generated diff is ready to view.

• AWS SDK (ignoring whitespace)
• No codegen difference in the Server Test
• No codegen difference in the Server Test Python

A new doc preview is ready to view.

[Velfi]

The module provider is a cool idea

[Velfi]

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?

[jdisanti]

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.

[Velfi]

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?

[Velfi]

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

[jdisanti]

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

[Velfi]

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

[Velfi]

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.

[jdisanti]

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.

[Velfi]

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?

[jdisanti]

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.

[Velfi]

this is a cool fn

[Velfi]

It would be kind of neat if you could do:

Suggested change

	output.shouldContain("#[non_exhaustive]")
	// on enum variant `T2Micro`
	output.shouldContain("#[deprecated]")
	// on enum itself
	output.shouldContain("#[deprecated(since = \"1.2.3\")]")
	output.shouldContain(Attribute.NonExhaustive)
	// on enum variant `T2Micro`
	output.shouldContain(Attribute.Deprecated())
	// on enum itself
	output.shouldContain(Attribute.deprecated(since = "1.2.3"))

you'd have to overload shouldContain though

[github-actions]

A new generated diff is ready to view.

• AWS SDK (ignoring whitespace)
• No codegen difference in the Server Test
• No codegen difference in the Server Test Python

A new doc preview is ready to view.

[github-actions]

A new generated diff is ready to view.

• AWS SDK (ignoring whitespace)
• No codegen difference in the Server Test
• No codegen difference in the Server Test Python

A new doc preview is ready to view.