From ae2a74b598a07f1d57826cb305482c02aa8c331c Mon Sep 17 00:00:00 2001 From: david-perez Date: Mon, 27 Jun 2022 18:19:19 +0200 Subject: [PATCH] Non-functional miscellaneous improvements (#1445) This commit contains non-functional (i.e. mostly stylistic, small refactors, documentation and consistency) improvements that I've batched and rolled up while working on #1342. * Make `extraDependencies` from `InlineDependency` class private * Miscellaneous improvements to `RustTypes.kt` * Remove dead code from `RuntimeTypes.kt` * Style change in `RuntimeTypes.kt` * Miscellaneous improvements to `SymbolVisitor.kt` * Miscellaneous improvements to `BuilderGenerator.kt` - Make it consistent with the `ServerBuilderGenerator` from #1342. * Miscellaneous improvements to `TestHelpers.kt` * Add docs to `headers_for_prefix` function - I keep rereading the implementation to see what this returns; it's not intuitive from the function name. --- .../rust/codegen/rustlang/CargoDependency.kt | 2 +- .../smithy/rust/codegen/rustlang/RustTypes.kt | 32 ++++----- .../rust/codegen/smithy/RuntimeTypes.kt | 6 +- .../rust/codegen/smithy/SymbolVisitor.kt | 47 +++++++------ .../smithy/generators/BuilderGenerator.kt | 69 +++++++++---------- .../rust/codegen/testutil/TestHelpers.kt | 2 +- rust-runtime/aws-smithy-http/src/header.rs | 2 + 7 files changed, 75 insertions(+), 85 deletions(-) diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/CargoDependency.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/CargoDependency.kt index a804112f99..16e27202ae 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/CargoDependency.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/CargoDependency.kt @@ -55,7 +55,7 @@ sealed class RustDependency(open val name: String) : SymbolDependencyContainer { class InlineDependency( name: String, val module: RustModule, - val extraDependencies: List = listOf(), + private val extraDependencies: List = listOf(), val renderer: (RustWriter) -> Unit ) : RustDependency(name) { override fun version(): String { diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustTypes.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustTypes.kt index 430e9ae536..84673f2239 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustTypes.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/rustlang/RustTypes.kt @@ -91,11 +91,11 @@ sealed class RustType { } data class Reference(val lifetime: kotlin.String?, override val member: RustType) : RustType(), Container { - override val name: kotlin.String = member.name + override val name = member.name } data class Option(override val member: RustType) : RustType(), Container { - override val name: kotlin.String = "Option" + override val name = "Option" override val namespace = "std::option" /** Convert `Option` to `Option<&T>` **/ @@ -105,7 +105,7 @@ sealed class RustType { } data class Box(override val member: RustType) : RustType(), Container { - override val name: kotlin.String = "Box" + override val name = "Box" override val namespace = "std::boxed" } @@ -115,7 +115,7 @@ sealed class RustType { } data class Vec(override val member: RustType) : RustType(), Container { - override val name: kotlin.String = "Vec" + override val name = "Vec" override val namespace = "std::vec" } @@ -148,25 +148,21 @@ fun RustType.asArgumentType(fullyQualified: Boolean = true): String { } /** Format this Rust type so that it may be used as an argument type in a function definition */ -fun RustType.asArgumentValue(name: String): String { - return when (this) { - is RustType.String, - is RustType.Box -> "$name.into()" +fun RustType.asArgumentValue(name: String) = + when (this) { + is RustType.String, is RustType.Box -> "$name.into()" else -> name } -} /** * For a given name, generate an `Argument` data class containing pre-formatted strings for using this type when - * writing a Rust function + * writing a Rust function. */ -fun RustType.asArgument(name: String): Argument { - return Argument( - "$name: ${this.asArgumentType()}", - this.asArgumentValue(name), - this.render(), - ) -} +fun RustType.asArgument(name: String) = Argument( + "$name: ${this.asArgumentType()}", + this.asArgumentValue(name), + this.render(), +) /** * Render this type, including references and generic parameters. @@ -298,7 +294,7 @@ data class RustMetadata( return this } - fun renderVisibility(writer: RustWriter): RustMetadata { + private fun renderVisibility(writer: RustWriter): RustMetadata { writer.writeInline( when (visibility) { Visibility.PRIVATE -> "" diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt index 71dabad958..7d12e30df6 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/RuntimeTypes.kt @@ -172,7 +172,6 @@ data class RuntimeType(val name: String?, val dependency: RustDependency?, val n val Display = stdfmt.member("Display") val From = RuntimeType("From", dependency = null, namespace = "std::convert") val TryFrom = RuntimeType("TryFrom", dependency = null, namespace = "std::convert") - val Infallible = RuntimeType("Infallible", dependency = null, namespace = "std::convert") val PartialEq = std.member("cmp::PartialEq") val StdError = RuntimeType("Error", dependency = null, namespace = "std::error") val String = RuntimeType("String", dependency = null, namespace = "std::string") @@ -235,8 +234,6 @@ data class RuntimeType(val name: String?, val dependency: RustDependency?, val n val HttpRequestBuilder = Http("request::Builder") val HttpResponseBuilder = Http("response::Builder") - val Hyper = CargoDependency.Hyper.asType() - fun eventStreamReceiver(runtimeConfig: RuntimeConfig): RuntimeType = RuntimeType( "Receiver", @@ -244,8 +241,7 @@ data class RuntimeType(val name: String?, val dependency: RustDependency?, val n "aws_smithy_http::event_stream" ) - fun jsonErrors(runtimeConfig: RuntimeConfig) = - forInlineDependency(InlineDependency.jsonErrors(runtimeConfig)) + fun jsonErrors(runtimeConfig: RuntimeConfig) = forInlineDependency(InlineDependency.jsonErrors(runtimeConfig)) val IdempotencyToken by lazy { forInlineDependency(InlineDependency.idempotencyToken()) } diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/SymbolVisitor.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/SymbolVisitor.kt index dd8322d5de..783df086e6 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/SymbolVisitor.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/SymbolVisitor.kt @@ -113,7 +113,9 @@ fun Symbol.mapRustType(f: (RustType) -> RustType): Symbol { /** Set the symbolLocation for this symbol builder */ fun Symbol.Builder.locatedIn(symbolLocation: SymbolLocation): Symbol.Builder { val currentRustType = this.build().rustType() - check(currentRustType is RustType.Opaque) { "Only Opaque can have their namespace updated" } + check(currentRustType is RustType.Opaque) { + "Only `Opaque` can have their namespace updated" + } val newRustType = currentRustType.copy(namespace = "crate::${symbolLocation.namespace}") return this.definitionFile("src/${symbolLocation.filename}") .namespace("crate::${symbolLocation.namespace}", "::") @@ -148,6 +150,18 @@ fun SymbolProvider.wrapOptional(member: MemberShape, value: String): String = va */ fun SymbolProvider.toOptional(member: MemberShape, value: String): String = value.letIf(!toSymbol(member).isOptional()) { "Some($value)" } +/** + * Services can rename their contained shapes. See https://awslabs.github.io/smithy/1.0/spec/core/model.html#service + * specifically, `rename` + */ +fun Shape.contextName(serviceShape: ServiceShape?): String { + return if (serviceShape != null) { + id.getName(serviceShape) + } else { + id.name + } +} + /** * Base converter from `Shape` to `Symbol`. Shapes are the direct contents of the `Smithy` model. `Symbols` carry information * about Rust types, namespaces, dependencies, metadata as well as other information required to render a symbol. @@ -168,18 +182,6 @@ class SymbolVisitor( return shape.accept(this) } - /** - * Services can rename their contained shapes. See https://awslabs.github.io/smithy/1.0/spec/core/model.html#service - * specifically, `rename` - */ - private fun Shape.contextName(): String { - return if (serviceShape != null) { - id.getName(serviceShape) - } else { - id.name - } - } - /** * Return the name of a given `enum` variant. Note that this refers to `enum` in the Smithy context * where enum is a trait that can be applied to [StringShape] and not in the Rust context of an algebraic data type. @@ -238,7 +240,8 @@ class SymbolVisitor( override fun doubleShape(shape: DoubleShape): Symbol = simpleShape(shape) override fun stringShape(shape: StringShape): Symbol { return if (shape.hasTrait()) { - symbolBuilder(shape, RustType.Opaque(shape.contextName().toPascalCase())).locatedIn(Models).build() + val rustType = RustType.Opaque(shape.contextName(serviceShape).toPascalCase()) + symbolBuilder(shape, rustType).locatedIn(Models).build() } else { simpleShape(shape) } @@ -285,7 +288,7 @@ class SymbolVisitor( return symbolBuilder( shape, RustType.Opaque( - shape.contextName() + shape.contextName(serviceShape) .replaceFirstChar { it.uppercase() } ) ) @@ -305,7 +308,7 @@ class SymbolVisitor( val isError = shape.hasTrait() val isInput = shape.hasTrait() val isOutput = shape.hasTrait() - val name = shape.contextName().toPascalCase().letIf(isError && config.renameExceptions) { + val name = shape.contextName(serviceShape).toPascalCase().letIf(isError && config.renameExceptions) { it.replace("Exception", "Error") } val builder = symbolBuilder(shape, RustType.Opaque(name)) @@ -318,7 +321,7 @@ class SymbolVisitor( } override fun unionShape(shape: UnionShape): Symbol { - val name = shape.contextName().toPascalCase() + val name = shape.contextName(serviceShape).toPascalCase() val builder = symbolBuilder(shape, RustType.Opaque(name)).locatedIn(Models) return builder.build() @@ -355,9 +358,7 @@ private const val SHAPE_KEY = "shape" private const val SYMBOL_DEFAULT = "symboldefault" private const val RENAMED_FROM_KEY = "renamedfrom" -fun Symbol.Builder.rustType(rustType: RustType): Symbol.Builder { - return this.putProperty(RUST_TYPE_KEY, rustType) -} +fun Symbol.Builder.rustType(rustType: RustType): Symbol.Builder = this.putProperty(RUST_TYPE_KEY, rustType) fun Symbol.Builder.renamedFrom(name: String): Symbol.Builder { return this.putProperty(RENAMED_FROM_KEY, name) @@ -366,9 +367,7 @@ fun Symbol.Builder.renamedFrom(name: String): Symbol.Builder { fun Symbol.renamedFrom(): String? = this.getProperty(RENAMED_FROM_KEY, String::class.java).orNull() fun Symbol.defaultValue(): Default = this.getProperty(SYMBOL_DEFAULT, Default::class.java).orElse(Default.NoDefault) -fun Symbol.Builder.setDefault(default: Default): Symbol.Builder { - return this.putProperty(SYMBOL_DEFAULT, default) -} +fun Symbol.Builder.setDefault(default: Default): Symbol.Builder = this.putProperty(SYMBOL_DEFAULT, default) /** * Type representing the default value for a given type. (eg. for Strings, this is `""`) @@ -406,7 +405,7 @@ fun Symbol.extractSymbolFromOption(): Symbol = this.mapRustType { it.stripOuter< fun Symbol.isRustBoxed(): Boolean = rustType().stripOuter() is RustType.Box // Symbols should _always_ be created with a Rust type & shape attached -fun Symbol.rustType(): RustType = this.getProperty(RUST_TYPE_KEY, RustType::class.java).get() +fun Symbol.rustType(): RustType = this.expectProperty(RUST_TYPE_KEY, RustType::class.java) fun Symbol.shape(): Shape = this.expectProperty(SHAPE_KEY, Shape::class.java) /** diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/BuilderGenerator.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/BuilderGenerator.kt index 4c73cdf9cb..1e1f3daf60 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/BuilderGenerator.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/BuilderGenerator.kt @@ -9,7 +9,6 @@ import software.amazon.smithy.codegen.core.Symbol import software.amazon.smithy.model.Model import software.amazon.smithy.model.shapes.MemberShape import software.amazon.smithy.model.shapes.StructureShape -import software.amazon.smithy.rust.codegen.rustlang.Attribute import software.amazon.smithy.rust.codegen.rustlang.RustReservedWords import software.amazon.smithy.rust.codegen.rustlang.RustType import software.amazon.smithy.rust.codegen.rustlang.RustWriter @@ -56,8 +55,8 @@ class OperationBuildError(private val runtimeConfig: RuntimeConfig) { fun serializationError(w: RustWriter, error: String) = "${w.format(runtimeConfig.operationBuildError())}::SerializationError($error.into())" } -/** setter names will never hit a reserved word and therefore never need escaping */ -fun MemberShape.setterName(): String = "set_${this.memberName.toSnakeCase()}" +// Setter names will never hit a reserved word and therefore never need escaping. +fun MemberShape.setterName() = "set_${this.memberName.toSnakeCase()}" class BuilderGenerator( private val model: Model, @@ -70,21 +69,21 @@ class BuilderGenerator( fun render(writer: RustWriter) { val symbol = symbolProvider.toSymbol(shape) - writer.docs("See #D", symbol) + writer.docs("See #D.", symbol) val segments = shape.builderSymbol(symbolProvider).namespace.split("::") writer.withModule(segments.last()) { renderBuilder(this) } } - private fun buildFn(implBlockWriter: RustWriter) { + private fun renderBuildFn(implBlockWriter: RustWriter) { val fallibleBuilder = StructureGenerator.fallibleBuilder(shape, symbolProvider) val outputSymbol = symbolProvider.toSymbol(shape) val returnType = when (fallibleBuilder) { - true -> "std::result::Result<${implBlockWriter.format(outputSymbol)}, ${implBlockWriter.format(runtimeConfig.operationBuildError())}>" + true -> "Result<${implBlockWriter.format(outputSymbol)}, ${implBlockWriter.format(runtimeConfig.operationBuildError())}>" false -> implBlockWriter.format(outputSymbol) } - implBlockWriter.docs("Consumes the builder and constructs a #D", outputSymbol) + implBlockWriter.docs("Consumes the builder and constructs a #D.", outputSymbol) implBlockWriter.rustBlock("pub fn build(self) -> $returnType") { conditionalBlock("Ok(", ")", conditional = fallibleBuilder) { // If a wrapper is specified, use the `::new` associated function to construct the wrapper @@ -94,20 +93,16 @@ class BuilderGenerator( } private fun RustWriter.missingRequiredField(field: String) { - val detailedMessage = "$field was not specified but it is required when building ${ - symbolProvider.toSymbol( - shape - ).name - }" + val detailedMessage = "$field was not specified but it is required when building ${symbolProvider.toSymbol(shape).name}" rust( - """#T::MissingField { field: ${field.dq()}, details: ${detailedMessage.dq()}}""", + """#T::MissingField { field: "$field", details: "$detailedMessage" } """, runtimeConfig.operationBuildError() ) } fun renderConvenienceMethod(implBlock: RustWriter) { val builderSymbol = shape.builderSymbol(symbolProvider) - implBlock.docs("Creates a new builder-style object to manufacture #D", structureSymbol) + implBlock.docs("Creates a new builder-style object to manufacture #D.", structureSymbol) implBlock.rustBlock("pub fn builder() -> #T", builderSymbol) { write("#T::default()", builderSymbol) } @@ -115,9 +110,8 @@ class BuilderGenerator( // TODO(EventStream): [DX] Consider updating builders to take EventInputStream as Into private fun renderBuilderMember(writer: RustWriter, memberName: String, memberSymbol: Symbol) { - // builder members are crate-public to enable using them - // directly in serializers/deserializers. During XML deserialization, `builder..take` is used to append to - // lists and maps + // Builder members are crate-public to enable using them directly in serializers/deserializers. + // During XML deserialization, `builder..take` is used to append to lists and maps. writer.write("pub(crate) $memberName: #T,", memberSymbol) } @@ -136,15 +130,21 @@ class BuilderGenerator( } } + /** + * Render a `set_foo` method. This is useful as a target for code generation, because the argument type + * is the same as the resulting member type, and is always optional. + */ private fun renderBuilderMemberSetterFn( writer: RustWriter, outerType: RustType, member: MemberShape, memberName: String ) { - // Render a `set_foo` method. This is useful as a target for code generation, because the argument type - // is the same as the resulting member type, and is always optional. + // TODO(https://github.com/awslabs/smithy-rs/issues/1302): This `asOptional()` call is superfluous except in + // the case where the shape is a `@streaming` blob, because [StreamingTraitSymbolProvider] always generates + // a non `Option`al target type: in all other cases the client generates `Option`al types. val inputType = outerType.asOptional() + writer.documentShape(member, model) writer.rustBlock("pub fn ${member.setterName()}(mut self, input: ${inputType.render(true)}) -> Self") { rust("self.$memberName = input; self") @@ -154,31 +154,29 @@ class BuilderGenerator( private fun renderBuilder(writer: RustWriter) { val builderName = "Builder" - val symbol = structureSymbol - writer.docs("A builder for #D", symbol) - Attribute.NonExhaustive.render(writer) - // Matching derives to the main structure + `Default` since we are a builder and everything is optional - val baseDerives = symbol.expectRustMetadata().derives + writer.docs("A builder for #D.", structureSymbol) + // Matching derives to the main structure + `Default` since we are a builder and everything is optional. + val baseDerives = structureSymbol.expectRustMetadata().derives val derives = baseDerives.derives.intersect(setOf(RuntimeType.Debug, RuntimeType.PartialEq, RuntimeType.Clone)) + RuntimeType.Default baseDerives.copy(derives = derives).render(writer) writer.rustBlock("pub struct $builderName") { - members.forEach { member -> + for (member in members) { val memberName = symbolProvider.toMemberName(member) - // All fields in the builder are optional + // All fields in the builder are optional. val memberSymbol = symbolProvider.toSymbol(member).makeOptional() renderBuilderMember(this, memberName, memberSymbol) } } writer.rustBlock("impl $builderName") { - members.forEach { member -> - // All fields in the builder are optional + for (member in members) { + // All fields in the builder are optional. val memberSymbol = symbolProvider.toSymbol(member) val outerType = memberSymbol.rustType() val coreType = outerType.stripOuter() val memberName = symbolProvider.toMemberName(member) // Render a context-aware builder method for certain types, e.g. a method for vectors that automatically - // appends + // appends. when (coreType) { is RustType.Vec -> renderVecHelper(member, memberName, coreType) is RustType.HashMap -> renderMapHelper(member, memberName, coreType) @@ -187,7 +185,7 @@ class BuilderGenerator( renderBuilderMemberSetterFn(this, outerType, member, memberName) } - buildFn(this) + renderBuildFn(this) } } @@ -236,17 +234,16 @@ class BuilderGenerator( /** * The core builder of the inner type. If the structure requires a fallible builder, this may use `?` to return - * errors + * errors. * ```rust * SomeStruct { - * field: builder.field, - * field2: builder.field2, - * field3: builder.field3.unwrap_or_default() - * field4: builder.field4.ok_or("field4 is required when building SomeStruct")? + * field1: builder.field1, + * field2: builder.field2.unwrap_or_default() + * field3: builder.field3.ok_or("field3 is required when building SomeStruct")? * } * ``` */ - protected fun coreBuilder(writer: RustWriter) { + private fun coreBuilder(writer: RustWriter) { writer.rustBlock("#T", structureSymbol) { members.forEach { member -> val memberName = symbolProvider.toMemberName(member) diff --git a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/testutil/TestHelpers.kt b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/testutil/TestHelpers.kt index 2198600376..043b46b3c9 100644 --- a/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/testutil/TestHelpers.kt +++ b/codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/testutil/TestHelpers.kt @@ -95,7 +95,7 @@ fun String.asSmithyModel(sourceLocation: String? = null): Model { } /** - * In tests, we frequently need to generate a struct, a builder, and an impl block to access said builder + * In tests, we frequently need to generate a struct, a builder, and an impl block to access said builder. */ fun StructureShape.renderWithModelBuilder(model: Model, symbolProvider: RustSymbolProvider, writer: RustWriter, forWhom: CodegenTarget = CodegenTarget.CLIENT) { StructureGenerator(model, symbolProvider, writer, this).render(forWhom) diff --git a/rust-runtime/aws-smithy-http/src/header.rs b/rust-runtime/aws-smithy-http/src/header.rs index 40a1daaf5e..a70c360713 100644 --- a/rust-runtime/aws-smithy-http/src/header.rs +++ b/rust-runtime/aws-smithy-http/src/header.rs @@ -73,6 +73,8 @@ pub fn many_dates( Ok(out) } +/// Returns an iterator over pairs where the first element is the unprefixed header name that +/// starts with the input `key` prefix, and the second element is the full header name. pub fn headers_for_prefix<'a>( headers: &'a http::HeaderMap, key: &'a str,