fix: candidateContractName is pure, not view - #102
Conversation
solc emitted warning 2018 for this helper: its body calls only `vm.split`, declared `external pure` on `Vm`, against a `memory` argument, and `vm` is a `constant` on `Test` — so nothing is read from state and the compiler can prove it. `pure` is accepted everywhere `view` was, so the sole caller (testEveryCandidateHasASnapshot, itself `view`) is unchanged. This also puts the helper back in line with its siblings in the same file, which already follow the compiler: `holdsName` and `artifactPath` are `pure`, while `snapshotContractNames`/`nodeTypes`/`constantDeclarations` are `view` because they genuinely call `vm.readDir`/`vm.readFile`. Closes #78 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
|
Reviewed 9c0b1e1: ready — Closes #78. CI green, 0 unresolved threads — vacuous, CodeRabbit reports |
Closes #78
The diff
One keyword in
test/src/lib/GeneratedSnapshotShape.t.sol, exactly the fix the issue proposed.Why it is correct
The body calls only
vm.split, declaredexternal pureatdependencies/forge-std-1.16.1/src/Vm.sol:1301, against amemoryargument, andvmis aconstantonTest. Nothing is read from state, which is why solc can prove the restriction and emits warning 2018.pureis accepted everywhereviewwas, so the sole caller —testEveryCandidateHasASnapshotat line 164 of the same file, itselfview— needs no change. Those two lines are the only occurrences of the symbol in the tree.It also puts the helper back in line with its siblings in the same file, which already track what they actually do:
holdsNameandartifactPatharepure, whilesnapshotContractNames/nodeTypes/constantDeclarationsareviewbecause they genuinely callvm.readDir/vm.readFile.candidateContractNamewas the odd one out, not a deliberate pattern.QA
n/a, and deliberately. State mutability on aninternalfunction is a compile-time restriction with no runtime representation: there is noSTATICCALL, no enforcement, nothing a test can observe. Proven rather than asserted — building both spellings and comparing the artifact forGeneratedSnapshotShapeTestgivesbytecode: IDENTICAL (len 39636)anddeployedBytecode: IDENTICAL (len 39522). A forge test that failed on base and passed here cannot exist, so writing one would mean asserting on source text, which this file deliberately does not do (it reads the AST precisely so formatting cannot affect it).test/src/lib/GeneratedSnapshotShape.t.sol:126pure->view(i.e. back to base). Killed by the compiler, not by a test.nix develop -c forge build --forceon the mutant printsCompiler run successful with warnings:/Warning (2018): Function state mutability can be restricted to pure --> test/src/lib/GeneratedSnapshotShape.t.sol:126:5; on this branch it printsCompiler run successful!with no solc warning anywhere in the tree. The mutant SURVIVES the test suite —forge test --match-contract GeneratedSnapshotShapeTestis 5 passed / 0 failed under both spellings, with gas identical to the unit (32114, 137663, 249440, 137661, 31717). That survival is the correct result, not a coverage gap: it is the same fact as the identical bytecode above. Behavioural coverage of the function istestEveryCandidateHasASnapshot, which is unaffected because the behaviour is unchanged.Vm.sol:1301declaringsplitasexternal pure, i.e. the reason the analysis is right rather than a restatement of its output.GeneratedSnapshotShape.t.sol:126, withview -> pureas the proposed fix and "no caller changes" as the stated ramification. Covered: warning gone, caller verified unchanged, no other occurrence of the symbol. The issue's own verification block additionally observed that a forge-lintwarning[unsafe-typecast]atsrc/lib/LibRainDeploy.sol:304survives this fix, so the build is not warning-free afterwards. I confirmed it still does. That is a different finding about a different file and is deliberately NOT bundled here; this PR closes the solc-warning finding as filed.Other verification
nix develop -c forge test: 168 passed, 47 failed. Every one of the 47 isvm.createSelectFork: environment variable <CHAIN>_RPC_URL not found— the fork tests CLAUDE.md documents as needing a gitignored.env. Environmental, untouched by this diff; CI has the secrets.nix develop -c forge fmt --checkpasses; pre-commit hooks pass.