foundry-rs
feat(`forge fmt`): should give an option to place each if condition in a separate line
Component
Forge
Describe the feature you would like
I'm researching the literature on coverage being a predictor of bugs.
While coverage is not the full story, there's strong correlation between having high coverage and finding more bugs (although the correlation is moderate when adjusting for size).
Either way, we can all agree that professionally we rely on coverage as a indicator of attention and effort put in the testing suite.
Branches, (called Decisions in Condition / Decision Coverage), are a key part of coverage as each branch implies some conditional action.
Branches choice are determined by Conditions (the boolean variables evaluated in the branch).
Forge fmt, currently inlines all conditions in a branch instead of separating them.
forge coverage summary correctly counts statements covered and will address conditions that haven't been covered.
However, generating the lcov report will ignore statement coverage and will lead most developers to miss this nuance.
Ask: Allow option to put each condition in a separate line instead of defaulting to inlining conditions
[fmt]
format_conditions = "inline" // "inline" | "multi"
I would also suggest defaulting to multi, however if this could lead to changes for other projects I wouldn't push for this.
POC
Toy example of conditions that can never be hit, which are ignored by the coverage metric since the if condition is in one line.
Code with realistic 70% coverage
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
contract Counter {
uint256 public number = 1;
function setNumber(uint256 newNumber) public {
if (
newNumber % 2 == 0 ||
newNumber % 2 == 1 ||
newNumber != 0 ||
newNumber != 1 ||
newNumber != 2
) {
number = newNumber;
}
}
function increment() public {
number++;
}
}
Code with deceptive 100% coverage
Running forge fmt leads to this, which causes lcov to report 100% coverage.
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
contract Counter {
uint256 public number = 1;
function setNumber(uint256 newNumber) public {
if (newNumber % 2 == 0 || newNumber % 2 == 1 || newNumber != 0 || newNumber != 1 || newNumber != 2) {
number = newNumber;
}
}
function increment() public {
number++;
}
}
Lcov for the deceptive snippet
Lcov for multi line snippet
foundry coverage
forge coverage
Warning: optimizer settings and `viaIR` have been disabled for accurate coverage reports.
If you encounter "stack too deep" errors, consider using `--ir-minimum` which enables `viaIR` with minimum optimization resolving most of the errors
[⠊] Compiling...
[⠔] Compiling 45 files with Solc 0.8.30
[⠒] Solc 0.8.30 finished in 480.34ms
Compiler run successful!
Analysing contracts...
Running tests...
Ran 2 tests for test/recon/CryticToFoundry.sol:CryticToFoundry
[PASS] invariant_number_never_zero() (runs: 0, calls: 0, reverts: 0)
[PASS] test_crytic() (gas: 211)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 4.62ms (1.76ms CPU time)
Ran 2 tests for test/Counter.t.sol:CounterTest
[PASS] testFuzz_SetNumber(uint256) (runs: 256, μ: 15342, ~: 15736)
[PASS] test_Increment() (gas: 14751)
Suite result: ok. 2 passed; 0 failed; 0 skipped; finished in 6.67ms (5.32ms CPU time)
Ran 2 test suites in 100.76ms (11.30ms CPU time): 4 tests passed, 0 failed, 0 skipped (4 total tests)
╭----------------------------------------+----------------+----------------+---------------+---------------╮
| File | % Lines | % Statements | % Branches | % Funcs |
+==========================================================================================================+
| script/Counter.s.sol | 0.00% (0/3) | 0.00% (0/1) | 100.00% (0/0) | 0.00% (0/2) |
|----------------------------------------+----------------+----------------+---------------+---------------|
| src/Counter.sol | 100.00% (5/5) | 61.54% (8/13) | 100.00% (1/1) | 100.00% (2/2) |
|----------------------------------------+----------------+----------------+---------------+---------------|
| test/recon/BeforeAfter.sol | 0.00% (0/7) | 0.00% (0/4) | 100.00% (0/0) | 0.00% (0/3) |
|----------------------------------------+----------------+----------------+---------------+---------------|
| test/recon/CryticTester.sol | 0.00% (0/2) | 0.00% (0/1) | 100.00% (0/0) | 0.00% (0/1) |
|----------------------------------------+----------------+----------------+---------------+---------------|
| test/recon/Setup.sol | 63.64% (7/11) | 77.78% (7/9) | 100.00% (0/0) | 33.33% (1/3) |
|----------------------------------------+----------------+----------------+---------------+---------------|
| test/recon/TargetFunctions.sol | 0.00% (0/14) | 0.00% (0/12) | 0.00% (0/4) | 0.00% (0/3) |
|----------------------------------------+----------------+----------------+---------------+---------------|
| test/recon/targets/AdminTargets.sol | 0.00% (0/2) | 0.00% (0/1) | 100.00% (0/0) | 0.00% (0/1) |
|----------------------------------------+----------------+----------------+---------------+---------------|
| test/recon/targets/DoomsdayTargets.sol | 0.00% (0/6) | 0.00% (0/3) | 0.00% (0/2) | 0.00% (0/2) |
|----------------------------------------+----------------+----------------+---------------+---------------|
| test/recon/targets/ManagersTargets.sol | 0.00% (0/11) | 0.00% (0/7) | 100.00% (0/0) | 0.00% (0/5) |
|----------------------------------------+----------------+----------------+---------------+---------------|
| Total | 19.67% (12/61) | 29.41% (15/51) | 14.29% (1/7) | 13.64% (3/22) |
╰----------------------------------------+----------------+----------------+---------------+---------------╯
Additional context
No response
while fmt change is a good nice to have, IMO it doesn't address the real issue which is coverage not adding branches for each condition (that is in the sample given there should be 5 branches instead one for the if clause)
while fmt change is a good nice to have, IMO it doesn't address the real issue which is coverage not adding branches for each condition (that is in the sample given there should be 5 branches instead one for the if clause)
Hmmm. Do we have to create a different issue? Or fix it here?
yep, let's do it in a different issue
kk, I'll create soon.
What about the current issue and related PR?