From 798c4c060cfd17e63b073480bbf21c5c3dd44212 Mon Sep 17 00:00:00 2001 From: l3ops Date: Fri, 8 Jul 2022 17:51:56 +0200 Subject: [PATCH] feat(rome_analyze): add a warning for unused suppression comments --- crates/rome_analyze/src/lib.rs | 59 +++++++++++++++---- crates/rome_analyze/src/matcher.rs | 16 ++++- .../src/categories.rs | 1 + crates/rome_js_analyze/src/lib.rs | 7 ++- .../noUnreachable/SuppressionComments.js.snap | 14 +++++ .../src/file_handlers/javascript.rs | 9 ++- npm/backend-jsonrpc/src/workspace.ts | 1 + 7 files changed, 89 insertions(+), 18 deletions(-) diff --git a/crates/rome_analyze/src/lib.rs b/crates/rome_analyze/src/lib.rs index 7bfb46ea072..65e58864063 100644 --- a/crates/rome_analyze/src/lib.rs +++ b/crates/rome_analyze/src/lib.rs @@ -162,6 +162,27 @@ where } } + for suppression in line_suppressions { + if suppression.did_suppress_signal { + continue; + } + + let signal = DiagnosticSignal::new(|| { + let diag = SuppressionDiagnostic::new( + ctx.file_id, + category!("suppressions/unused"), + suppression.comment_span, + "Suppression comment is not being used", + ); + + AnalyzerDiagnostic::from_error(diag.into()) + }); + + if let ControlFlow::Break(br) = (emit_signal)(&signal) { + return Some(br); + } + } + None } } @@ -203,6 +224,8 @@ struct PhaseRunner<'analyzer, 'phase, L: Language, Matcher, Break> { struct LineSuppression { /// Line index this comment is suppressing lint rules for line_index: usize, + /// Range of source text covered by the suppression comment + comment_span: TextRange, /// Range of source text this comment is suppressing lint rules for text_range: TextRange, /// Set to true if this comment has set the `suppress_all` flag to true @@ -211,6 +234,9 @@ struct LineSuppression { /// List of all the rules this comment has started suppressing (must be /// removed from the suppressed set on expiration) suppressed_rules: Vec>, + /// Set to `true` when a signal matching this suppression was emitted and + /// suppressed + did_suppress_signal: bool, } impl<'a, 'phase, L, Matcher, Break> PhaseRunner<'a, 'phase, L, Matcher, Break> @@ -344,14 +370,14 @@ where // if it matchs the current line index, otherwise perform a binary // search over all the previously seen suppressions to find one // with a matching range - let suppression = self - .line_suppressions - .last() - .filter(|suppression| { - suppression.line_index == *self.line_index - && suppression.text_range.start() <= start - }) - .or_else(|| { + let suppression = self.line_suppressions.last_mut().filter(|suppression| { + suppression.line_index == *self.line_index + && suppression.text_range.start() <= start + }); + + let suppression = match suppression { + Some(suppression) => Some(suppression), + None => { let index = self.line_suppressions.binary_search_by(|suppression| { if suppression.text_range.end() < entry.text_range.start() { Ordering::Less @@ -362,21 +388,26 @@ where } }); - Some(&self.line_suppressions[index.ok()?]) - }); + index.ok().map(|index| &mut self.line_suppressions[index]) + } + }; - let is_suppressed = suppression.map_or(false, |suppression| { + let suppression = suppression.filter(|suppression| { if suppression.suppress_all { return true; } + suppression .suppressed_rules .iter() .any(|filter| *filter == entry.rule) }); - // Emit the signal if the rule that created it is not currently being suppressed - if !is_suppressed { + // 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; + } else { (self.emit_signal)(&*entry.signal)?; } @@ -479,9 +510,11 @@ where let entry = LineSuppression { line_index, + comment_span: range, text_range: range, suppress_all, suppressed_rules: suppressions, + did_suppress_signal: false, }; self.line_suppressions.push(entry); diff --git a/crates/rome_analyze/src/matcher.rs b/crates/rome_analyze/src/matcher.rs index 5183bd53516..475e8cae15c 100644 --- a/crates/rome_analyze/src/matcher.rs +++ b/crates/rome_analyze/src/matcher.rs @@ -211,7 +211,7 @@ mod tests { let mut builder = RawSyntaxTreeBuilder::new(); builder.start_node(RawLanguageKind::ROOT); - builder.start_node(RawLanguageKind::EXPRESSION_LIST); + builder.start_node(RawLanguageKind::SEPARATED_EXPRESSION_LIST); builder.start_node(RawLanguageKind::LITERAL_EXPRESSION); builder.token_with_trivia( @@ -289,6 +289,16 @@ mod tests { &[TriviaPiece::new(TriviaPieceKind::Newline, 1)], ); + builder.token_with_trivia( + RawLanguageKind::SEMICOLON_TOKEN, + "//group/rule\n;\n", + &[ + TriviaPiece::new(TriviaPieceKind::SingleLineComment, 12), + TriviaPiece::new(TriviaPieceKind::Newline, 1), + ], + &[TriviaPiece::new(TriviaPieceKind::Newline, 1)], + ); + builder.finish_node(); builder.finish_node(); @@ -356,6 +366,10 @@ mod tests { category!("args/fileNotFound"), TextRange::new(TextSize::from(97), TextSize::from(108)) ), + ( + category!("suppressions/unused"), + TextRange::new(TextSize::from(110), TextSize::from(122)) + ), ] ); } diff --git a/crates/rome_diagnostics_categories/src/categories.rs b/crates/rome_diagnostics_categories/src/categories.rs index 2b9bf833b3a..71f25146928 100644 --- a/crates/rome_diagnostics_categories/src/categories.rs +++ b/crates/rome_diagnostics_categories/src/categories.rs @@ -99,6 +99,7 @@ define_dategories! { "suppressions/unknownGroup", "suppressions/unknownRule", + "suppressions/unused", // Used in tests and examples "args/fileNotFound", "flags/invalid", diff --git a/crates/rome_js_analyze/src/lib.rs b/crates/rome_js_analyze/src/lib.rs index a4a99671a2f..12f12354502 100644 --- a/crates/rome_js_analyze/src/lib.rs +++ b/crates/rome_js_analyze/src/lib.rs @@ -251,10 +251,11 @@ mod tests { let options = AnalyzerOptions::default(); analyze(FileId::zero(), &parsed.tree(), filter, &options, |signal| { - if let Some(mut diag) = signal.diagnostic() { - diag.set_severity(Severity::Warning); + if let Some(diag) = signal.diagnostic() { let code = diag.category().unwrap(); - panic!("unexpected diagnostic {code:?}"); + if code != category!("suppressions/unused") { + panic!("unexpected diagnostic {code:?}"); + } } ControlFlow::::Continue(()) diff --git a/crates/rome_js_analyze/tests/specs/correctness/noUnreachable/SuppressionComments.js.snap b/crates/rome_js_analyze/tests/specs/correctness/noUnreachable/SuppressionComments.js.snap index 4747bf8d91b..50d2f9815f0 100644 --- a/crates/rome_js_analyze/tests/specs/correctness/noUnreachable/SuppressionComments.js.snap +++ b/crates/rome_js_analyze/tests/specs/correctness/noUnreachable/SuppressionComments.js.snap @@ -1,5 +1,6 @@ --- source: crates/rome_js_analyze/tests/spec_tests.rs +assertion_line: 99 expression: SuppressionComments.js --- # Input @@ -45,4 +46,17 @@ SuppressionComments.js:5:5 lint/correctness/noUnreachable ━━━━━━━ ``` +``` +SuppressionComments.js:1:1 suppressions/unused ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! Suppression comment is not being used + + > 1 │ // rome-ignore lint(correctness/noUnreachable): this comment does nothing + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 2 │ function SuppressionComments1() { + 3 │ beforeReturn(); + + +``` + diff --git a/crates/rome_service/src/file_handlers/javascript.rs b/crates/rome_service/src/file_handlers/javascript.rs index 300cb161cce..90bdf63b1f5 100644 --- a/crates/rome_service/src/file_handlers/javascript.rs +++ b/crates/rome_service/src/file_handlers/javascript.rs @@ -10,7 +10,7 @@ use rome_analyze::{ AnalysisFilter, AnalyzerOptions, ControlFlow, GroupCategory, Never, QueryMatch, RegistryVisitor, RuleCategories, RuleCategory, RuleFilter, RuleGroup, }; -use rome_diagnostics::{Applicability, CodeSuggestion}; +use rome_diagnostics::{v2::category, Applicability, CodeSuggestion}; use rome_formatter::{FormatError, Printed}; use rome_fs::RomePath; use rome_js_analyze::{analyze, analyze_with_inspect_matcher, visit_registry, RuleError}; @@ -221,8 +221,15 @@ fn lint(params: LintParams) -> LintResults { .iter() .any(|diag| diag.severity() <= v2::Severity::Error); + let has_lint = params.filter.categories.contains(RuleCategories::LINT); + analyze(file_id, &tree, params.filter, &analyzer_options, |signal| { if let Some(mut diagnostic) = signal.diagnostic() { + // Do not report unused suppression comment diagnostics if this is a syntax-only analyzer pass + if !has_lint && diagnostic.category() == Some(category!("suppressions/unused")) { + return ControlFlow::::Continue(()); + } + diagnostic_count += 1; // We do now check if the severity of the diagnostics should be changed. diff --git a/npm/backend-jsonrpc/src/workspace.ts b/npm/backend-jsonrpc/src/workspace.ts index aab4a9342e9..606c1811690 100644 --- a/npm/backend-jsonrpc/src/workspace.ts +++ b/npm/backend-jsonrpc/src/workspace.ts @@ -403,6 +403,7 @@ export type Category = | "parse/noSuperWithoutExtends" | "suppressions/unknownGroup" | "suppressions/unknownRule" + | "suppressions/unused" | "args/fileNotFound" | "flags/invalid" | "semanticTests";