refactor(rome_diagnostics): refactor the display of diffs to how it worked in Rome JS #3306

leops · 2022-09-30T12:55:14Z

Summary

This PR brings back the rendering of diffs from Rome JS, and should be the last major change required to align the display of diagnostics in the Rust codebase with the previous JS incarnation.

For the rendering itself the code it mostly a direct adaptation of the existing JS logic for Rust. The only major difference is how one-line diffs are rendered (those are diffs that either perform only insertions or only deletions on a single line, this is different from what the code calls "single line diffs" which is when the whole file only spans a single line and we don't need to print line number). With the old logic those diffs would have been printed like this:

    2   │ - ····<>foo</>
      2 │ + ····foo

Now such a diff will be printed on a single line, highlighting only the parts the text that were added or removed, similarly to how Rustc displays these kinds of suggestions:

    2 │ ····<>foo</>
      │     --   ---

Internally, this PR changes how diffs are represented, from a sequence of insert-delete operations (replace a range of the original text with a new string), to a sequence string slices annotated with a "change tag" to know whether they belong to the old revision of the text, the new revision or both versions. Similarly to Rome JS, the Rust version also implements "compressed diffs" where large ranges of text that remained unchanged between the two versions are stored as a simple unchanged line count instead of storing the whole string. Additionally, in order to avoid allocating many short strings in each diff operation, all the string slices that make up a diff are stored in a single "dictionary" string that also de-duplicates identical substrings.

Finally, I also removed a number of unused ways of displaying suggestions in the legacy diagnostics, keeping only the diff display that's actually used (all the other rendering modes can still be implemented using the new diagnostics API)

Test Plan

Besides updating 75 snapshot files, I've added a few test cases for the generation of compressed diffs and their conversions to LSP TextEdit operations

netlify · 2022-09-30T15:45:50Z

✅ Deploy Preview for rometools ready!

Name	Link
🔨 Latest commit	`e4a1c0e`
🔍 Latest deploy log	https://app.netlify.com/sites/rometools/deploys/633aa0b31c801300083d9a46
😎 Deploy Preview	https://deploy-preview-3306--rometools.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

calibre-analytics · 2022-09-30T15:47:56Z

Comparing refactor(rome_diagnostics): refactor the display of diffs to how it worked in Rome JS Snapshot #8 to median since last deploy of rome.tools.

	LCP^?	CLS^?	TBT^?
Overall ^{Median across all pages and test profiles}	582ms ^from 559ms	0.049 ^no change	34ms ^from 41ms
Chrome Desktop ^{Chrome Desktop • Cable}	582ms ^from 592ms	0.049 ^from 0.006	223ms ^from 232ms
iPhone, 4G LTE ^{iPhone 12 • 4G LTE}	204ms ^from 217ms	0.077 ^no change	11ms ^from 9ms
Motorola Moto G Power, 3G connection ^{Motorola Moto G Power • Regular 3G}	611ms ^from 559ms	0.024 ^no change	34ms ^from 41ms

1 page tested

Home

Browser previews

Chrome Desktop	iPhone, 4G LTE	Motorola Moto G Power, 3G connection

Most significant changes

	Value	Budget
Total JavaScript Size in Bytes Chrome Desktop	1.24 MB ^{from 86.8 KB}
Total JavaScript Size in Bytes iPhone, 4G LTE	1.24 MB ^{from 86.8 KB}
Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection	1.24 MB ^{from 86.8 KB}
Cumulative Layout Shift Chrome Desktop	0.049 ^from 0.006
Number of Requests Chrome Desktop	39 ^from 5

_{5 other significant changes: Number of Requests on iPhone, 4G LTE, Number of Requests on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection}

_{Calibre: Site dashboard | View this PR | Edit settings | View documentation}

github-actions · 2022-09-30T15:52:32Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45879	45879	0
Passed	44939	44939	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

symbols/microsoft

Test result	`main` count	This PR count	Difference
Total	5946	5946	0
Passed	1621	1621	0
Failed	4325	4325	0
Panics	0	0	0
Coverage	27.26%	27.26%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	16257	16257	0
Passed	12395	12395	0
Failed	3862	3862	0
Panics	0	0	0
Coverage	76.24%	76.24%	0.00%

github-actions · 2022-09-30T15:53:01Z

Playground for commit 43d02cc

MichaReiser · 2022-09-30T16:55:03Z

crates/rome_cli/tests/snapshots/main_check/downgrade_severity.snap

@@ -36,9 +36,8 @@ file.js:1:1 lint/correctness/noDebugger  FIXABLE  ━━━━━━━━━━

  i Suggested fix: Remove debugger statement

-      | @@ -1 +0,0 @@
-  0   | - debugger;


I like it that the line numbers now start with 1 instead of 0. It's more intuitive and aligns with editors.

Furthermore, the new diffs look more clean... less noisy symbols that I can hardly make sense of.

The line number starting at 0 was definitely a bug, and it might have been one of the reasons the diff gutter could end up being misaligned

MichaReiser · 2022-09-30T16:56:50Z

crates/rome_cli/tests/snapshots/main_check/maximum_diagnostics.snap

-  2 2 |   for(;true;);for(;true;);for(;true;);for(;true;);for(;true;);for(;true;);
-  3 3 |   for(;true;);for(;true;);for(;true;);for(;true;);for(;true;);for(;true;);
-  4 4 |   for(;true;);for(;true;);for(;true;);for(;true;);for(;true;);for(;true;);
+     2    │ - for(;true;);for(;true;);for(;true;);for(;true;);for(;true;);for(;true;);


Is it intentional that we only show "context" lines after the change but not at the start or is the line 1 all empty?

Yes the first line of the file is not rendered in the context lines because it's empty, this is due to the files used in the snapshot being defined as constant string literals in the test code like this:

const ERRORS: &str = r#" for(;true;);for(;true;); ... "#;

Making all those strings start and end with the \n character

crates/rome_diagnostics/src/v2/display/diff.rs

MichaReiser · 2022-09-30T17:05:41Z

crates/rome_js_analyze/tests/specs/correctness/inlineVariable.js.snap

-6 6 |   
-7 7 |   statement(notInlinable);
+```diff
+@@ -1,8 +1,8 @@


What's the reason that we're hiding the line numbers here. Is it because these are just snapshots of the diff and not printed diagnostics?

The snapshots for code actions used to be rendered using the shared diff printing logic in rome_console, but since I moved this code to rome_diagnostics it's now no longer easily accessible outside of diagnostics. Instead I've opted to render code actions as actual Git diffs (using the printing logic built-in to the similar diffing library), and have the editor or GitHub provide the syntax highlighting or rich display for these as part of the markdown rendering.

MichaReiser · 2022-09-30T17:06:57Z

crates/rome_js_analyze/tests/specs/correctness/noCompareNegZero.js.snap

+    32 │ x·===·-0;
+       │       -  


I starred at this for a rather long time to notice that the one - below the - is not some dust on my screen... May it be possible to emphasise the - more in case it's for a single character?

What's the reason that we aren't showing any context lines for this frame?

In contexts that support rich styling such like the console or the HTML output, both the modified characters and the "diff underline" are emphasized and colored appropriately: https://deploy-preview-3306--rometools.netlify.app/docs/lint/rules/nocomparenegzero/#invalid
Ultimately I think single-character diffs are probably hard to read in most diff rendering mode, but my hope is that this way of printing those diffs makes it at least somewhat easier compared to unified diffs by not requiring the user to mentally parse the same line twice the find the difference, and printing an underline specifically to bring attention to what's above it in contexts that do not support styling.
I also intentionally did not print context lines in this mode, again to help bring the focus on the line being modified and not everything around it. In practice I expect that the line of code being modified will almost always be the same one that was highlighted in the diagnostic's location, and as such it doesn't matter too much that the context isn't printed since it should have been rendered already as part of the diagnostic code frame.

ematipico · 2022-10-01T06:47:37Z

I think the reason why single-line diffs were shown on two lines, was mostly because the logic was easily adaptable for "GitHub diffs", meaning the ability to suggest diffs as GitHub comments on PRs.

I am all about clarity and improvements. Let's just take that into consideration and see if this new logic is adaptable for the GitHub comments

leops · 2022-10-03T08:22:39Z

I think the reason why single-line diffs were shown on two lines, was mostly because the logic was easily adaptable for "GitHub diffs", meaning the ability to suggest diffs as GitHub comments on PRs.

In practice, due to the diff gutter and the invisible characters printing it's not possible to directly copy the output of a diagnostic into a diff that GitHub understands without some significant editing anyway. But as I noted above in my comment on code action snapshots, the similar library already contains all the logic required to print Git-compatible diffs, so maybe that's an alternate rendering mode we could expose through a CLI flag for instance.

…orked in Rome JS

leops force-pushed the refactor/code-frame branch from 0a3a552 to b6670f4 Compare September 30, 2022 15:29

Base automatically changed from refactor/code-frame to main September 30, 2022 15:43

leops force-pushed the refactor/diff-display branch from b5080d3 to 52f9d03 Compare September 30, 2022 15:45

leops temporarily deployed to netlify-playground September 30, 2022 15:45 Inactive

leops marked this pull request as ready for review September 30, 2022 15:45

leops requested review from ematipico and xunilrj as code owners September 30, 2022 15:45

leops requested a review from a team September 30, 2022 15:46

leops temporarily deployed to netlify-playground September 30, 2022 16:07 Inactive

MichaReiser approved these changes Sep 30, 2022

View reviewed changes

ematipico added the A-Diagnostic Area: errors and diagnostics label Oct 3, 2022

ematipico modified the milestone: 0.10.0 Oct 3, 2022

ematipico approved these changes Oct 3, 2022

View reviewed changes

leops temporarily deployed to netlify-playground October 3, 2022 08:23 Inactive

leops added 3 commits October 3, 2022 10:43

refactor(rome_diagnostics): refactor the display of diffs to how it w…

e8f6712

…orked in Rome JS

address PR review

3a0013b

update snapshots

e4a1c0e

leops force-pushed the refactor/diff-display branch from f0c9582 to e4a1c0e Compare October 3, 2022 08:43

leops temporarily deployed to netlify-playground October 3, 2022 08:43 Inactive

leops merged commit 35e99b4 into main Oct 3, 2022

leops deleted the refactor/diff-display branch October 3, 2022 08:59

refactor(rome_diagnostics): refactor the display of diffs to how it worked in Rome JS #3306

refactor(rome_diagnostics): refactor the display of diffs to how it worked in Rome JS #3306

Uh oh!

Conversation

leops commented Sep 30, 2022

Summary

Test Plan

Uh oh!

netlify bot commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for rometools ready!

Uh oh!

calibre-analytics bot commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 page tested

Browser previews

Most significant changes

Uh oh!

github-actions bot commented Sep 30, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

Uh oh!

github-actions bot commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ematipico commented Oct 1, 2022

Uh oh!

leops commented Oct 3, 2022

Uh oh!

Uh oh!

netlify bot commented Sep 30, 2022 •

edited

Loading

calibre-analytics bot commented Sep 30, 2022 •

edited

Loading

github-actions bot commented Sep 30, 2022 •

edited

Loading