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

Allow to flatten IgnoredAny #2436

Merged
merged 2 commits into from Jul 6, 2023
Merged

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Apr 29, 2023

This can be useful for use-case described in #2432 (if other problems will be solved) and if you have a generic struct with flatten field and want to pass an IgnoredAny for whatever reason.

I also implement PartialEq for IgnoredAny so it can be tested using assert_de_tokens.

Although any additional fields in struct by default are ignored, sometimes
this can be useful, if you use generic structures, for example
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Independent of #2437, I am not sure whether this still has a use case. Could you let me know whether this PR is still needed?

For deserializing struct containing flattened fields from a map, I expect people would not need to write #[serde(flatten)] inner: IgnoredAny because ignoring all other fields is already the default behavior.

Thanks anyway for the PR!

@Mingun
Copy link
Contributor Author

Mingun commented May 5, 2023

First of all, this is logical from consistency point-of-view.
Secondly, having this implementation allows to write a generic code:

#[derive(Deserialize)]
struct Generic<T> {
  my: String,
  fields: String,

  #[serde(flatten)]
  other: T,
}

type OnlyMyFields = Generic<IgnoreAny>;

It is better to preserve the principle of least astonishment:

  • we have a generic field
  • we know that we could pass any impl Deserialize type as generic parameter
  • we know that we could ignore some value using IgnoredAny
  • we know that IgnoredAny implements Deserialize
  • we know that we could flatten any compound values
  • we know that we could combine rules

So actually when mixing rules does not produce expected result, it is strange, especially if there is no any rationale explanations why their are not working.

@dtolnay
Copy link
Member

dtolnay commented May 5, 2023

Do we know of any code that uses or has wanted to use flatten in that manner with a generic type?

In general "least astonishment" is not compelling to me as it's in tension with "least bloat" and the intersection between features can be disproportionately costly to support for the sake of something that literally nobody might want in the first place.

@Mingun
Copy link
Contributor Author

Mingun commented May 6, 2023

The point is not whether we know or not. I believe that the library should not limit the user to strange, not explained restrictions.

can be disproportionately costly to support

I think, that here definitely not this case. In fact, this PR decreases cost for support, because it removes special case and adds an explicit test for it. In contrast, in current situation you actually supports invariant that IgnoredAny cannot be type of the #[serde(flatten)] field, but you do that without a test that check that this invariant is hold.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Sounds good. Thanks for the PR!

@dtolnay dtolnay merged commit 3686277 into serde-rs:master Jul 6, 2023
19 checks passed
@Mingun Mingun deleted the flatten-ignored-any branch July 7, 2023 04:29
crapStone added a commit to Calciumdibromid/CaBr2 that referenced this pull request Jul 10, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [serde](https://serde.rs) ([source](https://github.com/serde-rs/serde)) | dependencies | patch | `1.0.167` -> `1.0.171` |

---

### Release Notes

<details>
<summary>serde-rs/serde (serde)</summary>

### [`v1.0.171`](https://github.com/serde-rs/serde/releases/tag/v1.0.171)

[Compare Source](serde-rs/serde@v1.0.170...v1.0.171)

-   Support `derive(Deserialize)` on unit structs that have const generics ([#&#8203;2500](serde-rs/serde#2500), thanks [@&#8203;Baptistemontan](https://github.com/Baptistemontan))

### [`v1.0.170`](https://github.com/serde-rs/serde/releases/tag/v1.0.170)

[Compare Source](serde-rs/serde@v1.0.169...v1.0.170)

-   Produce error message on suffixed string literals inside serde attributes ([#&#8203;2242](serde-rs/serde#2242))
-   Support single identifier as unbraced default value for const generic parameter ([#&#8203;2449](serde-rs/serde#2449))

### [`v1.0.169`](https://github.com/serde-rs/serde/releases/tag/v1.0.169)

[Compare Source](serde-rs/serde@v1.0.168...v1.0.169)

-   Add Deserializer::deserialize_identifier support for adjacently tagged enums ([#&#8203;2475](serde-rs/serde#2475), thanks [@&#8203;Baptistemontan](https://github.com/Baptistemontan))
-   Fix unused_braces lint in generated Deserialize impl that uses braced const generic expressions ([#&#8203;2414](serde-rs/serde#2414))

### [`v1.0.168`](https://github.com/serde-rs/serde/releases/tag/v1.0.168)

[Compare Source](serde-rs/serde@v1.0.167...v1.0.168)

-   Allow `serde::de::IgnoredAny` to be the type for a `serde(flatten)` field ([#&#8203;2436](serde-rs/serde#2436), thanks [@&#8203;Mingun](https://github.com/Mingun))
-   Allow larger preallocated capacity for smaller elements ([#&#8203;2494](serde-rs/serde#2494))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi42LjAiLCJ1cGRhdGVkSW5WZXIiOiIzNi42LjAiLCJ0YXJnZXRCcmFuY2giOiJkZXZlbG9wIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Co-authored-by: crapStone <crapstone01@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1959
Reviewed-by: crapStone <crapstone01@gmail.com>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants