Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feature(formatter): format ASTs with missing tokens or nodes #1707

Merged
merged 3 commits into from
Oct 14, 2021

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Oct 12, 2021

Summary

Adds support for formatting ASTs with missing tokens or nodes which is the first step on our journey to support (partially) formatting source files with arbitrary syntax errors.

The main change is to return an Option<FormatElement> instead of a FormatElement from ToFormatElement.to_format_element if a node or token contains invalid content (e.g. a mandatory token or child node is missing). The parent has then the option to implement some recovery logic, for example inserting the node with its current formatting, or can propagate the formatting error (which most nodes will do).

This PR does not address the issue where tokens and/or nodes are present in places they aren't expected in a valid syntax tree. For example, the formatter messes up the following file

-----;
let x = 10;

becomes

let x = 10;

because script.items() only returns Stmts but the rslint parser doesn't generate a Stmt for ----. Supporting these cases will require significant changes to the rslint parser, which is why we address this issue separately. See my update comment on #1700 for a more extensive explanation of the problem and what my plans are.

Test Plan

Added a few spec tests but we should extend the test cases in the future.

if let Some(token) = self.function_token() {
tokens.push(formatter.format_token(&token));
}
tokens.push(formatter.format_token(&self.function_token()?)?);
Copy link
Contributor Author

@MichaReiser MichaReiser Oct 12, 2021

Choose a reason for hiding this comment

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

We need to bail out (return None) for tokens and children that are expected to be present in a valid sub-tree.

That means for mandatory fields (nodes and tokens) use formatter.format_token(&self.function_token()?)?); and only use

if let Some(token) = self.star_token() {
  tokens.push(formatter.format_token(&token)?)
}

for optional fields.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 13, 2021

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 57aa2fb
Status: ✅  Deploy successful!
Preview URL: https://a01b9632.tools-8rn.pages.dev

View logs

@MichaReiser MichaReiser changed the title Format broken asts feature: Format ASTs with missing tokens or nodes Oct 13, 2021
@MichaReiser MichaReiser requested a review from ematipico October 13, 2021 13:28
@MichaReiser MichaReiser self-assigned this Oct 13, 2021
@MichaReiser MichaReiser added the A-Formatter Area: formatter label Oct 13, 2021
@MichaReiser MichaReiser marked this pull request as ready for review October 13, 2021 13:28
])
fn to_format_element(&self, formatter: &Formatter) -> Option<FormatElement> {
Some(group_elements(format_elements![
formatter.format_token(&self.l_paren_token()?)?,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will become formatter.format_token(&self.l_paren_token())? if we implement my suggestion that mandatory tokens and nodes should be inserted by the parser (and thus, we can change the field accessors for mandatory fields to non-optional types).

@ematipico ematipico changed the title feature: Format ASTs with missing tokens or nodes feature(formatter): format ASTs with missing tokens or nodes Oct 13, 2021
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Comment on lines 123 to 124
/// Creates a [FormatElement] for the passed in node that will represent the node exactly as it
/// is currently formatted in the source text (it actually doesn't change the formatting at all).
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this comment hard to read, can it be reworded?

@MichaReiser MichaReiser merged commit 0d50332 into main Oct 14, 2021
@MichaReiser MichaReiser deleted the format-broken-asts branch October 14, 2021 06:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants