sway
sway copied to clipboard
FuelLabs
Fix relationship between `handle_comments` & `handle_newlines`
Comments should not have preceding newlines, nor extra ending newlines. All extra newlines should be handled by the newline handler instead. Going about it this way, we ensure that there are no conflicts between comment and newline maps and as a result we also get the benefit of securing the logic from #2787 so that comments preceding module_kind are guaranteed to start at the beginning of a file.
I had to relook at this again because I was looking at fixing #4185, and realized that it is actually an issue to do with the interaction between comments/newlines. New problems seem to have resurfaced, especially after my refactor of the comment formatting to do it at a local scope instead of at a global scope, where we had to reparse the entire module.
Problem
The main problem seems to be that comments are not part of the Sway AST. Currently, when we insert newlines, we're depending on the comparison between the unformatted leaf spans vs the formatted leaf spans to find out where to insert a newline sequence, and this in turn depends on finding newline sequences from a newline map that tracks newlines found between the end of a previous span and the start of the next span. Since comments are not part of the AST, we end up tracking all newlines found, even in the comments, and insert all of them at the end of the previous span with insert_after_span(). This directly resulted in the weird formatting you see in #4185 - all the newlines were inserted at the end of the doc comment span.
Because of this, while the current method is good enough for most cases, it may not work for ALL cases, which is what we see here: commenting out a function temporarily for whatever reason (to make a test pass, to compile, etc.).
I don't have a proposal of changing this just yet - I will have to brainstorm, perhaps look at how existing solutions that I like handle this, and I'll write here again. cc @FuelLabs/tooling for thoughts!
Yeah, I think my final conclusion after the MVP was the same. I think it is limited what we can ensure without having comments in the AST and we saw that with the MVP. We had an earlier discussion about this at #3789. After the rewrite I feel like most of the stuff is easier to manage but edge cases will still happen since we are still manually find correct places.
I feel like we could introduce something like a CommentedNode<T> which offers a .node() etc, to convert it to our regular AST node (i.e, return the inner T). IIRC something like this was also proposed by @mitchmindtree in a discussion we had earlier. This way we can feed the compiler exact same stuff that it was getting before this change by simply iterating over our ast nodes and calling .node() on them. We can also collect it like let ast_nodes = nodes.iter().map(|commented_node| commented_node.node()).collect(); before we feed it to compiler. We would still have our nice commented ast to use while formatting.
This would leave us with handle_newlines(). I think on its own (without interacting with current comment handler) that would work just fine since the tricky part is the relationship between handle_newlines() and comment handling.
Is there anything stopping us from handling comments in the same manner as rustfmt, with a visitor?
It only came up once, but I also liked the idea of an addition to the AST that supports comments, albeit convoluted as discussed prior.
I'm trying a preliminary approach to do some extra work to split by comments:
https://github.com/FuelLabs/sway/blob/d211a4a8f651db07d7a005cb2ffa269af7fb7225/swayfmt/src/utils/map/newline.rs#L147-L169
This could work because within this search space we're purely dealing with the space between leaf spans - it could be a robust enough solution to simply do some manual string manipulation work to take into account comments while searching as well.
Is there anything stopping us from handling comments in the same manner as rustfmt, with a visitor?
I think this would require comments to be within the AST, so it would require us to do something like what @kayagokalp mentioned (having a Commented counterpart of all AST nodes).
To follow up on this, #4548 and other prior efforts to localize comment insertion makes the statement in the said issue outdated, since we are no longer using handle_comments.
Regarding #4548, the work done in this PR improves newline insertion logic by taking into account comments as well, when we previously didn't.
@eureka-cpu Could you share some examples of code where this is broken?
A good place to start, would be the sway applications repo. Clone the repo and add comments in places you would expect to format a certain way and run the formatter on it to see the result.
I don't have any specific cases since I'm out of touch with the formatter.
