🎨 zb: Remove unsafe type casting code #1439
Conversation
|
Cool. I'll look at it in more detail later but in the meantime, please split the commit to keep commits atomic. Here, I see at least 3 logical changes: removal of unneeded checks (which I'm not too sure about), bumping of MSRV and removal of unsafe code. |
|
@sander-machado ping? |
|
@sander-machado I think Rust 1.86 is not too new anymore. Let's bump the MSRV as part of this PR? You'll want to rebase though. |
Great I'll update my PR once that is done! |
I meant, you bump the MSRV as part of this PR but it's about to happen with another PR that's about to be merged: #1620. Please rebase on that. |
sander-machado
force-pushed
the
remove-unsafe-upcasting-code
branch
2 times, most recently
from
d643b8f to
638e74f
Compare
Yeah I was referring to that one (had to bump it 1 more anyway), as that's a coworker who made it. But yeah that's not obvious for you ofcourse haha. I have updated my commits to hopefully be atomic and tried to use to correct emojis. Btw I noticed that if you click the name of the emoji you copy the emoji string which almost made me use that instead before I noticed maybe that's why you keep getting people use the string? |
CodSpeed Performance ReportMerging #1439 will not alter performanceComparing Summary
|
zeenix
left a comment
zeenix
left a comment
There was a problem hiding this comment.
LGTM apart from one thing and the usual nitpicks on commit messages: 😄
- They're missing the
zb:prefix. - second one could be shorter to be close to the guideline, something like
Replace unsafe downcast API w/ std's. - A description would be nice, even if a short one. For the second commit, you could use the same description as the PR's since this is the main commit.
- typo: build ins -> built-ins. However this becomes irrelevant if you change the title.
| // Ensure what we return can later be dowcasted safely. | ||
| let lock: &dyn Any = &*lock.read().await; | ||
| let _lock: &I = lock.downcast_ref::<I>().ok_or(Error::InterfaceNotFound)?; |
There was a problem hiding this comment.
the lock isn't the lock but rather the ref. Why is this change needed btw? It looks identical to the one we already have. 🤔
There was a problem hiding this comment.
you need to explicitly make the detour with the &dyn Any otherwise the compiler doesn't like it
There was a problem hiding this comment.
Ah ok. Would be good to change the variable names still. 😉
Rustc 1.86 stabilizes a new features called trait_upcasting. As quoted from the rust docs "Enable upcasts from dyn Trait1 to dyn Trait2 if Trait1 is a subtrait of Trait2". Which is exactly what this unsafe code was doing. I added the new syntax and increased the compiler version to 1.86 to enable this feature and remove these unsafe blocks. There were also multiple places where this check was called without any reason as it's checked at construction (and would just panic either way). Related to: rust-lang/rust#65991
sander-machado
force-pushed
the
remove-unsafe-upcasting-code
branch
from
638e74f to
e6b126a
Compare
2 participants
Rustc 1.86 stabilizes a new features called trait_upcasting. As quoted from the rust docs "Enable upcasts from dyn Trait1 to dyn Trait2 if Trait1 is a subtrait of Trait2". Which is exactly what this unsafe code was doing. I added the new syntax and increased the compiler version to 1.86 to enable this feature and remove these unsafe blocks. There were also multiple places where this check was called without any reason as it's checked at construction (and would just panic either way).
Related to: rust-lang/rust#65991