feat(linter): support --report-unused-disable-directive

[1zumii]

and --report-unused-disable-directives-severity

closed #7544

[graphite-app]

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• 0-merge - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

[codspeed-hq]

CodSpeed Performance Report

Merging #9223 will not alter performance

_{Comparing 1zumii:feat/7544 (fb9735d) with main (9c3549d)}

Summary

✅ 33 untouched benchmarks

[Boshen]

There's a test failure.

[camchenry]

Thanks for the contribution! This looks good overall, I have a few suggestions on some ways we can improve the performance a little bit. My main suggestion is for some improvements to the testing, such as adding an integration test with an example file so we can easily see what the output of this is.

[camchenry]

Suggested change

	/// Report directive comments like // eslint-disable-line when no errors would have been reported on that line anyway.
	/// Report directive comments like `// eslint-disable-line` when no errors would have been reported on that line anyway.

[camchenry]

instead of allocating an empty array that we have to keep growing (wasting memory allocations and CPU time), can we allocate with a preset capacity? I think we should get the number of unused enable/disable comments, and then use that as the capacity:

Suggested change

	let mut unused_directive_diagnostics: Vec<(String, Span)> = vec![];
	let unused_enable_comments = self.disable_directives.unused_enable_comments()
	let unused_disable_comments = self.disable_directives.collect_unused_disable_comments();
	let mut unused_directive_diagnostics: Vec<(String, Span)> = Vec::with_capacity(unused_enable_comments.len() + unused_disable_comments.len());

[camchenry]

To make fewer memory allocations for Strings, let's try and make this a vec of Cow instead. When rule_name is None, we should use a Cow::Borrowed that points to a &'static str const with the Unused eslint-disable directive (no problems were reported). text.

When rule_name is Some, it should be a Cow::Owned with a String inside of it with the formatted text string. This way, we only allocate space for more strings when we need to, rather than allocating space for the same string over and over again.

Suggested change

	let mut unused_directive_diagnostics: Vec<(String, Span)> = vec![];
	let mut unused_directive_diagnostics: Vec<(Cow<str>, Span)> = vec![];

[camchenry]

let's keep this separate from the linter def, as this part is going to get more complicated with nested configurations coming soon.

Suggested change

	let linter = Linter::new(LintOptions::default(), lint_config)
	.with_fix(fix_options.fix_kind())
	.with_report_unused_directives(match inline_config_options.report_unused_directives {
	ReportUnusedDirectives::WithoutSeverity(true) => Some(AllowWarnDeny::Warn),
	ReportUnusedDirectives::WithSeverity(Some(severity)) => Some(severity),
	_ => None,
	});
	let report_unused_directives = match inline_config_options.report_unused_directives {
	ReportUnusedDirectives::WithoutSeverity(true) => Some(AllowWarnDeny::Warn),
	ReportUnusedDirectives::WithSeverity(Some(severity)) => Some(severity),
	_ => None,
	};
	let linter = Linter::new(LintOptions::default(), lint_config)
	.with_fix(fix_options.fix_kind())
	.with_report_unused_directives(report_unused_directives);

[camchenry]

we should add a check for the length as well:

Suggested change

	assert_eq!(
	unused.first().map(|(_, span)| *span),
	comments.first().map(Comment::content_span)
	);
	assert_eq!(1, unused.len());
	assert_eq!(
	unused.first().map(|(_, span)| *span),
	comments.first().map(Comment::content_span)
	);

[camchenry]

this goes for all of the other tests in this file where there is a rule name, we should check the rule name too, not just the comment span:

Suggested change

	assert_eq!(unused.len(), 2);
	assert_eq!(
	unused.first().map(|(_, span)| *span),
	comments.first().map(Comment::content_span)
	);
	assert_eq!(
	unused.last().map(|(_, span)| *span),
	comments.first().map(Comment::content_span)
	);
	assert_eq!(unused.len(), 2);
	let comment_span = comments.first().unwrap().content_span();
	let disable_debugger = unused.first().unwrap();
	let disable_console = unused.last().unwrap();
	assert_eq!(disable_debugger.span, comment_span);
	assert_eq!(disable_debugger.rule_name, "no-debugger");
	assert_eq!(disable_console.span, comment_span);
	assert_eq!(disable_console.rule_name, "no-console"

[camchenry]

There should be an end-to-end test at the bottom of this file which showcases the report unused directives feature. Add a fixture in apps/oxlint/fixtures/report_unused_directives and add a file which has a number of different kinds of lint disables, then reference that file in a test which adds a snapshot.

[camchenry]

Can you modify these tests to also use oxlint-enable-* and oxlint-disable-* as well, to ensure we have coverage there too?