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

Make #[serde(alias)] works in #[serde(flatten)] context #2387

Merged
merged 2 commits into from Mar 7, 2023

Conversation

bebecue
Copy link
Contributor

@bebecue bebecue commented Mar 5, 2023

fix #1504

@oli-obk oli-obk self-assigned this Mar 5, 2023
@oli-obk
Copy link
Member

oli-obk commented Mar 6, 2023

Please also add a few tests for a struct like the one from the issue.

@bebecue
Copy link
Contributor Author

bebecue commented Mar 7, 2023

Please also add a few tests for a struct like the one from the issue.

Added in 01da3f7

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.

Thanks!

In #2160 (review) I was hopeful that somebody would look into what possibility of breakage this has for existing formats, but in the absence of anyone having been willing to do that, I think it will be all right to reassess based on any reported consequences.

@dtolnay dtolnay merged commit cbd1cbe into serde-rs:master Mar 7, 2023
19 checks passed
crapStone added a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 15, 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.152` -> `1.0.156` |

---

### Release Notes

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

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

[Compare Source](serde-rs/serde@v1.0.155...v1.0.156)

-   Documentation improvements

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

[Compare Source](serde-rs/serde@v1.0.154...v1.0.155)

-   Support `Serialize` and `Deserialize` impls for `core::ffi::CStr` and `alloc::ffi::CString` without "std" feature ([#&#8203;2374](serde-rs/serde#2374), thanks [@&#8203;safarir](https://github.com/safarir))

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

[Compare Source](serde-rs/serde@v1.0.153...v1.0.154)

-   Fix "undeclared lifetime" error in generated code when deriving Deserialize for an enum with both `flatten` and `'static` fields ([#&#8203;2383](serde-rs/serde#2383), thanks [@&#8203;Mingun](https://github.com/Mingun))

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

[Compare Source](serde-rs/serde@v1.0.152...v1.0.153)

-   Support `serde(alias = "…")` attribute used inside of flattened struct ([#&#8203;2387](serde-rs/serde#2387), thanks [@&#8203;bebecue](https://github.com/bebecue))

</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:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNTcuMSIsInVwZGF0ZWRJblZlciI6IjM1LjIuMyJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Co-authored-by: crapStone <crapstone01@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1812
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@janik-cloudflare
Copy link

janik-cloudflare commented Mar 23, 2023

Thanks a ton @bebecue, @dtolnay and everyone else! Just ran into this and it's fantastic to see that a fix is already out. Really appreciate your work on serde :)

Mingun added a commit to Mingun/serde that referenced this pull request May 6, 2023
Mingun added a commit to Mingun/serde that referenced this pull request Jun 9, 2023
Mingun added a commit to Mingun/serde that referenced this pull request Jul 8, 2023
Mingun added a commit to Mingun/serde that referenced this pull request Aug 1, 2023
dtolnay added a commit that referenced this pull request Aug 2, 2023
Simplify code in deserialize_in_place_struct and implement #2387 for in-place case
@banool
Copy link

banool commented Oct 6, 2023

I was hopeful that somebody would look into what possibility of breakage this has for existing formats, but in the absence of anyone having been willing to do that, I think it will be all right to reassess based on any reported consequences.

I know I'm well and truly late to the party hahah but this change did break some of our code.

At a high level you can see the issue like this:

git clone git@github.com:aptos-labs/aptos-core.git
# Change serde version in Cargo.toml to 1.0.153
cd testsuite/generate-format
cargo run -- --corpus aptos

With 1.0.152 this code works without error.

I tried the latest version of serde (1.0.188 at the time of writing) and the issue seems to persist.

I'm still trying to figure out how this change actually breaks our code, I just wanted to report in with an instance of a breakage. Once I know precisely what it changes for us I'll chime in again, though this looks pretty complicated.

@banool
Copy link

banool commented Oct 6, 2023

btw I looked into some issues in this repo. I know serde complies with semver but I also know that that only applies to public APIs, so I'm not 100% sure if this qualifies as a breaking change. I'll try figure that out and open a new issue if so.

@banool
Copy link

banool commented Oct 11, 2023

Moving to this issue now: #2627.

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.

Field aliases do not work in combination with flatten
6 participants