feat(rome_analyze): suppress rule via code actions

[ematipico]

Summary

This PR introduces some infrastructure changes to the analyzer. Before the AnalyzerSignal was returning only one action but now is able to return an iterator of actions.

Doing so allows us to create other actions, and in this PR we create actions to suppress lint rules via LSP. This feature will give us some other perks, like the ability to choose the suppression action when will have commands like rome check --review, where the user can choose different actions.

Note: Another perk will be the ability to create different fixes from the rules, although this is not supported now because it would require some more changes to the Rule trait and the RuleDiagnostic struct, this is something that we have access to.

Suppression action

The suppression of the rules opt-out.

Suppression actions are automatically generated for rules that belong to the category RuleCategory::LINT.

A new API has been introduced, called suppress which returns a small wrapper called SuppressAction.

Heuristic of the suppression comment

The heuristic for suppression comments is languages specific, so I had to create a new generic apply_suppression_comment closure that it's passed down to the specific implementation of the analyzer. The function accepts the token offset of where the diagnostic occurred, and a mutation and string that contains the rome-ignore .. comment.

Test Plan

Run the feature locally, here's a small video:

Screen.Recording.2022-11-04.at.17.09.25.mov

The current tests should stay the same because we should not show suppression actions in the CLI.

I will implement tests for automatically generated actions in another PR.

[netlify]

✅ Deploy Preview for docs-rometools canceled.

Name	Link
🔨 Latest commit	91cb65f
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/637c941f9ea34300082b4cf1

[github-actions]

Playground for commit d9fbe6f

[github-actions]

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%

[ematipico]

!bench_analyzer

[github-actions]

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.1±0.01ms     5.7 MB/sec    1.00      2.1±0.01ms     5.7 MB/sec
analyzer/index.js         1.00      5.8±0.01ms     5.6 MB/sec    1.00      5.8±0.02ms     5.6 MB/sec
analyzer/lint.ts          1.00      2.9±0.01ms    14.1 MB/sec    1.00      2.9±0.01ms    14.1 MB/sec
analyzer/parser.ts        1.00      6.8±0.02ms     7.1 MB/sec    1.00      6.9±0.02ms     7.1 MB/sec
analyzer/router.ts        1.00      4.9±0.02ms    12.6 MB/sec    1.00      4.8±0.01ms    12.6 MB/sec
analyzer/statement.ts     1.00      6.7±0.02ms     5.3 MB/sec    1.00      6.8±0.02ms     5.3 MB/sec
analyzer/typescript.ts    1.00     10.2±0.06ms     5.3 MB/sec    1.00     10.3±0.09ms     5.3 MB/sec

[calibre-analytics]

Comparing feat(rome_analyze): suppress rule via code actions Snapshot #9 to median since last deploy of rome.tools.

	LCP?	CLS?	TBT?
Overall Median across all pages and test profiles	2.33s  from 265ms	0.0 no change	147ms no change
Chrome Desktop Chrome Desktop • Cable	2.33s  from 265ms	0.0 no change	291ms  from 27ms
iPhone, 4G LTE iPhone 12 • 4G LTE	1.07s  from 238ms	0.0 no change	3ms no change
Motorola Moto G Power, 3G connection Motorola Moto G Power • Regular 3G	16.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.35 MB  from 86.8 KB
Total JavaScript Size in Bytes iPhone, 4G LTE	5.35 MB  from 86.8 KB
Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection	5.35 MB  from 86.8 KB
JS Parse & Compile Motorola Moto G Power, 3G connection	1.65s  from 29ms
JS Parse & Compile iPhone, 4G LTE	465ms  from 11ms

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

[MichaReiser]

What happens if you add a suppression to a JSX node?

<div>
  <>useless</> <- suppress this
</div>

[ematipico]

What happens if you add a suppression to a JSX node?

<div>
  <>useless</> <- suppress this
</div>

Uuuuuuh! Good question! I didn't think about this case! It will break for sure, because the insertion of the comment is hard coded.

Considering the fact that this new feature is exclusively opt-in, I can remove the feature of use_alt_text for now, and fix it later once I improve the testing suite for these kind of actions.

[leops]

I'm not sure it's safe to visit the ancestors of the node until we find a matching node, specifically I think this logic could fail in a case like this:

<elem
/><elem dangerouslySetInnerHTML={content} />

[ematipico]

The whole function is likely wrong and I plan to change it in the next PR. There are a lot of cases we need to get right and this simple function is not enough. I will push a few changes, but I need to review it in a second pass.

[leops]

Could this function be made generic for every SyntaxList instead of just JsxChildList ? We could then move it to BatchMutation directly

[ematipico]

It will do it in the next PR. The work involved is bigger than I expected

[MichaReiser]

We could avoid calling collect here when we move the chain_pieces iterator to rowan

tools/crates/rome_js_formatter/src/syntax_rewriter.rs

Lines 433 to 528 in f17ace3

	fn chain_pieces<F, S>(first: F, second: S) -> ChainTriviaPiecesIterator<F, S>
	where
	F: Iterator<Item = SyntaxTriviaPiece<JsLanguage>>,
	S: Iterator<Item = SyntaxTriviaPiece<JsLanguage>>,
	{
	ChainTriviaPiecesIterator::new(first, second)
	}
	/// Chain iterator that chains two iterators over syntax trivia together.
	///
	/// This is the same as Rust's [Chain] iterator but implements [ExactSizeIterator].
	/// Rust doesn't implement [ExactSizeIterator] because adding the sizes of both pieces may overflow.
	///
	/// Implementing [ExactSizeIterator] in our case is safe because this may only overflow if
	/// a source document has more than 2^32 trivia which isn't possible because our source documents are limited to 2^32
	/// length.
	struct ChainTriviaPiecesIterator<F, S> {
	first: Option<F>,
	second: S,
	}
	impl<F, S> ChainTriviaPiecesIterator<F, S> {
	fn new(first: F, second: S) -> Self {
	Self {
	first: Some(first),
	second,
	}
	}
	}
	impl<F, S> Iterator for ChainTriviaPiecesIterator<F, S>
	where
	F: Iterator<Item = SyntaxTriviaPiece<JsLanguage>>,
	S: Iterator<Item = SyntaxTriviaPiece<JsLanguage>>,
	{
	type Item = SyntaxTriviaPiece<JsLanguage>;
	fn next(&mut self) -> Option<Self::Item> {
	match &mut self.first {
	Some(first) => match first.next() {
	Some(next) => Some(next),
	None => {
	self.first.take();
	self.second.next()
	}
	},
	None => self.second.next(),
	}
	}
	fn size_hint(&self) -> (usize, Option<usize>) {
	match &self.first {
	Some(first) => {
	let (first_lower, first_upper) = first.size_hint();
	let (second_lower, second_upper) = self.second.size_hint();
	let lower = first_lower.saturating_add(second_lower);
	let upper = match (first_upper, second_upper) {
	(Some(first), Some(second)) => first.checked_add(second),
	_ => None,
	};
	(lower, upper)
	}
	None => self.second.size_hint(),
	}
	}
	}
	impl<F, S> FusedIterator for ChainTriviaPiecesIterator<F, S>
	where
	F: Iterator<Item = SyntaxTriviaPiece<JsLanguage>>,
	S: Iterator<Item = SyntaxTriviaPiece<JsLanguage>>,
	{
	}
	impl<F, S> ExactSizeIterator for ChainTriviaPiecesIterator<F, S>
	where
	F: ExactSizeIterator<Item = SyntaxTriviaPiece<JsLanguage>>,
	S: ExactSizeIterator<Item = SyntaxTriviaPiece<JsLanguage>>,
	{
	fn len(&self) -> usize {
	match &self.first {
	Some(first) => {
	let first_len = first.len();
	let second_len = self.second.len();
	// SAFETY: Should be safe because a program can never contain more than u32 pieces
	// because the text ranges are represented as u32 (and each piece must at least contain a single character).
	first_len + second_len
	}
	None => self.second.len(),
	}
	}
	}

[ematipico]

We had the function all along and I didn't know that?? Now things will get waaaay easier!

[ematipico]

I will move this function to my next PR, there's still some other changes I need to do in the batch APIs

Feedback addressed