Squash assorted Clippy and Rust doc warnings in codegen integration tests #3684
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ests This commit gets rid of some Clippy and Rust doc warnings we produce in generated code, which are surfaced in our codegen integration tests. The aim is that eventually we will be able to deny Clippy and Rust doc warnings in #3678 (despite us baking in `RUSTDOCFLAGS="-D warnings"` and `RUSTFLAGS="-D warnings"` in our CI Docker image, we curently clear these in codegen integration tests). Note that denying compiler warnings is separately tracked in #3194. The commit contains fixes for the following: - Unconditionally referring to the `crate::types` module in Rust docs when the Smithy model is such that it is not generated. - Broken intra-crate doc links for client documentation. - Incorrectly escaping Rust reserved keywords when referring to operation names in Rust docs. - An unnecessary semi-colon when rendering additional client plugins. - Not escaping brackets in text when rendering Rust docs containing HTML, that are interpreted as broken links. - A broken link to unit variants of unions. - Using `TryFrom` instead of `From` when infallibly converting from `&str` to `String` when deserializing an `@httpPayload` in the server. - Using a redundant closure with `unwrap_or_else` when falling back to a `@default` boolean or number value for a `document` shape, instead of `unwrap_or`. - Not opting into `clippy::enum_variant_names` when rendering the `Operation` enum in the server (since we render the operation names that were modeled as-is).
|
A new generated diff is ready to view.
A new doc preview is ready to view. |
7 tasks
aajtodd
approved these changes
Jun 11, 2024
drganjoo
approved these changes
Jun 11, 2024
| .any { | ||
| try { | ||
| codegenContext.symbolProvider.moduleForShape(it).name == ClientRustModule.types.name | ||
| } catch (ex: RuntimeException) { |
There was a problem hiding this comment.
Nit: I would prefer this to be a software.amazon.smithy.codegen.core.CodegenException instead of a RuntimeException, as the former explicitly indicates an exceptional code generation path that we are aware of. Using a RuntimeException might give the impression that there is an issue with the overall runtime or Kotlin libraries being used for code generation.
| @@ -40,7 +58,7 @@ class ClientDocsGenerator(private val codegenContext: ClientCodegenContext) : Li | |||
| either a successful output or a [`SdkError`](crate::error::SdkError). | |||
|
|
|||
| Some of these API inputs may be structs or enums to provide more complex structured information. | |||
| These structs and enums live in [`types`](crate::types). There are some simpler types for | |||
| ${typesModuleSentence}There are some simpler types for | |||
There was a problem hiding this comment.
By the time we reach this code during code generation, haven't we already generated all the necessary Rust code? I'm considering if there's a simpler way to determine whether crate::types was used or not, perhaps by maintaining a flag rather than traversing the model again.
| fun clientOperationFnDocsName( | ||
| operationShape: OperationShape, | ||
| symbolProvider: RustSymbolProvider, | ||
| ): String = symbolProvider.toSymbol(operationShape).name.toSnakeCase() |
There was a problem hiding this comment.
Is there no standard utility function available for retrieving the Rust type generated for a given Smithy Shape, or do we need to call .name.toSnakeCase() on the Symbol wherever needed?
| * This is useful when rendering Rust docs, since text within brackets might get misinterpreted as broken Markdown doc | ||
| * links (`[link text](https://example.com)`). | ||
| **/ | ||
| private fun Element.escapeBracketsInText() { |
There was a problem hiding this comment.
Nit: Just a thought: Should we do this only if the box brackets are not followed by text in parentheses? This would allow Smithy model authors to write linkable text in their documentation. For example, [this is clickable](https://example.com) should not be escaped.
| writer.rustTemplate(".unwrap_or_else(|| #{DefaultValue:W})", "DefaultValue" to defaultValue) | ||
| } | ||
| val node = member.expectTrait<DefaultTrait>().toNode()!! | ||
| if ((targetShape is DocumentShape && (node is BooleanNode || node is NumberNode)) || |
There was a problem hiding this comment.
Thinking out loud: Would defining a function, such as isBasicRustType(targetShape), provide any advantages, or is this the only place where we check for this?
| """, | ||
| ) | ||
| output shouldContain "<code>Link without href attribute</code>" | ||
| output shouldContain "Some text with \\[brackets\\]" |
There was a problem hiding this comment.
Nit: Should we use """Some text with \[brackets\]""" for increased readability?
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 17, 2024
RPC v2 CBOR is a new protocol that ~is being added~ has [recently been added](https://smithy.io/2.0/additional-specs/protocols/smithy-rpc-v2.html) to the Smithy specification. _(I'll add more details here as the patchset evolves)_ Credit goes to @jjant for initial implementation of the router, which I built on top of from his [`jjant/smithy-rpc-v2-exploration`](https://github.com/awslabs/smithy-rs/tree/jjant/smithy-rpc-v2-exploration) branch. Tracking issue: #3573. ## Caveats `TODO`s are currently exhaustively sprinkled throughout the patch documenting what remains to be done. Most of these need to be addressed before this can be merged in; some can be punted on to not make this PR bigger. However, I'd like to call out the major caveats and blockers here. I'll keep updating this list as the patchset evolves. - [x] RPC v2 has still not been added to the Smithy specification. It is currently being worked on over in the [`smithy-rpc-v2`](https://github.com/awslabs/smithy/tree/smithy-rpc-v2) branch. The following are prerrequisites for this PR to be merged; **until they are done CI on this PR will fail**: - [x] Smithy merges in RPC v2 support. - [x] Smithy releases a new version incorporating RPC v2 support. - Released in [Smithy v1.47](https://github.com/smithy-lang/smithy/releases/tag/1.47.0) - [x] smithy-rs updates to the new version. - Updated in #3552 - [x] ~Protocol tests for the protocol do not currently exist in Smithy. Until those get written~, this PR resorts to Rust unit tests and integration tests that use `serde` to round-trip messages and compare `serde`'s encoders and decoders with ours for correctness. - Protocol tests are under the [`smithy-protocol-tests`](https://github.com/smithy-lang/smithy/tree/main/smithy-protocol-tests/model/rpcv2Cbor) directory in Smithy. - We're keeping the `serde_cbor` round-trip tests for defense in depth. - [ ] #3709 - Currently only server-side support has been implemented, because that's what I'm most familiar. However, we're almost all the way there to add client-side support. - ~[ ] [Smithy `document` shapes](https://smithy.io/2.0/spec/simple-types.html#document) are not supported. RPC v2's specification currently doesn't indicate how to implement them.~ - [The spec](https://smithy.io/2.0/additional-specs/protocols/smithy-rpc-v2.html#shape-serialization) ended up leaving them as unsupported: "Document types are not currently supported in this protocol." ## Prerequisite PRs This section lists prerequisite PRs and issues that would make the diff for this one lighter or easier to understand. It's preferable that these PRs be merged prior to this one; some are hard prerequisites. They mostly relate to parts of the codebase I've had to touch or ~pilfer~ inspect in this PR where I've made necessary changes, refactors and "drive-by improvements" that are mostly unrelated, although some directly unlock things I've needed in this patchset. It makes sense to pull them out to ease reviewability and make this patch more semantically self-contained. - #2516 - #2517 - #2522 - #2524 - #2528 - #2536 - #2537 - #2531 - #2538 - #2539 - #2542 - #3684 - #3678 - #3690 - #3713 - #3726 - #3752 ## Testing <!--- Please describe in detail how you tested your changes --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ~RPC v2 has still not been added to the Smithy specification. It is currently being worked on over in the [`smithy-rpc-v2`](https://github.com/awslabs/smithy/tree/smithy-rpc-v2) branch.~ This can only be tested _locally_ following these steps: ~1. Clone [the Smithy repository](https://github.com/smithy-lang/smithy/tree/smithy-rpc-v2) and checkout the `smithy-rpc-v2` branch. 2. Inside your local checkout of smithy-rs pointing to this PR's branch, make sure you've added `mavenLocal()` as a repository in the `build.gradle.kts` files. [Example](8df82fd). 4. Inside your local checkout of Smithy's `smithy-rpc-v2` branch: 1. Set `VERSION` to the current Smithy version used in smithy-rs (1.28.1 as of writing, but [check here](https://github.com/awslabs/smithy-rs/blob/main/gradle.properties#L21)). 2. Run `./gradlew clean build pTML`.~ ~6.~ 1. In your local checkout of the smithy-rs's `smithy-rpc-v2` branch, run `./gradlew codegen-server-test:build -P modules='rpcv2Cbor'`. ~You can troubleshoot whether you have Smithy correctly set up locally by inspecting `~/.m2/repository/software/amazon/smithy/smithy-protocols-traits`.~ ## Checklist <!--- If a checkbox below is not applicable, then please DELETE it rather than leaving it unchecked --> - [ ] 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._
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit gets rid of some Clippy and Rust doc warnings we produce in
generated code, which are surfaced in our codegen integration tests. The
aim is that eventually we will be able to deny Clippy and Rust doc
warnings in #3678 (despite us baking in
RUSTDOCFLAGS="-D warnings"andRUSTFLAGS="-D warnings"in our CI Docker image, we curently clearthese in codegen integration tests). Note that denying compiler warnings
is separately tracked in #3194.
The commit contains fixes for the following:
crate::typesmodule in Rust docs whenthe Smithy model is such that it is not generated.
operation names in Rust docs.
plugins.
HTML, that are interpreted as broken links.
TryFrominstead ofFromwhen infallibly converting from&strtoStringwhen deserializing an@httpPayloadin the server.unwrap_or_elsewhen falling back to a@defaultboolean or number value for adocumentshape, instead ofunwrap_or.clippy::enum_variant_nameswhen rendering theOperationenum in the server (since we render the operation namesthat were modeled as-is).
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.