Implement Clone for error enums by zeenix · Pull Request #521 · tafia/quick-xml

zeenix · 2022-12-06T17:50:07Z

This would allow using crates to embed errors from this crate into their error types that implement Clone.

Unfortunately std::io::Error does not implement Clone so we've to loose some of its state during cloning. The most importat bit (error kind) is preserved though.

Mingun · 2022-12-06T19:27:47Z

Cloneable errors seems not in a trend in Rust, it is unknown what the long-term consequences of this. I'm not totally against this PR, but I would like to hear what the situations where you need to clone errors?

zeenix · 2022-12-06T19:46:35Z

Cloneable errors seems not in a trend in Rust, it is unknown what the long-term consequences of this. I'm not totally against this PR, but I would like to hear what the situations where you need to clone errors?

Thanks for the quick response and that's very fair. My own use case: I've a task that owns a socket and reads and parses messages from the socket and then forwards them to all interested parties (streams) through a broadcast channel. Since Result only implements Clone when E: Clone, I can not send a Result the channel. Currently I work around this by using a broadcast channel only for success and a separate channel for error but that means that I only communicate the error to one of the streams.

Please note that in my crate, I'm fixing the std::io::Error issue a bit differently, which won't work for you unless you're ok with breaking API (my error type is non_exhaustive) so I went with the approach in the PR.

Mingun

I'm fine with replacing Error::Io(IoError) to Error::Io(Arc<IoError>), which, I think, would be more elegant solution. We also can hide that under a feature flag (say, cloneable-errors), but I think that this is not required (besides, that is anti-pattern, because that create a incompatible API depending on the feature flags, which should be avoided). @dralley, what to you think?

Whatever path we choose, it is important to describe it in the documentation for Error, and add a changelog entry.

I think, that will resolve #360?

zeenix · 2022-12-07T12:41:04Z

I'm fine with replacing Error::Io(IoError) to Error::Io(Arc<IoError>), which, I think, would be more elegant solution.

I completely agree. I will revise the commits after hearing opinion of @dralley.

Whatever path we choose, it is important to describe it in the documentation for Error,
and add a changelog entry.

For sure. I'll do that as part of the update.

zeenix · 2022-12-07T12:47:18Z

I think, that will resolve #360?

Not sure. single consumer APIs should not require cloning.

dralley · 2022-12-07T21:37:57Z

I'm fine with Arc<IoError>. Agreed about not wanting divergent signatures.

zeenix · 2022-12-07T23:30:29Z

@Mingun Updated the commits. PTALA.

Mingun

impl<E: Error> Error for Arc<E> was implemented in Rust 1.52, so need to bump our MSRV. Please add a note about that to the Changelog, before "New Features" section

Changelog.md

We'll be making use `impl<E: Error> Error for Arc<E>` that was introduced in 1.52.

The `?` handles the tranformation for us.

This would allow using crates to embed errors from this crate into their error types that implement Clone. Unfortunately `std::io::Error` does not implement `Clone` [1] so we've to wrap it in an `Arc`.

zeenix · 2022-12-08T16:13:39Z

Lol, apparently GH wants me to only request review from one person. :) Sorry about adding and removing dance.

Mingun · 2022-12-08T19:36:39Z

Thanks!

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [quick-xml](https://github.com/tafia/quick-xml) | dependencies | minor | `0.26.0` -> `0.27.1` | --- ### Release Notes <details> <summary>tafia/quick-xml</summary> ### [`v0.27.1`](https://github.com/tafia/quick-xml/blob/HEAD/Changelog.md#0271----2022-12-28) [Compare Source](tafia/quick-xml@v0.27.0...v0.27.1) ##### Bug Fixes - [#530]: Fix an infinite loop leading to unbounded memory consumption that occurs when skipping events on malformed XML with the `overlapped-lists` feature active. - [#530]: Fix an error in the `Deserializer::read_to_end` when `overlapped-lists` feature is active and malformed XML is parsed [#530]: tafia/quick-xml#530 ### [`v0.27.0`](https://github.com/tafia/quick-xml/blob/HEAD/Changelog.md#0270----2022-12-25) [Compare Source](tafia/quick-xml@v0.26.0...v0.27.0) ##### New Features - [#521]: Implement `Clone` for all error types. This required changing `Error::Io` to contain `Arc<std::io::Error>` instead of `std::io::Error` since `std::io::Error` does not implement `Clone`. ##### Bug Fixes - [#490]: Ensure that serialization of map keys always produces valid XML names. In particular, that means that maps with numeric and numeric-like keys (for example, `"42"`) no longer can be serialized because [XML name] cannot start from a digit - [#500]: Fix deserialization of top-level sequences of enums, like ```xml <?xml version="1.0" encoding="UTF-8"?>  <A/> <B/> <C/> ``` - [#514]: Fix wrong reporting `Error::EndEventMismatch` after disabling and enabling `.check_end_names` - [#517]: Fix swapped codes for `\r` and `\n` characters when escaping them - [#523]: Fix incorrect skipping text and CDATA content before any map-like structures in serde deserializer, like ```xml unwanted text<struct>...</struct> ``` - [#523]: Fix incorrect handling of `xs:list`s with encoded spaces: they still act as delimiters, which is confirmed also by mature XmlBeans Java library - [#473]: Fix a hidden requirement to enable serde's `derive` feature to get quick-xml's `serialize` feature for `edition = 2021` or `resolver = 2` crates ##### Misc Changes - [#490]: Removed `$unflatten=` special prefix for fields for serde (de)serializer, because: - it is useless for deserializer - serializer was rewritten and does not require it anymore This prefix allowed you to serialize struct field as an XML element and now replaced by a more thoughtful system explicitly indicating that a field should be serialized as an attribute by prepending `@` character to its name - [#490]: Removed `$primitive=` prefix. That prefix allowed you to serialize struct field as an attribute instead of an element and now replaced by a more thoughtful system explicitly indicating that a field should be serialized as an attribute by prepending `@` character to its name - [#490]: In addition to the `$value` special name for a field a new `$text` special name was added: - `$text` is used if you want to map field to text content only. No markup is expected (but text can represent a list as defined by `xs:list` type) - `$value` is used if you want to map elements with different names to one field, that should be represented either by an `enum`, or by sequence of `enum`s (`Vec`, tuple, etc.), or by string. Use it when you want to map field to any content of the field, text or markup Refer to [documentation] for details. - [#521]: MSRV bumped to 1.52. - [#473]: `serde` feature that used to make some types serializable, renamed to `serde-types` - [#528]: Added documentation for XML to `serde` mapping [#473]: tafia/quick-xml#473 [#490]: tafia/quick-xml#490 [#500]: tafia/quick-xml#500 [#514]: tafia/quick-xml#514 [#517]: tafia/quick-xml#517 [#521]: tafia/quick-xml#521 [#523]: tafia/quick-xml#523 [#528]: tafia/quick-xml#528 [XML name]: https://www.w3.org/TR/xml11/#NT-Name [documentation]: https://docs.rs/quick-xml/0.27.0/quick_xml/de/index.html#difference-between-text-and-value-special-names </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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <[email protected]> Co-authored-by: crapStone <[email protected]> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1692 Reviewed-by: crapStone <[email protected]> Co-authored-by: Calciumdibromid Bot <[email protected]> Co-committed-by: Calciumdibromid Bot <[email protected]>

Mingun requested changes Dec 7, 2022

View reviewed changes

zeenix force-pushed the clone-on-error branch from bf171cc to 8025695 Compare December 7, 2022 23:29

zeenix requested a review from Mingun December 7, 2022 23:30

Mingun approved these changes Dec 8, 2022

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

zeenix force-pushed the clone-on-error branch from 40e8e5f to 003c7bc Compare December 8, 2022 15:26

zeenix requested a review from Mingun December 8, 2022 15:26

dralley reviewed Dec 8, 2022

View reviewed changes

Changelog.md Outdated Show resolved Hide resolved

zeenix added 3 commits December 8, 2022 17:10

Bump MSRV to 1.52

0fb0344

We'll be making use `impl<E: Error> Error for Arc<E>` that was introduced in 1.52.

Drop some unneeded Result::map_err calls

2e5d897

The `?` handles the tranformation for us.

Implement Clone for error enums

661d117

This would allow using crates to embed errors from this crate into their error types that implement Clone. Unfortunately `std::io::Error` does not implement `Clone` [1] so we've to wrap it in an `Arc`.

zeenix force-pushed the clone-on-error branch from 003c7bc to 661d117 Compare December 8, 2022 16:11

zeenix requested review from Mingun and dralley and removed request for Mingun and dralley December 8, 2022 16:12

Mingun merged commit a888cb6 into tafia:master Dec 8, 2022

zeenix deleted the clone-on-error branch December 8, 2022 21:24

Mingun mentioned this pull request Dec 11, 2022

Fix couple of bugs in serde deserializer #523

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Implement Clone for error enums#521

Implement Clone for error enums#521
Mingun merged 3 commits intotafia:masterfrom
zeenix:clone-on-error

zeenix commented Dec 6, 2022

Uh oh!

Mingun commented Dec 6, 2022

Uh oh!

zeenix commented Dec 6, 2022 •

edited

Loading

Uh oh!

Mingun left a comment •

edited

Loading

Uh oh!

zeenix commented Dec 7, 2022

Uh oh!

zeenix commented Dec 7, 2022

Uh oh!

dralley commented Dec 7, 2022

Uh oh!

zeenix commented Dec 7, 2022

Uh oh!

Mingun left a comment

Uh oh!

Uh oh!

Uh oh!

zeenix commented Dec 8, 2022

Uh oh!

Mingun commented Dec 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

zeenix commented Dec 6, 2022

Uh oh!

Mingun commented Dec 6, 2022

Uh oh!

zeenix commented Dec 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mingun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeenix commented Dec 7, 2022

Uh oh!

zeenix commented Dec 7, 2022

Uh oh!

dralley commented Dec 7, 2022

Uh oh!

zeenix commented Dec 7, 2022

Uh oh!

Mingun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zeenix commented Dec 8, 2022

Uh oh!

Mingun commented Dec 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zeenix commented Dec 6, 2022 •

edited

Loading

Mingun left a comment •

edited

Loading