Skip to content

add missing impl Errors (breaking change)#130

Merged
jamesmunns merged 1 commit into
jamesmunns:mainfrom
rursprung:add-missing-Error-impls
Oct 23, 2025
Merged

add missing impl Errors (breaking change)#130
jamesmunns merged 1 commit into
jamesmunns:mainfrom
rursprung:add-missing-Error-impls

Conversation

@rursprung

Copy link
Copy Markdown
Contributor

use thiserror to do this as it's already used in the crate. this updates it from v1 to v2 and also uses it in no_std environments as that's now supported by thiserror.

this was missing and thus it wasn't possible to use e.g. HostErr in a #[from] with thiserror.

@rursprung

Copy link
Copy Markdown
Contributor Author

just spotted in the diff that cargo fmt --all formatted a bit more than just my code... i hope that's ok? probably should add a CI check for proper formatting?

@rursprung

Copy link
Copy Markdown
Contributor Author

just spotted in the diff that cargo fmt --all formatted a bit more than just my code... i hope that's ok? probably should add a CI check for proper formatting?

i've undone the re-formatting here and instead did it for all crates in #131 where i also added a CI check for it


/// A conversion trait to convert a user error into a base Kind type
pub trait AsWireTxErrorKind {
pub trait AsWireTxErrorKind: core::error::Error {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This and line 150 makes this a breaking change according to cargo semver-checks:

james@magician ➜  postcard-rpc git:(add-missing-Error-impls) cargo semver-checks
    Building postcard-rpc v0.11.12 (current)
       Built [   9.633s] (current)
     Parsing postcard-rpc v0.11.12 (current)
      Parsed [   0.016s] (current)
    Building postcard-rpc v0.11.12 (baseline)
       Built [   9.127s] (baseline)
     Parsing postcard-rpc v0.11.12 (baseline)
      Parsed [   0.015s] (baseline)
    Checking postcard-rpc v0.11.12 -> v0.11.12 (no change)
     Checked [   0.020s] 153 checks: 152 pass, 1 fail, 0 warn, 11 skip

--- failure trait_added_supertrait: non-sealed trait added new supertraits ---

Description:
A non-sealed trait added one or more supertraits, which breaks downstream implementations of the trait
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#generic-bounds-tighten
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.41.0/src/lints/trait_added_supertrait.ron

Failed in:
  trait postcard_rpc::server::AsWireRxErrorKind gained Error in file /Users/james/personal/postcard-rpc/source/postcard-rpc/src/server/mod.rs:150
  trait postcard_rpc::server::AsWireTxErrorKind gained Error in file /Users/james/personal/postcard-rpc/source/postcard-rpc/src/server/mod.rs:97

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  22.978s] postcard-rpc

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

are these traits supposed to be implemented by anyone outside of postcard-rpc? if not then it wouldn't be breaking, right?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

They are public, so they can be implemented by anyone: https://docs.rs/postcard-rpc/latest/postcard_rpc/server/index.html#traits

@jamesmunns

Copy link
Copy Markdown
Owner

I'm overall fine with this, though this is a breaking change. We might have one soon anyway when the embassy-executor changes are worked through, so maybe we leave this PR open for a bit and we can merge it when getting ready for a v0.12.

@rursprung

Copy link
Copy Markdown
Contributor Author

I'm overall fine with this, though this is a breaking change. We might have one soon anyway when the embassy-executor changes are worked through, so maybe we leave this PR open for a bit and we can merge it when getting ready for a v0.12.

otherwise i can split it up and just add the impl Error for HostErr (which is the one i actually care about) in a separate PR to get that in as a non-breaking change?

@jamesmunns

Copy link
Copy Markdown
Owner

Feel free to break it up if you'd like! Happy to merge for v0.11.x as long as cargo semver-checks is happy, otherwise happy to merge as part of v0.12 in the nearish future.

@rursprung

Copy link
Copy Markdown
Contributor Author

Feel free to break it up if you'd like! Happy to merge for v0.11.x as long as cargo semver-checks is happy, otherwise happy to merge as part of v0.12 in the nearish future.

i've split it to #132 for now

use `thiserror` to do this as it's already used in the crate. this
updates it from v1 to v2 and also uses it in `no_std` environments as
that's now supported by `thiserror`.

this was missing and thus it wasn't possible to use e.g. `HostErr` in a
`#[from]` with `thiserror`.
@rursprung rursprung force-pushed the add-missing-Error-impls branch from 4ec7cbc to 75d0643 Compare August 4, 2025 10:11
@rursprung

Copy link
Copy Markdown
Contributor Author

rebased on top of #132

@rursprung rursprung requested a review from jamesmunns August 4, 2025 10:11
@jamesmunns jamesmunns changed the title add missing impl Errors add missing impl Errors (breaking change) Aug 4, 2025
@jamesmunns

Copy link
Copy Markdown
Owner

(updated the title so I don't forget and merge this without thinking about it :) )

@jamesmunns

Copy link
Copy Markdown
Owner

It's breaking change time!

@jamesmunns jamesmunns merged commit 5855252 into jamesmunns:main Oct 23, 2025
3 checks passed
@rursprung rursprung deleted the add-missing-Error-impls branch October 24, 2025 07:03
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.

2 participants