feat(rome_js_analyze): better JS suppressions #3855

ematipico · 2022-11-25T13:18:11Z

Summary

This PR refines the heuristic of how a suppression comment should be applied inside a mutation action. I tried to comment on the heuristic.

I also took the opportunity to move the chain_pieces from the formatter to the rome_rowan crate, and added some doc tests (which is quite lengthy, but it should be enough I hope).

Test Plan

I updated the test specs, to consider a new folder called suppression, which works the same as the current tests, but they extract only the suppression actions.

I added some test cases, especially inside JSX, because I noticed they are the ones that caused more issues.

Plus, I run locally the suppression actions against specs/ folder, printed the suppression actions, and checked locally that no strange code was emitted. This check was done manually and locally, I wasn't planning to update ALL the diagnostics.

If you think there is some edge case that requires some attention, please paste some snippets and I will try to add them to the test suite.

netlify · 2022-11-25T13:18:16Z

✅ Deploy Preview for docs-rometools canceled.

Name	Link
🔨 Latest commit	`4bb9446`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/6389ec852f0f3b0008fc44b7

github-actions · 2022-11-28T15:59:46Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45879	45879	0
Passed	44936	44936	0
Failed	943	943	0
Panics	0	0	0
Coverage	97.94%	97.94%	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	1757	1757	0
Failed	4189	4189	0
Panics	0	0	0
Coverage	29.55%	29.55%	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	12397	12397	0
Failed	3860	3860	0
Panics	0	0	0
Coverage	76.26%	76.26%	0.00%

calibre-analytics · 2022-11-28T16:01:26Z

Comparing feat(rome_js_analyze): better JS suppressions Snapshot #16 to median since last deploy of rome.tools.

	LCP^?	CLS^?	TBT^?
Overall ^{Median across all pages and test profiles}	2.41s ^from 299ms	0.0 ^no change	147ms ^no change
Chrome Desktop ^{Chrome Desktop • Cable}	2.41s ^from 299ms	0.0 ^no change	386ms ^from 18ms
iPhone, 4G LTE ^{iPhone 12 • 4G LTE}	1.17s ^from 244ms	0.0 ^no change	10ms ^no change
Motorola Moto G Power, 3G connection ^{Motorola Moto G Power • Regular 3G}	19.4s ^from 1.07s	0.0 ^no change	147ms ^no change

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	5.69 MB ^{from 86.8 KB}
Total JavaScript Size in Bytes iPhone, 4G LTE	5.69 MB ^{from 86.8 KB}
Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection	5.69 MB ^{from 86.8 KB}
JS Parse & Compile Motorola Moto G Power, 3G connection	1.67s ^from 28ms
JS Parse & Compile iPhone, 4G LTE	487ms ^from 11ms

_{27 other significant changes: JS Parse & Compile on Chrome Desktop, Total Blocking Time on Chrome Desktop, Largest Contentful Paint on Motorola Moto G Power, 3G connection, First Contentful Paint on Motorola Moto G Power, 3G connection, Time to Interactive 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, Total CSS Size in Bytes on Chrome Desktop, Total CSS Size in Bytes on iPhone, 4G LTE, Total CSS Size in Bytes on Motorola Moto G Power, 3G connection, Time to Interactive on Chrome Desktop, Number of Requests on Motorola Moto G Power, 3G connection, Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, Largest Contentful Paint on Chrome Desktop, First Contentful Paint on Chrome Desktop, Speed Index on Motorola Moto G Power, 3G connection, Time to Interactive on iPhone, 4G LTE, Largest Contentful Paint on iPhone, 4G LTE, First Contentful Paint on iPhone, 4G LTE, Speed Index on Chrome Desktop, Total HTML Size in Bytes on Chrome Desktop, Total HTML Size in Bytes on iPhone, 4G LTE, Total HTML Size in Bytes on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Chrome Desktop}

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

leops

Does this support inserting suppressions in multiline templates, eg.

const foo = `
text
${a == b}
`;

If so can we add a test case for this ?

crates/rome_js_analyze/src/suppression_action.rs

leops · 2022-11-28T16:55:39Z

crates/rome_js_analyze/tests/spec_tests.rs

@@ -91,7 +106,12 @@ fn write_analysis_to_snapshot(
    rome_js_analyze::analyze(FileId::zero(), &root, filter, &options, |event| {
        if let Some(mut diag) = event.diagnostic() {
            for action in event.actions() {
-                if !action.is_suppression() {
+                if check_action_type.is_suppression() {


Nit: this can be collapsed to a single if with the condition !action.is_suppression() || check_action_type.is_suppression()

Doing so will add the suggested actions to the snapshot of the suppressions, which is what I wanted to avoid

Sorry I messed up factorizing the logic expression, I think it's actually check_action_type.is_suppression() == action.is_suppression()

Same result 😅

crates/rome_js_analyze/src/suppression_action.rs

crates/rome_rowan/src/syntax_rewriter.rs

MichaReiser · 2022-11-30T09:03:14Z

crates/rome_rowan/src/syntax_rewriter.rs

+/// use rome_rowan::raw_language::{RawLanguageKind, RawSyntaxTreeBuilder};
+/// use rome_rowan::{chain_pieces, TriviaPiece, TriviaPieceKind};
+/// let mut builder = RawSyntaxTreeBuilder::new();
+///
+/// let mut node = RawSyntaxTreeBuilder::wrap_with_node(RawLanguageKind::ROOT, |builder| {
+///     builder.token_with_trivia(
+///         RawLanguageKind::LET_TOKEN,
+///         "\n\t let \t\t",
+///         &[TriviaPiece::whitespace(3)],
+///         &[TriviaPiece::whitespace(3)],
+///     );
+///     builder.token(RawLanguageKind::STRING_TOKEN, "a");
+///     builder.token_with_trivia(
+///         RawLanguageKind::SEMICOLON_TOKEN,
+///         "; \t\t",
+///         &[TriviaPiece::whitespace(1)],
+///         &[TriviaPiece::whitespace(1)],
+///     );
+/// });
+/// let mut tokens = node.tokens();
+/// let first_token = tokens.next().unwrap();
+/// let second_token = tokens.next().unwrap();
+/// let third_token = tokens.next().unwrap();
+///
+/// let leading_trivia = chain_pieces(
+///     first_token.leading_trivia().pieces(),
+///     third_token.leading_trivia().pieces()
+/// );
+///
+/// let new_first_token = first_token.with_leading_trivia_pieces(leading_trivia);
+///
+/// let new_token = format!("{:?}", new_first_token);
+/// assert_eq!(new_token, "LET_TOKEN@0..10 \"let\" [Whitespace(\"\\n\\t \"), Whitespace(\";\")] [Whitespace(\" \\t\\t\")]")


Using unwrap in doctests is discouraged because readers may end up copying the code including the unwrap calls. That's why it is recommended to wrap the test in a main function and hide these lines by prefixing them with #.

Suggested change

/// use rome_rowan::raw_language::{RawLanguageKind, RawSyntaxTreeBuilder};

/// use rome_rowan::{chain_pieces, TriviaPiece, TriviaPieceKind};

/// let mut builder = RawSyntaxTreeBuilder::new();

///

/// let mut node = RawSyntaxTreeBuilder::wrap_with_node(RawLanguageKind::ROOT, |builder| {

/// builder.token_with_trivia(

/// RawLanguageKind::LET_TOKEN,

/// "\n\t let \t\t",

/// &[TriviaPiece::whitespace(3)],

/// &[TriviaPiece::whitespace(3)],

/// );

/// builder.token(RawLanguageKind::STRING_TOKEN, "a");

/// builder.token_with_trivia(

/// RawLanguageKind::SEMICOLON_TOKEN,

/// "; \t\t",

/// &[TriviaPiece::whitespace(1)],

/// &[TriviaPiece::whitespace(1)],

/// );

/// });

/// let mut tokens = node.tokens();

/// let first_token = tokens.next().unwrap();

/// let second_token = tokens.next().unwrap();

/// let third_token = tokens.next().unwrap();

///

/// let leading_trivia = chain_pieces(

/// first_token.leading_trivia().pieces(),

/// third_token.leading_trivia().pieces()

/// );

///

/// let new_first_token = first_token.with_leading_trivia_pieces(leading_trivia);

///

/// let new_token = format!("{:?}", new_first_token);

/// assert_eq!(new_token, "LET_TOKEN@0..10 \"let\" [Whitespace(\"\\n\\t \"), Whitespace(\";\")] [Whitespace(\" \\t\\t\")]")

/// # use rome_rowan::raw_language::{RawLanguageKind, RawSyntaxTreeBuilder};

/// # use rome_rowan::{chain_pieces, TriviaPiece, TriviaPieceKind, SyntaxResult};

///

/// # fn main() -> SyntaxResult<()> {

/// let mut builder = RawSyntaxTreeBuilder::new();

///

/// let mut node = RawSyntaxTreeBuilder::wrap_with_node(RawLanguageKind::ROOT, |builder| {

/// builder.token_with_trivia(

/// RawLanguageKind::LET_TOKEN,

/// "\n\t let \t\t",

/// &[TriviaPiece::whitespace(3)],

/// &[TriviaPiece::whitespace(3)],

/// );

/// builder.token(RawLanguageKind::STRING_TOKEN, "a");

/// builder.token_with_trivia(

/// RawLanguageKind::SEMICOLON_TOKEN,

/// "; \t\t",

/// &[TriviaPiece::whitespace(1)],

/// &[TriviaPiece::whitespace(1)],

/// );

/// });

/// let mut tokens = node.tokens();

/// let first_token = tokens.next()?;

/// let second_token = tokens.next()?;

/// let third_token = tokens.next()?;

///

/// let leading_trivia = chain_pieces(

/// first_token.leading_trivia().pieces(),

/// third_token.leading_trivia().pieces()

/// );

///

/// let new_first_token = first_token.with_leading_trivia_pieces(leading_trivia);

///

/// let new_token = format!("{:?}", new_first_token);

/// assert_eq!(new_token, "LET_TOKEN@0..10 \"let\" [Whitespace(\"\\n\\t \"), Whitespace(\";\")] [Whitespace(\" \\t\\t\")]")

/// # Ok(())

/// # }

.cargo/config.toml

MichaReiser · 2022-11-30T09:04:12Z

crates/rome_js_analyze/src/utils/batch.rs

        &mut self,
        after_element: &AnyJsxChild,
-        new_element: &AnyJsxChild,
+        new_element: &[AnyJsxChild],


Nit: new_elements

Suggested change

new_element: &[AnyJsxChild],

new_elements: &[AnyJsxChild],

crates/rome_js_analyze/src/utils/batch.rs

crates/rome_js_analyze/src/suppression_action.rs

crates/rome_js_analyze/tests/suppression/correctness/noUndeclaredVariables.ts.snap

crates/rome_js_analyze/tests/suppression/correctness/noArrayIndexKey.jsx.snap

MichaReiser · 2022-11-30T09:45:34Z

crates/rome_js_analyze/src/suppression_action.rs

+                    (TriviaPieceKind::Newline, "\n"),
+                ])
+            };
+            mutation.replace_token_transfer_trivia(first_token_with_newline, new_token);


The replace_token_transfer_trivia implementation looks incorrect. Isn't it applying the trivia of the prev_token (leading) and next_token (trailing) twice

let leading_trivia: Vec<_> = prev_token .leading_trivia() .pieces() .chain(prev_token.leading_trivia().pieces()) .map(|piece| TriviaPiece::new(piece.kind(), TextSize::of(piece.text()))) .collect(); let trailing_trivia: Vec<_> = next_token .trailing_trivia() .pieces() .chain(next_token.trailing_trivia().pieces()) .map(|piece| TriviaPiece::new(piece.kind(), TextSize::of(piece.text()))) .collect();

Nit: replace_token_transfer_trivia can now use chain_trivia_pieces

This is done inside replace_token_transfer_trivia function. I added some other test cases to show that.

MichaReiser · 2022-11-30T09:58:05Z

Requesting changes because:

It seems to me that the mutation adds the suppression comment as JSX Text for one of the test cases
replace_token_transfer_trivia doesn't seem to be correctly implemented. Can you add tests for testing the path where trivia is merged (normal comments, multiline comments, etc)

crates/rome_rowan/src/trivia.rs

crates/rome_js_analyze/src/suppression_action.rs

crates/rome_js_analyze/tests/suppression/a11y/useKeyWithClickEvents.jsx.snap

crates/rome_js_analyze/src/suppression_action.rs

crates/rome_js_analyze/src/utils/batch.rs

ematipico force-pushed the feat/better-suppressions branch 2 times, most recently from 7a7f060 to 73993c4 Compare November 28, 2022 15:53

ematipico marked this pull request as ready for review November 28, 2022 16:00

ematipico requested review from leops, xunilrj, a team and MichaReiser as code owners November 28, 2022 16:00

ematipico force-pushed the feat/better-suppressions branch from 6c95f2b to 12224ec Compare November 28, 2022 16:15

leops reviewed Nov 28, 2022

View reviewed changes

MichaReiser reviewed Nov 28, 2022

View reviewed changes

crates/rome_js_analyze/src/suppression_action.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Nov 28, 2022

View reviewed changes

crates/rome_js_analyze/src/suppression_action.rs Show resolved Hide resolved

MichaReiser reviewed Nov 28, 2022

View reviewed changes

crates/rome_js_analyze/src/suppression_action.rs Outdated Show resolved Hide resolved

crates/rome_js_analyze/src/suppression_action.rs Outdated Show resolved Hide resolved

ematipico force-pushed the feat/better-suppressions branch 4 times, most recently from 8e62bc2 to e251685 Compare November 29, 2022 11:13

MichaReiser suggested changes Nov 30, 2022

View reviewed changes

ematipico added 9 commits November 30, 2022 14:34

feat(rome_js_analyze): better JS suppressions

429e385

chore: clippy and test

0581245

chore: handle javascript templates

33d53dc

chore: handle existing comments

dd186d0

chore: code suggestions

f21615a

chore: rebase

b232c01

chore: clippy

91f00e7

chore: add additional case

61e9422

chore: fixes and code suggestions

82c6399

ematipico force-pushed the feat/better-suppressions branch from 0b46100 to 82c6399 Compare November 30, 2022 14:34

ematipico requested review from MichaReiser and leops November 30, 2022 14:35

chore: nice clippy

0e1dd6f

ematipico added the A-Editors Area: editors label Nov 30, 2022

ematipico modified the milestone: 11.0.0 Nov 30, 2022

MichaReiser reviewed Dec 2, 2022

View reviewed changes

crates/rome_rowan/src/trivia.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Dec 2, 2022

View reviewed changes

crates/rome_rowan/src/trivia.rs Outdated Show resolved Hide resolved

crates/rome_js_analyze/src/suppression_action.rs Show resolved Hide resolved

crates/rome_js_analyze/tests/suppression/a11y/useKeyWithClickEvents.jsx.snap Show resolved Hide resolved

MichaReiser reviewed Dec 2, 2022

View reviewed changes

crates/rome_js_analyze/src/suppression_action.rs Outdated Show resolved Hide resolved

ematipico requested a review from MichaReiser December 2, 2022 10:21

chore: code suggestions

0f03d64

ematipico force-pushed the feat/better-suppressions branch from e01b0fa to 0f03d64 Compare December 2, 2022 10:59

chore: clipy

aad9c1b

MichaReiser reviewed Dec 2, 2022

View reviewed changes

crates/rome_js_analyze/src/utils/batch.rs Outdated Show resolved Hide resolved

MichaReiser approved these changes Dec 2, 2022

View reviewed changes

chore: code suggestion

4bb9446

ematipico merged commit dda034c into main Dec 2, 2022

ematipico deleted the feat/better-suppressions branch December 2, 2022 13:37

feat(rome_js_analyze): better JS suppressions #3855

feat(rome_js_analyze): better JS suppressions #3855

Uh oh!

Conversation

ematipico commented Nov 25, 2022 • edited by MichaReiser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

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

Uh oh!

✅ Deploy Preview for docs-rometools canceled.

Uh oh!

github-actions bot commented Nov 28, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

Uh oh!

calibre-analytics bot commented Nov 28, 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!

leops left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leops Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

ematipico Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leops Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ematipico Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaReiser Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

MichaReiser Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

ematipico Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented Nov 30, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ematipico commented Nov 25, 2022 •

edited by MichaReiser

Loading

netlify bot commented Nov 25, 2022 •

edited

Loading

calibre-analytics bot commented Nov 28, 2022 •

edited

Loading

ematipico Nov 29, 2022 •

edited

Loading

leops Nov 29, 2022 •

edited

Loading

MichaReiser Nov 30, 2022 •

edited

Loading