New Razor document formatting engine #11364

davidwengier · 2025-01-08T05:04:51Z

This PR moves from the old document formatting engine, to a new one that doesn't use the normal Razor compiler generated C# document, and is therefore able to work regardless of whether the system is configured for runtime or design time code-gen (ie, FUSE or not). Some details about how it works can be seen in the doc comments in CSharpFormattingPass.CSharpDocumentGenerator.cs. The existing engine is still used for On Type Formatting, as well as formatting the results of code actions, OnAutoInsert, snippet completions, etc. This means there isn't as much code removal as I'd like, but I'm hoping to follow up with more PRs in future to address some of this.

Looking at this commit at a time is probably overkill, but would be workable if you prefer it. If not, there are a couple of spots where the final git diff won't do you any favours, and I'll comment on them as appropriate.

As proof of the benefits of the new design, this also fixes #7933 almost by accident, by which I mean by design, but without specific action.
Finally, in a meeting about this engine Andrew asked how this engine would work for lambda attributes, and so I added tests for that, and also accidentally (ie deliberately) fixes #9777

I can't remember what this is for, but a passing formatting test isn't the worst thing in the world

Not sure if its a mark for or against the old engine, that these tests used to pass

These cover additional cases that would have shown up bugs in code as I was writing it

We don't need to do much with the new engine, just stop the couple of places where Html formatting actively breaks Razor

Basic theory is to generate a synthetic C# document, format it, and then interpret the results.

Waiting on the team consensus as to whether this is feature flag worthy

… formatter This class could do with a complete clean up, but I'm saving that for a follow up once we know if the old engine is still needed in parallel

The approach to implicit and explicit expressions couldn't handle them if they weren't at the start of a line, so these tweaks set us up for a different approach where that is handled.

… line, or how many of them there are

…tatements

Fixing these was not intentional, but probably speaks to the value of the new approach avoiding the generated C#

Each of these cases is an objective win, making Razor formatting behave the same way as Roslyn formatting.

Arguably the old behaviour is better, but given these are invalid Razor documents, it doesn't seem to me to be worth worrying about. Also this could improve with better recovery behaviour in the compiler.

…tween runtime and design time code gen

This is one of those situations where Roslyn simple doesn't indent anything, so we have to do a little bit more work to set things up for a better chance of success.

… formatted correctly

…es 🤦‍♂️

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.cs

davidwengier · 2025-01-08T05:13:28Z

...st/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/DocumentFormattingTest.cs

                    """,
            fileKind: FileKinds.Component);
    }

    [FormattingTestFact]
-    [WorkItem("https://github.com/dotnet/razor/issues/8606")]
-    public async Task FormatAttributesWithTransition()
+    public async Task FormatBindAttributes()


The diff on this is wonky, but FormatBindAttributes is an entirely new test and FormatAttributesWithTransition still exists on line 2551 above

davidwengier · 2025-01-08T05:15:12Z

...st/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/DocumentFormattingTest.cs

-                            @foreach (var item in Model.Images)
-                            {
+                    @foreach (var item in Model.Images)
+                    {


This change is due to the document being syntactically invalid. The Razor parser doesn't see the close brace as a C# token, and the section and div elements have start and end tags that don't match.

davidwengier · 2025-01-08T05:16:02Z

...st/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/DocumentFormattingTest.cs

                                {
                                    "Hello",
                                    "There"
                                },
-                            };
+                        };
                    }


These changes are a result of the new engine fixing #7933

davidwengier · 2025-01-08T05:16:25Z

...st/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/DocumentFormattingTest.cs

+                                },
+                                Data = Model.WorkOrders,
+                                Title = "Work Orders"
+                            };
                        }


These changes are a result of the new engine fixing #7933

davidwengier · 2025-01-08T05:16:31Z

...st/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/DocumentFormattingTest.cs

+                                },
+                                Data = Model.WorkOrders,
+                                Title = "Work Orders"
+                            };
                        }


These changes are a result of the new engine fixing #7933

davidwengier · 2025-01-08T05:16:39Z

...st/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/DocumentFormattingTest.cs

+                            {
+                                First = 1,
+                                Second = 2
+                            };
                        }


These changes are a result of the new engine fixing #7933

davidwengier · 2025-01-08T05:16:45Z

...st/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/DocumentFormattingTest.cs

+                            {
+                                First = 1,
+                                Second = 2
+                            };
                        }


These changes are a result of the new engine fixing #7933

DustinCampbell

Thanks for the helpful notes!

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxNodeExtensions.cs

...eAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.CSharpDocumentGenerator.cs

DustinCampbell · 2025-01-08T18:50:41Z

...eAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.CSharpDocumentGenerator.cs

+            _builder.AppendLine();
+            _builder.AppendLine(additionalLinesBuilder.ToString());
+
+            return SourceText.From(_builder.ToString());


Perf note: This will create a string on the LOH for sufficiently large files. Consider whether there's something better to use here than a StringBuilder so a TextReader can be passed to SourceText.From. That could be captured in an issue for later work, of course.

Definitely planning on perf being a separate PR. The old engine used to re-compile the Razor file 3 or 4 times, so we get a huge perf win from just not doing that, that it didn't seem worth complicating this PR. There are definitely some low hanging fruit, though I'd probably want to fix on-type formatting first, since that also recompiles Razor files a lot (and each time, the compiler produces the entire generated C# file as a string!).

I'll capture this and the blow comment in a new issue though, so thanks for the hint.

DustinCampbell · 2025-01-08T19:19:31Z

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.cs

+                var indentationString = formattedCSharpText.ToString(new TextSpan(formattedLine.Start, formattedIndentation))
+                    + htmlIndentString
+                    + lineInfo.AdditionalIndentation;


Perf note: Consider having a table indentation strings for common cases to avoid allocating a new string for each line.

davidwengier · 2025-01-09T23:07:32Z

Apologies, this PR was a bit of a bet towards putting this in without a feature flag, but we decided today that we do want a feature flag, so this PR is going to get a little noisier. The final diff should actually be simpler though.

…ToRuleThemAll

This reverts commit 161f6f4.

davidwengier · 2025-01-10T03:47:23Z

Apologies for the diff, I didn't rebase but I may as well have considering the whole new formatting engine has moved (back) to a separate file. The final diff is reasonably clean though, only 37 files changed, and all feedback has been addressed.

The new engine is now behind a feature flag, and is off by default.

ryzngard

ryzngard · 2025-01-10T22:16:33Z

...lysis.Razor.Workspaces/Formatting/Passes/New/CSharpFormattingPass.CSharpDocumentGenerator.cs

+        public static CSharpFormattingDocument Generate(RazorCodeDocument codeDocument, RazorFormattingOptions options)
+        {
+            using var _ = StringBuilderPool.GetPooledObject(out var builder);
+            using var lineInfo = new PooledArrayBuilder<LineInfo>(capacity: codeDocument.Source.Text.Lines.Count);


nit: lineInfoBuilder

ryzngard · 2025-01-10T22:22:42Z

...lysis.Razor.Workspaces/Formatting/Passes/New/CSharpFormattingPass.CSharpDocumentGenerator.cs

+            private TextLine _currentLine;
+            private int _currentFirstNonWhitespacePosition;
+
+            // These are set in GetCSharpDocumentContents so will never be observably null


❓ why are these lazily initialized? I would think they realistically are always going to be created.

I don't think of these as lazy, they're just fields by necessity. _currentToken tracks the current token being processed, but there is no way to pass that in to the Visit methods. As such, there simply isn't a default value that makes sense to intiialize it to, so I used null!.

_builder is owned externally so it can come from a pool, without worrying about leaking pooled objects. Doing that was based off a suggestion from Dustin earlier in the PR, and I think it makes sense.

It's a bit of a compromise that they have to exist, but unless we bring dotnet/roslyn#55183 over to Razor, I don't know another way. I'm all ears if you have a suggestion though?

Though having written all of that, I think maybe I've misunderstood your question/comment?

I think my brain scanned quickly. The StringBuilder being null and set later is really the oddity and I assumed below field(s) followed suit.

Looking at https://github.com/dotnet/razor/pull/11364/files/2e3adc7ab43a1deb9ed07684d745f69f8f50c8e9#r1911738908 I think that's potentially better representing things: this class is a bit of an oddball still.

Having SyntaxVisitor<TArgument, TResult> could help, but for lifetime of StringBuilder etc couldn't that be solved by making this disposable? and/or a ref struct since this never needs to escape this method or go to the heap.

Maybe viewing this class similar to a stream operation would be helpful. Thinking while typing: it operates on lines in a forward only manner. It generates SourceText and LineInfo after "consuming" the stream. You have a Current which is the current TextLine, some hardcoded metadata about formatting options, and a pointer back to the original SourceText. Maybe separating the enumeration and the generation would clean things up?

This is definitely not blocking and more just bouncing ideas. Can be left alone or done in a follow up

Definitely oddball. I wished for ref struct at one point too, but the class inherits from SyntaxVisitor, so that can't work. I like the stream idea, but again, I think its limited by SyntaxVisitor (the enumerator would have to set a bunch of private state fields on the generator). I do wonder if that is something that is worth changing. It's not really much of a visitor, since it absolutely can't just visit an arbitrary tree and it almost never actually uses the visitor pattern properly, but it was very useful for it to be one during development. Being able to type override and have completion show me which node types were available, unhandled candidates was great.

Maybe now that development is finished (lol!), it would be more sensible to simply have a method that has a giant switch expression, which would allow more freedom of design? Probably could even drive the whole thing from a single static method.

Will add a note to the follow up issue to keep thinking about this.

ryzngard · 2025-01-10T22:59:02Z

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/RazorFormattingPass.cs

-            if (children is [.., MarkupBlockSyntax block, RazorMetaCodeSyntax /* close brace */] &&
-                !context.CodeDocument.GetCSharpDocument().SourceMappings.Any(m => block.Span.Contains(m.OriginalSpan.AbsoluteIndex)))
+            // This doesn't cause any harm with the new engine, but its a waste of effort.
+            if (!_languageServerFeatureOptions.UseNewFormattingEngine)


nit: Should this move before the if above just to reduce some work?

Not sure I follow exactly. There is no work to avoid here, it isn't an early return, and the above if produces things this needs.

ryzngard · 2025-01-10T23:07:52Z

...st/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting_NetFx/DocumentFormattingTest.cs

+    }
+
+    [FormattingTestFact]
+    public async Task StartsWithWhitespace()


For completeness do we need an EndsWithWhitespace? I Guess it's covered here (and probably elsewhere)

I'm happy to add one, but my approach for creating new tests was simply pure reaction. Either there were quirks/bugs I found during development, or conditions I was adding that I wanted to validate. So if there is no EndsWithWhitespace, it means there was nothing odd about the tree when it came to formatting it :)

I'd say just add it for now. If nothing else serves as a regression test in case the tree changes. The tests are great, run quick, and provide a baseline comfort for changing both the syntax tree in the compiler and formatting behavior in tooling. I'd just err on more coverage even if it's not proven to be needed

run quick

They ran a lot quicker before I made them all run 8 times 😁

DustinCampbell

Looking good!

DustinCampbell · 2025-01-10T23:28:00Z

...lysis.Razor.Workspaces/Formatting/Passes/New/CSharpFormattingPass.CSharpDocumentGenerator.cs

+            private int _currentFirstNonWhitespacePosition;
+
+            // These are set in GetCSharpDocumentContents so will never be observably null
+            private StringBuilder _builder = null!;


Just noting that this can be passed into the constructor and the field can be readonly now.

I'll be honest, I have yet to find any implementation of this class that I actually like fully. I did this change at one point, but it felt like if I was doing that, I may as well just pass the lineInfo array in too, and do all of the work in the constructor. Which also felt wrong :)

Yeah, I think that'd be wrong. 😄 FWIW, I was only noting that _builder could be passed to the constructor. I don't think it's necessary since it's harder to pass the PooledArrayBuilder<LineInfo> there. However, if you wanted to pass both to the constructor, you could just grab an ImmutableArray<LineInfo>.Builder from ArrayBuilderPool and forego the PooledArrayBuidler` savings.

True. I guess PooledArrayBuilder is only helping us with very short documents.

davidwengier · 2025-01-12T20:56:49Z

Thanks for the reviews. Going to merge this, but there will be follow ups, so if I've misinterpreted anything or there is more to do, feel free to comment on #11371

davidwengier added 28 commits January 8, 2025 15:26

Add another test.

81264b1

I can't remember what this is for, but a passing formatting test isn't the worst thing in the world

Add some useful directive extension methods

e13da44

Add a helper method to calculate the indent level from an existing line

abf6e27

Fix tests that use tabs, or a custom tab size

54ca90b

Not sure if its a mark for or against the old engine, that these tests used to pass

Add or expand a few formatting tests

36efbe9

These cover additional cases that would have shown up bugs in code as I was writing it

Add some helper utiltiies with basic test coverageMore hlper methods

98d3a2a

Create a simple Html formatting pass

dd87d34

We don't need to do much with the new engine, just stop the couple of places where Html formatting actively breaks Razor

Add a new C# formatting pass

b60fa37

Basic theory is to generate a synthetic C# document, format it, and then interpret the results.

Expose Roslyn workspace helper

4d78e67

Dodgily hook up the new formatting engine

ef1c95e

Waiting on the team consensus as to whether this is feature flag worthy

Slight tweaks to the RazorFormattingPass to make it not break the new…

cf487c9

… formatter This class could do with a complete clean up, but I'm saving that for a follow up once we know if the old engine is still needed in parallel

Tweaks to formatting logic for explicit expressions

c2a3723

The approach to implicit and explicit expressions couldn't handle them if they weren't at the start of a line, so these tweaks set us up for a different approach where that is handled.

Format implicit or explicit expressions no matter where they are on a…

de4ab65

… line, or how many of them there are

Add tests and coverage for the quirks of single line empty explicit s…

10b692c

…tatements

Little bit of code cleanup and sharing

8e0514f

Unskip a few tests

f6b00ef

Fixing these was not intentional, but probably speaks to the value of the new approach avoiding the generated C#

Update expected output to match the new formatter

bcde950

Each of these cases is an objective win, making Razor formatting behave the same way as Roslyn formatting.

Update expected output to match the new engine behaviour

5b3e819

Arguably the old behaviour is better, but given these are invalid Razor documents, it doesn't seem to me to be worth worrying about. Also this could improve with better recovery behaviour in the compiler.

Fix start position calculation to allow for whitespace differences be…

1736268

…tween runtime and design time code gen

PR feedbacl

ebde05b

Fix build errors

74136b2

Fix failing test

6d53a14

This is one of those situations where Roslyn simple doesn't indent anything, so we have to do a little bit more work to set things up for a better chance of success.

Add tests for lambda parameters, and relax an if statement so they're…

6bf1b19

… formatted correctly

Language server and cohost test runners have different using directiv…

c503970

…es 🤦‍♂️

Remove ability to opt in to the old formatting engine

5cdd419

Remove old document formatting bits

766b9a5

Move new formatting engine, so there is only one

7449200

Remove ability to force design time code-gen in FUSE

161f6f4

davidwengier requested a review from a team as a code owner January 8, 2025 05:04

davidwengier commented Jan 8, 2025

View reviewed changes

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.cs Outdated Show resolved Hide resolved

davidwengier commented Jan 8, 2025

View reviewed changes

DustinCampbell reviewed Jan 8, 2025

View reviewed changes

SyntaxNodeExtensions related PR feedback

1620344

davidwengier mentioned this pull request Jan 9, 2025

New Formatting Engine Followup #11371

Open

davidwengier added 4 commits January 9, 2025 16:12

Formatting utilities PR feedback

5e61c87

CSharpDocumentGenerator PR feedback and clean up

431a0fe

Whitespace🤦‍♂️

9524144

...

d1fa870

davidwengier added 5 commits January 10, 2025 12:22

Merge remote-tracking branch 'upstream/main' into OneFormattingEngine…

3fd0ab2

…ToRuleThemAll

Add feature flag to allow opting in to the new formatting engine

162ad30

Put back the old formatting engine, but opt to use the new one in code

be8725b

Revert "Remove ability to force design time code-gen in FUSE"

beabfce

This reverts commit 161f6f4.

Run all (most) tests with the new engine, and the existing one

2e3adc7

davidwengier requested a review from a team as a code owner January 10, 2025 03:46

davidwengier removed the request for review from a team January 10, 2025 03:47

ryzngard approved these changes Jan 10, 2025

View reviewed changes

DustinCampbell approved these changes Jan 10, 2025

View reviewed changes

PR Feedback

2e1093f

davidwengier merged commit 140c23f into dotnet:main Jan 12, 2025
12 checks passed

davidwengier deleted the OneFormattingEngineToRuleThemAll branch January 12, 2025 20:56

dotnet-policy-service bot added this to the Next milestone Jan 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Razor document formatting engine #11364

New Razor document formatting engine #11364

davidwengier commented Jan 8, 2025 •

edited

Loading

davidwengier Jan 8, 2025

davidwengier Jan 8, 2025

davidwengier Jan 8, 2025

davidwengier Jan 8, 2025

davidwengier Jan 8, 2025

davidwengier Jan 8, 2025

davidwengier Jan 8, 2025

DustinCampbell left a comment

DustinCampbell Jan 8, 2025

davidwengier Jan 9, 2025

DustinCampbell Jan 8, 2025

davidwengier commented Jan 9, 2025

davidwengier commented Jan 10, 2025

ryzngard left a comment

ryzngard Jan 10, 2025

ryzngard Jan 10, 2025

davidwengier Jan 10, 2025

ryzngard Jan 11, 2025

davidwengier Jan 11, 2025 •

edited

Loading

ryzngard Jan 10, 2025

davidwengier Jan 12, 2025

ryzngard Jan 10, 2025

davidwengier Jan 10, 2025

ryzngard Jan 11, 2025

davidwengier Jan 12, 2025

DustinCampbell left a comment

DustinCampbell Jan 10, 2025

davidwengier Jan 10, 2025

DustinCampbell Jan 11, 2025

davidwengier Jan 11, 2025

davidwengier commented Jan 12, 2025

New Razor document formatting engine #11364

New Razor document formatting engine #11364

Conversation

davidwengier commented Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier commented Jan 9, 2025

davidwengier commented Jan 10, 2025

ryzngard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwengier commented Jan 12, 2025

davidwengier commented Jan 8, 2025 •

edited

Loading

davidwengier Jan 11, 2025 •

edited

Loading