dtolnay / dtolnay/thiserror

Semver question: Is the non-qualified call to `.as_dyn_error()` intentional?

Open
#452 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
5.5k
Forks
213
PR merge metrics
No merged PRs in 30d

Description

Right now the thiserror macro [will generate code](https://github.com/dtolnay/thiserror/blob/ec42ea70852f8db43e971700d1ccc184957d1b32/impl/src/expand.rs#L236) that invokes the hidden [`AsDynError` trait](https://github.com/dtolnay/thiserror/blob/ec42ea70852f8db43e971700d1ccc184957d1b32/src/aserror.rs#L6). However it doesn't use a fully qualified path, but instead uses method-call resolution, where an inherent method on the field type shadows the trait method.

Is this an intentional choice to allow external implementations of the method?

**Context**: In rootcause we got a [feature request](https://github.com/rootcause-rs/rootcause/issues/144) to support placing a rootcause report inside enums with thiserror impls using `#[error(transparent)]`.

While we don't recommend this way of combining thiserror and rootcause, we do consider it a helpful feature to make migrations of large codebases easier.

We added the feature using a `Deref` impl, but we had to [roll back the change](https://github.com/rootcause-rs/rootcause/pull/189).

However after rolling back the change, I noticed that the `#[error(transparent)]` code still worked. It turns out, this is because we also named our method `.as_dyn_error()`, so everything happened to still work.

This makes me a bit worried, as "internal" changes in thiserror can now cause accidental breakage in consumers that use both thiserror and rootcause.

It'd be good to know where you land on this. If external `as_dyn_error()` impls are fair game, then we will document that this is a supported (but non-recommended) way to combine rootcause with thiserror. Otherwise we'd rather you make the call fully-qualified so the behavior breaks now instead of by accident later.

Contributor guide

No contributing guide indexed for this repository

Research direction

Read impl/src/expand.rs around the generated .as_dyn_error() call and src/aserror.rs for the hidden AsDynError trait. Reproduce the method-resolution behavior described in the issue, then determine whether external methods are supported. Done means documenting that contract or deciding on a fully qualified call, with the compatibility impact addressed.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.