Fix formatting error when Roslyn adds an extra blank line - #84751
Fix formatting error when Roslyn adds an extra blank line#84751davidwengier wants to merge 5 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 030dd2e7-b902-42a5-99a6-7e4ebdd67046
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 030dd2e7-b902-42a5-99a6-7e4ebdd67046
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 030dd2e7-b902-42a5-99a6-7e4ebdd67046
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates Razor’s formatting line-mapping logic to correctly preserve cases where the Roslyn formatter expands the number of blank lines (not just inserts a single blank line between non-blank lines), and adds regression coverage (both unit-level and formatting-log-based) for the scenario.
Changes:
- Update
FormattingUtilities.GetOriginalDocumentChangesFromLineInfoto consume multiple inserted blank lines and handle “blank line expansion” cases while keeping original/formatted line indices synchronized. - Add a focused regression test covering incomplete object creation formatting with tabs vs spaces.
- Add a new formatting-log test case + captured inputs/options to ensure the real-world scenario remains covered.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/FormattingUtilities.cs | Extends line-mapping to handle multiple inserted blank lines and blank-line expansion cases. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/Formatting/DocumentFormattingTest.cs | Adds regression tests for the repro (tabs) and a control case (spaces). |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/Cohost/Formatting/FormattingLogTest.cs | Adds a new formatting-log-driven test entrypoint for the captured scenario. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/BoatsSectionRange092858221/InitialDocument.txt | Captured initial document for the formatting log scenario. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/BoatsSectionRange092858221/Options.json | Captured formatting options (notably tabs) for the scenario. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/BoatsSectionRange092858221/Range.json | Captured formatting range for the scenario. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/BoatsSectionRange092858221/HtmlChanges.json | Captured HTML edits that preceded formatting in the scenario. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/BoatsSectionRange092858221/FileKind.json | Captured Razor file kind for the scenario. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 030dd2e7-b902-42a5-99a6-7e4ebdd67046
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 030dd2e7-b902-42a5-99a6-7e4ebdd67046
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/FormattingUtilities.cs:376
- The comment says we should only treat this as a blank-line insertion when the next original line isn't blank, but the current check uses
nextOriginalLine.Span.Length != 0, which treats whitespace-only lines (e.g. indentation-only blank lines) as non-blank. That can cause us to incorrectly interpret an aligned blank line as an insertion and add extra newlines. UseGetFirstNonWhitespaceOffset()(as the new aligned-blank logic below does) to consistently detect non-blank lines.
while (iFormatted + 1 < formattedText.Lines.Count &&
formattedText.Lines[iFormatted + 1].Span.Length == 0 &&
iOriginal + 1 < originalText.Lines.Count &&
originalText.Lines[iOriginal + 1] is { } nextOriginalLine &&
nextOriginalLine.Span.Length != 0)
|
@chsienki needs re-approval after fixing conflicts and adding WorkItem attributes. TYVM |
We supported when blank lines were added, but only between non-blank lines, not when the number of blank lines expanded. Also needed tab indentation to repro this one, so probably why it wasn't found originally.
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/3041882
Microsoft Reviewers: Open in CodeFlow