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

Project Dallas #2626

Merged
merged 294 commits into from
Nov 30, 2022
Merged

Project Dallas #2626

merged 294 commits into from
Nov 30, 2022

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Nov 16, 2022

The F# compiler syntax tree. It will always have a special place in my heart and all that but in a lot of cases, it is not the tree Fantomas truly needs to restore the source code.

A couple of shortcomings (from Fantomas' point of view) come to mind:

  • Code comments are floating in the tree. We still need to do our own detection to which nodes the comment belongs. (the concept of Trivia)
  • There is a lot of stuff in the current tree we don't really use. This is why we so heavily use partial active patterns (in SourceParser.fs) to get around that. Think of DebugPoint, SynValData and others.
  • The way some nodes are grouped isn't typically useful for Fantomas. Recursive bindings or types for example. We consider the nested children to be on the same level, while they are not in the F# AST.
  • Something joining two nodes is more convenient to work it. For example, named computation expressions.

These shortcomings lead to a more complex story than necessary for newcomers.
Our guide on missing code comments is a good example of that. You really need to jump through a lot of hoops to fix this problem.

So, after carefully considering this, we think the situation could be improved by having our own syntax tree. In this PR we are laying the foundation for this. I believe it will unlock some more opportunities in the future due to the new approach.

In short, the premise goes a bit like this:

  • All new type nodes in the new Tree implement the Node interface. A Node can store TriviaNodes before and after. (See SyntaxOak.fs)
  • In Fangorn.fs we map the F# compiler AST to this new Node model.
  • In Flowering.fs we insert the found trivia (comments, newline, directives) into the existing tree.
  • And in CodePrinter2.fs we traverse the new tree to collect the WriterEvents

At the time of writing, 83% of the unit tests are passing. So, there still is some work left to do.
From a style point of view, we try to keep everything exactly as it was. But due to the size of this refactor, we don't necessarily guarantee that nothing will change. Of course, we will respect the style guide at all times. Some things might just change slightly due to convenience.

From an end-user point of view, this won't really affect the usage of the tool. Some performance will be gained, but I don't expect anything significant.

Some todo items:

  • Fix all remaining unit tests
  • Restore format selection
  • Verify everything works for existing codebases (Fantomas, FsAutocomplete, F# compiler to name a few)
  • Rename the new files and type names.
  • Add signature files where it makes sense.
  • Update the online tool to have a viewer for the new tree.
  • Update documentation

@nojaf
Copy link
Contributor Author

nojaf commented Nov 19, 2022

90% of Fantomas.Core.Tests are passing 🎉

@nojaf
Copy link
Contributor Author

nojaf commented Nov 21, 2022

93%

@auduchinok
Copy link
Contributor

In Fangorn.fs we map the F# compiler AST to this new Node model.
In Flowering.fs we insert the found trivia (comments, newline, directives) into the existing tree.

The names are interesting, but maybe it would be better to use self-evident names to make it easier for other, especially new, contributors? 🙂

@nojaf
Copy link
Contributor Author

nojaf commented Nov 21, 2022

@auduchinok yup, already part of my to-do list (see above) 😊.

@nojaf
Copy link
Contributor Author

nojaf commented Nov 21, 2022

96%

@nojaf
Copy link
Contributor Author

nojaf commented Nov 22, 2022

98%

@nojaf
Copy link
Contributor Author

nojaf commented Nov 23, 2022

Good morning all 😊, I was able to restore all unit tests of Fantomas.Core!
Next, I started looking at formatting selections. I think we will be able to pull this off with the new tree. I have one example working, the rest is still a work in progress.
(So at 78bbac7 not all tests of Fantomas.Core.Tests are passing, the FormatSelectionOnly will fail.)

I suspect I'll have the format selection wrapped up by the end of the week.

@nojaf nojaf marked this pull request as ready for review November 25, 2022 21:18
@nojaf
Copy link
Contributor Author

nojaf commented Nov 25, 2022

Works for Fantomas, FsAutocomplete & dotnet/fsharp.

@nojaf nojaf requested a review from dawedawe November 30, 2022 15:53
docs/docs/contributors/Transforming.md Outdated Show resolved Hide resolved
docs/docs/contributors/Transforming.md Outdated Show resolved Hide resolved
docs/docs/contributors/Transforming.md Outdated Show resolved Hide resolved
docs/docs/contributors/Transforming.md Outdated Show resolved Hide resolved
docs/docs/contributors/Transforming.md Outdated Show resolved Hide resolved
docs/docs/contributors/Traverse.md Outdated Show resolved Hide resolved
@nojaf nojaf merged commit bc7d6d5 into fsprojects:v5.2 Nov 30, 2022
@nojaf nojaf deleted the project-dallas branch December 5, 2022 19:10
@nojaf nojaf mentioned this pull request Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants