Warn against clippy::cast_possible_truncation in Wasmtime#9209
Merged
alexcrichton merged 2 commits intobytecodealliance:mainfrom Sep 6, 2024
Merged
Warn against clippy::cast_possible_truncation in Wasmtime#9209alexcrichton merged 2 commits intobytecodealliance:mainfrom
clippy::cast_possible_truncation in Wasmtime#9209alexcrichton merged 2 commits intobytecodealliance:mainfrom
Conversation
This commit explicitly enables the `clippy::cast_possible_truncation` lint in Clippy for just the `wasmtime::runtime` module. This does not enable it for the entire workspace since it's a very noisy lint and in general has a low signal value. For the domain that `wasmtime::runtime` is working in, however, this is a much more useful lint. We in general want to be very careful about casting between `usize`, `u32`, and `u64` and the purpose of this module-targeted lint is to help with just that. I was inspired to do this after reading over bytecodealliance#9206 where especially when refactoring code and changing types I think it would be useful to have locations flagged as "truncation may happen here" which previously weren't truncating. The failure mode for this lint is that panics might be introduced where truncation is explicitly intended. Most of the time though this isn't actually desired so the more practical consequence of this lint is to probably slow down wasmtime ever so slightly and bloat it ever so slightly by having a few more checks in a few places. This is likely best addressed in a more comprehensive manner, however, rather than specifically for just this one case. This problem isn't unique to just casts, but to many other forms of `.unwrap()` for example.
elliottt
approved these changes
Sep 6, 2024
Comment on lines
+67
to
+70
| .try_into() | ||
| .ok() | ||
| .and_then(|offset| object::from_bytes_mut::<U64Bytes<NE>>(&mut bytes[offset..]).ok()) | ||
| .ok_or_else(|| anyhow!("invalid dwarf relocations"))?; |
Member
There was a problem hiding this comment.
Does this indirect through Option because the error types are different between line 67 and 69? Otherwise, wouldn't a combination of and_then and unwrap_or_else work here?
Member
Author
There was a problem hiding this comment.
Yeah this had to do with different error types across the two functions and I figured options was the easiest way to go here. This is code is on the older side in Wasmtime and is probably ripe for some improvement.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit explicitly enables the
clippy::cast_possible_truncationlint in Clippy for just thewasmtime::runtimemodule. This does not enable it for the entire workspace since it's a very noisy lint and in general has a low signal value. For the domain thatwasmtime::runtimeis working in, however, this is a much more useful lint. We in general want to be very careful about casting betweenusize,u32, andu64and the purpose of this module-targeted lint is to help with just that. I was inspired to do this after reading over #9206 where especially when refactoring code and changing types I think it would be useful to have locations flagged as "truncation may happen here" which previously weren't truncating.The failure mode for this lint is that panics might be introduced where truncation is explicitly intended. Most of the time though this isn't actually desired so the more practical consequence of this lint is to probably slow down wasmtime ever so slightly and bloat it ever so slightly by having a few more checks in a few places. This is likely best addressed in a more comprehensive manner, however, rather than specifically for just this one case. This problem isn't unique to just casts, but to many other forms of
.unwrap()for example.