Fix(ish) formatting of RenderFragments (C# templates) (#12397)

Fixes #12310
Fixes #6632

This doesn't do a _perfect_ job of formatting these, because there is an
extra level of indentation in some (all?) cases, because the Html
formatter doesn't see the `@<tag>` as a tag. It's a _lot_ better than
what we did previously (see screenshot in
#12310), and likely needs
#11916 for us to be able to do
anything better, so I think this is still a net positive for users.

Author: davidwengier

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

@@ -6,9 +6,11 @@
 using System.Diagnostics;
 using System.Linq;
 using System.Text;
+using Microsoft.AspNetCore.Razor;
 using Microsoft.AspNetCore.Razor.Language;
 using Microsoft.AspNetCore.Razor.Language.Syntax;
 using Microsoft.AspNetCore.Razor.PooledObjects;
+using Microsoft.CodeAnalysis.ExternalAccess.Razor.Features;
 using Microsoft.CodeAnalysis.Razor.Workspaces;
 using Microsoft.CodeAnalysis.Text;
 using RazorSyntaxNode = Microsoft.AspNetCore.Razor.Language.Syntax.SyntaxNode;
@@ -147,6 +149,7 @@ private sealed class Generator(
             private readonly RazorCodeDocument _codeDocument = codeDocument;
             private readonly bool _insertSpaces = options.InsertSpaces;
             private readonly int _tabSize = options.TabSize;
+            private readonly RazorCSharpSyntaxFormattingOptions? _csharpSyntaxFormattingOptions = options.CSharpSyntaxFormattingOptions;
             private readonly StringBuilder _builder = builder;
             private readonly ImmutableArray<LineInfo>.Builder _lineInfoBuilder = lineInfoBuilder;
 
@@ -386,6 +389,18 @@ _sourceText.Lines[nodeStartLine] is { } previousLine &&
             public override LineInfo VisitCSharpStatementLiteral(CSharpStatementLiteralSyntax node)
             {
                 Debug.Assert(node.LiteralTokens.Count > 0);
+
+                // If this is the end of a multi-line CSharp template (ie, RenderFragment) then we need to close
+                // out the lambda expression that we started when we opened it.
+                if (node.LiteralTokens.Where(static t => !t.IsWhitespace()).FirstOrDefault() is { Content: ";" } &&
+                    node.TryGetPreviousSibling(out var previousSibling) &&
+                    previousSibling is CSharpTemplateBlockSyntax &&
+                    GetLineNumber(previousSibling.GetFirstToken()) != GetLineNumber(previousSibling.GetLastToken()))
+                {
+                    _builder.AppendLine("};");
+                    return CreateLineInfo();
+                }
+
                 return VisitCSharpLiteral(node, node.LiteralTokens[^1]);
             }
 
@@ -412,22 +427,47 @@ private LineInfo VisitCSharpLiteral(RazorSyntaxNode node, RazorSyntaxToken lastT
                     // Now emit the contents
                     var span = TextSpan.FromBounds(_currentFirstNonWhitespacePosition, node.EndPosition);
                     _builder.Append(_sourceText.ToString(span));
-                    // Append a comment at the end so whitespace isn't removed, as Roslyn thinks its the end of the line, but we know it isn't.
-                    _builder.AppendLine(" //");
 
                     // Putting a semi-colon on the end might make for invalid C#, but it means this line won't cause indentation,
                     // which is all we need. If we're in an explicit expression body though, we don't want to do this, as the
                     // close paren of the expression will do the same job (and the semi-colon would confuse that).
                     var emitSemiColon = node.Parent.Parent is not CSharpExplicitExpressionBodySyntax;
+
+                    var skipNextLineIfBrace = false;
+                    int formattedOffsetFromEndOfLine;
+
+                    // If the template is multiline we emit a lambda expression, otherwise just a null statement so there
+                    // is something there. See VisitMarkupTransition for more info
+                    if (token.Parent?.Parent.Parent is CSharpTemplateBlockSyntax template &&
+                        _sourceText.GetLinePositionSpan(template.Span).SpansMultipleLines())
+                    {
+                        emitSemiColon = false;
+                        skipNextLineIfBrace = true;
+                        _builder.AppendLine("() => {");
+
+                        // We only want to format up to the text we added, but if Roslyn inserted a newline before the brace
+                        // then that position will be different. If we're not given the options then we assume the default behaviour of
+                        // Roslyn which is to insert the newline.
+                        formattedOffsetFromEndOfLine = _csharpSyntaxFormattingOptions?.NewLines.IsFlagSet(RazorNewLinePlacement.BeforeOpenBraceInLambdaExpressionBody) ?? true
+                            ? 5
+                            : 7;
+                    }
+                    else
+                    {
+                        _builder.AppendLine("null");
+                        formattedOffsetFromEndOfLine = 4;
+                    }
+
                     if (emitSemiColon)
                     {
                         _builder.AppendLine(";");
                     }
 
                     return CreateLineInfo(
                         skipNextLine: emitSemiColon,
+                        skipNextLineIfBrace: skipNextLineIfBrace,
                         formattedLength: span.Length,
-                        formattedOffsetFromEndOfLine: 3,
+                        formattedOffsetFromEndOfLine: formattedOffsetFromEndOfLine,
                         processFormatting: true,
                         // We turn off check for new lines because that only works if the content doesn't change from the original,
                         // but we're deliberately leaving out a bunch of the original file because it would confuse the Roslyn formatter.
@@ -458,8 +498,25 @@ public override LineInfo VisitMarkupStartTag(MarkupStartTagSyntax node)
 
             public override LineInfo VisitMarkupEndTag(MarkupEndTagSyntax node)
             {
-                // Since this visitor only sees nodes at the start of a line, an end tag always means de-dent.
-                //return new("}");
+                return VisitEndTag(node);
+            }
+
+            private LineInfo VisitEndTag(BaseMarkupEndTagSyntax node)
+            {
+                // If this is the last line of a multi-line CSharp template (ie, RenderFragment), and the semicolon that ends
+                // if is on the same line, then we need to close out the lambda expression that we started when we opened it.
+                // If the semicolon is on the next line, then we'll take care of that when we get to it.
+                if (node.Parent.Parent.Parent is CSharpTemplateBlockSyntax template &&
+                    GetLineNumber(template.GetLastToken()) == GetLineNumber(_currentToken) &&
+                    GetLineNumber(template.GetFirstToken()) != GetLineNumber(template.GetLastToken()) &&
+                    template.GetLastToken().GetNextToken() is { } semiColonToken &&
+                    semiColonToken.Content == ";" &&
+                    GetLineNumber(semiColonToken) == GetLineNumber(_currentToken))
+                {
+                    _builder.AppendLine("};");
+                    return CreateLineInfo();
+                }
+
                 return EmitCurrentLineAsComment();
             }
 
@@ -537,29 +594,45 @@ private LineInfo VisitMarkupLiteral()
 
             public override LineInfo VisitMarkupTagHelperEndTag(MarkupTagHelperEndTagSyntax node)
             {
-                // Since this visitor only sees nodes at the start of a line, an end tag always means de-dent.
-                //return new("}");
-                return EmitCurrentLineAsComment();
+                return VisitEndTag(node);
             }
 
             public override LineInfo VisitMarkupTransition(MarkupTransitionSyntax node)
             {
-                // A transition to Html is treated the same as Html, which is to say nothing interesting.
-                // We could emit as a comment, so C# indentation is handled, but it is often that a markup transition
-                // appears after assigning a RenderFragment, eg
+                // A transition to Html means the start of a RenderFragment. These are challenging because conceptually
+                // they are like a Write() call, because their contents are sent to the output, but they can also contain
+                // statements. eg:
                 //
                 // RenderFragment f =
                 //     @<div>
-                //          <p>Some text</p>
+                //          @if (true)
+                //          {
+                //              <p>Some text</p>
+                //          }
                 //     </div>;
                 //
-                // If we just emit a comment there, the C# formatter will not indent it, and it will leave a hanging
-                // expression which affects future indentation. So instead we emit some fake C# just to make sure
-                // nothing is left open. A single semi-colon will suffice.
+                // If we convert that to C# the way we normally do, we end up with statements in a C# context where only
+                // expressions are valid. To avoid that, we need to emit C# such that we can be sure we're in a context
+                // where statements are valid. To do this we emit a block bodied lambda expression. Ironically this whole
+                // formatting engine arguably exists because the compiler loves to emit lambda expressions, but they're
+                // really annoying to format. This just happens to be the one case where a lambda is the right choice.
+
                 // Emit the whitespace, so user spacing is honoured if possible
                 _builder.Append(_sourceText.ToString(TextSpan.FromBounds(_currentLine.Start, _currentFirstNonWhitespacePosition)));
-                _builder.AppendLine(";");
-                return CreateLineInfo();
+
+                // If its a one-line render fragment, then we don't need to worry.
+                if (GetLineNumber(node.Parent.GetLastToken()) == GetLineNumber(_currentToken))
+                {
+                    _builder.AppendLine("null;");
+                    return CreateLineInfo();
+                }
+
+                // Roslyn may move the opening brace to the next line, depending on its options. Unlike with code block
+                // formatting where we put the opening brace on the next line ourselves (and Roslyn might bring it back)
+                // if we do that for lambdas, Roslyn won't adjust the opening brace position at all. See, told you lambdas
+                // were annoying to format.
+                _builder.AppendLine("() => {");
+                return CreateLineInfo(skipNextLineIfBrace: true);
             }
 
             public override LineInfo VisitRazorCommentBlock(RazorCommentBlockSyntax node)

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

@@ -1,4 +1,5 @@
-// Licensed to the .NET Foundation under one or more agreements.
+
+// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System;
@@ -148,10 +149,11 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
             else if (lineInfo.SkipNextLineIfBrace)
             {
                 // If the next line is a brace, we skip it. This is used to skip the opening brace of a class
-                // that we insert, but Roslyn settings might place on the same like as the class declaration.
-                if (iFormatted + 1 < formattedCSharpText.Lines.Count &&
-                    formattedCSharpText.Lines[iFormatted + 1] is { Span.Length: > 0 } nextLine &&
-                    nextLine.CharAt(0) == '{')
+                // that we insert, but Roslyn settings might place on the same line as the class declaration,
+                // or skip the opening brace of a lambda definition we insert, but Roslyn might place it on the
+                // next line. In that case, we can't place it on the next line ourselves because Roslyn doesn't
+                // adjust the indentation of opening braces of lambdas in that scenario.
+                if (NextLineIsOnlyAnOpenBrace(formattedCSharpText, iFormatted))
                 {
                     iFormatted++;
                 }
@@ -160,10 +162,7 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
                 // it up to the previous line, so we would want to skip the next line in the original document
                 // in that case. Fortunately its illegal to have `@code {\r\n {` in a Razor file, so there can't
                 // be false positives here.
-                if (iOriginal + 1 < changedText.Lines.Count &&
-                    changedText.Lines[iOriginal + 1] is { } nextOriginalLine &&
-                    nextOriginalLine.GetFirstNonWhitespaceOffset() is { } firstChar &&
-                    nextOriginalLine.CharAt(firstChar) == '{')
+                if (NextLineIsOnlyAnOpenBrace(changedText, iOriginal))
                 {
                     iOriginal++;
                 }
@@ -222,6 +221,13 @@ public async Task<ImmutableArray<TextChange>> ExecuteAsync(FormattingContext con
         return SourceTextDiffer.GetMinimalTextChanges(context.SourceText, changedText, DiffKind.Char);
     }
 
+    private static bool NextLineIsOnlyAnOpenBrace(SourceText text, int lineNumber)
+        => lineNumber + 1 < text.Lines.Count &&
+            text.Lines[lineNumber + 1] is { Span.Length: > 0 } nextLine &&
+            nextLine.GetFirstNonWhitespaceOffset() is { } firstNonWhitespace &&
+            nextLine.Start + firstNonWhitespace == nextLine.End - 1 &&
+            nextLine.CharAt(firstNonWhitespace) == '{';
+
     private async Task<SourceText> FormatCSharpAsync(SourceText generatedCSharpText, RazorFormattingOptions options, CancellationToken cancellationToken)
     {
         using var helper = new RoslynWorkspaceHelper(_hostServicesProvider);