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
#2696)

## Motivation and Context
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).

## Checklist
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
  • Loading branch information
jdisanti authored and david-perez committed May 18, 2023
1 parent 587e654 commit 28742d5
Show file tree
Hide file tree
Showing 48 changed files with 619 additions and 225 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,9 @@ message = "Avoid extending IMDS credentials' expiry unconditionally, which may i
references = ["smithy-rs#2687", "smithy-rs#2694"]
meta = { "breaking" = false, "tada" = false, "bug" = true }
author = "ysaito1001"

[[smithy-rs]]
message = "Fix compiler errors in generated code when naming shapes after types in the Rust standard library prelude."
references = ["smithy-rs#2696"]
meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "client"}
author = "jdisanti"
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
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 as ::std::str::FromStr>::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 28742d5

Please sign in to comment.