Skip to content

Commit

Permalink
Fix client operation name collisions with the standard library prelude
Browse files Browse the repository at this point in the history
Operations named `Send` or `Sync` (and probably others) were colliding
with the types in the standard library prelude and causing compiler
errors. This PR adds tests that include all the type names from the Rust
prelude, and fixes the compiler errors they cause.

In the future, the `no_implicit_prelude` attribute can be added to
certain code generated modules to better enforce that there can't be
name collisions, but for now, the `tokio::test` macro doesn't compile
with that attribute enabled (and likely other macros from other
libraries).
  • Loading branch information
jdisanti committed May 12, 2023
1 parent 0e90d65 commit 17d4b71
Show file tree
Hide file tree
Showing 31 changed files with 540 additions and 173 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ private fun renderCustomizableOperationSendMethod(
val combinedGenerics = operationGenerics + handleGenerics

val codegenScope = arrayOf(
*RuntimeType.preludeScope,
"combined_generics_decl" to combinedGenerics.declaration(),
"handle_generics_bounds" to handleGenerics.bounds(),
"SdkSuccess" to RuntimeType.sdkSuccess(runtimeConfig),
Expand All @@ -238,11 +239,11 @@ private fun renderCustomizableOperationSendMethod(
#{handle_generics_bounds:W}
{
/// Sends this operation's request
pub async fn send<T, E>(self) -> Result<T, SdkError<E>>
pub async fn send<T, E>(self) -> #{Result}<T, SdkError<E>>
where
E: std::error::Error + Send + Sync + 'static,
O: #{ParseHttpResponse}<Output = Result<T, E>> + Send + Sync + Clone + 'static,
Retry: #{ClassifyRetry}<#{SdkSuccess}<T>, #{SdkError}<E>> + Send + Sync + Clone,
E: std::error::Error + #{Send} + #{Sync} + 'static,
O: #{ParseHttpResponse}<Output = #{Result}<T, E>> + #{Send} + #{Sync} + #{Clone} + 'static,
Retry: #{ClassifyRetry}<#{SdkSuccess}<T>, #{SdkError}<E>> + #{Send} + #{Sync} + #{Clone},
{
self.handle.client.call(self.operation).await
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ class AwsPresignedFluentBuilderMethod(
) : FluentClientCustomization() {
private val codegenScope = (
presigningTypes + arrayOf(
*RuntimeType.preludeScope,
"Error" to AwsRuntimeType.presigning().resolve("config::Error"),
"SdkError" to RuntimeType.sdkError(runtimeConfig),
)
Expand All @@ -264,7 +265,7 @@ class AwsPresignedFluentBuilderMethod(
pub async fn presigned(
self,
presigning_config: #{PresigningConfig},
) -> Result<#{PresignedRequest}, #{SdkError}<#{OpError}>>
) -> #{Result}<#{PresignedRequest}, #{SdkError}<#{OpError}>>
""",
*codegenScope,
"OpError" to section.operationErrorType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ class FilterEndpointTests(
class S3ProtocolOverride(codegenContext: CodegenContext) : RestXml(codegenContext) {
private val runtimeConfig = codegenContext.runtimeConfig
private val errorScope = arrayOf(
*RuntimeType.preludeScope,
"Bytes" to RuntimeType.Bytes,
"ErrorMetadata" to RuntimeType.errorMetadata(runtimeConfig),
"ErrorBuilder" to RuntimeType.errorMetadataBuilder(runtimeConfig),
Expand All @@ -143,7 +144,7 @@ class S3ProtocolOverride(codegenContext: CodegenContext) : RestXml(codegenContex
override fun parseHttpErrorMetadata(operationShape: OperationShape): RuntimeType {
return ProtocolFunctions.crossOperationFn("parse_http_error_metadata") { fnName ->
rustBlockTemplate(
"pub fn $fnName(response_status: u16, _response_headers: &#{HeaderMap}, response_body: &[u8]) -> Result<#{ErrorBuilder}, #{XmlDecodeError}>",
"pub fn $fnName(response_status: u16, _response_headers: &#{HeaderMap}, response_body: &[u8]) -> #{Result}<#{ErrorBuilder}, #{XmlDecodeError}>",
*errorScope,
) {
rustTemplate(
Expand Down
16 changes: 16 additions & 0 deletions codegen-client-test/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,22 @@ val allCodegenTests = "../codegen-core/common-test-models".let { commonModels ->
""".trimIndent(),
imports = listOf("$commonModels/naming-obstacle-course-structs.smithy"),
),
CodegenTest(
"crate#Config",
"naming_test_prelude_ops",
"""
, "codegen": { "renameErrors": false }
""".trimIndent(),
imports = listOf("$commonModels/naming-obstacle-course-prelude-ops.smithy"),
),
CodegenTest(
"crate#Config",
"naming_test_prelude_structs",
"""
, "codegen": { "renameErrors": false }
""".trimIndent(),
imports = listOf("$commonModels/naming-obstacle-course-prelude-structs.smithy"),
),
CodegenTest("aws.protocoltests.json#TestService", "endpoint-rules"),
CodegenTest("com.aws.example.rust#PokemonService", "pokemon-service-client", imports = listOf("$commonModels/pokemon.smithy", "$commonModels/pokemon-common.smithy")),
CodegenTest("com.aws.example.rust#PokemonService", "pokemon-service-awsjson-client", imports = listOf("$commonModels/pokemon-awsjson.smithy", "$commonModels/pokemon-common.smithy")),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.writable
import software.amazon.smithy.rust.codegen.core.smithy.ModuleDocProvider
import software.amazon.smithy.rust.codegen.core.smithy.ModuleProvider
import software.amazon.smithy.rust.codegen.core.smithy.ModuleProviderContext
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
import software.amazon.smithy.rust.codegen.core.smithy.contextName
import software.amazon.smithy.rust.codegen.core.smithy.module
import software.amazon.smithy.rust.codegen.core.smithy.traits.SyntheticInputTrait
Expand Down Expand Up @@ -122,7 +123,7 @@ class ClientModuleDocProvider(
operation call. For example, this can be used to add an additional HTTP header:
```ignore
## async fn wrapper() -> Result<(), $moduleUseName::Error> {
## async fn wrapper() -> #{Result}<(), $moduleUseName::Error> {
## let client: $moduleUseName::Client = unimplemented!();
use #{http}::header::{HeaderName, HeaderValue};
Expand All @@ -142,6 +143,7 @@ class ClientModuleDocProvider(
## }
```
""".trimIndent(),
*RuntimeType.preludeScope,
"http" to CargoDependency.Http.toDevDependency().toType(),
)
}
Expand Down Expand Up @@ -194,6 +196,10 @@ object ClientModuleProvider : ModuleProvider {
operationModuleName,
parent = ClientRustModule.Operation,
documentationOverride = "Types for the `$contextName` operation.",
// TODO(https://github.com/tokio-rs/tokio/issues/5683): Uncomment the NoImplicitPrelude attribute once this Tokio issue is resolved
// // Disable the Rust prelude since every prelude type should be referenced with its
// // fully qualified name to avoid name collisions with the generated operation shapes.
// additionalAttributes = listOf(Attribute.NoImplicitPrelude)
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ package software.amazon.smithy.rust.codegen.client.smithy.customizations
import software.amazon.smithy.model.shapes.OperationShape
import software.amazon.smithy.model.traits.IdempotencyTokenTrait
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
import software.amazon.smithy.rust.codegen.core.rustlang.writable
import software.amazon.smithy.rust.codegen.core.smithy.CodegenContext
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType.Companion.preludeScope
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.util.findMemberWithTrait
Expand All @@ -28,12 +29,13 @@ class IdempotencyTokenGenerator(codegenContext: CodegenContext, operationShape:
val memberName = symbolProvider.toMemberName(idempotencyTokenMember)
return when (section) {
is OperationSection.MutateInput -> writable {
rust(
rustTemplate(
"""
if ${section.input}.$memberName.is_none() {
${section.input}.$memberName = Some(${section.config}.make_token.make_idempotency_token());
${section.input}.$memberName = #{Some}(${section.config}.make_token.make_idempotency_token());
}
""",
*preludeScope,
)
}
else -> emptySection
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ 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
import software.amazon.smithy.rust.codegen.core.rustlang.writable
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType
import software.amazon.smithy.rust.codegen.core.smithy.RustCrate
import software.amazon.smithy.rust.codegen.core.smithy.customize.OperationCustomization
import software.amazon.smithy.rust.codegen.core.smithy.customize.OperationSection
Expand Down Expand Up @@ -166,6 +167,7 @@ class EndpointsDecorator : ClientCodegenDecorator {

override fun section(section: OperationSection): Writable {
val codegenScope = arrayOf(
*RuntimeType.preludeScope,
"Params" to typesGenerator.paramsStruct(),
"ResolveEndpointError" to types.resolveEndpointError,
)
Expand All @@ -174,10 +176,10 @@ class EndpointsDecorator : ClientCodegenDecorator {
rustTemplate(
"""
let params_result = #{Params}::builder()#{builderFields:W}.build()
.map_err(|err|#{ResolveEndpointError}::from_source("could not construct endpoint parameters", err));
.map_err(|err| #{ResolveEndpointError}::from_source("could not construct endpoint parameters", err));
let (endpoint_result, params) = match params_result {
Ok(params) => (${section.config}.endpoint_resolver.resolve_endpoint(&params), Some(params)),
Err(e) => (Err(e), None)
#{Ok}(params) => (${section.config}.endpoint_resolver.resolve_endpoint(&params), #{Some}(params)),
#{Err}(e) => (#{Err}(e), #{None})
};
""",
"builderFields" to builderFields(typesGenerator.params, section),
Expand All @@ -188,7 +190,7 @@ class EndpointsDecorator : ClientCodegenDecorator {
is OperationSection.MutateRequest -> writable {
// insert the endpoint the bag
rustTemplate("${section.request}.properties_mut().insert(endpoint_result);")
rustTemplate("""if let Some(params) = params { ${section.request}.properties_mut().insert(params); }""")
rustTemplate("""if let #{Some}(params) = params { ${section.request}.properties_mut().insert(params); }""", *codegenScope)
}

else -> emptySection
Expand All @@ -199,8 +201,8 @@ class EndpointsDecorator : ClientCodegenDecorator {
val node = this
return writable {
when (node) {
is StringNode -> rust("Some(${node.value.dq()}.to_string())")
is BooleanNode -> rust("Some(${node.value})")
is StringNode -> rustTemplate("#{Some}(${node.value.dq()}.to_string())", *RuntimeType.preludeScope)
is BooleanNode -> rustTemplate("#{Some}(${node.value})", *RuntimeType.preludeScope)
else -> PANIC("unsupported default value: $node")
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.rustBlock
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.RuntimeType
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType.Companion.preludeScope
import software.amazon.smithy.rust.codegen.core.smithy.generators.EnumGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.EnumGeneratorContext
import software.amazon.smithy.rust.codegen.core.smithy.generators.EnumMemberModel
Expand Down Expand Up @@ -60,16 +61,17 @@ data class InfallibleEnumType(
}

override fun implFromStr(context: EnumGeneratorContext): Writable = writable {
rust(
rustTemplate(
"""
impl std::str::FromStr for ${context.enumName} {
type Err = std::convert::Infallible;
impl ::std::str::FromStr for ${context.enumName} {
type Err = ::std::convert::Infallible;
fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
Ok(${context.enumName}::from(s))
fn from_str(s: &str) -> #{Result}<Self, Self::Err> {
#{Ok}(${context.enumName}::from(s))
}
}
""",
*preludeScope,
)
}

Expand Down Expand Up @@ -98,7 +100,7 @@ data class InfallibleEnumType(
""".trimIndent(),
)
context.enumMeta.render(this)
rust("struct $UnknownVariantValue(pub(crate) String);")
rustTemplate("struct $UnknownVariantValue(pub(crate) #{String});", *preludeScope)
rustBlock("impl $UnknownVariantValue") {
// The generated as_str is not pub as we need to prevent users from calling it on this opaque struct.
rustBlock("pub(crate) fn as_str(&self) -> &str") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ 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
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.isOptional
import software.amazon.smithy.rust.codegen.core.smithy.makeOptional
import software.amazon.smithy.rust.codegen.core.smithy.mapRustType
Expand Down Expand Up @@ -72,17 +73,18 @@ class NestedAccessorGenerator(private val codegenContext: CodegenContext) {
""
}
if (path.isEmpty()) {
rust("Some(input)")
rustTemplate("#{Some}(input)", *preludeScope)
} else {
val head = path.first()
if (symbolProvider.toSymbol(head).isOptional()) {
rust(
rustTemplate(
"""
let input = match ${ref}input.${symbolProvider.toMemberName(head)} {
None => return None,
Some(t) => t
#{None} => return #{None},
#{Some}(t) => t
};
""",
*preludeScope,
)
} else {
rust("let input = input.${symbolProvider.toMemberName(head)};")
Expand Down
Loading

0 comments on commit 17d4b71

Please sign in to comment.