feat(fmt): WIP update to latest solang-parser #1612

jpopesculian · 2022-05-13T21:39:12Z

Motivation

Build fails with the newest version of solang-parser if dependencies are not locked

Solution

Update to use the latest version of solang-parser and also lock the solang-parser version

mattsse

looking good so far,

we can merge this once fmt command module in the cli crate is reenabled and compiles

gakonst · 2022-05-16T22:08:50Z

As discussed, let's go with the following plan:

get this ASAP to a mergeable state so that we can re-enable the fmt code
make forge fmt reconstruct all of the source code incl. comments, so that people can at least use it in CI etc.
iterate on adding support for formatting more nodes

gakonst

looks good so far, some thoughts/nits..

what remains to get this over the line?

gakonst · 2022-05-17T23:38:14Z

fmt/src/ast_eq.rs

+    Pure(),
+    View(),
+    Constant(),
+    Payable(),


Should these be Pure instead of Pure()? Given it's an enum with no internal value? @seanyoung maybe a Solang thing

I don't see why they can't be Pure rather than Pure().

In the solang parse tree, pure has a has a source file location value: Pure(Loc) but I don't see how that is relevant

This is just related to the way I wrote the macro here. There's a hidden .. that covers the Loc in the enum. I'll pull it out to make it unhidden and more clear.

gakonst · 2022-05-17T23:40:02Z

fmt/src/ast_eq.rs

+use itertools::Itertools;
+use solang_parser::pt::*;
+
+pub trait AstEq {


Is the reason why this trait exists because we cannot derive on the larpop code-gened files? https://github.com/hyperledger-labs/solang/blob/c63b552373f9660027568e0af4040a82d8821de7/solang-parser/src/solidity.lalrpop#L20

@seanyoung is there a way for us to do that?

if a #[derive(Eq)] or something like that can be done in the solang-parser, by all means.

However, I think here there is a specific type of equality is required here, with sorted function attributes for example. I'm not sure how else we could deal with this.

Yeah, agree. A derive macro would definitely make this more succinct, but we would have to cover the cases where order matters and cases where they don't which means making the proc_macro a little more complicated. In the end, because I just use this for writing tests to make sure the formatted parse tree matches, its not super critical I think. Its more just a convenience and double checking my work

gakonst · 2022-05-17T23:40:38Z

fmt/src/formatter.rs

@@ -2,11 +2,13 @@

 use std::fmt::Write;

+// use crate::operators::Operator;


Suggested change

// use crate::operators::Operator;

gakonst · 2022-05-17T23:44:27Z

fmt/src/formatter.rs

+        // let op = Operator::for_expr(expr);
+        // let is_literal = |exp: &Expression| {
+        //     matches!(
+        //         exp,
+        //         Expression::AddressLiteral(..)
+        //             | Expression::ArrayLiteral(..)
+        //             | Expression::BoolLiteral(..)
+        //             | Expression::HexLiteral(..)
+        //             | Expression::HexNumberLiteral(..)
+        //             | Expression::NumberLiteral(..)
+        //             | Expression::RationalNumberLiteral(..)
+        //             | Expression::StringLiteral(..)
+        //     )
+        // };
+


should this be removed? or do we need it elsewhere and still WIP?

I can remove it, I've decided to go with a different method for handling unary / binary operators and operator precedence anyways since

gakonst · 2022-05-17T23:44:46Z

fmt/src/formatter.rs

+                write!(self, "]")?;
+            }
+            // Expression::Assign(_, var_expr, value_expr) => {
+            //     // TODO handle with operators?


what do we need to handle here?

I'll get rid of this section as per my comment above for right now

there are a lot of expressions we need to handle here, but they basically get split up into a few categories, binary operators, unary operators and literal expresions and a bunch of one off stuff like assigning, identifiers, member access, function calls etc. My plan was to start with binary operators and build up the correct parentheses management / operator precedence, indentation etc after I get comments working

gakonst · 2022-05-17T23:46:06Z

fmt/src/formatter.rs

-        if !event.doc.is_empty() {
-            event.doc.visit(self)?;
-            writeln!(self)?;


why is it OK to remove all these?

The doc comments are no longer attached to individual elements and exist as their own nodes

We made that change because solc accepts doc comments anywhere in a file. This is valid solidity:

contract c {} /** foo */

gakonst · 2022-05-17T23:46:39Z

fmt/src/formatter.rs

+                "(formatted Parse Tree == expected Parse Tree) in {}",
+                filename
+            );
+            // panic!("(formatted Parse Tree == expected Parse Tree) in {}", filename)


Suggested change

// panic!("(formatted Parse Tree == expected Parse Tree) in {}", filename)

gakonst · 2022-05-17T23:51:43Z

fmt/src/formatter.rs

+    test_directory! { EnumDefinition }
+    test_directory! { ErrorDefinition }
+    test_directory! { EventDefinition }
+    // test_directory! { ExpressionPrecedence }


should we re-enable this?

this was an experiment related to the operator precedence as mentioned above. i'll also remove

gakonst · 2022-05-17T23:51:59Z

fmt/src/helpers.rs

+        // TODO handle me
+        todo!()


what needs to go here?

i'll expand the todo message here and return false. it shouldn't panic either way

gakonst · 2022-05-17T23:53:32Z

fmt/src/visit.rs

+                // TODO implement me
+                Ok(())


should this log trace::warn!("Formatting not yet implemented for Using") or something? Same for all our other unimplemented stuff?

jpopesculian · 2022-05-18T13:34:05Z

Okay, I cleaned up the PR, sorry a lot of work in progress stuff got mixed in because I wasn't sure if this was just here as feedback or if it was going to be merged in before comments were done. I probably should have submitted it as a draft, but this at least fixes the update for solang_parser and doc comments, using statements and a few other small things. I'll hopefully have the comments done soon and I'll open that in a separate PR. And then started working on expressions which I think is the next hardest thing to tackle here. After that, there's the Yul stuff, and incremental improvements I think

rkrasiuk

good stuff!

rkrasiuk · 2022-05-19T19:25:46Z

fmt/src/formatter.rs

+                let mut lines = doc_comment
+                    .comment
+                    .lines()
+                    .map(|line| line.trim_start())
+                    .peekable()
+                    .collect::<Vec<_>>();
+                if lines.last() == Some(&"") {
+                    lines.pop();
+                }


Suggested change

let mut lines = doc_comment

.comment

.lines()

.map(|line| line.trim_start())

.peekable()

.collect::<Vec<_>>();

if lines.last() == Some(&"") {

lines.pop();

}

let mut lines = doc_comment

.comment

.trim_end()

.lines()

.map(|line| line.trim_start())

.peekable()

.collect::<Vec<_>>();

much cleaner and correcter! thanks!

…andling

* Make AstEq more clear * Code reorganization * Handling easy todos * Remove useless comments

…s via lib

mattsse

lgtm!

seanyoung

This pr updates the solang-parser crate in fmt, but cli still using an old version: https://github.com/jpopesculian/foundry/blob/fmt/cli/Cargo.toml#L29

This means the formattter could re-format something that the cli code then cannot parse.

jpopesculian force-pushed the fmt branch from 9685e0c to 3e18711 Compare May 13, 2022 21:39

gakonst mentioned this pull request May 13, 2022

Fix solang breaking issues in fmt #1611

Closed

2 tasks

onbjerg added the L-ignore Log: ignore PR in changelog label May 14, 2022

mattsse requested changes May 14, 2022

View reviewed changes

jpopesculian force-pushed the fmt branch from 2e59925 to 02539e4 Compare May 16, 2022 11:53

jpopesculian force-pushed the fmt branch from 9551533 to ef62c40 Compare May 17, 2022 09:10

gakonst reviewed May 17, 2022

View reviewed changes

jpopesculian force-pushed the fmt branch from 1017663 to 4de2e7b Compare May 18, 2022 13:25

rkrasiuk reviewed May 19, 2022

View reviewed changes

jpopesculian and others added 11 commits May 20, 2022 09:51

Update solang-parser to use the latest v0.1.13

fd90cde

added doc comments back

89306cd

fixed spacing for DocComments and added function import statements

b325fe6

WIP adding operators for order of operations and proper parentheses h…

7d9d0c9

…andling

lock solang-parser version

c0be701

switch to nightly formatter

b1d2ec9

add ast comparator for testing

e070efb

comment out WIP code

9f748a2

fmt fix

f0a59b1

Cleanup PR

104ae14

* Make AstEq more clear * Code reorganization * Handling easy todos * Remove useless comments

Fix variable declarations with expressions and expose useful function…

580a32e

…s via lib

jpopesculian force-pushed the fmt branch from 4de2e7b to 580a32e Compare May 20, 2022 09:45

jpopesculian added 2 commits May 20, 2022 12:16

re-enable forge fmt

b744ca4

formatting fix

def6a7c

mattsse approved these changes May 20, 2022

View reviewed changes

seanyoung reviewed May 20, 2022

View reviewed changes

gakonst merged commit 0e6c8fb into foundry-rs:master May 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fmt): WIP update to latest solang-parser #1612

feat(fmt): WIP update to latest solang-parser #1612

jpopesculian commented May 13, 2022

mattsse left a comment

gakonst commented May 16, 2022

gakonst left a comment •

edited

Loading

gakonst May 17, 2022

seanyoung May 18, 2022

jpopesculian May 18, 2022

gakonst May 17, 2022

seanyoung May 18, 2022

jpopesculian May 18, 2022

gakonst May 17, 2022

gakonst May 17, 2022

jpopesculian May 18, 2022

gakonst May 17, 2022

jpopesculian May 18, 2022

jpopesculian May 18, 2022

gakonst May 17, 2022

jpopesculian May 18, 2022

seanyoung May 18, 2022

gakonst May 17, 2022

gakonst May 17, 2022

jpopesculian May 18, 2022

gakonst May 17, 2022

jpopesculian May 18, 2022

gakonst May 17, 2022

jpopesculian commented May 18, 2022

rkrasiuk left a comment

rkrasiuk May 19, 2022

jpopesculian May 20, 2022

mattsse left a comment

seanyoung left a comment

		@@ -2,11 +2,13 @@

		use std::fmt::Write;

		// use crate::operators::Operator;

feat(fmt): WIP update to latest solang-parser #1612

feat(fmt): WIP update to latest solang-parser #1612

Conversation

jpopesculian commented May 13, 2022

Motivation

Solution

mattsse left a comment

Choose a reason for hiding this comment

gakonst commented May 16, 2022

gakonst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpopesculian commented May 18, 2022

rkrasiuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

seanyoung left a comment

Choose a reason for hiding this comment

gakonst left a comment •

edited

Loading