Moving formatting service to common layer #10761

alexgav · 2024-08-19T05:33:03Z

Summary of the changes

Moving RazorFormattingService to common layer. It's immediately needed for OnAutoInsert support, but will eventually be needed by the formatting endpoints themselves.

Fixes:
Part of #9519

alexgav · 2024-08-19T18:06:28Z

~~Switching back to draft while I investigate test failures~~ fixed, ready for review

…n layer

It requires services not easily available and will require extra work. For now leaving it in LSP server layer. Current need for the formatting service in cohosting is for OnAutoInsert which doesn't need HTML formatting.

DustinCampbell

Other than some minor clean up feedback (that you can take or leave), looks good to me!

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/RazorFormattingPassBase.cs

DustinCampbell · 2024-08-20T17:00:05Z

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/RazorFormattingPassBase.cs

@@ -126,7 +128,7 @@ directiveCode.Children is [RazorDirectiveSyntax directive] &&

        return false;

-        static bool TryGetWhitespace(SyntaxList<RazorSyntaxNode> children, [NotNullWhen(true)] out CSharpStatementLiteralSyntax? whitespaceBeforeSectionName, [NotNullWhen(true)] out UnclassifiedTextLiteralSyntax? whitespaceAfterSectionName)
+        static bool TryGetWhitespace(AspNetCore.Razor.Language.Syntax.SyntaxList<RazorSyntaxNode> children, [NotNullWhen(true)] out CSharpStatementLiteralSyntax? whitespaceBeforeSectionName, [NotNullWhen(true)] out UnclassifiedTextLiteralSyntax? whitespaceAfterSectionName)


Consider adding a using type alias at the type of the file for SyntaxList:

using RazorSyntaxNodeList = Microsoft.AspNetCore.Razor.Language.Syntax.SyntaxList<Microsoft.AspNetCore.Razor.Language.Syntax.RazorSyntaxNode>;

@davidwengier: It feels like some global usings for common type aliases like these might be useful. What do you think?

@davidwengier: It feels like some global usings for common type aliases like these might be useful. What do you think?

I personally would love that for consistency, totally open to suggestions here.

Also since there is a list of SyntaxNode and a list of RazorSyntaxNode, I ended up with RazorSyntaxNodeList and RazorRazorSyntaxNodeList :) The latter sound silly though follows the pattern. Suggestions are welcome.

I honestly do not understand why Razor has both SyntaxNode and RazorSyntaxNode. It's very unclear and inconsistent. RazorSyntaxNode inherits directly from SyntaxNode and adds no extra implementation. Most SyntaxNode subtypes in the Razor compiler actually inherit from RazorSyntaxNode. Heck, even CSharpSyntaxNode inherits from RazorSyntaxNode. There are only a few types that directly inherit from SyntaxNode, and I wonder if those are mistakes? For example, SyntaxToken inherits from RazorSyntaxNode but SyntaxTrivia inherits from SyntaxNode? It's all wacky. 😄

Maybe @333fred, @chsienki, or @jjonescz understand some nuance between SyntaxNode and RazorSyntaxNode that I don't see?

I certainly don't know of any nuances between these. While I suspect there may have once been an attempt to separate razor from C#/VB, as you mentioned it doesn't exist today. The whole syntax implementation is a bit wacky, given the whole "copied from pre-CTP 1 Roslyn" nature of it.

Yeah, I just chatted with @chsienki and he didn't know of any reason. It doesn't look to me like anything actually creates SyntaxTrivia instances. That one could probably just be removed.

We're thinking about using SyntaxTrivia if the request for razor preprocessor directives ever goes forward.

@davidwengier: It feels like some global usings for common type aliases like these might be useful. What do you think?

No complaints. These type names have always been annoying. Sometimes SyntaxNode is a razor node, sometimes its a roslyn node 🤷‍♂️

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Common/AdhocLanguageServices.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Common/AdhocServices.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Common/AdhocWorkspaceServices.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/FormattingUtilities.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/FormattingVisitor.cs

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

* upstream/main: (71 commits) Fix after merge PR feedback Bump Roslyn version Moving formatting service to common layer (dotnet#10761) Move GetSyntaxTree to document snapshot Inject file path service into the document snapshot Remove code document parameter and just use document snapshot Update NOTICE.txt (dotnet#10768) Allow @@ as a fallback (dotnet#10752) Rework how we get generated documents Directly test the component definition service in cohosting Add missing test case Defer to C# for component attribute GTD in cohosting Allow LSP and cohosting to provide specialized methods to get a syntax tree for a C# document Dev Container (dotnet#10751) Use a proper Try pattern Add tests for co-hosted GTD Rework IDocumentPositionInfoStrategy and use correctly in co-hosted GTD Add DocumentMappingSerice to RazorDocumentServiceBase Move IDocumentPositionInfoStrategy and friends to Workspaces layer ...

davidwengier · 2024-08-27T03:08:10Z

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/RazorFormattingPassBase.cs

@@ -72,7 +77,7 @@ private IEnumerable<TextEdit> FormatRazor(FormattingContext context, RazorSyntax
        {
            // Disclaimer: CSharpCodeBlockSyntax is used a _lot_ in razor so these methods are probably
            // being overly careful to only try to format syntax forms they care about.
-            TryFormatCSharpBlockStructure(context, edits, source, node);
+            TryFormatCSharpBlockStructure(context, edits, source, node); // TODO


What is this TODO for?

My bad, that should've been deleted. It was added when I was passing in hardcoded value for "bracesOnNextLine" options. I ended up not passing it in the parameter list for this method, but forgot to remove TODO. Happy to remove it unless you are editing that file as a part of PR, in which case I'd appreciate if you removed it.

alexgav requested a review from a team as a code owner August 19, 2024 05:33

alexgav marked this pull request as draft August 19, 2024 05:33

alexgav marked this pull request as ready for review August 19, 2024 17:30

alexgav marked this pull request as draft August 19, 2024 18:06

alexgav marked this pull request as ready for review August 20, 2024 04:21

alexgav added 8 commits August 20, 2024 06:58

Moving RazorFormattingService to common layer

8905962

Cleanup

312c1e6

Switch Razor LSP server code to use RazorFormattingService from commo…

ad40d28

…n layer

Removing HTML formatting from common layer (for now)

cdc5d96

It requires services not easily available and will require extra work. For now leaving it in LSP server layer. Current need for the formatting service in cohosting is for OnAutoInsert which doesn't need HTML formatting.

Move AdhocServices, AdhoWorkspaceServices to common layer

d7eae09

Cleanup moved resources

0b07435

Basic implementations for remote formatting classes

ba00d25

Removing hardcoded option value

0291c12

alexgav force-pushed the dev/alexgav/MoveFormattingServiceToCommonLayer branch from 18d9e8e to 0291c12 Compare August 20, 2024 14:05

DustinCampbell approved these changes Aug 20, 2024

View reviewed changes

Cleanup per CR suggestions

000cd6a

davidwengier approved these changes Aug 20, 2024

View reviewed changes

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

Cleanup doc-comments

e73ada8

alexgav enabled auto-merge (squash) August 20, 2024 22:37

alexgav merged commit d38698f into main Aug 21, 2024
12 checks passed

alexgav deleted the dev/alexgav/MoveFormattingServiceToCommonLayer branch August 21, 2024 01:49

dotnet-policy-service bot added this to the Next milestone Aug 21, 2024

dibarbet modified the milestones: Next, 17.12 P2 Aug 26, 2024

davidwengier reviewed Aug 27, 2024

View reviewed changes

This was referenced Aug 29, 2024

[Automated] PRs inserted in VS build main-35228.257 #10810

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35228.301 #10812

Closed

dotnet-bot mentioned this pull request Sep 14, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35313.170 #10886

Closed

ryzngard mentioned this pull request Oct 1, 2024

Merge main into features/extract-to-component #10948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving formatting service to common layer #10761

Moving formatting service to common layer #10761

alexgav commented Aug 19, 2024

alexgav commented Aug 19, 2024 •

edited

Loading

DustinCampbell left a comment

DustinCampbell Aug 20, 2024

DustinCampbell Aug 20, 2024

alexgav Aug 20, 2024

alexgav Aug 20, 2024

DustinCampbell Aug 20, 2024

DustinCampbell Aug 20, 2024

333fred Aug 20, 2024

DustinCampbell Aug 20, 2024

333fred Aug 20, 2024

davidwengier Aug 20, 2024

davidwengier Aug 27, 2024

alexgav Aug 27, 2024

davidwengier Aug 28, 2024

Moving formatting service to common layer #10761

Moving formatting service to common layer #10761

Conversation

alexgav commented Aug 19, 2024

Summary of the changes

alexgav commented Aug 19, 2024 • edited Loading

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

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

alexgav commented Aug 19, 2024 •

edited

Loading