Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(forge): format new expressions #6408

Merged
merged 7 commits into from
Nov 23, 2023
Merged

Conversation

beeb
Copy link
Contributor

@beeb beeb commented Nov 23, 2023

The formatter currently doesn't handle new expressions, which leads to #4619.

This PR fixes that by adding the relevant case in the formatter. The tests have been adjusted to reflect this. They do not pass without the fix, but pass with the fix added.

Please note that the existing test cases had new uint[](length) in several places, where the type is not normalized to uint256. In my opinion, the behavior after the fix (convert those to uint256) is correct, and the test files were wrong.

Thus, I adjusted all occurences of new uint[] in the fmt.sol files.

A new test case for new Contract() expressions has been added to the FunctionCall test, since those expression can be seen as a function call to the constructor.

NOTE: cargo fmt changed some formatting in formatter.rs, no sure if that's ok.

Motivation

The formatter is broken for expression which start with new.

Solution

Handle the case where an expression is Expression::New.

Closes #4619

@beeb beeb requested a review from rkrasiuk as a code owner November 23, 2023 11:36
Comment on lines 2290 to 2293
Expression::New(_, expr) => {
write!(self.buf(), "new ")?;
self.visit_expr(expr.loc(), expr)?;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the fix

Copy link
Member

Choose a reason for hiding this comment

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

oh nice!

@mattsse mattsse force-pushed the fix/format-new-expr branch from af15350 to bb0999b Compare November 23, 2023 12:18
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty!

lgtm

Comment on lines 2290 to 2293
Expression::New(_, expr) => {
write!(self.buf(), "new ")?;
self.visit_expr(expr.loc(), expr)?;
}
Copy link
Member

Choose a reason for hiding this comment

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

oh nice!

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this has a smol regression, see failing tests:

ConstructorVictim victim =new ConstructorVictim(

There's a whitespace missing that I think we can check for

I believe before, new expressions where formatted as is, but I like visiting the expression better

@beeb
Copy link
Contributor Author

beeb commented Nov 23, 2023

The failing workflow indicates that there is potentially still a problem with the implementation. But the last two reported mismatches (in testdata/cheats/RecordAccountAccesses.t.sol) are IMO badly formatted in main currently.

@mattsse
Copy link
Member

mattsse commented Nov 23, 2023

But the last two reported mismatches (in testdata/cheats/RecordAccountAccesses.t.sol) are IMO badly formatted in main currently.

agree because previously it formatted using the src directly

@beeb
Copy link
Contributor Author

beeb commented Nov 23, 2023

@mattsse I had a deeper look and the problem only happens when the right side of the assignment is a Expression::New and doesn't fit on a single line.
Do you have an idea why there is no space in that case? I can't figure it out rn.

@mattsse
Copy link
Member

mattsse commented Nov 23, 2023

I might have, I'll have a look.

could be that there's some trim applied

@beeb
Copy link
Contributor Author

beeb commented Nov 23, 2023

Ok got it! I was using write! instead of write_chunk!

@mattsse
Copy link
Member

mattsse commented Nov 23, 2023

amazing, ty!

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

gg

@mattsse mattsse force-pushed the fix/format-new-expr branch from 3018443 to d022c3e Compare November 23, 2023 15:08
@beeb
Copy link
Contributor Author

beeb commented Nov 23, 2023

Regarding formatting, do you know why my nightly rustfmt added semicolons for the return statements? I used the nightly from 2023-11-22

@beeb
Copy link
Contributor Author

beeb commented Nov 23, 2023

Ah I figured it out. Actually I formatted wrongly with rustfmt stable at first, which added semicolons, and then switched back to nightly and it kept the semicolons. But if they are not there to begin with it doesn't add them. Maybe there is a way to enforce one or the other in the config file.

EDIT: https://rust-lang.github.io/rustfmt/?version=master&search=#trailing_semicolon to enforce them, but there is no way to remove them I think.

@mattsse
Copy link
Member

mattsse commented Nov 23, 2023

the recent nightly fmt appears to be broken to some extent at least on macos

I'm currently using +nightly-2023-11-14 which is not affected but also does not respect trailing_semicolon...

I noticed this 2 weeks ago or so, let's hope gets fixed soon

@mattsse mattsse merged commit 55dd5de into foundry-rs:master Nov 23, 2023
19 checks passed
@beeb beeb deleted the fix/format-new-expr branch November 23, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

forge fmt doesn't format contract instantiation arguments
2 participants