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

Derive Eq and Hash wherever possible #2223

Merged
merged 17 commits into from
Jan 19, 2023
Merged

Conversation

david-perez
Copy link
Contributor

@david-perez david-perez commented Jan 18, 2023

In server SDKs, these traits can be implemented by any shape except if
the shape's closure contains:

  1. A float, double, or document shape: floating point types in
    Rust do not implement Eq. Similarly, document shapes may
    contain arbitrary JSON-like data containing floating point values.
  2. A @streaming shape: all the streaming data would need to be
    buffered first to compare it.

Additionally, the Hash trait cannot be implemented by shapes whose
closure contains:

  1. A map shape: we render map shapes as std::collections::HashMap,
    which do not implement Hash. See
    Disallow @uniqueItems on list shapes whose closure reaches a map shape smithy#1567.

In client SDKs, these traits cannot be derived on any code-generated
Rust types corresponding to Smithy shapes
, since e.g. adding new
optional members to a structure is a backwards-compatible change, and
doing so alters the semantics of these traits.

However, this commit does implement these traits for the
aws_smithy_types::date_time::DateTime and aws_smithy_types::Blob
runtime types.

This change is necessary to efficiently implement the @uniqueItems
constraint trait in server SDKs.

Testing

I added tests.

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.

In server SDKs, these traits can be implemented by any shape _except_ if
the shape's closure contains:

1. A `float`, `double`, or `document` shape: floating point types in
   Rust do not implement `Eq`. Similarly, [`document` shapes] may
   contain arbitrary JSON-like data containing floating point values.
2. A [@streaming] shape: all the streaming data would need to be
   buffered first to compare it.

Additionally, the `Hash` trait cannot be implemented by shapes whose
closure contains:

1. A `map` shape: we render `map` shapes as `std::collections::HashMap`,
   which _do not_ implement `Hash`. See
   smithy-lang/smithy#1567.

In **client SDKs, these traits cannot be derived on any code-generated
Rust types corresponding to Smithy shapes**, since e.g. adding new
optional members to a structure [is a backwards-compatible change], and
doing so alters the semantics of these traits.

However, this commit does implement these traits for the
`aws_smithy_types::date_time::DateTime` and `aws_smithy_types::Blob`
runtime types.

This change is necessary to efficiently implement the `@uniqueItems`
constraint trait in server SDKs.

[`Eq`]: https://doc.rust-lang.org/std/cmp/trait.Eq.html
[`Hash`]: https://doc.rust-lang.org/std/hash/trait.Hash.html
[`document` shapes]: https://smithy.io/2.0/spec/simple-types.html#document
[@streaming]: https://smithy.io/2.0/spec/streaming.html
[is a backwards-compatible change]: https://smithy.io/2.0/guides/evolving-models.html#updating-structures
@david-perez david-perez requested review from a team as code owners January 18, 2023 13:44
@david-perez david-perez requested review from drganjoo and weihanglo and removed request for drganjoo and weihanglo January 18, 2023 13:44
@david-perez
Copy link
Contributor Author

I'll try to pull out the changes to constrained types in a separate PR so that this patch is more easily reviewable by the @awslabs/rust-sdk-owners team.

In server SDKs, these traits can be implemented by any shape _except_ if
the shape's closure contains:

1. A `float`, `double`, or `document` shape: floating point types in
   Rust do not implement `Eq`. Similarly, [`document` shapes] may
   contain arbitrary JSON-like data containing floating point values.
2. A [@streaming] shape: all the streaming data would need to be
   buffered first to compare it.

Additionally, the `Hash` trait cannot be implemented by shapes whose
closure contains:

1. A `map` shape: we render `map` shapes as `std::collections::HashMap`,
   which _do not_ implement `Hash`. See
   smithy-lang/smithy#1567.

In **client SDKs, these traits cannot be derived on any code-generated
Rust types corresponding to Smithy shapes**, since e.g. adding new
optional members to a structure [is a backwards-compatible change], and
doing so alters the semantics of these traits.

However, this commit does implement these traits for the
`aws_smithy_types::date_time::DateTime` and `aws_smithy_types::Blob`
runtime types.

This change is necessary to efficiently implement the `@uniqueItems`
constraint trait in server SDKs.

This commit also introduces a constrained shape symbol metadata provider
(`ConstrainedShapeSymbolMetadataProvider.kt`), to centralize generation
of Rust metadata (derives, visibility) in one place, instead of each
constrained type generator having to manually adjust metadata. Some
constrained type methods are now conditionally generated based on
visibility, instead of relying on `#[allow(dead_code)]`.

[`Eq`]: https://doc.rust-lang.org/std/cmp/trait.Eq.html
[`Hash`]: https://doc.rust-lang.org/std/hash/trait.Hash.html
[`document` shapes]: https://smithy.io/2.0/spec/simple-types.html#document
[@streaming]: https://smithy.io/2.0/spec/streaming.html
[is a backwards-compatible change]: https://smithy.io/2.0/guides/evolving-models.html#updating-structures
@smithy-lang smithy-lang deleted a comment from github-actions bot Jan 18, 2023
@smithy-lang smithy-lang deleted a comment from github-actions bot Jan 18, 2023
@david-perez
Copy link
Contributor Author

Actually, I can't break this PR down any further:

  • If I remove Eq, Hash from aws_smithy_types::date_time::DateTime or aws_smithy_types::Blob, any server shape with these types in its closure cannot implement Eq or Hash.
  • If I pull out the constrained shape symbol metadata provider, I have to derive Eq and Hash for constrained types, which may have aws_smithy_types::date_time::DateTime or aws_smithy_types::Blob in its closure.

So this patch has to be merged atomically. I've updated the changelog and the commit message to better reflect all changes.


@awslabs/rust-sdk-owners team: you just have to review the changes to these files:

  • rust-runtime/aws-smithy-types/src/date_time/mod.rs
  • rust-runtime/aws-smithy-types/src/lib.rs
  • CHANGELOG.next.toml

@awslabs/smithy-rs-server team: you have to review everything.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez
Copy link
Contributor Author

david-perez commented Jan 18, 2023

@crisidev @unexge All right I added Eq and Hash to aws_smithy_http_server_python::types::Blob, DateTime}, and added DeriveEqAndHashSymbolMetadataProvider and ConstrainedShapeSymbolMetadataProvider to the Python symbol provider in PythonCodegenServerPlugin, in f1f0b3d and ad47354.

Copy link
Collaborator

@rcoh rcoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm re: eq/hash derives

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@crisidev crisidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'd wait for @unexge review as well on this.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@david-perez david-perez merged commit adf30a8 into main Jan 19, 2023
@david-perez david-perez deleted the davidpz/derive-eq-and-hash branch January 19, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants