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

Fixed a couple inconsistencies in the MessageError<M> vtable #61

Conversation

Michael-F-Bryan
Copy link

@Michael-F-Bryan Michael-F-Bryan commented Jan 21, 2020

I'm doing a review of anyhow and noticed a couple inconsistencies when constructing the vtable for MessageError<M>.

The current code is sound, but I thought I'd make things easier for future reviewers because I needed to spend a couple minutes reassuring myself it's okay to use something like object_drop_front::<M> instead of object_drop_front::<MessageError<M>>.

The lines I'm referring to:

pub(crate) fn from_adhoc<M>(message: M, backtrace: Option<Backtrace>) -> Self
where
    M: Display + Debug + Send + Sync + 'static,
{
    use crate::wrapper::MessageError;
    let error: MessageError<M> = MessageError(message);
    let vtable = &ErrorVTable {
        object_drop: object_drop::<MessageError<M>>,
        object_ref: object_ref::<MessageError<M>>,
        #[cfg(feature = "std")]
        object_mut: object_mut::<MessageError<M>>,
        object_boxed: object_boxed::<MessageError<M>>,
        object_downcast: object_downcast::<M>,                             <-----
        object_drop_rest: object_drop_front::<M>,                            <-----
    };

    // Safety: MessageError is repr(transparent) so it is okay for the
    // vtable to allow casting the MessageError<M> to M.
    unsafe { Error::construct(error, vtable, backtrace) }
}

Copy link
Owner

@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.

I think the original code is correct and I would prefer to keep it. object_drop_rest is a downcast in general, and we should be using the same type as for the downcast. The vtable setup in Error::from_adhoc is consistent with all the other vtables in Error::from_display, Error::from_context, Error::from_boxed, and Error::context, some of which rely on object_downcast and object_drop_rest being matched.

Thanks anyway!

@dtolnay dtolnay closed this Jan 21, 2020
@Michael-F-Bryan
Copy link
Author

Thanks @dtolnay, when you put it that way it makes more sense.

I haven't read the code further down in any detail, so didn't know Error::from_display() and friends use the same convention or rely on the two functions using the same type.

@Michael-F-Bryan Michael-F-Bryan deleted the inconsistent-messageerror-vtable branch January 21, 2020 04:30
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