Better support for mixed formatting #2526

ajaybhargavb · 2020-09-19T03:38:59Z

Part of https://github.com/dotnet/aspnetcore/issues/14271
Part of https://github.com/dotnet/aspnetcore/issues/23530
Part of https://github.com/dotnet/aspnetcore/issues/25475
Fixes https://github.com/dotnet/aspnetcore/issues/25470

Added support for complex mixed formatting scenarios
Added a (reflection)way to get csharp indentation for a specific line of the projected C# document
- Will move from reflection to external access in the future
Updated FormattingContext to contain the C# workspace for ease of use across the formatting pipeline
Made some other miscellaneous bug fixes
Updated tests

ajaybhargavb · 2020-09-22T10:43:50Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

+
+            // Add a marker at the position where we need the indentation.
+            var changedText = context.CSharpSourceText;
+            var marker = $"{context.NewLineString}#line default{context.NewLineString}#line hidden{context.NewLineString}";


I tried a bunch of different things to use as markers. But this is the only one I found to work in all cases.

Could you go a little bit into why the other ones didn't work? Do they just appear in other scenarios? How did you determine that this whole string wouldn't appear otherwise?

That's a hell of a marker lol

Could you go a little bit into why the other ones didn't work?

Not entirely sure. Roslyn's IIndentationService doesn't like it when I provide markers like /**/ or empty lines etc. The API returns wacky results that don't make any sense. I plan to follow up with them to understand this further.

ajaybhargavb · 2020-09-22T10:46:40Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingContext.cs

@@ -110,14 +162,14 @@ public async Task<FormattingContext> WithTextAsync(SourceText changedText)

            var codeDocument = engine.ProcessDesignTime(changedSourceDocument, OriginalSnapshot.FileKind, importSources, OriginalSnapshot.Project.TagHelpers);

-            var newContext = Create(Uri, OriginalSnapshot, codeDocument, Options, Range);
+            var newContext = Create(Uri, OriginalSnapshot, codeDocument, Options, Range, IsFormatOnType);


Was a mistake from before.

ajaybhargavb · 2020-09-22T10:47:39Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingVisitor.cs

@@ -48,7 +48,10 @@ public override void VisitRazorCommentBlock(RazorCommentBlockSyntax node)
                    // We need to generate a formatting span at this position. So insert a marker in its place.
                    comment = (SyntaxToken)SyntaxFactory.Token(SyntaxKind.Marker, string.Empty).Green.CreateRed(razorCommentSyntax, razorCommentSyntax.StartCommentStar.EndPosition);
                }
+
+                _currentIndentationLevel++;


This makes it so that,

@* some comment *@

becomes.

@* some comment *@

ryanbrandenburg

Nothing structural, just had a couple questions.

ryanbrandenburg · 2020-09-22T16:26:50Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer.Common/RazorCodeDocumentExtensions.cs

@@ -11,6 +11,8 @@ internal static class RazorCodeDocumentExtensions
    {
        private static readonly object UnsupportedKey = new object();
        private static readonly object SourceTextKey = new object();
+        private static readonly object CSharpSourceTextKey = new object();


Weird that this should be a "key" when it's just an empty object. Makes sense, it's just not what I would have expected.

Yup. This is what we use for storing arbitrary things in the CodeDocument. The other option is to use a string but using an object makes it so that no one else can retrieve it directly without using the methods defined here.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

ryanbrandenburg · 2020-09-22T16:35:25Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

-            Range range,
-            DocumentUri uri,
-            FormattingOptions options,
+            FormattingContext context,


👏 love me some context.

ryanbrandenburg · 2020-09-22T16:38:18Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

+
+            // Add a marker at the position where we need the indentation.
+            var changedText = context.CSharpSourceText;
+            var marker = $"{context.NewLineString}#line default{context.NewLineString}#line hidden{context.NewLineString}";


Could you go a little bit into why the other ones didn't work? Do they just appear in other scenarios? How did you determine that this whole string wouldn't appear otherwise?

ryanbrandenburg · 2020-09-22T16:39:15Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

+            var csharpSourceText = context.CodeDocument.GetCSharpSourceText();
+            var spanToFormat = projectedRange.AsTextSpan(csharpSourceText);
+            var root = await context.CSharpWorkspaceDocument.GetSyntaxRootAsync(cancellationToken);
+            var workspace = context.CSharpWorkspace;


Much cleaner now, good refactor.

ryanbrandenburg · 2020-09-22T16:40:51Z

...or/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/DefaultRazorFormattingService.cs

@@ -58,7 +58,7 @@ public override async Task<TextEdit[]> FormatAsync(DocumentUri uri, DocumentSnap
            }

            var codeDocument = await documentSnapshot.GetGeneratedOutputAsync();
-            var context = FormattingContext.Create(uri, documentSnapshot, codeDocument, options, range);
+            using var context = FormattingContext.Create(uri, documentSnapshot, codeDocument, options, range);


src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

ajaybhargavb · 2020-09-22T10:51:12Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormattingPass.cs

                csharpEdits.AddRange(edits.Where(e => range.Contains(e.Range)));
            }

            return csharpEdits;
        }

+        private List<TextChange> AdjustIndentation(FormattingContext context, CancellationToken cancellationToken, Range range = null)


This would eventually move into FormattingPassBase once I get onTypeFormatting to support mixed cases as well.

Killer job commenting all of this. It reads really easy

TanayParikh

Is there a timeline/issue for moving from reflection to external access? Concerned with perf/maintainability/regressions from non-strongly typed dependencies from this reflection.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingContext.cs

NTaylorMullen

Looks incredible! Now all that being said It would be nice to incorporate possibly a few of WTE's edge-case test cases here to ensure that things work as expected for them.

Really impressed, super clear, joy to review. 👏

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

NTaylorMullen · 2020-09-22T22:53:28Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

+
+            // Add a marker at the position where we need the indentation.
+            var changedText = context.CSharpSourceText;
+            var marker = $"{context.NewLineString}#line default{context.NewLineString}#line hidden{context.NewLineString}";


That's a hell of a marker lol

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormattingPass.cs

NTaylorMullen · 2020-09-22T23:07:24Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormattingPass.cs

                csharpEdits.AddRange(edits.Where(e => range.Contains(e.Range)));
            }

            return csharpEdits;
        }

+        private List<TextChange> AdjustIndentation(FormattingContext context, CancellationToken cancellationToken, Range range = null)


Killer job commenting all of this. It reads really easy

ajaybhargavb · 2020-09-24T00:21:53Z

🆙 📅

Is there a timeline/issue for moving from reflection to external access?

~~Will file an issue and update here.~~ https://github.com/dotnet/aspnetcore/issues/26259

ajaybhargavb · 2020-09-24T00:25:05Z

It would be nice to incorporate possibly a few of WTE's edge-case test cases

I did incorporate some of them in my tests. That is how I found the @* *@ indentation problem. Will look into incorporating more of those.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs

ghost · 2020-09-24T03:53:11Z

Hello @ajaybhargavb!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Better support for mixed formatting

b296c4f

ajaybhargavb force-pushed the ajbaaska/razor-csharp-formatting branch from 8521542 to b296c4f Compare September 22, 2020 10:29

ajaybhargavb marked this pull request as ready for review September 22, 2020 10:42

ajaybhargavb requested review from NTaylorMullen, ryanbrandenburg and TanayParikh as code owners September 22, 2020 10:42

ajaybhargavb commented Sep 22, 2020

View reviewed changes

ryanbrandenburg approved these changes Sep 22, 2020

View reviewed changes

ajaybhargavb commented Sep 22, 2020

View reviewed changes

TanayParikh approved these changes Sep 22, 2020

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingContext.cs Show resolved Hide resolved

NTaylorMullen approved these changes Sep 22, 2020

View reviewed changes

Feedback and bug fixes

a61047f

NTaylorMullen approved these changes Sep 24, 2020

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormatter.cs Outdated Show resolved Hide resolved

Feedback and fix tests

e059ff1

ajaybhargavb force-pushed the ajbaaska/razor-csharp-formatting branch from 8a8c89b to e059ff1 Compare September 24, 2020 03:46

ajaybhargavb added the auto-merge Squash merge once all PR checks are complete and reviewers have approved label Sep 24, 2020

ajaybhargavb added 2 commits September 23, 2020 22:12

fix

b788d62

fix

0220d47

ajaybhargavb mentioned this pull request Sep 24, 2020

Add cross-plat tests to Razor.slim.sln #2548

Closed

ghost merged commit b4d8ca7 into master Sep 24, 2020

ghost deleted the ajbaaska/razor-csharp-formatting branch September 24, 2020 06:48

This was referenced Sep 24, 2020

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

ajaybhargavb mentioned this pull request Oct 9, 2021

Move Roslyn's IIndentationService to ExternalAccess.Razor #5144

Closed

NTaylorMullen mentioned this pull request Oct 9, 2021

On-by-default Razor formatting #5326

Closed

12 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better support for mixed formatting #2526

Better support for mixed formatting #2526

ajaybhargavb commented Sep 19, 2020 •

edited

Loading

ajaybhargavb Sep 22, 2020

ryanbrandenburg Sep 22, 2020

NTaylorMullen Sep 22, 2020

ajaybhargavb Sep 23, 2020

ajaybhargavb Sep 22, 2020

ajaybhargavb Sep 22, 2020

ryanbrandenburg left a comment

ryanbrandenburg Sep 22, 2020

ajaybhargavb Sep 23, 2020

ryanbrandenburg Sep 22, 2020

ryanbrandenburg Sep 22, 2020

ryanbrandenburg Sep 22, 2020

ryanbrandenburg Sep 22, 2020

ajaybhargavb Sep 22, 2020

NTaylorMullen Sep 22, 2020

TanayParikh left a comment

NTaylorMullen left a comment

NTaylorMullen Sep 22, 2020

NTaylorMullen Sep 22, 2020

ajaybhargavb commented Sep 24, 2020 •

edited

Loading

ajaybhargavb commented Sep 24, 2020

ghost commented Sep 24, 2020

Better support for mixed formatting #2526

Better support for mixed formatting #2526

Conversation

ajaybhargavb commented Sep 19, 2020 • 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

ryanbrandenburg 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

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

TanayParikh left a comment

Choose a reason for hiding this comment

NTaylorMullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajaybhargavb commented Sep 24, 2020 • edited Loading

ajaybhargavb commented Sep 24, 2020

ghost commented Sep 24, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ajaybhargavb commented Sep 19, 2020 •

edited

Loading

ajaybhargavb commented Sep 24, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.