Skip to content

Commit

Permalink
fix(fmt): fix disable line for first / last block lines (foundry-rs#8602
Browse files Browse the repository at this point in the history 
)

* fix(fmt): fix disable line for first / last block lines

* Fix win spacing

* Reuse visit_block in visit_fn

* Fix win failure
  • Loading branch information
@grandizzy
grandizzy authored Aug 8, 2024
1 parent 4cdebf7 commit 5ee33c8
Show file tree
Hide file tree
Showing 5 changed files with 256 additions and 57 deletions.
171 changes: 119 additions & 52 deletions crates/fmt/src/formatter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,17 @@ impl<'a, W: Write> Formatter<'a, W> {
Ok(())
}

/// Write unformatted src and comments for given location.
fn write_raw_src(&mut self, loc: Loc) -> Result<()> {
let disabled_stmts_src = String::from_utf8(self.source.as_bytes()[loc.range()].to_vec())
.map_err(FormatterError::custom)?;
self.write_raw(disabled_stmts_src.trim_end())?;
self.write_whitespace_separator(true)?;
// Remove comments as they're already included in disabled src.
let _ = self.comments.remove_all_comments_before(loc.end());
Ok(())
}

/// Returns number of blank lines in source between two byte indexes
fn blank_lines(&self, start: usize, end: usize) -> usize {
// because of sorting import statements, start can be greater than end
Expand Down Expand Up @@ -1192,22 +1203,114 @@ impl<'a, W: Write> Formatter<'a, W> {
}
}

write_chunk!(self, "{{")?;
// Determine if any of start / end of the block is disabled and block lines boundaries.
let is_start_disabled = self.inline_config.is_disabled(loc.with_end(loc.start()));
let is_end_disabled = self.inline_config.is_disabled(loc.with_start(loc.end()));
let end_of_first_line = self.find_next_line(loc.start()).unwrap_or_default();
let end_of_last_line = self.find_next_line(loc.end()).unwrap_or_default();

if let Some(statement) = statements.first() {
self.write_whitespace_separator(true)?;
self.write_postfix_comments_before(CodeLocation::loc(statement).start())?;
// Write first line of the block:
// - as it is until the end of line, if format disabled
// - start block if line formatted
if is_start_disabled {
self.write_raw_src(loc.with_end(end_of_first_line))?;
} else {
write_chunk!(self, "{{")?;
}

self.indented(1, |fmt| {
fmt.write_lined_visitable(loc, statements.iter_mut(), |_, _| false)?;
Ok(())
})?;
// Write comments and close block if no statement.
if statements.is_empty() {
self.indented(1, |fmt| {
fmt.write_prefix_comments_before(loc.end())?;
fmt.write_postfix_comments_before(loc.end())?;
Ok(())
})?;

write_chunk!(self, "}}")?;
return Ok(true)
}

// Determine writable statements by excluding statements from disabled start / end lines.
// We check the position of last statement from first line (if disabled) and position of
// first statement from last line (if disabled) and slice accordingly.
let writable_statments = match (
statements.iter().rposition(|stmt| {
is_start_disabled &&
self.find_next_line(stmt.loc().end()).unwrap_or_default() ==
end_of_first_line
}),
statements.iter().position(|stmt| {
is_end_disabled &&
self.find_next_line(stmt.loc().end()).unwrap_or_default() == end_of_last_line
}),
) {
// We have statements on both disabled start / end lines.
(Some(start), Some(end)) => {
if start == end || start + 1 == end {
None
} else {
Some(&mut statements[start + 1..end])
}
}
// We have statements only on disabled start line.
(Some(start), None) => {
if start + 1 == statements.len() {
None
} else {
Some(&mut statements[start + 1..])
}
}
// We have statements only on disabled end line.
(None, Some(end)) => {
if end == 0 {
None
} else {
Some(&mut statements[..end])
}
}
// No statements on disabled start / end line.
(None, None) => Some(statements),
};

if !statements.is_empty() {
// Write statements that are not on any disabled first / last block line.
let mut statements_loc = loc;
if let Some(writable_statements) = writable_statments {
if let Some(first_statement) = writable_statements.first() {
statements_loc = statements_loc.with_start(first_statement.loc().start());
self.write_whitespace_separator(true)?;
self.write_postfix_comments_before(statements_loc.start())?;
}
// If last line is disabled then statements location ends where last block line starts.
if is_end_disabled {
if let Some(last_statement) = writable_statements.last() {
statements_loc = statements_loc.with_end(
self.find_next_line(last_statement.loc().end()).unwrap_or_default(),
);
}
}
self.indented(1, |fmt| {
fmt.write_lined_visitable(
statements_loc,
writable_statements.iter_mut(),
|_, _| false,
)?;
Ok(())
})?;
self.write_whitespace_separator(true)?;
}
write_chunk!(self, loc.end(), "}}")?;

// Write last line of the block:
// - as it is from where statements location ends until the end of last line, if format
// disabled
// - close block if line formatted
if is_end_disabled {
self.write_raw_src(loc.with_start(statements_loc.end()).with_end(end_of_last_line))?;
} else {
if end_of_first_line != end_of_last_line {
self.write_whitespace_separator(true)?;
}
write_chunk!(self, loc.end(), "}}")?;
}

Ok(false)
}
Expand Down Expand Up @@ -3045,51 +3148,15 @@ impl<'a, W: Write> Visitor for Formatter<'a, W> {
if fmt.inline_config.is_disabled(body_loc.with_end(body_loc.start())) {
match body {
Statement::Block { statements, .. } if !statements.is_empty() => {
// TODO: move this logic in `visit_body` fn and reuse it.
// Retain enabled statements.
statements.retain(|stmt| {
!fmt.inline_config.is_disabled(
body_loc.with_end(CodeLocation::loc(stmt).start()),
)
});

// Disabled statement stops where first enabled statement starts or
// where body ends (if no statement enabled).
let disabled_stmts_end = match statements.first() {
Some(stmt) => CodeLocation::loc(stmt).start(),
None => body_loc.end(),
};

// Write non formatted statements. This includes the curly bracket
// block start, comments and any other disabled statement.
let disabled_stmts_src = String::from_utf8(
fmt.source.as_bytes()
[body_loc.with_end(disabled_stmts_end).range()]
.to_vec(),
)
.map_err(FormatterError::custom)?;
fmt.write_whitespace_separator(false)?;
fmt.write_raw(disabled_stmts_src.trim_end())?;
// Remove all comments as they're already included in disabled src.
let _ = fmt.comments.remove_all_comments_before(disabled_stmts_end);

// Write enabled statements.
fmt.indented(1, |fmt| {
fmt.write_lined_visitable(
body_loc.with_start(disabled_stmts_end),
statements.iter_mut(),
|_, _| false,
)?;
Ok(())
})?;

// Write curly bracket block end.
fmt.write_whitespace_separator(true)?;
write_chunk!(fmt, body_loc.end(), "}}")?;

fmt.visit_block(body_loc, statements, false, false)?;
return Ok(())
}
_ => {}
_ => {
// Attrs should be written on same line if first line is disabled
// and there's no statement.
attrs_multiline = false
}
}
}

Expand Down
3 changes: 1 addition & 2 deletions crates/fmt/testdata/InlineDisable/fmt.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@ function testFunc(uint256 num, bytes32 data , address receiver)

function testAttrs(uint256 num, bytes32 data, address receiver)
// forgefmt: disable-next-line
public payable attr1 Cool( "hello" )
{}
public payable attr1 Cool( "hello" ) {}

// forgefmt: disable-next-line
function testParams(uint256 num, bytes32 data , address receiver)
Expand Down
68 changes: 68 additions & 0 deletions crates/fmt/testdata/Repros/fmt.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,71 @@ contract Format {
) {}
}
}

// https://github.com/foundry-rs/foundry/issues/3830
contract TestContract {
function test(uint256 a) public {
if (a > 1) {
a = 2;
} // forgefmt: disable-line
}

function test1() public {
assembly{ sstore( 1, 1) /* inline comment*/ // forgefmt: disable-line
sstore(2, 2)
}
}

function test2() public {
assembly{ sstore( 1, 1) // forgefmt: disable-line
sstore(2, 2)
sstore(3, 3)// forgefmt: disable-line
sstore(4, 4)
}
}

function test3() public {
// forgefmt: disable-next-line
assembly{ sstore( 1, 1)
sstore(2, 2)
sstore(3, 3)// forgefmt: disable-line
sstore(4, 4)
}// forgefmt: disable-line
}

function test4() public {
// forgefmt: disable-next-line
assembly{
sstore(1, 1)
sstore(2, 2)
sstore(3, 3)// forgefmt: disable-line
sstore(4, 4)
}// forgefmt: disable-line
if (condition) execute(); // comment7
}

function test5() public {
assembly { sstore(0, 0) }// forgefmt: disable-line
}

function test6() returns (bool) { // forgefmt: disable-line
if ( true ) { // forgefmt: disable-line
}
return true ; } // forgefmt: disable-line

function test7() returns (bool) { // forgefmt: disable-line
if (true) { // forgefmt: disable-line
uint256 a = 1; // forgefmt: disable-line
}
return true;
}

function test8() returns (bool) { // forgefmt: disable-line
if ( true ) { // forgefmt: disable-line
uint256 a = 1;
} else {
uint256 b = 1; // forgefmt: disable-line
}
return true;
}
}
66 changes: 66 additions & 0 deletions crates/fmt/testdata/Repros/original.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,69 @@ contract Format {
) {}
}
}

// https://github.com/foundry-rs/foundry/issues/3830
contract TestContract {
function test(uint256 a) public {
if (a > 1) {
a = 2;
} // forgefmt: disable-line
}

function test1() public {
assembly { sstore( 1, 1) /* inline comment*/ // forgefmt: disable-line
sstore(2, 2)
}
}

function test2() public {
assembly { sstore( 1, 1) // forgefmt: disable-line
sstore(2, 2)
sstore(3, 3) // forgefmt: disable-line
sstore(4, 4)
}
}

function test3() public {
// forgefmt: disable-next-line
assembly{ sstore( 1, 1)
sstore(2, 2)
sstore(3, 3) // forgefmt: disable-line
sstore(4, 4)
}// forgefmt: disable-line
}

function test4() public {
// forgefmt: disable-next-line
assembly {
sstore( 1, 1)
sstore(2, 2)
sstore(3, 3) // forgefmt: disable-line
sstore(4, 4)
}// forgefmt: disable-line
if (condition) execute(); // comment7
}

function test5() public {
assembly { sstore(0, 0) }// forgefmt: disable-line
}

function test6() returns (bool) { // forgefmt: disable-line
if ( true ) { // forgefmt: disable-line
}
return true ; } // forgefmt: disable-line

function test7() returns (bool) { // forgefmt: disable-line
if (true) { // forgefmt: disable-line
uint256 a = 1; // forgefmt: disable-line
}
return true ; }

function test8() returns (bool) { // forgefmt: disable-line
if ( true ) { // forgefmt: disable-line
uint256 a = 1;
} else {
uint256 b = 1; // forgefmt: disable-line
}
return true ; }
}
5 changes: 2 additions & 3 deletions testdata/default/fuzz/invariant/common/InvariantPreserveState.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@ contract Handler is DSTest {
Vm constant vm = Vm(HEVM_ADDRESS);

function thisFunctionReverts() external {
if (block.number < 10) {} else {
revert();
}
if (block.number < 10) {}
else revert();
}

function advanceTime(uint256 blocks) external {
Expand Down

0 comments on commit 5ee33c8

Please sign in to comment.