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

feat(fmt): Add support for comments #1682

Closed
wants to merge 4 commits into from

Conversation

jpopesculian
Copy link
Contributor

@jpopesculian jpopesculian commented May 20, 2022

Opening a draft to get some feedback on comments. Essentially the way comments are handled is by splitting them into two categories, prefixes and postfixes and types line and block comments

/* this is a prefix block comment */
let var = 1; // this is a postfix line comment

and trying to appropriately insert them where necessary, where prefixes need to start on a new line and line comments can't have anything follow them. As comments aren't in the parse tree the challenge is to try to figure out when to insert them appropriately and how to deal with the whitespace that they may or may not create.

This is essentially done by the adding a Comments context to the formatter and popping off comments as chunks get written via the write_chunk function and sometimes manually where more control is needed.

What I start doing here as well is to use the IndentWriter more by allowing the writing of multiple lines at a time as chunks can now be multiline given they contain line or prefix comments, and foregoing some of the assumptions of item size / line count before

The next steps are probably

  • Replace all write! functions where necessary with write_chunk!
  • Replace all write_items_separated with write_chunks_separated
  • Lots and lots of tests
  • Cleanup / refactor to find more patterns like write_chunks_separated (this will most likely be important when starting to really implement expressions and handling nesting / operator grouping)

@gakonst
Copy link
Member

gakonst commented May 20, 2022

Could you rebase/cherry-pick the last few commits on latest master? Just merged the previous PR.

The FormatBuffer now implements fmt::Write and handles the indentation
logic, tracking characters and line length. Formatter continues to
control the rest of the logic for how to build strings
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.

just some preliminary comments,

Additional functional documents would make the review somewhat easier later :)

use itertools::Itertools;
use solang_parser::pt::*;

fn trim_comments(s: &str) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

nice, this basically extracts the text content from the comment?
we need some tests for this

}
_ => out.push(ch),
},
_ => out.push(ch),
Copy link
Member

Choose a reason for hiding this comment

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

are we missing the following:

/**
  * <content>
  */ 

if I understand this correctly, then we would also need to strip the whitespace between \n...* in a block quote?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that should be handled by the CommentState::Block which essentially drops all characters until */ is found or EOF

} else {
// line has something
let next_code = src[comment.loc().end()..]
.lines()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

trim_comments needs to be called before lines, due to multi-line comments

@jpopesculian
Copy link
Contributor Author

jpopesculian commented May 23, 2022

I'm afk for a week, so I'm leaving some information here in case anyone wants to pick it up. The next steps (outside of replacing write! calls with write_chunk! calls) is to use the FormatBuffer to handle indented lines

So essentially the FormatBuffer should handle the indentation as follows

// prefix comment
start_of_a_group
  // prefix comment 2
  .continue_the_group_here;
  // postfix comment

// new prefix comment
start_a_new_group2
  .continue_group2_here;

The start of the group will be indented at the level described in level by the FormatBuffer, where the successive lines of the group will be indented by 1. The start of groups would include things like contract/func/var definitions and statements. The group here is important as write_chunk! can possibly write newlines via comments and the groups hold the context as to the indentation of the comments and the content after the comments

So the next step here is to implement a start_group func which will write the postfix comments (possibly on new lines) to the correct indentation and then set the is_beginning_of_group, write the prefix comments and then you can deal with the next chunks.

@gakonst
Copy link
Member

gakonst commented May 23, 2022

@rkrasiuk wanna take a stab at the above?

@jpopesculian
Copy link
Contributor Author

closing for #1833

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants