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

Do not generate public empty modules #1803

Merged
merged 8 commits into from
Oct 4, 2022
Merged

Do not generate public empty modules #1803

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ce22621
Mark `operation_handler` module as private in the generated server co…
LukeMathWalker Oct 3, 2022
17d87fb
Add changelog.next entry.
LukeMathWalker Oct 3, 2022
afafbcc
Rename `operationModule` to `operation`.
LukeMathWalker Oct 3, 2022
3e5a56c
Ensure that `operation` is added to the list of public modules for th…
LukeMathWalker Oct 3, 2022
8f1b0a8
Remove DefaultModules from codegen-core
LukeMathWalker Oct 4, 2022
b836237
Merge branch 'main' into fix-public-empty-modules
LukeMathWalker Oct 4, 2022
91533fe
Fix Python server.
LukeMathWalker Oct 4, 2022
a98d41e
Expose TestDefaultPublicModules.
LukeMathWalker Oct 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.next.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@
# references = ["smithy-rs#920"]
# meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"}
# author = "rcoh"

[[smithy-rs]]
message = """
Mark `operation` and `operation_handler` modules as private in the generated server crate.
Both modules did not contain any public types, therefore there should be no actual breakage when updating.
"""
references = ["smithy-rs#1803"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "server"}
author = "LukeMathWalker"

[[smithy-rs]]
message = "Pokémon Service example code now runs clippy during build."
references = ["smithy-rs#1727"]
Expand Down
6 changes: 5 additions & 1 deletion ...lient/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/CodegenVisitor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ 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.rustlang.Visibility
import software.amazon.smithy.rust.codegen.core.smithy.DefaultPublicModules
import software.amazon.smithy.rust.codegen.core.smithy.RustCrate
import software.amazon.smithy.rust.codegen.core.smithy.RustSymbolProvider
Expand Down Expand Up @@ -81,10 +83,12 @@ class CodegenVisitor(
symbolProvider = RustCodegenPlugin.baseSymbolProvider(model, service, symbolVisitorConfig)

codegenContext = ClientCodegenContext(model, symbolProvider, service, protocol, settings)
val publicModules = DefaultPublicModules.toMutableMap()
publicModules["operation"] = RustModule.operation(Visibility.PUBLIC)
rustCrate = RustCrate(
context.fileManifest,
symbolProvider,
DefaultPublicModules,
publicModules,
codegenContext.settings.codegenConfig,
)
protocolGenerator = protocolGeneratorFactory.buildProtocolGenerator(codegenContext)
Expand Down
3 changes: 2 additions & 1 deletion ...zon/smithy/rust/codegen/client/smithy/generators/client/CustomizableOperationGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.GenericTypeArg
import software.amazon.smithy.rust.codegen.core.rustlang.RustGenerics
import software.amazon.smithy.rust.codegen.core.rustlang.RustModule
import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
import software.amazon.smithy.rust.codegen.core.rustlang.Visibility
import software.amazon.smithy.rust.codegen.core.rustlang.asType
import software.amazon.smithy.rust.codegen.core.rustlang.docs
import software.amazon.smithy.rust.codegen.core.rustlang.rust
Expand All @@ -34,7 +35,7 @@ class CustomizableOperationGenerator(
private val smithyTypes = CargoDependency.SmithyTypes(runtimeConfig).asType()

fun render(crate: RustCrate) {
crate.withModule(RustModule.Operation) { writer ->
crate.withModule(RustModule.operation(Visibility.PUBLIC)) { writer ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have expected this to be enough to get the module to show up in lib.rs as pub mod operation, but apparently it wasn't - I had to add https://github.com/awslabs/smithy-rs/pull/1803/files#diff-f7da198414ffe3cefc591164d3b1bb41d67e22812d7ba3a86b8abd6d798cec49R87
I'd be curious to understand why.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From reading the code, it definitely seems like it should emit the mod statement when it's missing from DefaultPublicModules.

writer.docs("Operation customization and supporting types")
writer.rust("pub mod customize;")
}
Expand Down
8 changes: 7 additions & 1 deletion codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustModule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ data class RustModule(val name: String, val rustMetadata: RustMetadata, val docu

val Config = public("config", documentation = "Configuration for the service.")
val Error = public("error", documentation = "All error types that operations can return.")
val Operation = public("operation", documentation = "All operations that this crate can perform.")

/**
* Helper method to generate the `operation` Rust module.
* Its visibility depends on the generation context (client or server).
*/
fun operation(visibility: Visibility): RustModule =
default("operation", visibility = visibility, documentation = "All operations that this crate can perform.")
}
}
1 change: 0 additions & 1 deletion ...-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/CodegenDelegator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ open class RustCrate(
*/
val DefaultPublicModules = setOf(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a better approach is to move DefaultPublicModules into the client and server modules, since it doesn't seem like there's a good reason to share it in codegen-core. This would also allow us to tailor the module docs to the client/servers separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 8f1b0a8 I removed DefaultPublicModules and gave client/server/test the responsibility to handle their own list of default public modules.
I haven't inlined the definitions of the various module in those lists, but happy to do so if you think that's useful at this stage.

RustModule.Error,
RustModule.Operation,
RustModule.public("model", documentation = "Data structures used by operation inputs/outputs."),
RustModule.public("input", documentation = "Input structures for operations."),
RustModule.public("output", documentation = "Output structures for operations."),
Expand Down
5 changes: 2 additions & 3 deletions ...in/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerServiceGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import software.amazon.smithy.rust.codegen.core.rustlang.RustModule
import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
import software.amazon.smithy.rust.codegen.core.rustlang.Visibility
import software.amazon.smithy.rust.codegen.core.smithy.CodegenContext
import software.amazon.smithy.rust.codegen.core.smithy.DefaultPublicModules
import software.amazon.smithy.rust.codegen.core.smithy.RustCrate
import software.amazon.smithy.rust.codegen.core.smithy.generators.protocol.ProtocolSupport
import software.amazon.smithy.rust.codegen.server.smithy.generators.protocol.ServerProtocol
Expand Down Expand Up @@ -41,7 +40,7 @@ open class ServerServiceGenerator(
* which assigns a symbol location to each shape.
*/
fun render() {
rustCrate.withModule(DefaultPublicModules["operation"]!!) { writer ->
rustCrate.withModule(RustModule.operation(Visibility.PRIVATE)) { writer ->
ServerProtocolTestGenerator(codegenContext, protocolSupport, protocolGenerator).render(writer)
}

Expand All @@ -52,7 +51,7 @@ open class ServerServiceGenerator(
}
}
}
rustCrate.withModule(RustModule.public("operation_handler", "Operation handlers definition and implementation.")) { writer ->
rustCrate.withModule(RustModule.private("operation_handler", "Operation handlers definition and implementation.")) { writer ->
renderOperationHandler(writer, operations)
}
rustCrate.withModule(
Expand Down