fix(analyzer): suppression comment fails with inner comments in functions

[fireairforce]

Summary

closes: #4519

This is a little change, but it's hard for me to find and fix it.

Use the case from the issue as examples:

// biome-ignore lint/complexity/useArrowFunction: not work
const foo0 = function (bar: string) {
	// biome-ignore lint/style/noParameterAssign: work
	bar = "baz";
};

There are two suppression comments in the code:

// biome-ignore lint/complexity/useArrowFunction: not work
// biome-ignore lint/style/noParameterAssign: work

In the logic of the https://github.com/biomejs/biome/blob/main/crates/biome_analyze/src/lib.rs#L371 , the method flush_matches will choose the second suppression for the code: function (bar: string), so it will not match the rule lint/complexity/useArrowFunction but match lint/style/noParameterAssign. So it will cause the problem of the issue describes.

I just change the logic of self.suppressions.line_suppressions.binary_search(|suppression| {...}) and change the priority of the suppresion_range.

Test Plan

Sorry, i can't add test case for this change, because this test case need to enable two rules (lint/complexity/useArrowFunction and lint/style/noParameterAssign) for a test file, now the biome rule's test case of biome_js_analyzer is generated by the directory name(only allow enable single rule in a file). But i test my pr at my local file, and it can be passed: i change the code of quick_test and enable two rules for a test case and it can work.

[codspeed-hq]

CodSpeed Performance Report

Merging #4714 will degrade performances by 16.66%

_{Comparing fix-4519 (58d2499) with next (3914be8)}

Summary

❌ 11 regressions
✅ 86 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	next	fix-4519	Change
❌	css_analyzer[bootstrap_18416142857265205439.css]	216.6 ms	244.1 ms	-11.27%
❌	css_analyzer[bulma_5641719244145477318.css]	618.2 ms	690.7 ms	-10.5%
❌	css_analyzer[foundation_11602414662825430680.css]	147.4 ms	166.6 ms	-11.53%
❌	css_analyzer[tachyons_11778168428173736564.css]	120.6 ms	133 ms	-9.34%
❌	js_analyzer[css_16118272471217147034.js]	18.6 ms	19.9 ms	-6.69%
❌	js_analyzer[index_3894593175024091846.js]	45.2 ms	54.2 ms	-16.66%
❌	js_analyzer[lint_13640784270757307929.ts]	35.2 ms	39 ms	-9.69%
❌	js_analyzer[parser_13571644119461115204.ts]	64.7 ms	70.5 ms	-8.2%
❌	js_analyzer[router_17129688031671448157.ts]	18.8 ms	20.2 ms	-7.23%
❌	js_analyzer[statement_263793315104667298.ts]	59.9 ms	65 ms	-7.74%
❌	js_analyzer[typescript_3735799142832611563.ts]	97.5 ms	105.3 ms	-7.45%

[dyc3]

Would it make sense to add a cli test for this particular case? Just so we have something to make sure this doesn't break in the future.

[ematipico]

We already have tests for suppressions where we enable multiple rules. You can add a new one in this file:

biome/crates/biome_js_analyze/src/lib.rs

Lines 719 to 766 in 3914be8

	#[test]
	fn suppression_range_should_report_after_end() {
	const SOURCE: &str = "
	// biome-ignore-start lint/suspicious/noDoubleEquals: single rule
	// biome-ignore-start lint/style/useConst: single rule
	a == b;
	let c;
	// biome-ignore-end lint/suspicious/noDoubleEquals: single rule
	a == b;
	let c;
	";
	let parsed = parse(
	SOURCE,
	JsFileSource::js_module(),
	JsParserOptions::default(),
	);
	let filter = AnalysisFilter {
	categories: RuleCategoriesBuilder::default().with_lint().build(),
	enabled_rules: Some(&[
	RuleFilter::Rule("suspicious", "noDoubleEquals"),
	RuleFilter::Rule("style", "useConst"),
	]),
	..AnalysisFilter::default()
	};
	let options = AnalyzerOptions::default();
	analyze(
	&parsed.tree(),
	filter,
	&options,
	Vec::new(),
	JsFileSource::js_module(),
	None,
	|signal| {
	if let Some(diag) = signal.diagnostic() {
	let code = diag.category().unwrap();
	if code != category!("lint/suspicious/noDoubleEquals") {
	panic!("unexpected diagnostic {code:?}");
	}
	}
	ControlFlow::<Never>::Continue(())
	},
	);
	}

[fireairforce]

@ematipico i run the test case you show to me, we i change this SOURCE from:

const SOURCE: &str = " 
 // biome-ignore-start lint/suspicious/noDoubleEquals: single rule 
 // biome-ignore-start lint/style/useConst: single rule 
 a == b; 
 let c; 
 // biome-ignore-end lint/suspicious/noDoubleEquals: single rule 
 a == b; 
 let c; 
  
         "; 

to:

const SOURCE: &str = " 
 // biome-ignore-start lint/suspicious/noDoubleEquals: single rule 
 a == b; 
 let c; 
 // biome-ignore-end lint/suspicious/noDoubleEquals: single rule 
 a == b; 
 let c; 
  
         "; 

It can also pass the test, is this correct?

[ematipico]

Changing the source code isn't enough, you need to check the assertions too. I don't remember them right now, but if you change source input, you need to make sure that the assertions make sense with the new input.

Looking at the code, it should fail.

[chansuke]

based on the Rust documentation for partition_point, is my understanding correct that the order suppression.text_range.end() < entry.text_range.start() follows Rust conventions for finding the boundary point of non-overlapping ranges?

[fireairforce]

yes, i think this will be the right way here