Ignore formatting changes from Html that change non-whitespace characters - #84747
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eefa5f3-9ccf-4834-908f-b65ec8b5f77b
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eefa5f3-9ccf-4834-908f-b65ec8b5f77b
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eefa5f3-9ccf-4834-908f-b65ec8b5f77b
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eefa5f3-9ccf-4834-908f-b65ec8b5f77b
|
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 hardens Razor HTML formatting by filtering out HTML-formatter edits that change non-whitespace characters (to avoid destructive “formatting” in Razor/script scenarios), and adds regression coverage via both targeted unit tests and captured formatting logs.
Changes:
- Filter out HTML-formatter
TextChanges whose replacement would alter non-whitespace content inHtmlFormattingPass. - Add focused unit tests covering “equivalent non-whitespace” edits and mixed safe/unsafe edits around length changes.
- Add two real-world formatting-log regressions (PageForGrid / PageForMultiGrid) and corresponding test entries.
Reviewed changes
Copilot reviewed 14 out of 14 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/Passes/HtmlFormattingPass.cs | Adds filtering to drop HTML-formatter edits that change non-whitespace content. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/Formatting/HtmlFormattingPassTest.cs | Adds unit tests validating non-whitespace-preserving edits are kept and unsafe edits don’t disrupt surrounding whitespace edits. |
| src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/Formatting/DocumentFormattingTest.cs | Adds an end-to-end regression ensuring non-whitespace corruption in <script> is ignored. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/Cohost/Formatting/FormattingLogTest.cs | Registers new formatting-log-based regressions as tests. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/PageForMultiGrid/InitialDocument.txt | Adds captured initial document for the MultiGrid formatting regression. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/PageForMultiGrid/HtmlChanges.json | Adds captured HTML formatter changes for the MultiGrid regression. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/PageForMultiGrid/Options.json | Adds captured formatting options for the MultiGrid regression. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/PageForMultiGrid/Range.json | Adds captured range info for the MultiGrid regression. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/PageForMultiGrid/FileKind.json | Adds captured file kind for the MultiGrid regression. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/PageForGrid/InitialDocument.txt | Adds captured initial document for the Grid formatting regression. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/PageForGrid/HtmlChanges.json | Adds captured HTML formatter changes for the Grid regression. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/PageForGrid/Options.json | Adds captured formatting options for the Grid regression. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/PageForGrid/Range.json | Adds captured range info for the Grid regression. |
| src/Razor/src/Razor/test/Microsoft.VisualStudio.LanguageServices.Razor.UnitTests/TestFiles/FormattingLog/PageForGrid/FileKind.json | Adds captured file kind for the Grid regression. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/Razor/src/Razor/test/Microsoft.CodeAnalysis.Razor.CohostingShared.UnitTests/Formatting/DocumentFormattingTest.cs:1334
- The expected formatted output has inconsistent indentation on the
const ids = ...line (it appears to have extra leading spaces compared to the surrounding<text>block and theinput/htmlFormattedstrings). This is likely to make the regression test fail for the wrong reason.
const ids = grid.getIds();
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4eefa5f3-9ccf-4834-908f-b65ec8b5f77b
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/HtmlFormattingPass.cs:214
FilterOutNonWhitespaceChangesallocateschangedTextviaoriginalText.WithChanges(candidateChanges)even thoughformattedText(computed earlier in this method) should already represent the result of applyingcandidateChanges. ReusingformattedTextavoids an extra full-text rewrite/allocation in this warning-path.
var changedText = originalText.WithChanges(candidateChanges);
src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/Passes/HtmlFormattingPass.cs:245
- The warning message is very generic and can be noisy in logs without giving enough diagnostic value. Including how many changes were dropped (and total) would make the warning actionable when this happens in the wild.
_logger.LogWarning("Ignoring non-whitespace changes returned by the HTML formatter.");
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/3042344 / https://devdiv.visualstudio.com/DevDiv/_workitems/edit/3040290
This is an odd one, where the JavaScript formatter is removing legitimate code because its confused by Razor concepts. Not a lot we can do about it in terms of nice formatting, but we can not error out and crash at least. So this does that.
Microsoft Reviewers: Open in CodeFlow