chore: scope forge-lint disables for the 37 findings on main - #140
Conversation
`forge lint -D warnings` reports 37 findings on unmodified `main`. Every one
is a false positive or a deliberate construct the test asserts, so each gets
a scoped `//forge-lint: disable-next-line(<rule>)` and a reason. No code
changes.
- boolean-cst x3, `src/lib/parse/LibParseMeta.sol`: `lookupWord` returns
`(bool, uint256)` and the org bans named returns, so the found flag can only
be a literal in the return tuple. The rule is about a boolean constant used
as a condition operand; a return value is not one.
- incorrect-shift x3: the rule documents itself as a Yul `shl`/`shr` argument
order check. All three sites are Solidity `<<`, whose operand order is fixed
by the language, in the canonical `1 << n` single-bit idiom.
- unsafe-typecast x27: `bytes32("<literal>")`. solc rejects a literal wider
than 32 bytes at compile time, so there is no runtime value to truncate.
- unsafe-typecast x2: `uint8(x & 0xFF)`, masked to 8 bits in the same
expression.
- unsafe-typecast x1: `bytes1(uint8(offset >> 8))` where `offset` is uint16.
- unsafe-typecast x1: `bytes1(uint8(offset))` where `offset` is uint16. This
one does truncate, and that is the point: it writes the low byte of a 16 bit
big-endian offset, which the surrounding test asserts is read back whole.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
|
Warning Review limit reachedNext included review available in 22 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
forge lint -D warningsreports 37 findings on unmodifiedmainand fails therainix-sol / staticjob. Every one of the 37 is a false positive or a construct the surrounding test deliberately asserts, so each gets a scoped//forge-lint: disable-next-line(<rule>)and a one-line reason. No behaviour, and no code, changes: the diff is 74 added comment lines and nothing else.QA
forge lint -D warningsingithub:rainlanguage/rainix/8657b83b68f41957ab85da91132c3f652c1f32c0#sol-shellis the gate and the test. Fails on base: 37 findings, exit 1. Passes here: 0 findings, exit 0.forge test(128 passed, 17 suites),forge fmt --check,slither .(0 results) andrainix-sol-single-contractall pass on both sides, confirming the diff is inert.LibGenParseMeta.buildMeta.t.sol:52). Each produced exactly 1 finding, of the expected rule, at the exact line that directive guards: 38 pass, 0 fail. So no directive is redundant and none is silencing a neighbour. Scoping check: three directives, one per rule, were repointed at a different rule id —boolean-cst->unsafe-typecastatLibParseMeta.sol:163,incorrect-shift->boolean-cstatbuildMeta.t.sol:25,unsafe-typecast->incorrect-shiftatsourceRelativeOffset.t.sol:105. All three let the original finding straight back through, so these are per-rule disables and not blanket ones.incorrect-shiftis documented as a Yulshl/shrargument-order check; none of its three sites is Yul. For the shift inbuildMeta.t.solthe test is its own oracle — the next line assertsMETA_ITEM_MASK == type(uint32).max, which a transposed shift could not satisfy. For the 32-byte literals the oracle is solc, which rejects an over-wide literal at compile time. Precedent as a second reader: the org already carries these exact disables for these exact shapes (see per-finding table).forge lint -D warningsfindings onmain, each decided as fix-or-disable and said which; covered, all 37 below. The other red gate onmain,pre-commit run --all-files, is chore: apply pre-commit formatting to main #139 and is not touched here.Why main has never been through this
rainix added
forge lint -D warningsto the sharedrainix-sol-staticworkflow at 2026-09-15T20:59Z.main's last run was 2026-09-15T17:16Z (#137). A fresh clone of unmodifiedmainreproduces all 37.The decision on each finding
Zero of the 37 are real. Each line below says why, and names where the org already made the same call.
boolean-cstx3 —src/lib/parse/LibParseMeta.sol:162, 179, 184— false positive, disabled.The three are
return (false, 0),return (true, index)andreturn (false, 0), the three exits oflookupWord, which is declaredreturns (bool, uint256). The org bans named returns, so the found/not-found flag can only be written as a literal in the return tuple — there is no other spelling.boolean-cstexists to catch a boolean constant standing where a condition belongs (if (x == true),require(true), a constant operand of&&/||); a return value is not a condition operand. Each of the three is the sole exit of a distinct control path and the paireduint256is meaningful at each (0is the documented not-found value,indexis read from the matched item), so nothing here is dead, tautological or unreachable.rain.lib.memkv/test/lib/LibMemoryKVSlow.solhas the identical shape —existsreturning(bool, uint256)— already carrying this disable.incorrect-shiftx3 —LibGenParseMeta.buildMeta.t.sol:24,LibParseMeta.lookupWord.t.sol:123, 132— false positive, disabled.The rule documents itself as a Yul check: it warns when the first argument to a Yul
shl/shris dynamic and the second is a literal, becauseshr(value, 8)shifts the literal. None of the three sites is Yul. All three are Solidity's<<, whose operand order is fixed by the language as value-then-amount and cannot be transposed, in the canonical single-bit idiom1 << n:lookupWord.t.sol:123—if (shifted == (1 << i))walks bit positions to find the one bitwordBitmappedset.wordBitmappedbuilds that value as Yulshl(byte(0, hashed), 1), the same one-bit shape.lookupWord.t.sol:132—1 << fakeBitPosbuilds a one-bit expansion.buildMeta.t.sol:24—(1 << (META_ITEM_SIZE * 8)) - 1is0xFFFFFFFF. The order is pinned by the test itself: the very next line assertsMETA_ITEM_MASK == type(uint32).max, and transposed operands would give(4 * 8) << 1 = 64,- 1 = 63. Both assertions cannot hold unless the order is the one written.rainlang/test/src/lib/parse/LibParseSlow.soldisables this rule for the same1 << <dynamic>idiom.unsafe-typecastx27 —bytes32("<literal>")— false positive, disabled.lookupWord.t.sol:26, 27, 28, 46, 61, 89, 160, 161, 162, 176, 180, 188, 192, 197, 204, 207, 208, 209, 286, 329andbuildMeta.t.sol:178, 179, 180, 211, 219, 228, 229. Each is an explicit conversion of a string literal tobytes32. solc validates the width at compile time — a literal wider than 32 bytes is a compile error, not a silent truncation — and every one of the 27 is 3 to 9 ASCII characters. There is no runtime value for the cast to truncate.rainlang/test/src/lib/parse/LibParse.parseWord.t.solcarries this disable for the same shape, reasoned as "Casting a small literal is safe to typecast."unsafe-typecastx2 —LibParseMeta.lookupWord.t.sol:154, 257— false positive, disabled.Both are
uint8(x & 0xFF): the operand is masked to 8 bits in the same expression, so the cast is width-preserving. Worth noting the rule is inconsistent here rather than wrong about safety — the sibling lines two and three above each of these,uint8((x >> 16) & 0xFF)anduint8((x >> 8) & 0xFF), are equally safe and are not flagged. Only the flagged pair needs a directive.unsafe-typecastx1 —LibBytecode.sourceRelativeOffset.t.sol:104— false positive, disabled.bytes1(uint8(offset >> 8))whereoffsetis auint16fuzz parameter, so the shifted value is at most0xFF.unsafe-typecastx1 —LibBytecode.sourceRelativeOffset.t.sol:105— real truncation, deliberate, disabled.bytes1(uint8(offset))on the sameuint16genuinely drops the high byte, and that is the point: line 104 writes the high byte and line 105 the low byte of a 16-bit big-endian offset. This is the one finding where silencing could hide something, so it is the one to check hardest — and it cannot, because the test exists to catch exactly that.testSourceRelativeOffsetHighByteReferenceassumesoffset >= 0x100to force a non-zero high byte, then assertssourceRelativeOffsetreturns the fulloffsetand agrees withLibBytecodeSlow. A wrong truncation fails two assertions two lines down. This repo already carries the identical pattern and reasoning attest/abstract/BytecodeTest.sol:65-70.Merge order
rainix-sol / staticrunsforge lintbeforepre-commit, andmainis red at both. Neither fix can be green alone: this branch clears lint and then fails at pre-commit, #139 clears pre-commit but never reaches it because lint fails first. Whichever lands first therefore merges withstaticred at the step the other PR fixes. #139 goes first, then this rebases onto it and runs fully green before merging.🤖 Generated with Claude Code
https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN