foundry-rs / foundry-rs/foundry

feat(`forge fmt`): should give an option to place each if condition in a separate line

Open
#12,508 8 comments 1 reaction 1 assignee Claimed by @Syzygy106 View on GitHub
Cmd-forge-fmt T-feature
Dominant language
Rust
Stars
10.6k
Forks
2.6k
Avg merge
18h 20m
Merged PRs (30d)
510

Description

### 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

```toml
[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

```solidity
// 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.

```solidity
// 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

Image

### Lcov for multi line snippet

Image

### foundry coverage

```python
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_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.