block_import: switch to Box<dyn Any> for intermediates representation#4809
block_import: switch to Box<dyn Any> for intermediates representation#4809
Conversation
| } | ||
|
|
||
| /// Take interemdiate by given key, and remove it from the processing list. | ||
| pub fn take_intermediate<T: 'static>(&mut self, key: &[u8]) -> Result<Box<T>, Error> { |
There was a problem hiding this comment.
Sorry for not being clear.
What I actually wanted was this Result<Option<Box<T>>, Error>.
The Err would just be returned when the downcast fails. I think this makes it easier if at some point we need to debug this, as there is a clear distinction between not-found and wrong-type.
There was a problem hiding this comment.
@bkchr The error type for downcast is Box<dyn Any>. I think the intention of the API is that:
- If downcast succeeds, return the downcasted type in
Ok. - If downcast fails, return the original value in
Err.
So there's really no additional errors to be returned/debugged.
In take_intermediate, we mostly want to do the same thing both when the key is not found, and when the downcast fails -- stop block importing and return error, so I'd suggest we have the return type to be either Option<Box<T>> or Result<Box<T>, ConsensusError>. WDYT?
There was a problem hiding this comment.
And indeed we can distinguish not-found/wrong-type in ConsensusError -- ConsensusError::NoIntermediate, ConsensusError::IntermediateWrongType.
There was a problem hiding this comment.
Yeah, I know that downcast doesn't return an error. However in the take_intermediate case, the value will be removed and is lost.
The problem I see with Any is that &T and T give you a different TypeId and down-casting will fail: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=872b02f7968b9528140d3ec8ed7628be
And I think this is such a subtile difference, that it easily could be done wrong. Making it easier for the user to differentiate between not-found and wrong-type will really help with these kind of errors, especially when you are new to Rust.
That both should make the import fail, was nothing I wanted to question here :)
2 participants
Use built-in rust dynamic types
Box<dyn Any>. This avoids the extra step of encoding/decoding.