chore: update solang-parser #4661

DaniPopes · 2023-03-29T04:41:01Z

Motivation

Relevant changes:

Fix panic when lexing ".9" at the beginning of a file hyperledger-solang/solang#1230
- Fixes a panic that we had to for check ourselves in Chisel (dc6eec5)
Improve parser hyperledger-solang/solang#1237
- Upstreams many of the parser "ext" traits in forge-fmt and forge-doc (2017824)

~~Waiting on solang-parser v0.2.4 release, draft until then.~~

Blocked by ~~gakonst/ethers-rs#2334 and~~ an Ethers release.

Solution

mattsse

awesome!
love that we could simplify this

lgtm, pending release

DaniPopes · 2023-04-07T16:30:10Z

Looks like 2 different errors in tests (CI):

IfStatement/original.sol: looks like a previous bug, where the if statement on line 89 wasn't getting inlined?
InlineDisable/original.sol: brackets get removed on empty blocks when disabled inline, I cannot figure out where this happens

cc @rkrasiuk @jpopesculian pls advise

mattsse · 2023-04-12T21:52:48Z

@rkrasiuk please take a look at the failing tests, this is now a blocker for any PR that needs new ethers release

rkrasiuk · 2023-04-15T04:11:13Z

@DaniPopes thanks for the update, you are an OG! looking at failing tests rn

rkrasiuk · 2023-04-15T05:32:49Z

regarding 2, i narrowed it down to the line below

foundry/fmt/src/formatter.rs

Line 2332 in 37d4625

return_source_if_disabled!(self, func.loc());

not sure yet under which circumstances the location is invalid, tried changing it to

return_source_if_disabled!(self, func.loc().with_end(body.loc().end()));

but that results in different errors 🤷‍♂️

mattsse · 2023-04-15T11:24:42Z

I've added a smaller repro IfStatement2 and tracked it down to:

foundry/fmt/src/formatter.rs

Line 1188 in 2e25e08

_ => self.visit_block(stmt.loc(), &mut vec![stmt], attempt_single_line, true),

called with FunctionCall(execute())
and the fits_on_single fires here:

foundry/fmt/src/formatter.rs

Line 1153 in 2e25e08

if fits_on_single {

but couldn't make any additional progress because not familiar with the assumptions here, could please take another look at the visit_block function?

DaniPopes · 2023-04-15T17:45:24Z

Isn't (1) the intended behaviour? visit_block gets called with (true, true) so the single stmt gets inlined because it fits

mattsse · 2023-04-15T18:14:50Z

Isn't (1) the intended behaviour?

yes, was just pointing out the callgraph

so the single stmt gets inlined because it fits

yes, but I didn't before, so something changed here

DaniPopes · 2023-04-15T21:00:08Z

The old manual implementation of .loc() for FunctionDefinition includes the block, while solang-parser's actual parser excludes it. This breaks an internal assumption that there is only whitespace between item[n].loc().end() and item[n + 1].loc().start()

In Formatter::write_lined_visitable, last_byte_written gets assigned to item.loc().end(), and unwritten_whitespace returns the wrong value which includes the block:

TRACE SU:function: forge_fmt::formatter: BEFORE: "<OMITTED>\n\n// forgefmt: disable-start\n\n"
TRACE SU:function: forge_fmt::formatter: AFTER:  "<OMITTED>\n\n// forgefmt: disable-start\n\nfunction test1() {}"
TRACE SU: forge_fmt::formatter: Unwritten whitespace: "{}\n\n"
TRACE SU:function: forge_fmt::formatter: BEFORE: "<OMITTED>\n\n// forgefmt: disable-start\n\nfunction test1() {}{}\n\n"

Should we revert to the old manual trait with just the special case for FunctionDefinition?

mattsse · 2023-04-16T09:10:56Z

Should we revert to the old manual trait with just the special case for FunctionDefinition?

this sounds reasonable, would give us a bit more flexibility

DaniPopes · 2023-04-16T20:39:28Z

Done, this fixes point number 2.

Number 1 is fixed with:

diff --git a/fmt/testdata/IfStatement/fmt.sol b/fmt/testdata/IfStatement/fmt.sol
index 4c430108..cb2f8874 100644
--- a/fmt/testdata/IfStatement/fmt.sol
+++ b/fmt/testdata/IfStatement/fmt.sol
@@ -104,9 +104,7 @@ contract IfStatement {
             execute();
         }
 
-        if (condition && ((condition || anotherLongCondition))) {
-            execute();
-        }
+        if (condition && ((condition || anotherLongCondition))) execute();
 
         // if statement
         if (condition) execute();
diff --git a/fmt/testdata/IfStatement2/fmt.sol b/fmt/testdata/IfStatement2/fmt.sol
index f9c27b1f..10ae4360 100644
--- a/fmt/testdata/IfStatement2/fmt.sol
+++ b/fmt/testdata/IfStatement2/fmt.sol
@@ -2,8 +2,6 @@ contract IfStatement {
     function test() external {
         bool anotherLongCondition;
 
-        if (condition && ((condition || anotherLongCondition))) {
-            execute();
-        }
+        if (condition && ((condition || anotherLongCondition))) execute();
     }
 }

But I guess the question here is still why did this change.

DaniPopes · 2023-04-16T20:41:19Z

fmt/src/formatter.rs

+    /// Casts the current `W` writer as a `String` reference. Should only be used for debugging.
+    #[allow(dead_code)]
+    unsafe fn buf_contents(&self) -> &String {
+        match &self.temp_bufs[..] {
+            [] => unsafe { *(&self.buf.w as *const W as *const &mut String) },
+            [.., buf] => &buf.w,
        }
    }


Used inline with trace!("{}", self.buf_contents())

This avoids adding fmt::Debug bounds everywhere, makes debugging faster. LMK if this should be removed.

DaniPopes · 2023-04-16T20:42:42Z

fmt/src/formatter.rs

    fn visit_parser_error(&mut self, loc: Loc) -> Result<()> {
        Err(FormatterError::InvalidParsedItem(loc))
    }
 }
-
-#[cfg(test)]
-mod tests {


moved to ../tests/it/formatter.rs

DaniPopes · 2023-04-16T20:48:04Z

fmt/src/solang_ext/loc.rs

diff in: 815e1d8

DaniPopes · 2023-04-17T08:42:22Z

Some failing anvil test unrelated

mattsse · 2023-04-17T09:33:09Z

merged the anvil issue from master,

failures:
formatter::IfStatement
formatter::IfStatement2

still fails though

DaniPopes · 2023-04-17T09:36:14Z

yep honestly i don't know why, i think it's fine just keeping the change by patching the tests

mattsse · 2023-04-17T09:39:35Z

fine with that, can reiterate when we receive feedback, but this pr is now blocking for a while

mattsse

lgtm!

wip update solang

2017824

DaniPopes marked this pull request as draft March 29, 2023 04:41

remove Chisel check

dc6eec5

mattsse approved these changes Apr 1, 2023

View reviewed changes

mattsse added the Cmd-forge-fmt Command: forge fmt label Apr 1, 2023

DaniPopes added 6 commits April 5, 2023 12:04

Merge branch 'master' into feat/update-solang

5bee3cb

update patch section

33ad729

bump

85a1447

update patch

b607b70

update patch to git

2ec65b0

Merge branch 'master' into feat/update-solang

22ac733

bump ethers

37d4625

mattsse marked this pull request as ready for review April 12, 2023 21:48

mattsse requested a review from rkrasiuk April 12, 2023 21:53

add repro

2e25e08

add tracing and move tests to tests/it/

fc01de0

DaniPopes added 2 commits April 16, 2023 21:55

cargo update

a080d89

add CodeLocationExt

815e1d8

DaniPopes commented Apr 16, 2023

View reviewed changes

Merge branch 'master' into feat/update-solang

d542565

Merge branch 'master' into feat/update-solang

c165099

This comment was marked as resolved.

Sign in to view

patch

e529ae5

mattsse approved these changes Apr 18, 2023

View reviewed changes

mattsse merged commit a9ad3ae into foundry-rs:master Apr 18, 2023

DaniPopes deleted the feat/update-solang branch April 18, 2023 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update solang-parser #4661

chore: update solang-parser #4661

DaniPopes commented Mar 29, 2023 •

edited

Loading

mattsse left a comment

DaniPopes commented Apr 7, 2023 •

edited

Loading

mattsse commented Apr 12, 2023

rkrasiuk commented Apr 15, 2023

rkrasiuk commented Apr 15, 2023

mattsse commented Apr 15, 2023

DaniPopes commented Apr 15, 2023

mattsse commented Apr 15, 2023

DaniPopes commented Apr 15, 2023 •

edited

Loading

mattsse commented Apr 16, 2023

DaniPopes commented Apr 16, 2023

DaniPopes Apr 16, 2023

DaniPopes Apr 16, 2023

DaniPopes Apr 16, 2023

DaniPopes commented Apr 17, 2023

mattsse commented Apr 17, 2023

DaniPopes commented Apr 17, 2023 •

edited

Loading

mattsse commented Apr 17, 2023

This comment was marked as resolved.

mattsse left a comment

chore: update solang-parser #4661

chore: update solang-parser #4661

Conversation

DaniPopes commented Mar 29, 2023 • edited Loading

Motivation

Solution

mattsse left a comment

Choose a reason for hiding this comment

DaniPopes commented Apr 7, 2023 • edited Loading

mattsse commented Apr 12, 2023

rkrasiuk commented Apr 15, 2023

rkrasiuk commented Apr 15, 2023

mattsse commented Apr 15, 2023

DaniPopes commented Apr 15, 2023

mattsse commented Apr 15, 2023

DaniPopes commented Apr 15, 2023 • edited Loading

mattsse commented Apr 16, 2023

DaniPopes commented Apr 16, 2023

DaniPopes Apr 16, 2023

Choose a reason for hiding this comment

DaniPopes Apr 16, 2023

Choose a reason for hiding this comment

DaniPopes Apr 16, 2023

Choose a reason for hiding this comment

DaniPopes commented Apr 17, 2023

mattsse commented Apr 17, 2023

DaniPopes commented Apr 17, 2023 • edited Loading

mattsse commented Apr 17, 2023

This comment was marked as resolved.

mattsse left a comment

Choose a reason for hiding this comment

DaniPopes commented Mar 29, 2023 •

edited

Loading

DaniPopes commented Apr 7, 2023 •

edited

Loading

DaniPopes commented Apr 15, 2023 •

edited

Loading

DaniPopes commented Apr 17, 2023 •

edited

Loading