Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XML serializer fails Clippy when serializing Strings #1831

Closed
david-perez opened this issue Oct 7, 2022 · 0 comments · Fixed by #1832
Closed

XML serializer fails Clippy when serializing Strings #1831

david-perez opened this issue Oct 7, 2022 · 0 comments · Fixed by #1832
Labels
bug Something isn't working

Comments

@david-perez
Copy link
Contributor

It can serialize Option<String>, which is why we never realized this basic shortcoming (only the client SDKs heavily exercise the restXml protocol in CI).

The problem manifests itself easily when generating a server SDK, since required is honored as non-Optional String.

Model:

$version: "2.0"

namespace com.amazonaws.simple

use aws.protocols#restXml

@restXml
service SimpleService {
    version: "2022-01-01",
    operations: [
        SimpleOperation,
    ],
}

@http(uri: "/simple-operation", method: "GET")
operation SimpleOperation {
    output: SimpleOperationOutput,
}

structure SimpleOperationOutput {
    @required
    requiredString: String,
}

Generated code (input.required_string is of type String):

pub fn serialize_structure_crate_output_simple_operation_output(
    input: &crate::output::SimpleOperationOutput,
    writer: aws_smithy_xml::encode::ElWriter,
) -> Result<(), aws_smithy_http::operation::SerializationError> {
    #[allow(unused_mut)]
    let mut scope = writer.finish();
    {
        let mut inner_writer = scope.start_el("requiredString").finish();
        inner_writer.data(&input.required_string.as_ref());
    }
    scope.finish();
    Ok(())
}

Clippy output:

error: this expression creates a reference which is immediately dereferenced by the compiler
  --> simple/rust-server-codegen/src/xml_ser.rs:10:27
   |
10 |         inner_writer.data(&input.required_string.as_ref());
   |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `input.required_string.as_ref()`
   |
   = note: `-D clippy::needless-borrow` implied by `-D warnings`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

Relevant code:

https://github.com/awslabs/smithy-rs/blob/e78da5598a2d94d23fd18e30492a20ce3603f844/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/serialize/XmlBindingTraitSerializerGenerator.kt#L292-L292

@david-perez david-perez added the bug Something isn't working label Oct 7, 2022
david-perez added a commit that referenced this issue Oct 10, 2022
The logic that keeps track of whether we're working with a borrowed or
an owned value is flawed. It just so happened to work when requesting
owned values from borrowed values because we called `autoDeref` before
creating the value expression. That logic should instead be used
_within_ `ValueExpression` to appease Clippy there too.

Fixes #1831.
david-perez added a commit that referenced this issue Oct 13, 2022
The logic that keeps track of whether we're working with a borrowed or
an owned value is flawed. It just so happened to work when requesting
owned values from borrowed values because we called `autoDeref` before
creating the value expression. That logic should instead be used
_within_ `ValueExpression` to appease Clippy there too.

Fixes #1831.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant