diff --git a/.changeset/new_top_level_suppression_for_the_analyzer.md b/.changeset/new_top_level_suppression_for_the_analyzer.md new file mode 100644 index 000000000000..df754eec5f59 --- /dev/null +++ b/.changeset/new_top_level_suppression_for_the_analyzer.md @@ -0,0 +1,23 @@ +--- +cli: minor +--- + +# New top-level suppression for the analyzer + +The Biome analyzer now supports a new top-level suppression. These suppression have to be placed at the top of the file, and they **must** be multiple line comments. + +The analyzer rules specified inside the block comment will be suppressed for the whole file. + +In the example, we suppress the rules `lint/style/useConst` and `lint/suspicious/noDebugger` for the whole file: + +```js +// main.js +/** + * biome-ignore lint/style/useConst: i like let + * biome-ignore lint/suspicious/noDebugger: needed now + */ + +let path = "/path"; +let _tmp = undefined; +debugger +``` diff --git a/.changeset/remove_support_for_legacy_suppressions.md b/.changeset/remove_support_for_legacy_suppressions.md new file mode 100644 index 000000000000..4d84d68a8228 --- /dev/null +++ b/.changeset/remove_support_for_legacy_suppressions.md @@ -0,0 +1,13 @@ +--- +cli: major +--- + +# Remove support for legacy suppressions + +Biome used to support "legacy suppressions" that looked like this: + +```js +// biome-ignore lint(style/useWhile): reason +``` + +This format is no longer supported. diff --git a/Cargo.lock b/Cargo.lock index 85bac7a2f284..7289ff55f0f4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -878,6 +878,7 @@ dependencies = [ "biome_json_parser", "biome_json_syntax", "biome_rowan", + "biome_suppression", "biome_test_utils", "insta", "natord", diff --git a/crates/biome_analyze/src/diagnostics.rs b/crates/biome_analyze/src/diagnostics.rs index 44cf2631c876..71929bf4234a 100644 --- a/crates/biome_analyze/src/diagnostics.rs +++ b/crates/biome_analyze/src/diagnostics.rs @@ -1,7 +1,7 @@ -use biome_console::MarkupBuf; +use biome_console::{markup, MarkupBuf}; use biome_diagnostics::{ advice::CodeSuggestionAdvice, category, Advices, Category, Diagnostic, DiagnosticExt, - DiagnosticTags, Error, Location, Severity, Visit, + DiagnosticTags, Error, Location, LogCategory, Severity, Visit, }; use biome_rowan::TextRange; use std::borrow::Cow; @@ -141,7 +141,7 @@ impl AnalyzerDiagnostic { #[derive(Debug, Diagnostic, Clone)] #[diagnostic(severity = Warning)] -pub struct SuppressionDiagnostic { +pub struct AnalyzerSuppressionDiagnostic { #[category] category: &'static Category, #[location(span)] @@ -151,9 +151,12 @@ pub struct SuppressionDiagnostic { message: String, #[tags] tags: DiagnosticTags, + + #[advice] + advice: SuppressionAdvice, } -impl SuppressionDiagnostic { +impl AnalyzerSuppressionDiagnostic { pub(crate) fn new( category: &'static Category, range: TextRange, @@ -164,15 +167,33 @@ impl SuppressionDiagnostic { range, message: message.to_string(), tags: DiagnosticTags::empty(), + advice: SuppressionAdvice::default(), } } - pub(crate) fn with_tags(mut self, tags: DiagnosticTags) -> Self { - self.tags |= tags; + pub(crate) fn note(mut self, message: impl Into, range: impl Into) -> Self { + self.advice.messages.push((message.into(), range.into())); self } } +#[derive(Debug, Default, Clone)] +struct SuppressionAdvice { + messages: Vec<(String, TextRange)>, +} + +impl Advices for SuppressionAdvice { + fn record(&self, visitor: &mut dyn Visit) -> std::io::Result<()> { + for (message, range) in &self.messages { + visitor.record_log(LogCategory::Info, &markup! {{message}})?; + let location = Location::builder().span(range); + + visitor.record_frame(location.build())? + } + Ok(()) + } +} + /// Series of errors encountered when running rules on a file #[derive(Debug, PartialEq, Eq, Clone)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] diff --git a/crates/biome_analyze/src/lib.rs b/crates/biome_analyze/src/lib.rs index 69f48c157168..c4a3bbad5bef 100644 --- a/crates/biome_analyze/src/lib.rs +++ b/crates/biome_analyze/src/lib.rs @@ -1,5 +1,6 @@ #![deny(rustdoc::broken_intra_doc_links)] +use rustc_hash::{FxHashMap, FxHashSet}; use std::cmp::Ordering; use std::collections::{BTreeMap, BinaryHeap}; use std::fmt::{Debug, Display, Formatter}; @@ -27,7 +28,7 @@ pub use crate::categories::{ ActionCategory, RefactorKind, RuleCategories, RuleCategoriesBuilder, RuleCategory, SourceActionKind, }; -pub use crate::diagnostics::{AnalyzerDiagnostic, RuleError, SuppressionDiagnostic}; +pub use crate::diagnostics::{AnalyzerDiagnostic, AnalyzerSuppressionDiagnostic, RuleError}; pub use crate::matcher::{InspectMatcher, MatchQueryParams, QueryMatcher, RuleKey, SignalEntry}; pub use crate::options::{AnalyzerConfiguration, AnalyzerOptions, AnalyzerRules}; pub use crate::query::{AddVisitor, QueryKey, QueryMatch, Queryable}; @@ -45,8 +46,7 @@ pub use crate::signals::{ }; pub use crate::syntax::{Ast, SyntaxVisitor}; pub use crate::visitor::{NodeVisitor, Visitor, VisitorContext, VisitorFinishContext}; -use biome_console::markup; -use biome_diagnostics::{category, Applicability, Diagnostic, DiagnosticExt, DiagnosticTags}; +use biome_diagnostics::{category, Diagnostic, DiagnosticExt}; use biome_rowan::{ AstNode, BatchMutation, Direction, Language, SyntaxElement, SyntaxToken, TextRange, TextSize, TokenAtOffset, TriviaPieceKind, WalkEvent, @@ -127,6 +127,7 @@ where let mut line_index = 0; let mut line_suppressions = Vec::new(); + let mut top_level_suppressions = TopLevelSuppressions::default(); for (index, (phase, mut visitors)) in phases.into_iter().enumerate() { let runner = PhaseRunner { @@ -144,6 +145,7 @@ where range: ctx.range, suppression_action: suppression_action.as_ref(), options: ctx.options, + top_level_suppressions: &mut top_level_suppressions, }; // The first phase being run will inspect the tokens and parse the @@ -176,11 +178,22 @@ where } let signal = DiagnosticSignal::new(|| { - SuppressionDiagnostic::new( + if suppression.already_suppressed { + AnalyzerSuppressionDiagnostic::new( + category!("suppressions/unused"), + suppression.comment_span, + "Suppression comment has no effect because another suppression comment suppresses the same rule.", + ).note( + "This is the suppression comment that was used.", + top_level_suppressions.range.unwrap_or_default() + ) + } else { + AnalyzerSuppressionDiagnostic::new( category!("suppressions/unused"), suppression.comment_span, "Suppression comment has no effect. Remove the suppression or make sure you are suppressing the correct rule.", ) + } }); if let ControlFlow::Break(br) = (emit_signal)(&signal) { @@ -222,6 +235,36 @@ struct PhaseRunner<'analyzer, 'phase, L: Language, Matcher, Break, Diag> { range: Option, /// Analyzer options options: &'phase AnalyzerOptions, + + top_level_suppressions: &'phase mut TopLevelSuppressions, +} + +#[derive(Debug, Default)] +struct TopLevelSuppressions { + filters: FxHashSet>, + range: Option, +} + +impl TopLevelSuppressions { + pub(crate) fn insert(&mut self, filter: RuleFilter<'static>) { + self.filters.insert(filter); + } + + pub(crate) fn suppressed_rule(&self, filter: &RuleKey) -> bool { + self.filters.iter().any(|f| f == filter) + } + + pub(crate) fn expand_range(&mut self, range: TextRange) { + if let Some(current_range) = self.range.as_mut() { + current_range.cover(range); + } else { + self.range = Some(range); + } + } + + pub(crate) fn has_filter(&self, filter: &RuleFilter) -> bool { + self.filters.contains(filter) + } } /// Single entry for a suppression comment in the `line_suppressions` buffer @@ -238,12 +281,14 @@ struct LineSuppression { suppress_all: bool, /// List of all the rules this comment has started suppressing (must be /// removed from the suppressed set on expiration) - suppressed_rules: Vec>, + suppressed_rules: FxHashSet>, /// List of all the rule instances this comment has started suppressing. - suppressed_instances: Vec<(RuleFilter<'static>, String)>, + suppressed_instances: FxHashMap>, /// Set to `true` when a signal matching this suppression was emitted and /// suppressed did_suppress_signal: bool, + /// Set to `true` when this line suppresses a signal that was already suppressed by another entity e.g. top-level suppression + already_suppressed: bool, } impl<'a, 'phase, L, Matcher, Break, Diag> PhaseRunner<'a, 'phase, L, Matcher, Break, Diag> @@ -373,6 +418,11 @@ where } } + if self.top_level_suppressions.suppressed_rule(&entry.rule) { + self.signal_queue.pop(); + break; + } + // Search for an active suppression comment covering the range of // this signal: first try to load the last line suppression and see // if it matches the current line index, otherwise perform a binary @@ -421,11 +471,11 @@ where suppression .suppressed_instances .iter() - .any(|(filter, v)| *filter == entry.rule && v == value.as_ref()) + .any(|(v, filter)| *filter == entry.rule && v == value.as_ref()) }) }); - // If the signal is being suppressed mark the line suppression as + // If the signal is being suppressed, mark the line suppression as // hit, otherwise emit the signal if let Some(suppression) = suppression { suppression.did_suppress_signal = true; @@ -445,17 +495,18 @@ where fn handle_comment( &mut self, token: &SyntaxToken, - is_leading: bool, - index: usize, + _is_leading: bool, + _index: usize, text: &str, range: TextRange, ) -> ControlFlow { let mut suppress_all = false; - let mut suppressed_rules = Vec::new(); - let mut suppressed_instances = Vec::new(); - let mut has_legacy = false; + let mut suppressed_rules = FxHashSet::default(); + let mut suppressed_instances = FxHashMap::default(); + let mut is_top_level_suppression = false; + let mut already_suppressed = false; - for result in (self.parse_suppression_comment)(text) { + for result in (self.parse_suppression_comment)(text, token.text_range()) { let kind = match result { Ok(kind) => kind, Err(diag) => { @@ -475,9 +526,11 @@ where SuppressionKind::Everything => (None, None), SuppressionKind::Rule(rule) => (Some(rule), None), SuppressionKind::RuleInstance(rule, instance) => (Some(rule), Some(instance)), - SuppressionKind::MaybeLegacy(rule) => (Some(rule), None), + SuppressionKind::TopLevel(rule) => (Some(rule), None), }; + is_top_level_suppression |= kind.is_top_level(); + if let Some(rule) = rule { let group_rule = rule.split_once('/'); @@ -489,15 +542,28 @@ where }; match (key, instance) { - (Some(key), Some(value)) => suppressed_instances.push((key, value.to_owned())), + (Some(key), Some(value)) => { + suppressed_instances.insert(value.to_owned(), key); + if kind.is_top_level() { + self.top_level_suppressions.insert(key); + } + if self.top_level_suppressions.has_filter(&key) { + already_suppressed = true + } + } (Some(key), None) => { - suppressed_rules.push(key); - has_legacy |= matches!(kind, SuppressionKind::MaybeLegacy(_)); + if kind.is_top_level() { + self.top_level_suppressions.insert(key); + } + if self.top_level_suppressions.has_filter(&key) { + already_suppressed = true + } + suppressed_rules.insert(key); } _ if range_match(self.range, range) => { // Emit a warning for the unknown rule let signal = DiagnosticSignal::new(move || match group_rule { - Some((group, rule)) => SuppressionDiagnostic::new( + Some((group, rule)) => AnalyzerSuppressionDiagnostic::new( category!("suppressions/unknownRule"), range, format_args!( @@ -505,7 +571,7 @@ where ), ), - None => SuppressionDiagnostic::new( + None => AnalyzerSuppressionDiagnostic::new( category!("suppressions/unknownGroup"), range, format_args!( @@ -527,21 +593,9 @@ where } } - // Emit a warning for legacy suppression syntax - if has_legacy && range_match(self.range, range) { - let signal = DiagnosticSignal::new(move || { - SuppressionDiagnostic::new( - category!("suppressions/deprecatedSuppressionComment"), - range, - "Suppression is using a deprecated syntax", - ) - .with_tags(DiagnosticTags::DEPRECATED_CODE) - }); - - let signal = signal - .with_action(|| update_suppression(self.root, token, is_leading, index, text)); - - (self.emit_signal)(&signal)?; + if is_top_level_suppression { + self.top_level_suppressions.expand_range(range); + return ControlFlow::Continue(()); } if !suppress_all && suppressed_rules.is_empty() && suppressed_instances.is_empty() { @@ -581,6 +635,7 @@ where suppressed_rules, suppressed_instances, did_suppress_signal: false, + already_suppressed, }; self.line_suppressions.push(entry); @@ -624,8 +679,12 @@ fn range_match(filter: Option, range: TextRange) -> bool { /// Signature for a suppression comment parser function /// -/// This function receives the text content of a comment and returns a list of -/// lint suppressions as an optional lint rule (if the lint rule is `None` the +/// This function receives two parameters: +/// 1. The text content of a comment. +/// 2. The range of the token the comment belongs too. The range is calculated from [SyntaxToken::text_range], so the range +/// includes all trivia. +/// +/// It returns the lint suppressions as an optional lint rule (if the lint rule is `None` the /// comment is interpreted as suppressing all lints) /// /// # Examples @@ -635,75 +694,26 @@ fn range_match(filter: Option, range: TextRange) -> bool { /// - `// biome-ignore lint/style/useWhile` -> `vec![Rule("style/useWhile")]` /// - `// biome-ignore lint/style/useWhile(foo)` -> `vec![RuleWithValue("style/useWhile", "foo")]` /// - `// biome-ignore lint/style/useWhile lint/nursery/noUnreachable` -> `vec![Rule("style/useWhile"), Rule("nursery/noUnreachable")]` -/// - `// biome-ignore lint(style/useWhile)` -> `vec![MaybeLegacy("style/useWhile")]` -/// - `// biome-ignore lint(style/useWhile) lint(nursery/noUnreachable)` -> `vec![MaybeLegacy("style/useWhile"), MaybeLegacy("nursery/noUnreachable")]` -type SuppressionParser = fn(&str) -> Vec>; +/// - `/** biome-ignore lint/style/useWhile */` if the comment is top-level -> `vec![TopLevel("style/useWhile")]` +type SuppressionParser = fn(&str, TextRange) -> Vec>; +#[derive(Debug, Clone)] /// This enum is used to categorize what is disabled by a suppression comment and with what syntax pub enum SuppressionKind<'a> { + // TODO: docs + TopLevel(&'a str), /// A suppression disabling all lints eg. `// biome-ignore lint` Everything, /// A suppression disabling a specific rule eg. `// biome-ignore lint/style/useWhile` Rule(&'a str), /// A suppression to be evaluated by a specific rule eg. `// biome-ignore lint/correctness/useExhaustiveDependencies(foo)` RuleInstance(&'a str, &'a str), - /// A suppression using the legacy syntax to disable a specific rule eg. `// biome-ignore lint(style/useWhile)` - MaybeLegacy(&'a str), } -fn update_suppression( - root: &L::Root, - token: &SyntaxToken, - is_leading: bool, - index: usize, - text: &str, -) -> Option> { - let old_token = token.clone(); - let new_token = token.clone().detach(); - - let old_trivia = if is_leading { - old_token.leading_trivia() - } else { - old_token.trailing_trivia() - }; - - let old_trivia: Vec<_> = old_trivia.pieces().collect(); - - let mut text = text.to_string(); - - while let Some(range_start) = text.find("lint(") { - let range_end = range_start + text[range_start..].find(')')?; - text.replace_range(range_end..range_end + 1, ""); - text.replace_range(range_start + 4..range_start + 5, "/"); +impl<'a> SuppressionKind<'a> { + pub const fn is_top_level(&self) -> bool { + matches!(self, SuppressionKind::TopLevel(_)) } - - let new_trivia = old_trivia.iter().enumerate().map(|(piece_index, piece)| { - if piece_index == index { - (piece.kind(), text.as_str()) - } else { - (piece.kind(), piece.text()) - } - }); - - let new_token = if is_leading { - new_token.with_leading_trivia(new_trivia) - } else { - new_token.with_trailing_trivia(new_trivia) - }; - - let mut mutation = BatchMutation::new(root.syntax().clone()); - mutation.replace_token_discard_trivia(old_token, new_token); - - Some(AnalyzerAction { - rule_name: None, - category: ActionCategory::QuickFix, - applicability: Applicability::Always, - message: markup! { - "Rewrite suppression to use the newer syntax" - } - .to_owned(), - mutation, - }) } /// Payload received by the function responsible to mark a suppression comment diff --git a/crates/biome_analyze/src/matcher.rs b/crates/biome_analyze/src/matcher.rs index a88858bcb53d..b75e6cffcdc2 100644 --- a/crates/biome_analyze/src/matcher.rs +++ b/crates/biome_analyze/src/matcher.rs @@ -351,6 +351,7 @@ mod tests { fn parse_suppression_comment( comment: &'_ str, + _range: TextRange, ) -> Vec, Infallible>> { comment .trim_start_matches("//") diff --git a/crates/biome_analyze/src/syntax.rs b/crates/biome_analyze/src/syntax.rs index 7852c8957b34..44b5cbd5e860 100644 --- a/crates/biome_analyze/src/syntax.rs +++ b/crates/biome_analyze/src/syntax.rs @@ -174,7 +174,7 @@ mod tests { let mut analyzer = Analyzer::new( &metadata, &mut matcher, - |_| -> Vec> { unreachable!() }, + |_, _| -> Vec> { unreachable!() }, Box::new(TestAction), &mut emit_signal, ); diff --git a/crates/biome_cli/tests/cases/mod.rs b/crates/biome_cli/tests/cases/mod.rs index dfb7f5de94ce..a54d4f7a6b90 100644 --- a/crates/biome_cli/tests/cases/mod.rs +++ b/crates/biome_cli/tests/cases/mod.rs @@ -22,4 +22,5 @@ mod reporter_github; mod reporter_gitlab; mod reporter_junit; mod reporter_summary; +mod suppressions; mod unknown_files; diff --git a/crates/biome_cli/tests/cases/suppressions.rs b/crates/biome_cli/tests/cases/suppressions.rs new file mode 100644 index 000000000000..e6cdfc638e28 --- /dev/null +++ b/crates/biome_cli/tests/cases/suppressions.rs @@ -0,0 +1,44 @@ +use crate::run_cli; +use crate::snap_test::{assert_cli_snapshot, SnapshotPayload}; +use biome_console::BufferConsole; +use biome_fs::MemoryFileSystem; +use biome_service::DynRef; +use bpaf::Args; +use std::path::Path; + +#[test] +fn unused_suppression_after_top_level() { + let mut fs = MemoryFileSystem::default(); + let mut console = BufferConsole::default(); + + let file_path = Path::new("file.js"); + fs.insert( + file_path.into(), + *b"/** +* biome-ignore lint/style/useConst: reason +*/ + + +let foo = 2; +/** +* biome-ignore lint/style/useConst: reason +*/ +let bar = 33;", + ); + + let result = run_cli( + DynRef::Borrowed(&mut fs), + &mut console, + Args::from([("lint"), file_path.as_os_str().to_str().unwrap()].as_slice()), + ); + + assert!(result.is_ok(), "run_cli returned {result:?}"); + + assert_cli_snapshot(SnapshotPayload::new( + module_path!(), + "unused_suppression_after_top_level", + fs, + console, + result, + )); +} diff --git a/crates/biome_cli/tests/commands/check.rs b/crates/biome_cli/tests/commands/check.rs index d33d4a10360e..d9c42e4be2f6 100644 --- a/crates/biome_cli/tests/commands/check.rs +++ b/crates/biome_cli/tests/commands/check.rs @@ -1377,36 +1377,6 @@ fn no_supported_file_found() { result, )); } - -#[test] -fn deprecated_suppression_comment() { - let mut fs = MemoryFileSystem::default(); - let mut console = BufferConsole::default(); - - let file_path = Path::new("file.js"); - fs.insert( - file_path.into(), - *b"// biome-ignore lint(suspicious/noDoubleEquals): test -a == b;", - ); - - let result = run_cli( - DynRef::Borrowed(&mut fs), - &mut console, - Args::from([("check"), file_path.as_os_str().to_str().unwrap()].as_slice()), - ); - - assert!(result.is_err(), "run_cli returned {result:?}"); - - assert_cli_snapshot(SnapshotPayload::new( - module_path!(), - "deprecated_suppression_comment", - fs, - console, - result, - )); -} - #[test] fn print_verbose() { let mut fs = MemoryFileSystem::default(); diff --git a/crates/biome_cli/tests/commands/lint.rs b/crates/biome_cli/tests/commands/lint.rs index f43ab9014172..155a414a75f8 100644 --- a/crates/biome_cli/tests/commands/lint.rs +++ b/crates/biome_cli/tests/commands/lint.rs @@ -1559,35 +1559,6 @@ fn no_supported_file_found() { )); } -#[test] -fn deprecated_suppression_comment() { - let mut fs = MemoryFileSystem::default(); - let mut console = BufferConsole::default(); - - let file_path = Path::new("file.js"); - fs.insert( - file_path.into(), - *b"// biome-ignore lint(suspicious/noDoubleEquals): test -a == b;", - ); - - let result = run_cli( - DynRef::Borrowed(&mut fs), - &mut console, - Args::from([("lint"), file_path.as_os_str().to_str().unwrap()].as_slice()), - ); - - assert!(result.is_ok(), "run_cli returned {result:?}"); - - assert_cli_snapshot(SnapshotPayload::new( - module_path!(), - "deprecated_suppression_comment", - fs, - console, - result, - )); -} - #[test] fn print_verbose() { let mut fs = MemoryFileSystem::default(); diff --git a/crates/biome_cli/tests/snapshots/main_cases_suppressions/unused_suppression_after_top_level.snap b/crates/biome_cli/tests/snapshots/main_cases_suppressions/unused_suppression_after_top_level.snap new file mode 100644 index 000000000000..c4e339836c93 --- /dev/null +++ b/crates/biome_cli/tests/snapshots/main_cases_suppressions/unused_suppression_after_top_level.snap @@ -0,0 +1,50 @@ +--- +source: crates/biome_cli/tests/snap_test.rs +expression: content +--- +## `file.js` + +```js +/** +* biome-ignore lint/style/useConst: reason +*/ + + +let foo = 2; +/** +* biome-ignore lint/style/useConst: reason +*/ +let bar = 33; +``` + +# Emitted Messages + +```block +file.js:7:1 suppressions/unused ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Suppression comment has no effect because another suppression comment suppresses the same rule. + + 6 │ let foo = 2; + > 7 │ /** + │ ^^^ + > 8 │ * biome-ignore lint/style/useConst: reason + > 9 │ */ + │ ^^ + 10 │ let bar = 33; + + i This is the suppression comment that was used. + + > 1 │ /** + │ ^^^ + > 2 │ * biome-ignore lint/style/useConst: reason + > 3 │ */ + │ ^^ + 4 │ + + +``` + +```block +Checked 1 file in