feat(lint): add UnsafeTypecast lint #11046
Conversation
TropicalDog17
requested review from
grandizzy,
yash-atreya and
zerosnacks
as code owners
|
Any updates, can update the tests for you if needed @TropicalDog17 |
…foundry into feat/unsafe-typecast-lint
|
Hi @0xClandestine, I've updated the test, but not sure how to emit a helpful fix for the lint, can you help me on that? Thank a lot! |
maybe in this case a warning is enough, and no fix suggestion is required, but you could tell them to consider adding an explicit check like: if (a > type(uint32).max) revert TypecastOverflow(); |
| solar_ast::LitKind::Number(num) => { | ||
| if is_negative_number_literal(num) { | ||
| Some(hir::ElementaryType::Int(solar_ast::TypeSize::ZERO)) | ||
| } else { | ||
| Some(hir::ElementaryType::UInt(solar_ast::TypeSize::ZERO)) | ||
| } | ||
| } |
There was a problem hiding this comment.
the compiler performs checks when assigning literal values, i think it is safe to remove this check, which would also remove the need of the bigint dep.
Please return None and add a comment explaining the reason why
Assigning values which cannot fit into the type gives a compiler error. For example:
uint8 foo = 300;The largest value an uint8 can hold is (2 8) - 1 = 255. So, the compiler says:
value 300 does not fit into type uint8
ref: https://solang.readthedocs.io/en/latest/language/types.html
…foundry into feat/unsafe-typecast-lint
| } | ||
|
|
||
| /// Infers the elementary type of a source expression. | ||
| /// For cast chains, returns the ultimate source type, not intermediate cast results. |
There was a problem hiding this comment.
Can we check intermediate casts for cast chains? I think that's why this isn't throwing:
function _addInt128(uint64 a, int128 b) internal pure returns (uint64) {
return uint64(uint128(int128(uint128(a)) + b));
}There was a problem hiding this comment.
Would suggest adding a test, you can also reproduce using the following commands:
foundryup --pr 11046
git clone https://github.com/Layr-Labs/eigenlayer-contracts.git
git checkout 44ece6d
forge lint src/contracts/core/AllocationManager.sol --only-lint unsafe-typecastThere was a problem hiding this comment.
good call, fixed to support cast chains.
do you expect any other cases not covered by the unit tests?
There was a problem hiding this comment.
I think this is a good start. Will make another small pr in the future if anything else comes to mind.
|
Sick! Ty, will review this evening sorry for the delay. |
There was a problem hiding this comment.
Some nits for coverage and readability, and a false positive was found. Will throw together a gist with my suggested changes.
EDIT: https://gist.github.com/0xClandestine/06adf9eeba08ca08661d97fba035e542
|
@TropicalDog17 can you address the feedback provided by @0xClandestine pls? |
|
what's missing here @0xrusowsky |
|
the fix is shortsighted and will still yield false positives + clippy needs to be addressed. i'll take over to bring it to the finish line |
|
@0xClandestine can you do a final pass on the unit tests and the stderr outputs please? everything should be good now, based on the feedback u gave cc: @mattsse @grandizzy for rust checks |
| return false; | ||
| }; | ||
|
|
||
| for source_ty in source_types { |
There was a problem hiding this comment.
nit can nicely write as
source_types
.iter()
.any(|source_ty| is_unsafe_elementary_typecast(source_ty, target_type))
There was a problem hiding this comment.
lgtm, thank you! @0xrusowsky I believe only docs pr remaining to be done here
|
i'll make sure docs are added and merge |
Motivation
Closes #11029
Solution
PR Checklist