Add `expr` formatting to `sway-fmt-v2` #2338

eureka-cpu · 2022-07-15T21:58:21Z

Closes #2300
Closes #992
Closes #2467

Currently not all expressions have vertical handling, but they do have horizontal. There could also be more tests, but it may be a good idea to start getting reviews now since this has grown beyond the scope of what it was originally. This also fixes an issue of accidentally adding single line formatting for non-literal structures. There are also some changes to Shape that made multiline formatting easier. I've gone ahead and removed/added features that we weren't/are using.

kayagokalp

Hey @eureka-cpu, I am taking a quick look and wanted to have a little comment. First of all, this seems really great 🔥, for the tests of comments at #2311, I couldn't add comments with expr test because we didn't have expr formatting back then and the way comment stuff works were duplicating comments next to exprs. With this PR, we should be able to add expr comments as well, so it might be a good idea to add a test_expr_comments with this introduction of expr formatting.

I don't know if you were planning to add such tests but wanted to give a heads-up on that 👍

…zation

… projection as well

mitchmindtree

Looking super clean and readable @eureka-cpu, nice work!

I've left a few nits that would be good to address, but no showstoppers. I'll leave it up to you whether we want to address them in this PR or if we want to handle them in smaller follow-up PRs.

`Shape` changes

I have a slight hesitation about exposing the formatter's Shape state as done in this PR as I can imagine it might lead to cases where a Format implementation could easily forget to reset the line style or width heuristics at the end of an item's formatting, leading to following items being mysteriously incorrect only on occasion.

We could potentially avoid this with methods on Formatter that scope these temporary shaping changes, e.g.

formatter.with_line_style(new_style, |formatter| {
    // Do some formatting with `new_style`.
});
// Automatically revert to original style after scope exited.

With an API along these lines, a Format implementation could never accidentally leave the Formatter's inner state in some unexpected state.

All that said, this might require some more significant redesign, and we can investigate diving into this in future issues/PRs. Your existing approach appears to be working so happy to land it!

sway-fmt-v2/src/fmt.rs

mitchmindtree · 2022-08-09T06:21:05Z

sway-fmt-v2/src/fmt.rs

+    assert(balance_test_contract_balance == 3);
+
+    true
+}"#;


In a future PR it might be useful to split this test up into (or create more) small tests for each kind of expression, as it might make it easier to spot the cause of a failing test in CI when we inevitably break formatting by mistake in the future.

While splitting to smaller tests we could also introduce a commented version of those new tests, just like we have for other types

Just like:

sway/sway-fmt-v2/src/fmt.rs

Line 530 in a23accd

fn test_enum_comments() {

This test is actually specifically what @mohammadfawaz gave me for formatting method calls, hence why it was used. But I can see that smaller tests would be better, since we may only end up breaking one specific thing like inline method calls.

mitchmindtree · 2022-08-09T06:32:44Z

sway-fmt-v2/src/items/item_enum.rs

+                });
+
+                let mut value_pairs_iter = value_pairs.iter().enumerate().peekable();
+                for (var_index, (type_field, comma_token)) in value_pairs_iter.clone() {


Ahh I'm just noticing this line might not be quite what we're after. We call value_pairs_iter.clone() here and then use value_pairs_iter.peek() later on during the loop. As a result, the value_pairs_iter.peek() will only ever return the first value which I think is probably not what we want?

To fix this, we could change this line to:

Suggested change

for (var_index, (type_field, comma_token)) in value_pairs_iter.clone() {

while let Some((var_index, (type_field, comma_token))) = value_pairs_iter.next() {

Using while let allows us to avoid consuming the iterator upon starting iteration, and as a result is often used when working with peekable iterators.

I was curious of that, I believe this has been there for some time

mitchmindtree · 2022-08-09T06:57:02Z

sway-fmt-v2/src/items/item_struct.rs

+                });
+
+                let mut value_pairs_iter = value_pairs.iter().enumerate().peekable();
+                for (var_index, (type_field, comma_token)) in value_pairs_iter.clone() {


Same here (w.r.t. using while let instead)

mitchmindtree · 2022-08-09T07:01:01Z

sway-fmt-v2/src/utils/expr/asm_block.rs

+        let mut collected_spans = vec![ByteSpan::from(self.register.span())];
+        if let Some(value) = &self.value_opt {
+            collected_spans.append(&mut value.leaf_spans());
+            // TODO: determine if we are allowing comments between `:` and expr


Ideally we should support comments everywhere that the parser supports them. Perhaps we should open dedicated issues that we can link to in these TODOs so its easy to keep track.

Currently we are supporting this kind of comments as long as it is parsed.

Example

// This can be parsed and supported let number: /* this number is for counting */ u64 = 10; // This cannot be parsed and not supported let number: // this number is for counting u64 = 10;

Weird thing is I thought I get rid of these comments with #2416 and it seems like we do not have it currently (here). Would it be possible while rebasing this PR, those come back accidentally?

To summarize we should be currently supporting all of those TODOs and after we decided to support every possible place the comments are deleted as they were no longer relevant 🤔

mitchmindtree · 2022-08-09T07:02:16Z

sway-fmt-v2/src/utils/expr/asm_block.rs

+        let mut collected_spans = vec![ByteSpan::from(self.register.span())];
+        if let Some(ty) = &self.ty_opt {
+            collected_spans.append(&mut ty.leaf_spans());
+            // TODO: determine if we are allowing comments between `:` and ty


mitchmindtree · 2022-08-09T08:00:25Z

sway-fmt-v2/src/utils/expr/mod.rs

+                write!(formatted_code, "{} ", match_token.span().as_str())?;
+                value.format(formatted_code, formatter)?;
+                MatchBranch::open_curly_brace(formatted_code, formatter)?;
+                let branches = branches.clone().into_inner();


I've noticed there are quite a lot of places where we clone() in order to call into_inner(), even though we only need the inner type by reference.

In this case, the Braces type has a get method which allows us to use the existing instance directly rather than cloning the whole structure.

It might be worth checking all our uses of .clone().into_inner() for similar cases to this as we can likely avoid quite a bit of unnecessary allocation.

mitchmindtree · 2022-08-09T08:02:44Z

sway-fmt-v2/src/utils/expr/mod.rs

+}
+
+/// Collects various expr field's ByteSpans.
+fn visit_expr(expr: &Expr) -> Vec<ByteSpan> {


This function might be better named expr_leaf_spans as we are just matching on the expr to collect leaf spans, not really doing visitor traversal or anything like that.

mitchmindtree · 2022-08-09T08:03:37Z

sway-fmt-v2/src/utils/expr/tests.rs

+
+// TODO:
+// path, literal, abicast, struct, tuple, parens, block, array, asm, return, if, match, while, func app, index, field projection, tuple field projection, ref, deref, operators, bitwise stuff, comp operators
+// logical operators, reassignment


Could we open an issue for these and link it here?

sure! I have taken care of a few of them. I believe we would need all of them as tests before this PR can be approved.

*at least the horizontal ones

mitchmindtree · 2022-08-09T08:05:51Z

sway-fmt-v2/src/utils/punctuated.rs

+    keywords::CommaToken, punctuated::Punctuated, token::PunctKind, StorageField, TypeField,
+};
 use sway_types::{Ident, Spanned};



Suggested change

In general I think it's good to keep use statements together as it allows rustfmt to automatically order them, but not that important.

kayagokalp

Looks great 🚀. Left couple of nits regarding some possible rebasing issue, and expanded @mitchmindtree's great review around the usage of while let.

If you would like to merge this and address changes in a separate PR, just ping me and I can happily approve as they are not blocking issues. I am not approving rn just to give you a chance to look to the reviews 🙌

kayagokalp · 2022-08-09T10:24:59Z

sway-fmt-v2/src/utils/expr/asm_block.rs

+        let mut collected_spans = vec![ByteSpan::from(self.register.span())];
+        if let Some(value) = &self.value_opt {
+            collected_spans.append(&mut value.leaf_spans());
+            // TODO: determine if we are allowing comments between `:` and expr


Currently we are supporting this kind of comments as long as it is parsed.

Example

// This can be parsed and supported let number: /* this number is for counting */ u64 = 10; // This cannot be parsed and not supported let number: // this number is for counting u64 = 10;

Weird thing is I thought I get rid of these comments with #2416 and it seems like we do not have it currently (here). Would it be possible while rebasing this PR, those come back accidentally?

To summarize we should be currently supporting all of those TODOs and after we decided to support every possible place the comments are deleted as they were no longer relevant 🤔

kayagokalp · 2022-08-09T10:27:47Z

sway-fmt-v2/src/utils/expr/asm_block.rs

+        let mut collected_spans = Vec::new();
+        for instruction in &self.instructions {
+            collected_spans.append(&mut instruction.leaf_spans());
+            // TODO: probably we shouldn't allow for comments in between the instruction and comma since it may/will result in build failure after formatting


kayagokalp · 2022-08-09T10:27:57Z

sway-fmt-v2/src/utils/expr/asm_block.rs

+        let mut collected_spans = vec![ByteSpan::from(self.register.span())];
+        if let Some(ty) = &self.ty_opt {
+            collected_spans.append(&mut ty.leaf_spans());
+            // TODO: determine if we are allowing comments between `:` and ty


kayagokalp · 2022-08-09T10:42:41Z

sway-fmt-v2/src/fmt.rs

+    assert(balance_test_contract_balance == 3);
+
+    true
+}"#;


While splitting to smaller tests we could also introduce a commented version of those new tests, just like we have for other types

Just like:

sway/sway-fmt-v2/src/fmt.rs

Line 530 in a23accd

fn test_enum_comments() {

kayagokalp · 2022-08-09T10:47:08Z

sway-fmt-v2/src/utils/punctuated.rs

+            LineStyle::Multiline => {
+                writeln!(formatted_code)?;
+                let mut value_pairs_iter = self.value_separator_pairs.iter().peekable();
+                for (type_field, comma_token) in value_pairs_iter.clone() {


^ Same (w.r.t usage of while let)

kayagokalp · 2022-08-09T10:47:32Z

sway-fmt-v2/src/utils/punctuated.rs

+        match formatter.shape.code_line.line_style {
+            LineStyle::Normal => {
+                let value_pairs = &self.value_separator_pairs;
+                for (type_field, punctuation) in value_pairs.iter() {


I guess #2338 (comment) is also relevant here

kayagokalp · 2022-08-09T10:55:53Z

sway-fmt-v2/src/utils/punctuated.rs

+            LineStyle::Inline => {
+                write!(formatted_code, " ")?;
+                let mut value_pairs_iter = self.value_separator_pairs.iter().peekable();
+                for (type_field, punctuation) in value_pairs_iter.clone() {


^ Same (w.r.t usage of while let)

kayagokalp · 2022-08-09T10:59:36Z

sway-fmt-v2/src/utils/expr/struct_field.rs

+        let mut collected_spans = vec![ByteSpan::from(self.field_name.span())];
+        if let Some((colon_token, expr)) = &self.expr_opt {
+            collected_spans.push(ByteSpan::from(colon_token.span()));
+            // TODO: determine if we are allowing comments between `:` and expr


^ Same (w.r.t old comment (deleted in #2416) and does not exists currently re-spawning)

sway/sway-fmt-v2/src/utils/expr.rs

Line 596 in a23accd

collected_spans.push(ByteSpan::from(expr.0.span()));

kayagokalp

🚀

eureka-cpu · 2022-08-09T19:30:50Z

Looking super clean and readable @eureka-cpu, nice work!

I've left a few nits that would be good to address, but no showstoppers. I'll leave it up to you whether we want to address them in this PR or if we want to handle them in smaller follow-up PRs.

Shape changes

I have a slight hesitation about exposing the formatter's Shape state as done in this PR as I can imagine it might lead to cases where a Format implementation could easily forget to reset the line style or width heuristics at the end of an item's formatting, leading to following items being mysteriously incorrect only on occasion.

We could potentially avoid this with methods on Formatter that scope these temporary shaping changes, e.g.
formatter.with_line_style(new_style, |formatter| {
    // Do some formatting with `new_style`.
});
// Automatically revert to original style after scope exited.
With an API along these lines, a Format implementation could never accidentally leave the Formatter's inner state in some unexpected state.

All that said, this might require some more significant redesign, and we can investigate diving into this in future issues/PRs. Your existing approach appears to be working so happy to land it!

I'm curious if this way will solve the problem of having nested formats, for instance in a let statement where the inner value is an expr that has another expr within it. We would need a way to continually update the state of the shape to fit its formatting needs. I took this approach with this in mind, but if the way you mentioned also takes this into account I'd rather that!

eureka-cpu · 2022-08-09T19:31:43Z

I will create some issues around these comments and address them in separate PRs, thanks so much!

eureka-cpu · 2022-08-09T21:44:14Z

I will create some issues around these comments and address them in separate PRs, thanks so much!

#2493 #2494 #2495 #2496 #2497 #2498 #2499 #2500 #2501

wip expr

6109dd2

eureka-cpu added forc big this task is hard and will take a while formatter labels Jul 15, 2022

eureka-cpu added this to the swayfmt-v2 milestone Jul 15, 2022

eureka-cpu self-assigned this Jul 15, 2022

eureka-cpu added 8 commits July 15, 2022 16:59

add files

5c5861b

Merge branch 'master' into eureka-cpu/2300

8796821

fix imports and separate logic into corresponding files

12e356f

resolve errors and organize files

7ec5552

add code_block

c26ecc2

add assignable formatting

9222d38

add basic formatting of collection expr

deb3a9e

basic formatting for conditionals

ff8c402

sezna mentioned this pull request Jul 18, 2022

StorageInitializers are not (correctly) added to the formatted output #2339

Closed

eureka-cpu added 2 commits July 19, 2022 09:25

Merge branch 'master' into eureka-cpu/2300

c82b8fd

add Delimiter basic expressions

9ee653c

kayagokalp mentioned this pull request Jul 19, 2022

Inserting Comments #2311

Merged

eureka-cpu mentioned this pull request Jul 25, 2022

[Tracking] swayfmt-v2 MVP #1516

Closed

eureka-cpu added 10 commits July 26, 2022 12:37

test passing

c6a12aa

resolve merge conflicts, organize methods

a0e215d

Merge branch 'master' into eureka-cpu/2300

bacf936

add space between } and ( in method calls

aff7db0

resolve merge conflicts

f473542

add in LineStyle adjustments, update punctuated

29ebea9

Merge branch 'master' into eureka-cpu/2300

3bfaf48

fix reset_line_style test

90facf4

remove println

e98bd6e

resolve merge conflicts

1971cff

kayagokalp reviewed Aug 4, 2022

View reviewed changes

eureka-cpu added 7 commits August 7, 2022 22:07

organize ty

dd903eb

change indent_style to shape, update doc comments

e7f8295

add shape file

9ced638

add useful comments and remove unnecessary methods in shape

4aedfaa

get new flow working, tests passing, small adjustments to code organi…

a09791f

…zation

update comments and organize

85bc1c4

Merge branch 'master' into eureka-cpu/2300

2da01da

eureka-cpu requested review from kayagokalp, mitchmindtree and mohammadfawaz August 8, 2022 23:01

clippy strikes again

ebae028

eureka-cpu marked this pull request as ready for review August 8, 2022 23:05

eureka-cpu requested a review from sezna August 8, 2022 23:19

revert method_chain_width back to just chain_width to encompass field…

b2091fb

… projection as well

eureka-cpu added the enhancement New feature or request label Aug 8, 2022

mitchmindtree approved these changes Aug 9, 2022

View reviewed changes

kayagokalp reviewed Aug 9, 2022

View reviewed changes

kayagokalp self-requested a review August 9, 2022 18:25

kayagokalp approved these changes Aug 9, 2022

View reviewed changes

Merge branch 'master' into eureka-cpu/2300

20b490a

eureka-cpu merged commit 0a4ac5d into master Aug 9, 2022

eureka-cpu deleted the eureka-cpu/2300 branch August 9, 2022 19:31

eureka-cpu mentioned this pull request Aug 9, 2022

Use while let in place of for in peekable() iterations in sfv2 #2496

Closed

	for (var_index, (type_field, comma_token)) in value_pairs_iter.clone() {
	while let Some((var_index, (type_field, comma_token))) = value_pairs_iter.next() {

Add expr formatting to sway-fmt-v2 #2338

Add expr formatting to sway-fmt-v2 #2338

Uh oh!

Conversation

eureka-cpu commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kayagokalp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchmindtree left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Shape changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kayagokalp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kayagokalp left a comment

Choose a reason for hiding this comment

Uh oh!

eureka-cpu commented Aug 9, 2022

Shape changes

Uh oh!

eureka-cpu commented Aug 9, 2022

Uh oh!

eureka-cpu commented Aug 9, 2022

Uh oh!

Reviewers

Assignees

Add `expr` formatting to `sway-fmt-v2` #2338

Add `expr` formatting to `sway-fmt-v2` #2338

eureka-cpu commented Jul 15, 2022 •

edited

Loading

kayagokalp left a comment •

edited

Loading

mitchmindtree left a comment •

edited

Loading

`Shape` changes

`Shape` changes