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

Refactor swayfmt to support arbitrary lexed trees #6806

Merged
merged 6 commits into from
Jan 7, 2025

Conversation

ironcev
Copy link
Member

@ironcev ironcev commented Jan 5, 2025

Description

This PR refactors swayfmt to be able generate code from arbitrary lexed trees. Arbitrary means a lexed tree that can be fully or partially created in-memory by manipulating the tree structure in code. It does not need to necessarily be backed by source code. This is needed to be able to reuse swayfmt for generating source code from tools that analyze and modify Sway code, as explained in #6779.

This PR makes the swayfmt independent of spans backed by the source code, by doing the following changes:

  • Keywords are rendered by using theirs AS_STR associated constants.
  • The Token trait is extended with the AS_STR associated constant, and tokens are rendered by using that constant.
  • Idents are rendered by using theirs as_str() methods. This method takes the textual implementation from Ident::name_override_opt if it is provided, and ignores the span in that case.
  • Literals are rendered based on the literal value and not their spans. The exception are numeric literals that do not have empty spans. Those are considered to be backed by source code and are rendered as written in code, e.g., 1_000_000u64.

The PR also fixes some existing bugs in swayfmt:

The PR also removes the path parameter from the parse_file. It was used only to provide an unnecessary dummy source_id which was always pointing to the package root file.

Closes #6779.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@ironcev ironcev self-assigned this Jan 5, 2025
Copy link

codspeed-hq bot commented Jan 5, 2025

CodSpeed Performance Report

Merging #6806 will not alter performance

Comparing ironcev/6779-refactor-swayfmt (6d536d2) with master (76c060b)

Summary

✅ 22 untouched benchmarks

@ironcev ironcev marked this pull request as ready for review January 5, 2025 19:42
@ironcev ironcev requested review from a team as code owners January 5, 2025 19:42
@JoshuaBatty
Copy link
Member

Nice one @ironcev great work with this. Being able to format in memory structures is a great addition. Just a couple of code nits with trailing , in the macros.

@JoshuaBatty JoshuaBatty requested review from a team January 6, 2025 00:20
@ironcev
Copy link
Member Author

ironcev commented Jan 6, 2025

@alfiedotwtf Thanks for such a detailed review of token definitions! To keep this PR focused, I propose to have these potential changes as a separate PR, and combine it with the redefiniton of the & token which we already know we need to do to support references to references, e.g., &&&T.

I've opened a separate issue for that: #6808. Is this ok for you?

@ironcev ironcev requested a review from alfiedotwtf January 6, 2025 10:51
@alfiedotwtf
Copy link
Contributor

I've opened a separate issue for that: #6808. Is this ok for you?

My comments may not have been correct, but creating a new issue so we don't derail this PR is a good idea :)

@alfiedotwtf
Copy link
Contributor

I've got no other suggestions to the PR. With my write!() comments, let's ignore it for this PR so I'll approve now, but I think it could be worth testing out that possible big optimisation win.

Copy link
Member

@bitzoic bitzoic left a comment

Choose a reason for hiding this comment

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

No more commas!

@ironcev ironcev requested a review from a team January 7, 2025 11:45
@IGI-111 IGI-111 merged commit 2357676 into master Jan 7, 2025
41 checks passed
@IGI-111 IGI-111 deleted the ironcev/6779-refactor-swayfmt branch January 7, 2025 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor swayfmt to generate code from arbitrary lexed trees, not necessarily backed by source code
6 participants