Fix client operation name collisions with the standard library prelude #2696

jdisanti · 2023-05-12T00:16:16Z

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

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.

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

github-actions · 2023-05-12T02:08:54Z

A new generated diff is ready to view.

A new doc preview is ready to view.

ysaito1001

Thanks for this effort! To contribute, if we find more instances of unqualified prelude in the codebase, we can fix them on-the-fly as we go in respective PRs?

ysaito1001 · 2023-05-12T16:04:31Z

codegen-client-test/build.gradle.kts

+        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"),
+        ),


Great tests to add!

ysaito1001 · 2023-05-12T16:20:55Z

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/RuntimeType.kt

@@ -193,14 +193,57 @@ data class RuntimeType(val path: String, val dependency: RustDependency? = null)
     * The companion object contains commonly used RuntimeTypes
     */
    companion object {
+        /** Scope that contains all Rust prelude types, but not macros or functions */


Nice! Maybe it's useful to include a link to prelude contents like this one?

Fixed in 49d10ae

david-perez · 2023-05-12T16:25:22Z

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/AwsFluentClientDecorator.kt

@@ -223,6 +223,7 @@ private fun renderCustomizableOperationSendMethod(
    val combinedGenerics = operationGenerics + handleGenerics

    val codegenScope = arrayOf(
+        *RuntimeType.preludeScope,


I wonder if we should just merge this scope directly in rustTemplate and friends.

I definitely thought about it. That and deleting rust() and renaming rustTemplate() to rust(). I'd like to get rid of the old style #T formatters entirely to tidy things up. All of this is quite a bit of work, and I wanted to get this initial fix out first.

david-perez · 2023-05-12T16:28:50Z

...software/amazon/smithy/rust/codegen/client/smithy/generators/client/FluentClientGenerator.kt

@@ -112,12 +113,12 @@ class FluentClientGenerator(
                }

                #{client_docs:W}
-                ##[derive(std::fmt::Debug)]
+                ##[derive(::std::fmt::Debug)]


Yeah I think prepending all types with :: is a very good idea and something we should somehow enforce everywhere.

If we can use no_implicit_prelude, then it will be enforced by the compiler. But there are many dependency macros that will need to get fixed before we can do that.

david-perez · 2023-05-12T16:31:45Z

codegen-core/common-test-models/naming-obstacle-course-prelude-ops.smithy

+       Err,
+       String,
+       ToString,
+       Vec,


TryFrom, TryInto, and FromIterator are missing? https://doc.rust-lang.org/std/prelude/index.html

Would be nice to put the prelude link in these files.

We generate Rust 2021 edition crates.

Great catch! Fixed in 49d10ae

david-perez · 2023-05-12T16:33:48Z

codegen-core/common-test-models/naming-obstacle-course-prelude-structs.smithy

+/// Confounds model generation machinery by using structs named after every item in the Rust prelude
+@awsJson1_1
+@service(sdkId: "Config")
+service Config {


It feels like there's not a need for having these be two separate integration tests.

I had split them apart due to name collisions within the Smithy model, but it just occurred to me that I can probably use namespaces to fix that.

david-perez · 2023-05-12T16:36:22Z

codegen-core/common-test-models/naming-obstacle-course-prelude-ops.smithy

+/// Confounds model generation machinery by using operations named after every item in the Rust prelude
+@awsJson1_1
+@service(sdkId: "Config")
+service Config {


These are the kinds of tests that I think are better done in {client,server}IntegrationTests. In Kotlin we can easily code generate Smithy models that more exhaustively test this e.g. union / enum shapes with names of types from the prelude are not covered.

That's a great idea. I refactored them into Kotlin unit tests in 49d10ae and also added them to the server codegen test suite.

david-perez · 2023-05-12T16:37:04Z

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustWriter.kt

@@ -142,6 +143,21 @@ fun <T : AbstractCodeWriter<T>> T.conditionalBlock(
    return this
 }

+fun RustWriter.conditionalBlockTemplate(


Thanks for adding this.

…collisions

github-actions · 2023-05-12T19:52:16Z

A new generated diff is ready to view.

A new doc preview is ready to view.

…collisions

ysaito1001

Re-approved after new commits. Looks fantastic! I really like how test models are now automatically derived from preludeScope and used by both client & server tests as per David's comment.

github-actions · 2023-05-12T22:13:58Z

A new generated diff is ready to view.

A new doc preview is ready to view.

github-actions · 2023-05-12T23:24:58Z

A new generated diff is ready to view.

A new doc preview is ready to view.

#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._

jdisanti force-pushed the jdisanti-fix-prelude-collisions branch from 6936f69 to 17d4b71 Compare May 12, 2023 00:25

Update changelog

a1207f9

jdisanti marked this pull request as ready for review May 12, 2023 00:27

jdisanti requested review from a team as code owners May 12, 2023 00:27

jdisanti added the needs-sdk-review label May 12, 2023

jdisanti added 2 commits May 11, 2023 17:57

Fix SDK compilation

8be1ff5

Test fixes

e214771

jdisanti requested a review from a team as a code owner May 12, 2023 01:18

jdisanti requested a review from unexge May 12, 2023 01:18

jdisanti added the needs-server-review label May 12, 2023

ysaito1001 approved these changes May 12, 2023

View reviewed changes

ysaito1001 removed the needs-sdk-review label May 12, 2023

david-perez approved these changes May 12, 2023

View reviewed changes

david-perez removed the needs-server-review label May 12, 2023

jdisanti added 2 commits May 12, 2023 10:12

Fix codegen-server tests

57ccc34

Merge remote-tracking branch 'origin/main' into jdisanti-fix-prelude-…

d2303a3

…collisions

Velfi enabled auto-merge May 12, 2023 18:48

jdisanti disabled auto-merge May 12, 2023 20:16

jdisanti added 2 commits May 12, 2023 14:09

Incorporate feedback

49d10ae

Merge remote-tracking branch 'origin/main' into jdisanti-fix-prelude-…

62d0106

…collisions

jdisanti force-pushed the jdisanti-fix-prelude-collisions branch from f7926ff to 62d0106 Compare May 12, 2023 21:10

ysaito1001 approved these changes May 12, 2023

View reviewed changes

Fix tests

af6eea4

jdisanti enabled auto-merge May 12, 2023 22:25

jdisanti added this pull request to the merge queue May 12, 2023

Merged via the queue into main with commit 040d0e4 May 12, 2023

jdisanti deleted the jdisanti-fix-prelude-collisions branch May 12, 2023 23:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix client operation name collisions with the standard library prelude #2696

Fix client operation name collisions with the standard library prelude #2696

jdisanti commented May 12, 2023 •

edited

Loading

github-actions bot commented May 12, 2023

ysaito1001 left a comment

ysaito1001 May 12, 2023

ysaito1001 May 12, 2023

jdisanti May 12, 2023

david-perez May 12, 2023

jdisanti May 12, 2023

david-perez May 12, 2023

jdisanti May 12, 2023

david-perez May 12, 2023

jdisanti May 12, 2023

david-perez May 12, 2023

jdisanti May 12, 2023

david-perez May 12, 2023 •

edited

Loading

jdisanti May 12, 2023

david-perez May 12, 2023

github-actions bot commented May 12, 2023

ysaito1001 left a comment

github-actions bot commented May 12, 2023

github-actions bot commented May 12, 2023

Fix client operation name collisions with the standard library prelude #2696

Fix client operation name collisions with the standard library prelude #2696

Conversation

jdisanti commented May 12, 2023 • edited Loading

Motivation and Context

Checklist

github-actions bot commented May 12, 2023

ysaito1001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-perez May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 12, 2023

ysaito1001 left a comment

Choose a reason for hiding this comment

github-actions bot commented May 12, 2023

github-actions bot commented May 12, 2023

jdisanti commented May 12, 2023 •

edited

Loading

david-perez May 12, 2023 •

edited

Loading