Skip to content

Feat/format conditions multi line#12636

Open
Syzygy106 wants to merge 10 commits intofoundry-rs:masterfrom
Syzygy106:feat/format-conditions-multi-line
Open

Feat/format conditions multi line#12636
Syzygy106 wants to merge 10 commits intofoundry-rs:masterfrom
Syzygy106:feat/format-conditions-multi-line

Conversation

@Syzygy106
Copy link
Contributor

Motivation

Closes #12508

When conditions in if statements are formatted inline (all on one line), coverage analysis tools like lcov cannot properly distinguish which individual conditions within the chain are actually covered by tests. This happens because the tool sees only one line with the condition, making it difficult to determine branch coverage for each condition.

For example:

if (newNumber % 2 == 0 || newNumber % 2 == 1 || newNumber != 0 || newNumber != 1 || newNumber != 2) {
    number = newNumber;
}

With inline formatting, if a test only covers the first condition (e.g., newNumber % 2 == 0), lcov may show the entire line as covered, but it cannot indicate which specific conditions were actually tested. This makes it difficult to identify untested conditions and achieve accurate branch coverage analysis.

Solution

Added a new format_conditions configuration option in foundry.toml that allows placing each condition on a separate line:

[fmt]
format_conditions = "multi"  # or "inline" (default)

When format_conditions = "multi", logical operators (||, &&) in if conditions are formatted with each condition on a separate line:

if (
    newNumber % 2 == 0
        ||
    newNumber % 2 == 1
        ||
    newNumber != 0
        ||
    newNumber != 1
        ||
    newNumber != 2
) {
    number = newNumber;
}

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@Syzygy106
Copy link
Contributor Author

======================================================================

1. With format_conditions = 'inline' (default):

// 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++;
    }
}


======================================================================

2. With format_conditions = 'multi':

// 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++;
    }
}

Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, makes sense IMO (pending @0xrusowsky ). Please update the readme too to reflect the new config https://github.com/foundry-rs/foundry/tree/master/crates/fmt#configuration and add an ui_test as in https://github.com/foundry-rs/foundry/tree/master/crates/fmt#testing

@0xrusowsky
Copy link
Contributor

the impl LGTM, but imo it would be nice to make the output prettier.

if possible, let's aim for:

if (
    newNumber % 2 == 0
        || newNumber % 2 == 1
        || newNumber != 0
        || newNumber != 1
        || newNumber != 2
) {

i think you should be able to achieve if with an extra box when the multiline config is enabled

Comment on lines +34 to +44
// Normalize trailing newlines: ensure exactly one \n at the end
// This must happen AFTER scan_eof() to catch any newlines added during EOF-flush
// If there's no newline at all, add one
if !self.out.ends_with('\n') {
self.out.push('\n');
} else {
// If there are multiple newlines, remove extras to leave exactly one
while self.out.ends_with("\n\n") {
self.out.pop();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to introduce this logic?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Syzygy106 can we bring this over the finish line?

gakonst and others added 2 commits January 20, 2026 18:30
Address review comment from @0xrusowsky: the trailing newline normalization
in eof() was a workaround for a test file issue (original.sol had CRLF line
endings and a trailing blank line).

The proper fix is to clean up the test file instead of post-processing in eof().
This removes the unnecessary normalization logic and fixes the root cause.

Amp-Thread-ID: https://ampcode.com/threads/T-019bdc92-a774-749b-a831-7d9de5021e45
Co-authored-by: Amp <amp@ampcode.com>
grandizzy
grandizzy previously approved these changes Jan 20, 2026
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think this should be good to merge, @0xrusowsky @DaniPopes mind to have a 2nd look? ty!

@onbjerg onbjerg enabled auto-merge February 4, 2026 16:59
@onbjerg onbjerg added this pull request to the merge queue Feb 4, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 4, 2026
@onbjerg onbjerg added this pull request to the merge queue Feb 4, 2026
@grandizzy grandizzy removed this pull request from the merge queue due to the queue being cleared Feb 4, 2026
@onbjerg onbjerg enabled auto-merge (squash) February 4, 2026 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

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

6 participants