Make `RustReservedWordsSymbolProvider` configurable #2382

jdisanti · 2023-02-15T23:13:10Z

Motivation and Context

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.

The `toEnumVariantName` function existed on symbol provider to work around enum definitions not being shapes. In the future when we refactor to use `EnumShape` instead of `EnumTrait`, there will be `MemberShape`s for each enum member. This change incrementally moves us to that future by creating fake `MemberShape`s in the enum generator from the enum definition.

github-actions · 2023-02-15T23:32:10Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

github-actions · 2023-02-16T01:01:58Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

github-actions · 2023-02-16T01:54:50Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

jdisanti · 2023-02-22T19:12:56Z

...ent/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientReservedWords.kt

+    ),
+    enumMemberMap = mapOf(
+        // Unknown is used as the name of the variant containing unexpected values
+        "Unknown" to "UnknownValue",


From a comment on #2377, these should have constants.

hlbarber

Is the change from send to send_value shown in the Server Test intended?

It looks like we've lost this mapping

                "build" -> "build_value"
                "builder" -> "builder_value"
                "default" -> "default_value"
                "send" -> "send_value"
                // To avoid conflicts with the `make_operation` and `presigned` functions on generated inputs
                "make_operation" -> "make_operation_value"
                "presigned" -> "presigned_value"
                "customize" -> "customize_value"
                // To avoid conflicts with the error metadata `meta` field
                "meta" -> "meta_value"

on the server side, and it's been restored for the client side only?

jdisanti · 2023-03-10T17:49:20Z

Is the change from send to send_value shown in the Server Test intended?

Does send need to be reserved on the server side? My thinking on this was that send is reserved on the client side due to us adding a send() function to fluent builders to actually send a request, but that seems unnecessary on the server side.

Good catch though, I need to add a breaking-change label to this PR.

jdisanti · 2023-03-10T17:52:40Z

These are preserved in ServerReservedWords:

"build" -> "build_value"
"builder" -> "builder_value"
"default" -> "default_value"

These seem unnecessary for server, but you tell me:

"send" -> "send_value"
"make_operation" -> "make_operation_value"
"presigned" -> "presigned_value"
"customize" -> "customize_value"
"meta" -> "meta_value"

github-actions · 2023-03-10T20:09:40Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

david-perez

The list of server reserved words looks good.

david-perez · 2023-03-10T17:15:12Z

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


-class RustReservedWordSymbolProvider(private val base: RustSymbolProvider, private val model: Model) :
-    WrappingSymbolProvider(base) {
+data class RustReservedWordConfig(


nit: RustReservedWordsConfig to match the rest of the uses?

david-perez · 2023-03-13T09:01:16Z

...ent/src/main/kotlin/software/amazon/smithy/rust/codegen/client/smithy/ClientReservedWords.kt

+import software.amazon.smithy.rust.codegen.core.smithy.generators.UnionGenerator
+
+val ClientReservedWords = RustReservedWordConfig(
+    structMemberMap = StructureGenerator.structMemberMap +


nit: I consciously make the effort to use "structure" to refer to Smithy structure shapes, and use struct only to refer to Rust structs.

david-perez · 2023-03-13T09:12:51Z

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

+            container is StructureShape -> when (val mapped = reservedWordConfig.structMemberMap[baseName]) {
+                null -> reservedWordReplacedName
+                else -> mapped
            }


Suggested change

container is StructureShape -> when (val mapped = reservedWordConfig.structMemberMap[baseName]) {

null -> reservedWordReplacedName

else -> mapped

}

container is StructureShape ->

reservedWordConfig.structMemberMap.getOrDefault(baseName, reservedWordReplacedName)

These can be slightly simplified.

david-perez · 2023-03-13T09:14:02Z

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

-                // Real models won't end in `_` so it's safe to stop here
-                "UnknownValue" -> "UnknownValue_"
-                else -> reservedWordReplacedName
+            container is EnumShape || container.hasTrait<EnumTrait>() -> when (val mapped = reservedWordConfig.enumMemberMap[baseName]) {


EnumTrait is only applicable to string shapes; a member shape's container cannot be a string shape.

Suggested change

container is EnumShape || container.hasTrait<EnumTrait>() -> when (val mapped = reservedWordConfig.enumMemberMap[baseName]) {

container is EnumShape -> when (val mapped = reservedWordConfig.enumMemberMap[baseName]) {

Hmmm... Yeah, I think this is dead code. toMemberName isn't even called for enum rendering. I think I'll keep it around in the event that someone refactors EnumGenerator to use toMemberName though since this is technically correct.

david-perez · 2023-03-13T09:16:44Z

...e/src/test/kotlin/software/amazon/smithy/rust/codegen/core/rustlang/RustReservedWordsTest.kt

@@ -22,18 +22,108 @@ import software.amazon.smithy.rust.codegen.core.util.lookup
 internal class RustReservedWordSymbolProviderTest {


Great tests.

github-actions · 2023-03-13T19:12:24Z

A new generated diff is ready to view.

AWS SDK (ignoring whitespace)
No codegen difference in the Client Test
Server Test (ignoring whitespace)
No codegen difference in the Server Test Python

A new doc preview is ready to view.

jdisanti added 3 commits February 15, 2023 13:03

Fix escaping of Self in symbol providers

c1add65

Clean up an old hack

3337f93

jdisanti changed the base branch from main to jdisanti-fix-escape-self February 15, 2023 23:13

jdisanti force-pushed the jdisanti-reserved-word-configurability branch from ee7aade to 4860d89 Compare February 16, 2023 00:35

Make RustReservedWordsSymbolProvider configurable

4f59076

jdisanti force-pushed the jdisanti-reserved-word-configurability branch from 4860d89 to 4f59076 Compare February 16, 2023 01:36

jdisanti marked this pull request as ready for review February 16, 2023 02:12

jdisanti requested review from a team as code owners February 16, 2023 02:12

jdisanti requested review from drganjoo, weihanglo and PC-LoadLetter and removed request for a team February 16, 2023 02:12

jdisanti added needs-review A tag for PRs waiting on a review from one of the repo admins needs-sdk-review needs-server-review and removed needs-review A tag for PRs waiting on a review from one of the repo admins labels Feb 16, 2023

Velfi approved these changes Feb 17, 2023

View reviewed changes

Velfi removed the needs-sdk-review label Feb 17, 2023

jdisanti mentioned this pull request Feb 22, 2023

Remove toEnumVariantName from RustSymbolProvider #2377

Merged

jdisanti commented Feb 22, 2023

View reviewed changes

jdisanti force-pushed the jdisanti-fix-escape-self branch from 3337f93 to 4a4f425 Compare February 22, 2023 22:01

Base automatically changed from jdisanti-fix-escape-self to main February 23, 2023 00:02

ysaito1001 approved these changes Feb 27, 2023

View reviewed changes

hlbarber reviewed Mar 10, 2023

View reviewed changes

jdisanti added the breaking-change This will require a breaking change label Mar 10, 2023

jdisanti added 2 commits March 10, 2023 11:20

Merge origin/main

bedd1f8

Update changelog

4097d45

david-perez approved these changes Mar 13, 2023

View reviewed changes

Incorporate feedback

a88b847

jdisanti enabled auto-merge March 13, 2023 18:53

jdisanti added this pull request to the merge queue Mar 13, 2023

Merged via the queue into main with commit 4835af9 Mar 13, 2023

jdisanti deleted the jdisanti-reserved-word-configurability branch March 13, 2023 19:41

Velfi removed the needs-server-review label Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `RustReservedWordsSymbolProvider` configurable #2382

Make `RustReservedWordsSymbolProvider` configurable #2382

jdisanti commented Feb 15, 2023 •

edited

Loading

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Feb 16, 2023

jdisanti Feb 22, 2023

hlbarber left a comment

jdisanti commented Mar 10, 2023

jdisanti commented Mar 10, 2023 •

edited

Loading

github-actions bot commented Mar 10, 2023

david-perez left a comment

david-perez Mar 10, 2023

david-perez Mar 13, 2023

david-perez Mar 13, 2023

david-perez Mar 13, 2023

jdisanti Mar 13, 2023

david-perez Mar 13, 2023

github-actions bot commented Mar 13, 2023

	container is EnumShape \|\| container.hasTrait<EnumTrait>() -> when (val mapped = reservedWordConfig.enumMemberMap[baseName]) {
	container is EnumShape -> when (val mapped = reservedWordConfig.enumMemberMap[baseName]) {

		@@ -22,18 +22,108 @@ import software.amazon.smithy.rust.codegen.core.util.lookup
		internal class RustReservedWordSymbolProviderTest {

Make RustReservedWordsSymbolProvider configurable #2382

Make RustReservedWordsSymbolProvider configurable #2382

Conversation

jdisanti commented Feb 15, 2023 • edited Loading

Motivation and Context

Checklist

github-actions bot commented Feb 15, 2023

github-actions bot commented Feb 16, 2023

github-actions bot commented Feb 16, 2023

jdisanti Feb 22, 2023

Choose a reason for hiding this comment

hlbarber left a comment

Choose a reason for hiding this comment

jdisanti commented Mar 10, 2023

jdisanti commented Mar 10, 2023 • edited Loading

github-actions bot commented Mar 10, 2023

david-perez left a comment

Choose a reason for hiding this comment

david-perez Mar 10, 2023

Choose a reason for hiding this comment

david-perez Mar 13, 2023

Choose a reason for hiding this comment

david-perez Mar 13, 2023

Choose a reason for hiding this comment

david-perez Mar 13, 2023

Choose a reason for hiding this comment

jdisanti Mar 13, 2023

Choose a reason for hiding this comment

david-perez Mar 13, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 13, 2023

Make `RustReservedWordsSymbolProvider` configurable #2382

Make `RustReservedWordsSymbolProvider` configurable #2382

jdisanti commented Feb 15, 2023 •

edited

Loading

jdisanti commented Mar 10, 2023 •

edited

Loading