Add initial GDScript formatter along with tests and front-ends #97383

Scony · 2024-09-23T21:01:05Z

This PR is the first PR in the series of PRs aiming to implement the GDScript formatter within the engine - partially from scratch and partially by reusing #76211.

Rationale: #76211 (comment)

In particular, it lays the groundwork for adding GDScript formatting capability to the engine by introducing:

a very minimal GDScript noop formatter
unit tests covering noop formatter
ability to format the code from the editor (from the Edit menu or via SHIFT+ALT+F shortcut)
ability to check file formatting via commandline (e.g. using ./bin/godot.linuxbsd.editor.x86_64 --headless --format --check-only -s script.gd

The in-editor functionality is currently hidden behind an editor setting that is disabled by default.

Tentative plan for followup PRs:

Add test runner similar to the one for parser that would be able to turn input-output GDScript file pairs (similar to this) into tests.
Add the most important safety checks to the formatter:
- correctness check that asserts the parse tree before and after formatting is the same (with some minor exceptions)
- stability check that asserts the following - format_code(format_code(code)) == format_code(code) i.e. once the code is formatted, any further formatting attempt won't change the formatting
Extend formatter to actually format very simple statements and blocks
Extend formatter to preserve and format comments correctly and add comment persistence safety check
Extend formatter to support very simple expressions
Extend formatter to support basic strategies
(...)
Enable formatter by default in the builds without tests and add exposed ~~editor~~ project settings that can alter the formatter behavior

arkology · 2024-09-24T03:53:11Z

Add exposed editor settings that can alter the formatter behavior

I think it's important to note obvious thing that formatter rules should be not in editor settings but in separate Resource file (or not Resource, doesn't matter), which is quite important while working with version control systems.

Scony · 2024-09-24T06:13:39Z

Add exposed editor settings that can alter the formatter behavior

I think it's important to note obvious thing that formatter rules should be not in editor settings but in separate Resource file (or not Resource, doesn't matter), which is quite important while working with version control systems.

Good point - fixed.

core/object/script_language.h

adamscott · 2024-09-26T12:53:57Z

How do you plan to do the formatting? Like, I understand that we should start by the beginning and have "a very minimal GDScript formatter with a few hard-coded rules", but currently, the parsing is done with strings, which is to say the least, very lacking.

Right know, this PR sets up a foundation for formatting, but without any foundation for actually format code.

I'm a little bit worried.

Scony · 2024-09-26T13:39:37Z

How do you plan to do the formatting? Like, I understand that we should start by the beginning and have "a very minimal GDScript formatter with a few hard-coded rules", but currently, the parsing is done with strings, which is to say the least, very lacking.

Right know, this PR sets up a foundation for formatting, but without any foundation for actually format code.

I'm a little bit worried.

Well, I'm planning to deliver PRs within 200-300 LOC so this one brings no actual formatter on purpose - there's just no room for everything.

The formatter draft will be added later on - once the test machinery and safety checks are added. This way the first scenarios where actual formatting happens will be covered by all possible checks from the very beginning.

As for the formatter itself, it will be done basically by traversing the AST. Ideally I'd traverse a parse tree (like I do in https://github.com/Scony/godot-gdscript-toolkit), but in Godot's sources there are no means to get parse tree. Perhaps I'll also utilize the tokenizer (as a helper) since the stream of tokens proven to be parsable may act a bit like a poor's man parse tree (after some cleaning).

Other than this, I'm planning to add formatting strategies at every formatting level so that formatter can try few different approaches such as: format to single line, format to condensed lines, format to as many lines as possible etc. to check which one produces lines within the limit while providing the least amount of lines at the same time.

Ofc. the above plan is tentative - the idea may evolve over time.

Overall, I'm planning to utilize code from #76211 along with experience I've gained during 5 years of https://github.com/Scony/godot-gdscript-toolkit development. Also - due to the fact that I'm going to deliver really small PRs - I hope to get some help and utilize experience of other Godot contributors along the way as well.

AThousandShips · 2024-09-26T13:47:10Z

I'd say implementing the framework for a feature separately is not a good workflow, if it's functionality specifically for that one feature. It makes reviewing the framework hard without having the actual needs and contexts of the feature present and clear, it's hard to review based on hypotheticals, and adding code that does nothing is also not optimal IMO

Scony · 2024-09-26T14:12:10Z

I'd say implementing the framework for a feature separately is not a good workflow, if it's functionality specifically for that one feature. It makes reviewing the framework hard without having the actual needs and contexts of the feature present and clear, it's hard to review based on hypotheticals, and adding code that does nothing is also not optimal IMO

Well, I wouldn't call a few tests and frontends a framework. It's just as small PR with a tiny fraction of feature (in a noop fashion just like @vnen suggested here: #76211 (comment)) covered by tests and including means to execute. It already works - it formats empty files and files with a single pass statement.

I know it's not much but it's the quality-first approach. Formatter as a feature is basically just like a mathematical function - it must work perfectly. And I'm convinced it should work perfectly from the very beginning as otherwise the chances of success decrease.

modules/gdscript/gdscript_formatter.cpp

adamscott · 2024-09-27T16:04:56Z

@Scony, can you add the GDScript formatter editor functionality behind a project settings flag? Or an editor one. Because I think that we should maybe merge such a feature, but explicitly say that it's experimental and be off by default (for the time being).

Scony · 2024-09-29T09:45:59Z

@Scony, can you add the GDScript formatter editor functionality behind a project settings flag? Or an editor one. Because I think that we should maybe merge such a feature, but explicitly say that it's experimental and be off by default (for the time being).

Are you sure it makes sense to do it at this point? I anticipate some users may complain on social media and such... I'd at least wait until we have all the statements and simple expressions formattable. Till then I'd leave it as it is now - "enabled only for builds with tests" - so if someone wants to try something, it will be possible yet discouraging.

adamscott · 2024-09-30T16:31:05Z

To be honest, I don't really like the exclusivity of a feature to a non-specific build flag. The TESTS_ENABLED flag is about actual tests, not features.

Scony · 2024-10-01T18:52:52Z

To be honest, I don't really like the exclusivity of a feature to a non-specific build flag. The TESTS_ENABLED flag is about actual tests, not features.

I've been thinking about it and you're right - this will not only be cleaner approach but it will also enable more people willing to help to test formatter in the future.

I've updated PR. I'm just wondering what @akien-mga take will be, but IMO as long as we're in the dev stage and as long as we provide working chunks of formatter, it should be all good.

sockeye-d · 2024-10-01T19:12:25Z

@Scony Will this work with the Godot language server, so external editors will benefit?

HolonProduction · 2024-10-02T06:33:28Z

I've updated PR. I'm just wondering what @akien-mga take will be, but IMO as long as we're in the dev stage and as long as we provide working chunks of formatter, it should be all good.

Not sure if this is possible, but is there maybe a way to mark settings as experimental?

Scony · 2024-10-02T07:19:54Z

@Scony Will this work with the Godot language server, so external editors will benefit?

This PR (among other things) brings the ability to format from the command line, so this can already be attached to editors. As for LSP - I haven't been thinking about it, but it sounds like a good idea for a followup PR.

Scony · 2024-10-02T07:20:44Z

I've updated PR. I'm just wondering what @akien-mga take will be, but IMO as long as we're in the dev stage and as long as we provide working chunks of formatter, it should be all good.

Not sure if this is possible, but is there maybe a way to mark settings as experimental?

That would be perfect, but I have no clue either - any1 knows if it's possible?

dalexeev · 2024-10-02T07:53:59Z

Not sure if this is possible, but is there maybe a way to mark settings as experimental?

That would be perfect, but I have no clue either - any1 knows if it's possible?

Yes, you can mark project and editor settings as experimental/deprecated, they are just properties of ProjectSettings and EditorSettings classes.

HolonProduction · 2024-10-02T11:43:09Z

As for LSP - I haven't been thinking about it, but it sounds like a good idea for a followup PR.

In general, including this into LSP with this interface is doable, but I just did a quick search in the LSP spec, and the first sentence about formatting documents already mentions client capability tracking which isn't implemented at the moment (or I haven't found it yet 😅). So I wouldn't rush it, and aim for a fully compliant implementation, instead of some compromise which ends up working fine with VSCode.

sockeye-d · 2024-10-02T17:19:12Z

Yes, as part of the handshaking process the client must communicate to the server its capabilities. Does Godot not already do that? I thought it was required to set up a connection

Scony · 2024-10-02T19:30:37Z

I've updated PR. I'm just wondering what @akien-mga take will be, but IMO as long as we're in the dev stage and as long as we provide working chunks of formatter, it should be all good.

Not sure if this is possible, but is there maybe a way to mark settings as experimental?

Done. Turns out it was as easy as:

                </member>
-               <member name="text_editor/behavior/formatter/enabled" type="bool" setter="" getter="">
-                       If [code]true[/code], the experimental code formatter can be used to auto-format GDScirpt files. Currently formatter is not implemented and does not format anything.
+               <member name="text_editor/behavior/formatter/enabled" type="bool" setter="" getter="" experimental="Currently formatter is not implemented and does not format anything.">
+                       If [code]true[/code], the experimental code formatter can be used to auto-format GDScirpt files.
                        Please note that this setting requires editor restart to take an action.
                </member>
                <member name="text_editor/behavior/general/empty_selection_clipboard" type="bool" setter="" getter="">

HolonProduction · 2024-10-02T19:44:50Z

Yes, as part of the handshaking process the client must communicate to the server its capabilities. Does Godot not already do that? I thought it was required to set up a connection

I only started looking into the LSP a few weeks ago so take that with a grain of salt, but from what I see, Godot straight up never reads the client capabilities when initializing (can't find a concise version history of LSP, but maybe capabilities weren't a thing in 2019 where this code dates back to 🤷). The LSP is also currently written in a way which would support multiple clients which isn't intended by the spec, so this isn't exactly straight forward. I was going to write up a proposal about possible LSP refactoring when I find the time, so I'd stop the topic here since it isn't really relevant to this PR.

HolonProduction

In general I'd probably add "experimental" somewhere to the editor setting name as well, because the end goal would be to remove it once we have something working.

core/object/script_language_extension.h

main/main.cpp

editor/editor_settings.cpp

doc/classes/EditorSettings.xml

doc/classes/ScriptLanguageExtension.xml

editor/plugins/script_text_editor.cpp

Calinou · 2024-12-06T15:12:32Z

main/main.cpp

@@ -654,6 +655,7 @@ void Main::print_help(const char *p_binary) {
 	print_help_option("-s, --script <script>", "Run a script.\n");
 	print_help_option("--main-loop <main_loop_name>", "Run a MainLoop specified by its global class name.\n");
 	print_help_option("--check-only", "Only parse for errors and quit (use with --script).\n");
+	print_help_option("--format", "Format the file in-place or (if used with --check-only) check if file is formatted correctly. This option must be used with --script and GDScript file.\n");


Suggested change

print_help_option("--format", "Format the file in-place or (if used with --check-only) check if file is formatted correctly. This option must be used with --script and GDScript file.\n");

print_help_option("--format", "Format the script in-place (use with --script) or check if the file is formatted correctly (use with --script --check-only).\n");

I wonder about CI, if we would want a "format to stdout". And the help option should be preceded by [EXPERIMENTAL] imho.

I wonder about CI, if we would want a "format to stdout". And the help option should be preceded by [EXPERIMENTAL] imho.

As for the "format to stdout" - IMO we can add it on-demand (when and if required) once we have some actual formatting implemented. Chances are this won't be ever needed.

As for the [EXPERIMENTAL] can you elaborate? Or do you just mean smth like print_help_option("--format", "[EXPERIMENTAL] Format the script(...)?

This change lays the groundwork for adding GDScript formatting capability to the engine. In particular, this change introduces: - a noop GDScript formatter - unit tests covering noop formatter - ability to format the code from the editor - ability to check file formatting via commandline

Scony requested review from a team as code owners September 23, 2024 21:01

Scony force-pushed the formatter-1 branch from 0371422 to 207a77b Compare September 23, 2024 21:03

Calinou added enhancement topic:gdscript topic:editor labels Sep 23, 2024

Calinou added this to the 4.x milestone Sep 23, 2024

Scony force-pushed the formatter-1 branch from 207a77b to bca0709 Compare September 24, 2024 14:11

HolonProduction reviewed Sep 25, 2024

View reviewed changes

core/object/script_language.h Show resolved Hide resolved

Scony force-pushed the formatter-1 branch 2 times, most recently from 08fcda0 to a941776 Compare September 25, 2024 20:01

Scony requested a review from a team as a code owner September 25, 2024 20:01

Scony force-pushed the formatter-1 branch from a941776 to fbb07c8 Compare September 25, 2024 20:06

adamscott reviewed Sep 26, 2024

View reviewed changes

modules/gdscript/gdscript_formatter.cpp Outdated Show resolved Hide resolved

Scony force-pushed the formatter-1 branch from fbb07c8 to c07880c Compare September 26, 2024 19:16

Scony requested a review from a team as a code owner September 26, 2024 19:16

Scony force-pushed the formatter-1 branch from c07880c to 0c491e3 Compare September 27, 2024 15:35

Scony force-pushed the formatter-1 branch from 0c491e3 to 422bb2a Compare October 1, 2024 18:30

Scony mentioned this pull request Oct 2, 2024

Weird GDFormat style guidelines Scony/godot-gdscript-toolkit#295

Closed

Scony force-pushed the formatter-1 branch from 422bb2a to 5f1cab8 Compare October 2, 2024 19:29

HolonProduction reviewed Oct 2, 2024

View reviewed changes

core/object/script_language_extension.h Outdated Show resolved Hide resolved

main/main.cpp Outdated Show resolved Hide resolved

editor/editor_settings.cpp Outdated Show resolved Hide resolved

doc/classes/EditorSettings.xml Outdated Show resolved Hide resolved

Scony force-pushed the formatter-1 branch from 5f1cab8 to bda737e Compare October 3, 2024 07:02

Scony requested review from adamscott and HolonProduction October 11, 2024 11:49

NathanLovato mentioned this pull request Dec 4, 2024

Add script formatter, comment parsing in parser, and editor settings #76211

Draft