New Razor document formatting engine #11364
Conversation
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
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
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.cs
Outdated
Show resolved
Hide resolved
.../src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/CSharpOnTypeFormattingPass.cs
Outdated
Show resolved
Hide resolved
...or/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/HtmlOnTypeFormattingPass.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This file is unchanged, just elevated out from being a nested class of the old C# formatting pass base.
| @if (true) | ||
| { | ||
| @if (true) | ||
| { |
There was a problem hiding this comment.
This change is because the document is syntactically invalid, and the Razor syntax tree doesn't parse the close brace as a C# token.
| """, | ||
| fileKind: FileKinds.Component); | ||
| } | ||
|
|
||
| [FormattingTestFact] | ||
| [WorkItem("https://github.com/dotnet/razor/issues/8606")] | ||
| public async Task FormatAttributesWithTransition() | ||
| public async Task FormatBindAttributes() |
There was a problem hiding this comment.
The diff on this is wonky, but FormatBindAttributes is an entirely new test and FormatAttributesWithTransition still exists on line 2551 above
| @foreach (var item in Model.Images) | ||
| { | ||
| @foreach (var item in Model.Images) | ||
| { |
There was a problem hiding this comment.
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.
| { | ||
| "Hello", | ||
| "There" | ||
| }, | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
These changes are a result of the new engine fixing #7933
| }, | ||
| Data = Model.WorkOrders, | ||
| Title = "Work Orders" | ||
| }; | ||
| } |
There was a problem hiding this comment.
These changes are a result of the new engine fixing #7933
| }, | ||
| Data = Model.WorkOrders, | ||
| Title = "Work Orders" | ||
| }; | ||
| } |
There was a problem hiding this comment.
These changes are a result of the new engine fixing #7933
| { | ||
| First = 1, | ||
| Second = 2 | ||
| }; | ||
| } |
There was a problem hiding this comment.
These changes are a result of the new engine fixing #7933
| { | ||
| First = 1, | ||
| Second = 2 | ||
| }; | ||
| } |
There was a problem hiding this comment.
These changes are a result of the new engine fixing #7933
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxNodeExtensions.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxNodeExtensions.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxNodeExtensions.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxNodeExtensions.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxNodeExtensions.cs
Outdated
Show resolved
Hide resolved
...eAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.CSharpDocumentGenerator.cs
Outdated
Show resolved
Hide resolved
...eAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.CSharpDocumentGenerator.cs
Outdated
Show resolved
Hide resolved
...eAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.CSharpDocumentGenerator.cs
Outdated
Show resolved
Hide resolved
| _builder.AppendLine(); | ||
| _builder.AppendLine(additionalLinesBuilder.ToString()); | ||
|
|
||
| return SourceText.From(_builder.ToString()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| var indentationString = formattedCSharpText.ToString(new TextSpan(formattedLine.Start, formattedIndentation)) | ||
| + htmlIndentString | ||
| + lineInfo.AdditionalIndentation; |
There was a problem hiding this comment.
Perf note: Consider having a table indentation strings for common cases to avoid allocating a new string for each line.
|
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. |
This reverts commit 161f6f4.
|
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. |
Fixes #10402
Fixes #10864
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