Skip to content

Make Error::type_id() an inherent method on dyn Error #156252

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.

Sign up for GitHub

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

Open
Jules-Bertholet wants to merge 1 commit into
base: main
Choose a base branch
from
+27 −14
Open

Make Error::type_id() an inherent method on dyn Error #156252

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 27 additions & 14 deletions library/core/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,14 @@ pub trait Error: Debug + Display {
None
}

/// Gets the `TypeId` of `self`.
#[doc(hidden)]
#[unstable(
feature = "error_type_id",
reason = "this is memory-unsafe to override in user code",
issue = "60784"
feature = "error_type_id_internals",
reason = "this is an perma-unstable implementation detail of `<dyn Error + 'static>::type_id()`, \
and memory-unsafe to override in user code",
issue = "none"
)]
fn type_id(&self, _: private::Internal) -> TypeId
fn __type_id(&self) -> TypeId
Copy link
Copy Markdown
Member

@Mark-Simulacrum Mark-Simulacrum May 10, 2026 •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

What am I missing for why this doesn't allow users to unsoundly override this, i.e., the point of having the private::Internal here previously?

View changes since the review

Copy link
Copy Markdown
Contributor Author

@Jules-Bertholet Jules-Bertholet May 10, 2026

Choose a reason for hiding this comment

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

That's what the perma-unstable feature gate is for.

Copy link
Copy Markdown
Contributor

@seanmonstar seanmonstar May 10, 2026

Choose a reason for hiding this comment

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

There was an argument against allowing it to be just unstable in the original PR, that people using nightly can still make unsound things.

Copy link
Copy Markdown
Contributor Author

@Jules-Bertholet Jules-Bertholet May 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

To make an unsound thing with this, you would have to not only use nightly, but also look through the source code, see the doc(hidden) item, and then choose to enable the feature gate despite the warning. If you are that determined to shoot yourself in the foot, Rust cannot stop you, and there are plenty of other feature gates you could use instead. For example, pin!() used to rely on a feature gate much like this (removed by #139114).

Copy link
Copy Markdown
Member

@Mark-Simulacrum Mark-Simulacrum May 10, 2026

Choose a reason for hiding this comment

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

I think I agree, but the PR description reads like this addresses that soundness problem - it's not obvious to me how this is an improvement over what's on main right now...

Copy link
Copy Markdown
Contributor Author

@Jules-Bertholet Jules-Bertholet May 10, 2026

Choose a reason for hiding this comment

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

After this PR, the fn type_id() method on dyn Error could be soundly stabilized, as it's impossible to override it. Only the __type_id() method on the trait itself needs to remain unstable, and that method is not particularly useful

where
Self: 'static,
{
Expand Down Expand Up @@ -260,19 +260,18 @@ pub trait Error: Debug + Display {
fn provide<'a>(&'a self, request: &mut Request<'a>) {}
}

mod private {
// This is a hack to prevent `type_id` from being overridden by `Error`
// implementations, since that can enable unsound downcasting.
#[unstable(feature = "error_type_id", issue = "60784")]
#[derive(Debug)]
pub struct Internal;
}

#[unstable(feature = "never_type", issue = "35121")]
impl Error for ! {}

// Copied from `any.rs`.
impl dyn Error + 'static {
/// Gets the [`TypeId`] of the concrete type underlying `self`.
#[unstable(feature = "error_type_id", issue = "60784")]
#[inline]
pub fn type_id(&self) -> TypeId {
self.__type_id()
}

/// Returns `true` if the inner type is the same as `T`.
#[stable(feature = "error_downcast", since = "1.3.0")]
#[inline]
Expand All @@ -281,7 +280,7 @@ impl dyn Error + 'static {
let t = TypeId::of::<T>();

// Get `TypeId` of the type in the trait object (`self`).
let concrete = self.type_id(private::Internal);
let concrete = self.type_id();

// Compare both `TypeId`s on equality.
t == concrete
Expand Down Expand Up @@ -315,6 +314,13 @@ impl dyn Error + 'static {
}

impl dyn Error + 'static + Send {
/// Gets the [`TypeId`] of the concrete type underlying `self`.
#[unstable(feature = "error_type_id", issue = "60784")]
#[inline]
pub fn type_id(&self) -> TypeId {
<dyn Error + 'static>::type_id(self)
}

/// Forwards to the method defined on the type `dyn Error`.
#[stable(feature = "error_downcast", since = "1.3.0")]
#[inline]
Expand All @@ -338,6 +344,13 @@ impl dyn Error + 'static + Send {
}

impl dyn Error + 'static + Send + Sync {
/// Gets the [`TypeId`] of the concrete type underlying `self`.
#[unstable(feature = "error_type_id", issue = "60784")]
#[inline]
pub fn type_id(&self) -> TypeId {
<dyn Error + 'static>::type_id(self)
}

/// Forwards to the method defined on the type `dyn Error`.
#[stable(feature = "error_downcast", since = "1.3.0")]
#[inline]
Expand Down
Loading