Pretty printing of Fe AST #540

g-r-a-n-t · 2021-09-07T21:09:17Z

What was wrong?

We can't print the AST.

I wanted to fix #531 by adding a $ to the front of lowered tuple struct names (e.g. $tuple_bool_u256), but wasn't able to do so without breaking the lowering tests. This is because the lowering tests compare the RON serialized AST of a lowered source file to that of an AST from a parsed file. So by adding a character to the lowered AST that is illegal to the parser, we are unable to validate that part of the lowering phase.

By making the AST printable, we can replace the existing lowering tests with insta snapshots and not have to be bound by grammar rules when lowering.

How was it fixed?

To-Do

OPTIONAL: Update Spec if applicable
Add entry to the release notes (may forgo for trivial changes)
Clean up commit history

sbillig · 2021-09-07T23:31:03Z

@g-r-a-n-t Does Display give us enough functionality to indent the tree so that it's pretty?

Also, if I had my druthers, the parser ast tests would compare asts with spans and the text content of the span, but other ast printing would ignore the spans. Eg from rowan https://crates.io/crates/rowan

Root@0..7
  BinaryExpr@0..7
    BinaryExpr@0..5
      BinaryExpr@0..3
        Literal@0..1
          Number@0..1 "1"
        Plus@1..2 "+"
        Literal@2..3
          Number@2..3 "2"
      Plus@3..4 "+"
      Literal@4..5
        Number@4..5 "3"
    Plus@5..6 "+"
    Literal@6..7
      Number@6..7 "4"

Not suggesting you have to implement this; just something to consider as you ponder the design.

g-r-a-n-t · 2021-09-08T00:03:16Z

Does Display give us enough functionality to indent the tree so that it's pretty?

I'm using indenter the same way it's used in Yultsur's AST Display impl, so indents shouldn't be a problem. There may be other reasons to not use Display, but I can't think of any, so I just started doing it this way.

crates/lowering/src/names.rs

g-r-a-n-t · 2021-09-10T20:38:19Z

crates/parser/src/ast.rs

+    }
+}
+
+impl fmt::Display for SimpleImportName {


I just left the imports out for now, since we may not be settled on their design.

crates/parser/src/ast.rs

g-r-a-n-t · 2021-09-10T20:39:52Z

crates/parser/src/ast.rs

+                    write!(f, "{}{}", op.kind, operand)
+                }
+            },
+            Expr::CompOperation { left, op, right } => {


do these need parens?

I dont think so

Yeah, eg (1 == 2) == (3 == 4)

(a == b) == (c == d) is equivalent to a == b == (c == d), right? this is at least what the tests are telling me, as the first set of parens are dropped when printing

crates/parser/src/ast.rs

codecov-commenter · 2021-09-10T21:34:22Z

Codecov Report

Attention: Patch coverage is 69.68504% with 77 lines in your changes missing coverage. Please review.

Project coverage is 87.28%. Comparing base (f1327bb) to head (2e84c24).
Report is 589 commits behind head on master.

Files with missing lines	Patch %	Lines
crates/parser/src/ast.rs	69.68%	77 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
- Coverage   87.92%   87.28%   -0.64%     
==========================================
  Files          87       87              
  Lines        5654     5907     +253     
==========================================
+ Hits         4971     5156     +185     
- Misses        683      751      +68

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

g-r-a-n-t

noticed a few more things while working on #546

crates/parser/src/ast.rs

sbillig · 2021-09-13T23:14:13Z

crates/parser/src/ast.rs

+fn inner_expr_needs_parens(outer: &Expr, inner: &Expr) -> bool {
+    expr_power(outer) > expr_power(inner)
+}


The paren insertion logic isn't right. Eg the parens of 2 * (3 / 4) are dropped. For left-associative ops, using > for the lhs of a binop, and >= for the rhs would suffice, but ** is right-associative, so (2 ** 3) ** 4 needs to keep its parens but 2 ** (3 ** 4) doesn't.

Maybe there's a more elegant way, but I think of it as left and right binding power, ie the amount of attraction power an op has on its left and right operands, as in infix_binding_power. A single expr_power fn isn't sufficient, as the power an outer expr exerts on its lhs and rhs might be different, and the power an inner expr responds with may differ depending on whether it's on the left or right.

Ok, updated it to use left and right binding power. I think it's correct now.

sbillig · 2021-09-13T23:15:42Z

crates/parser/src/ast.rs

+                    write!(f, "{}{}", op.kind, operand)
+                }
+            },
+            Expr::CompOperation { left, op, right } => {


Yeah, eg (1 == 2) == (3 == 4)

g-r-a-n-t · 2021-09-20T19:39:48Z

I'm merging this @sbillig.

I'm fairly confident the paren stuff is correct now. It's also only going to be used for testing, so it's not something that needs to be perfect.

g-r-a-n-t commented Sep 10, 2021

View reviewed changes

g-r-a-n-t force-pushed the print-ast branch 2 times, most recently from 90f122f to 3a3aadc Compare September 10, 2021 21:08

g-r-a-n-t commented Sep 10, 2021

View reviewed changes

crates/parser/src/ast.rs Outdated Show resolved Hide resolved

g-r-a-n-t requested a review from sbillig September 10, 2021 21:23

g-r-a-n-t force-pushed the print-ast branch 2 times, most recently from 3c25a9d to 861d321 Compare September 10, 2021 22:18

g-r-a-n-t changed the title ~~Implement display for AST~~ Pretty printing of Fe AST Sep 10, 2021

g-r-a-n-t commented Sep 13, 2021

View reviewed changes

crates/parser/src/ast.rs Outdated Show resolved Hide resolved

crates/parser/src/ast.rs Outdated Show resolved Hide resolved

crates/parser/src/ast.rs Outdated Show resolved Hide resolved

crates/parser/src/ast.rs Outdated Show resolved Hide resolved

sbillig reviewed Sep 13, 2021

View reviewed changes

g-r-a-n-t force-pushed the print-ast branch from dbf3d82 to 80af5ed Compare September 15, 2021 22:58

Pretty printing of Fe AST implemented via Display trait.

7beb5cd

g-r-a-n-t force-pushed the print-ast branch from 80af5ed to 7beb5cd Compare September 15, 2021 23:21

g-r-a-n-t requested a review from sbillig September 16, 2021 15:58

Empty struct and event defs print pass

2e84c24

g-r-a-n-t merged commit 0eb0898 into ethereum:master Sep 20, 2021

sbillig mentioned this pull request Oct 22, 2021

Pretty-print AST #326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pretty printing of Fe AST #540

Pretty printing of Fe AST #540

g-r-a-n-t commented Sep 7, 2021 •

edited

Loading

sbillig commented Sep 7, 2021

g-r-a-n-t commented Sep 8, 2021 •

edited

Loading

g-r-a-n-t Sep 10, 2021

g-r-a-n-t Sep 10, 2021

g-r-a-n-t Sep 10, 2021

sbillig Sep 13, 2021

g-r-a-n-t Sep 15, 2021 •

edited

Loading

codecov-commenter commented Sep 10, 2021 •

edited

Loading

g-r-a-n-t left a comment

sbillig Sep 13, 2021

g-r-a-n-t Sep 15, 2021

sbillig Sep 13, 2021

g-r-a-n-t commented Sep 20, 2021

Pretty printing of Fe AST #540

Pretty printing of Fe AST #540

Conversation

g-r-a-n-t commented Sep 7, 2021 • edited Loading

What was wrong?

How was it fixed?

To-Do

sbillig commented Sep 7, 2021

g-r-a-n-t commented Sep 8, 2021 • edited Loading

g-r-a-n-t Sep 10, 2021

Choose a reason for hiding this comment

g-r-a-n-t Sep 10, 2021

Choose a reason for hiding this comment

g-r-a-n-t Sep 10, 2021

Choose a reason for hiding this comment

sbillig Sep 13, 2021

Choose a reason for hiding this comment

g-r-a-n-t Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Sep 10, 2021 • edited Loading

Codecov Report

g-r-a-n-t left a comment

Choose a reason for hiding this comment

sbillig Sep 13, 2021

Choose a reason for hiding this comment

g-r-a-n-t Sep 15, 2021

Choose a reason for hiding this comment

sbillig Sep 13, 2021

Choose a reason for hiding this comment

g-r-a-n-t commented Sep 20, 2021

g-r-a-n-t commented Sep 7, 2021 •

edited

Loading

g-r-a-n-t commented Sep 8, 2021 •

edited

Loading

g-r-a-n-t Sep 15, 2021 •

edited

Loading

codecov-commenter commented Sep 10, 2021 •

edited

Loading