Formatting engine changes to allow it to (mostly) run on runtime code-gen #11303
Conversation
Fixes IfBlock_TopLevel
Fixes IndentsCodeBlockDirectiveStart
Fixes IfBlock_TopLevel_WithOtherCode. Previous code worked by luck, when offsetting the position of the annotation, later, design time code-gen happens to have enough pragmas to make the calculation work.
There was a problem hiding this comment.
Copilot reviewed 5 out of 9 changed files in this pull request and generated no comments.
Files not reviewed (4)
- src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Extensions/RazorSyntaxNodeExtensions.cs: Evaluated as low risk
- src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/CSharpFormatter.cs: Evaluated as low risk
- src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/FormattingVisitor.cs: Evaluated as low risk
- src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/CSharpFormattingPass.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/RazorFormattingPass.cs:24
- The constructor syntax is incorrect. It should be
internal sealed class RazorFormattingPass : IFormattingPass { private readonly ILogger _logger; public RazorFormattingPass(ILoggerFactory loggerFactory) { _logger = loggerFactory.CreateLogger<RazorFormattingPass>(); } }
internal sealed class RazorFormattingPass(ILoggerFactory loggerFactory) : IFormattingPass
| var token = root.FindToken(projectedDocumentIndex, findInsideTrivia: true); | ||
|
|
||
| // We use a marker if the projected location is in trivia, because we can't add annotations to a specific piece of trivia | ||
| var isInTrivia = projectedDocumentIndex < token.SpanStart || projectedDocumentIndex >= token.Span.End; |
There was a problem hiding this comment.
Do we need to check if the trivia is something other than whitespace?
There was a problem hiding this comment.
Not sure I fully understand your question. We want to do this if the token is in trivia, or whitespace, and as far as I'm aware in Roslyn whitespace can only ever be trivia, or part thereof, so this should cover all of the bases.
There was a problem hiding this comment.
I think I might have just misunderstood what was going on. At first, I thought this was specifically looking for whitespace, not all trivia
| changes.Add(new TextChange(new TextSpan(sourceText.Lines[i].Start, 0), indentationString)); | ||
| } | ||
| } | ||
|
|
||
| var children = code.Children; |
There was a problem hiding this comment.
nit: if we're going to keep a local it should be moved up and used on line 102
There was a problem hiding this comment.
Thanks for this suggestion, it turns out I can go one better, and just capture children in the pattern instead of code. :)
There was a problem hiding this comment.
This is why you get paid the not publicly available number of bucks!
|
Don't worry, eventually it calcifies and the pain stops :) |
| @@ -93,13 +93,13 @@ private static bool TryFormatSectionBlock(FormattingContext context, ref PooledA | |||
| if (node is CSharpCodeBlockSyntax directiveCode && | |||
| directiveCode.Children is [RazorDirectiveSyntax directive, ..] && | |||
| directive.DirectiveDescriptor?.Directive == SectionDirective.Directive.Directive && | |||
| directive.Body is RazorDirectiveBodySyntax { CSharpCode: { } code }) | |||
| directive.Body is RazorDirectiveBodySyntax { CSharpCode: { Children: var children } }) | |||
There was a problem hiding this comment.
I can't comment on the commit message but I will say it made me chuckle
Part of #10402
This PR makes various updates to our formatting engine to allow it to run on runtime code-gen. After this PR there are still 5 formatting tests that fail, representing two different bugs (the one in the issue, and a cascading value parameter issue), but those are proving very stubborn, so thought I'd put this up first. Plus it's Friday afternoon :)
Comments in the code hopefully explain what is going on, but honestly, it doesn't matter if anyone understands the formatting engine, as long as the tests pass :D
Each commit is a self-contained change for those who are extra curious about which bit fixes which failure.