Fix #369 (Loss of `Diagnostic` information when `Report` is converted to `Box<dyn Diagnostic>`) #370

TheLostLambda · 2024-04-25T23:03:13Z

Fixes #369

The culprit was:

miette/src/eyreish/error.rs

Line 728 in ea4296d

impl<E> Diagnostic for ErrorImpl<E> where E: Diagnostic {}

In conjunction with:

miette/src/eyreish/error.rs

Lines 531 to 537 in ea4296d

    
           unsafe fn object_boxed<E>(e: Own<ErasedErrorImpl>) -> Box<dyn Diagnostic + Send + Sync + 'static> 
        
           where 
        
               E: Diagnostic + Send + Sync + 'static, 
        
           { 
        
               // Attach ErrorImpl<E>'s native StdError vtable. The StdError impl is below. 
        
               e.cast::<ErrorImpl<E>>().boxed() 
        
           }

Which converts a Box<ErrorImpl<E>> into the Box<dyn Diagnostic>, hence the resultant object ends up with the empty Diagnostic impl defined above!

This fix moves the _object out of the Box<ErrorImpl<E>> to create a Box<E> before conversion to Box<dyn Diagnostic>, attaching the correct Diagnostic impl for the original error E.

Miri seems happy, and I've actually only added safe code, even though it's in an unsafe function because of .boxed() which I left untouched.

As a side note, the only reason converting to Box<dyn Error> worked is because of this manual pass-through done here:

miette/src/eyreish/error.rs

Lines 719 to 726 in ea4296d

    
           impl<E> StdError for ErrorImpl<E> 
        
           where 
        
               E: StdError, 
        
           { 
        
               fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { 
        
                   unsafe { ErrorImpl::diagnostic(self.erase()).source() } 
        
               } 
        
           }

But now that we aren't ever trying to convert ErrorImpl<E>s into trait objects anymore — directly converting Es instead — I've deleted that unused code.

I didn't even fix this one, it was just passing!

Diagnostic impls are no longer reset to default when converting a `Report` into a `Box<dyn Diagnostic>`. Also prevented `Report` vtables and handlers from being kept around on the heap after conversion.

TheLostLambda added 3 commits April 25, 2024 15:09

test: un-ignore test_boxed_sources

4a4111b

I didn't even fix this one, it was just passing!

test(boxed): add failing test for bug zkat#369

b087000

fix(report): conversion into Box<dyn Diagnostic>

c4f6431

Diagnostic impls are no longer reset to default when converting a `Report` into a `Box<dyn Diagnostic>`. Also prevented `Report` vtables and handlers from being kept around on the heap after conversion.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #369 (Loss of `Diagnostic` information when `Report` is converted to `Box<dyn Diagnostic>`) #370

Fix #369 (Loss of `Diagnostic` information when `Report` is converted to `Box<dyn Diagnostic>`) #370

TheLostLambda commented Apr 25, 2024 •

edited

	unsafe fn object_boxed<E>(e: Own<ErasedErrorImpl>) -> Box<dyn Diagnostic + Send + Sync + 'static>
	where
	E: Diagnostic + Send + Sync + 'static,
	{
	// Attach ErrorImpl<E>'s native StdError vtable. The StdError impl is below.
	e.cast::<ErrorImpl<E>>().boxed()
	}

	impl<E> StdError for ErrorImpl<E>
	where
	E: StdError,
	{
	fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
	unsafe { ErrorImpl::diagnostic(self.erase()).source() }
	}
	}

Fix #369 (Loss of Diagnostic information when Report is converted to Box<dyn Diagnostic>) #370

Are you sure you want to change the base?

Fix #369 (Loss of Diagnostic information when Report is converted to Box<dyn Diagnostic>) #370

Conversation

TheLostLambda commented Apr 25, 2024 • edited

Fix #369 (Loss of `Diagnostic` information when `Report` is converted to `Box<dyn Diagnostic>`) #370

Fix #369 (Loss of `Diagnostic` information when `Report` is converted to `Box<dyn Diagnostic>`) #370

TheLostLambda commented Apr 25, 2024 •

edited