diff --git a/Cargo.lock b/Cargo.lock index 074ed19b..ce816de8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2867,6 +2867,16 @@ dependencies = [ "regex", ] +[[package]] +name = "pgt_suppressions" +version = "0.0.0" +dependencies = [ + "pgt_analyse", + "pgt_diagnostics", + "pgt_text_size", + "tracing", +] + [[package]] name = "pgt_test_macros" version = "0.0.0" @@ -2976,6 +2986,7 @@ dependencies = [ "pgt_query_ext", "pgt_schema_cache", "pgt_statement_splitter", + "pgt_suppressions", "pgt_text_size", "pgt_typecheck", "rustc-hash 2.1.0", diff --git a/Cargo.toml b/Cargo.toml index b5d6dd01..15c6f02f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -78,6 +78,7 @@ pgt_query_ext = { path = "./crates/pgt_query_ext", version = "0.0.0 pgt_query_proto_parser = { path = "./crates/pgt_query_proto_parser", version = "0.0.0" } pgt_schema_cache = { path = "./crates/pgt_schema_cache", version = "0.0.0" } pgt_statement_splitter = { path = "./crates/pgt_statement_splitter", version = "0.0.0" } +pgt_suppressions = { path = "./crates/pgt_suppressions", version = "0.0.0" } pgt_text_edit = { path = "./crates/pgt_text_edit", version = "0.0.0" } pgt_text_size = { path = "./crates/pgt_text_size", version = "0.0.0" } pgt_tokenizer = { path = "./crates/pgt_tokenizer", version = "0.0.0" } diff --git a/crates/pgt_analyse/src/categories.rs b/crates/pgt_analyse/src/categories.rs index e5dd51c2..02819a4e 100644 --- a/crates/pgt_analyse/src/categories.rs +++ b/crates/pgt_analyse/src/categories.rs @@ -16,6 +16,27 @@ pub enum RuleCategory { Transformation, } +impl TryFrom for RuleCategory { + type Error = String; + + fn try_from(value: String) -> Result { + value.as_str().try_into() + } +} + +impl TryFrom<&str> for RuleCategory { + type Error = String; + + fn try_from(value: &str) -> Result { + match value { + "lint" => Ok(Self::Lint), + "action" => Ok(Self::Action), + "transformation" => Ok(Self::Transformation), + _ => Err(format!("Invalid Rule Category: {}", value)), + } + } +} + /// Actions that suppress rules should start with this string pub const SUPPRESSION_ACTION_CATEGORY: &str = "quickfix.suppressRule"; diff --git a/crates/pgt_diagnostics/src/location.rs b/crates/pgt_diagnostics/src/location.rs index cbd8e646..e17ace9c 100644 --- a/crates/pgt_diagnostics/src/location.rs +++ b/crates/pgt_diagnostics/src/location.rs @@ -41,13 +41,13 @@ impl Eq for Location<'_> {} #[derive(Debug, Clone, Copy, Eq, PartialEq, Serialize, Deserialize)] #[cfg_attr(feature = "schema", derive(schemars::JsonSchema))] #[serde(rename_all = "camelCase")] -pub enum Resource

{ +pub enum Resource { /// The diagnostic is related to the content of the command line arguments. Argv, /// The diagnostic is related to the content of a memory buffer. Memory, /// The diagnostic is related to a file on the filesystem. - File(P), + File(Path), } impl

Resource

{ diff --git a/crates/pgt_suppressions/Cargo.toml b/crates/pgt_suppressions/Cargo.toml new file mode 100644 index 00000000..ee723b3b --- /dev/null +++ b/crates/pgt_suppressions/Cargo.toml @@ -0,0 +1,18 @@ + +[package] +authors.workspace = true +categories.workspace = true +description = "Provides an API that parses suppressions from SQL files, and provides a way to check if a diagnostic is suppressed." +edition.workspace = true +homepage.workspace = true +keywords.workspace = true +license.workspace = true +name = "pgt_suppressions" +repository.workspace = true +version = "0.0.0" + +[dependencies] +pgt_analyse = { workspace = true } +pgt_diagnostics = { workspace = true } +pgt_text_size = { workspace = true } +tracing = { workspace = true } diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs new file mode 100644 index 00000000..2577ea41 --- /dev/null +++ b/crates/pgt_suppressions/src/lib.rs @@ -0,0 +1,351 @@ +use std::collections::HashMap; +pub mod parser; +pub mod suppression; + +use pgt_analyse::RuleFilter; +use pgt_diagnostics::{Diagnostic, MessageAndDescription}; + +pub mod line_index; + +use line_index::LineIndex; + +use crate::{ + parser::SuppressionsParser, + suppression::{RangeSuppression, RuleSpecifier, Suppression, SuppressionDiagnostic}, +}; + +type Line = usize; + +#[derive(Debug, Default, Clone)] +pub struct Suppressions { + file_suppressions: Vec, + line_suppressions: std::collections::HashMap, + range_suppressions: Vec, + pub diagnostics: Vec, + line_index: LineIndex, +} + +impl From<&str> for Suppressions { + fn from(doc: &str) -> Self { + SuppressionsParser::parse(doc) + } +} +impl From for Suppressions { + fn from(doc: String) -> Self { + SuppressionsParser::parse(doc.as_str()) + } +} + +impl Suppressions { + /// Some diagnostics can be turned off via the configuration. + /// This will mark suppressions that try to suppress these disabled diagnostics as errors. + pub fn get_disabled_diagnostic_suppressions_as_errors( + &self, + disabled_rules: &[RuleFilter<'_>], + ) -> Vec { + let mut diagnostics = vec![]; + + { + let disabled = self + .file_suppressions + .iter() + .filter(|s| s.rule_specifier.is_disabled(disabled_rules)); + + for suppr in disabled { + diagnostics.push(suppr.to_disabled_diagnostic()); + } + } + + { + let disabled = self + .line_suppressions + .iter() + .filter(|(_, s)| s.rule_specifier.is_disabled(disabled_rules)); + + for (_, suppr) in disabled { + diagnostics.push(suppr.to_disabled_diagnostic()); + } + } + + { + let disabled = self.range_suppressions.iter().filter(|s| { + s.start_suppression + .rule_specifier + .is_disabled(disabled_rules) + }); + + for range_suppr in disabled { + diagnostics.push(range_suppr.start_suppression.to_disabled_diagnostic()); + } + } + + diagnostics + } + + pub fn get_unused_suppressions_as_errors( + &self, + diagnostics: &[D], + ) -> Vec { + let mut results = vec![]; + + let mut diagnostics_by_line: HashMap> = HashMap::new(); + for diag in diagnostics { + if let Some(line) = diag + .location() + .span + .and_then(|sp| self.line_index.line_for_offset(sp.start())) + { + let entry = diagnostics_by_line.entry(line); + entry + .and_modify(|current| { + current.push(diag); + }) + .or_insert(vec![diag]); + } + } + + // Users may use many suppressions for a single diagnostic, like so: + // ``` + // -- pgt-ignore lint/safety/banDropTable + // -- pgt-ignore lint/safety/banDropColumn + // + // ``` + // So to find a matching diagnostic for any suppression, we're moving + // down lines until we find a line where there's no suppression. + for (line, suppr) in &self.line_suppressions { + let mut expected_diagnostic_line = line + 1; + while self + .line_suppressions + .contains_key(&expected_diagnostic_line) + { + expected_diagnostic_line += 1; + } + + if diagnostics_by_line + .get(&expected_diagnostic_line) + .is_some_and(|diags| { + diags.iter().any(|d| { + d.category() + .is_some_and(|cat| match RuleSpecifier::try_from(cat.name()) { + Ok(spec) => suppr.matches(&spec), + Err(_) => false, + }) + }) + }) + { + continue; + } else { + results.push(SuppressionDiagnostic { + span: suppr.suppression_range, + message: MessageAndDescription::from( + "This suppression has no effect.".to_string(), + ), + }) + } + } + + results + } + + pub fn is_suppressed(&self, diagnostic: &D) -> bool { + diagnostic + .category() + .map(|c| match RuleSpecifier::try_from(c.name()) { + Ok(specifier) => { + self.by_file_suppression(&specifier) + || self.by_range_suppression(diagnostic, &specifier) + || self.by_line_suppression(diagnostic, &specifier) + } + Err(_) => false, + }) + .unwrap_or(false) + } + + fn by_file_suppression(&self, specifier: &RuleSpecifier) -> bool { + self.file_suppressions.iter().any(|s| s.matches(specifier)) + } + + fn by_line_suppression( + &self, + diagnostic: &D, + specifier: &RuleSpecifier, + ) -> bool { + self.get_eligible_line_suppressions_for_diagnostic(diagnostic) + .iter() + .any(|s| s.matches(specifier)) + } + + fn by_range_suppression( + &self, + diagnostic: &D, + specifier: &RuleSpecifier, + ) -> bool { + self.range_suppressions.iter().any(|range_suppr| { + range_suppr.start_suppression.matches(specifier) + && diagnostic + .location() + .span + .is_some_and(|sp| range_suppr.suppressed_range.contains_range(sp)) + }) + } + + fn get_eligible_line_suppressions_for_diagnostic( + &self, + diagnostic: &D, + ) -> Vec<&Suppression> { + diagnostic + .location() + .span + .and_then(|span| self.line_index.line_for_offset(span.start())) + .filter(|line_no| *line_no > 0) + .map(|mut line_no| { + let mut eligible = vec![]; + + // one-for-one, we're checking the lines above a diagnostic location + // until there are no more suppressions + line_no -= 1; + while let Some(suppr) = self.line_suppressions.get(&line_no) { + eligible.push(suppr); + line_no -= 1; + } + + eligible + }) + .unwrap_or_default() + } +} + +#[cfg(test)] +mod tests { + use pgt_diagnostics::{Diagnostic, MessageAndDescription}; + use pgt_text_size::TextRange; + + use crate::suppression::SuppressionDiagnostic; + + #[derive(Clone, Debug, Diagnostic)] + #[diagnostic(category = "lint", severity = Error)] + pub struct TestDiagnostic { + #[location(span)] + pub span: TextRange, + } + + #[test] + fn correctly_suppresses_diagnostics_at_top_level() { + let doc = r#" + -- pgt-ignore-all lint + + select 1; + "#; + + let len_doc: u32 = doc.len().try_into().unwrap(); + + let suppressions = super::Suppressions::from(doc); + + assert!(suppressions.is_suppressed(&TestDiagnostic { + span: TextRange::new((len_doc - 10).into(), len_doc.into()), + })); + } + + #[test] + fn correctly_suppresses_diagnostics_at_line() { + let doc = r#" + select 2; + + -- pgt-ignore lint + select 1; + "#; + + let suppressions = super::Suppressions::from(doc); + + assert!(suppressions.is_suppressed(&TestDiagnostic { + span: TextRange::new(67.into(), 76.into()), + })); + } + + #[test] + fn correctly_suppresses_with_multiple_line_diagnostics() { + let doc = r#" + select 2; + + -- pgt-ignore lint + -- pgt-ignore syntax + select 1; + "#; + + let suppressions = super::Suppressions::from(doc); + + assert!(suppressions.is_suppressed(&TestDiagnostic { + span: TextRange::new(100.into(), 109.into()), + })); + } + + #[test] + fn correctly_suppresses_diagnostics_with_ranges() { + let doc = r#" + select 2; + + -- pgt-ignore-start lint + select 1; + -- pgt-ignore-end lint + "#; + + let suppressions = super::Suppressions::from(doc); + + assert!(suppressions.is_suppressed(&TestDiagnostic { + span: TextRange::new(73.into(), 82.into()), + })); + } + + #[test] + fn marks_disabled_rule_suppressions_as_errors() { + let doc = r#" + select 2; + + -- pgt-ignore lint/safety/banDropTable + select 1; + "#; + + let suppressions = super::Suppressions::from(doc); + + let disabled_diagnostics = suppressions.get_disabled_diagnostic_suppressions_as_errors(&[ + pgt_analyse::RuleFilter::Group("safety"), + ]); + + assert_eq!(disabled_diagnostics.len(), 1); + + assert_eq!( + disabled_diagnostics[0], + SuppressionDiagnostic { + span: TextRange::new(36.into(), 74.into()), + message: MessageAndDescription::from("This rule has been disabled via the configuration. The suppression has no effect.".to_string()) + } + ); + } + + #[test] + fn marks_unused_suppressions_as_errors() { + let doc = r#" + select 2; + + -- pgt-ignore lint + select 1; + "#; + + // no diagnostics + let diagnostics: Vec = vec![]; + + let suppressions = super::Suppressions::from(doc); + + let unused_diagnostics = suppressions.get_unused_suppressions_as_errors(&diagnostics); + + assert_eq!(unused_diagnostics.len(), 1); + + assert_eq!( + unused_diagnostics[0], + SuppressionDiagnostic { + span: TextRange::new(36.into(), 54.into()), + message: MessageAndDescription::from("This suppression has no effect.".to_string()) + } + ); + } +} diff --git a/crates/pgt_suppressions/src/line_index.rs b/crates/pgt_suppressions/src/line_index.rs new file mode 100644 index 00000000..16af72dd --- /dev/null +++ b/crates/pgt_suppressions/src/line_index.rs @@ -0,0 +1,43 @@ +use pgt_text_size::TextSize; + +#[derive(Debug, Default, Clone)] +pub(crate) struct LineIndex { + line_offset: Vec, +} + +impl LineIndex { + pub fn new(doc: &str) -> Self { + let line_offset = std::iter::once(0) + .chain(doc.match_indices(&['\n', '\r']).filter_map(|(i, _)| { + let bytes = doc.as_bytes(); + + match bytes[i] { + // Filter out the `\r` in `\r\n` to avoid counting the line break twice + b'\r' if i + 1 < bytes.len() && bytes[i + 1] == b'\n' => None, + _ => Some(i + 1), + } + })) + .map(|i| TextSize::try_from(i).expect("integer overflow")) + .collect(); + + Self { line_offset } + } + + pub fn offset_for_line(&self, idx: usize) -> Option<&pgt_text_size::TextSize> { + self.line_offset.get(idx) + } + + pub fn line_for_offset(&self, offset: TextSize) -> Option { + self.line_offset + .iter() + .enumerate() + .filter_map(|(i, line_offset)| { + if offset >= *line_offset { + Some(i) + } else { + None + } + }) + .next_back() + } +} diff --git a/crates/pgt_suppressions/src/parser.rs b/crates/pgt_suppressions/src/parser.rs new file mode 100644 index 00000000..663e52fe --- /dev/null +++ b/crates/pgt_suppressions/src/parser.rs @@ -0,0 +1,353 @@ +use std::{ + iter::{Enumerate, Peekable}, + str::Lines, +}; + +use pgt_diagnostics::MessageAndDescription; +use pgt_text_size::TextRange; + +use crate::{ + Suppressions, + line_index::LineIndex, + suppression::{RangeSuppression, Suppression, SuppressionDiagnostic, SuppressionKind}, +}; + +#[derive(Debug)] +pub(crate) struct SuppressionsParser<'a> { + file_suppressions: Vec, + line_suppressions: std::collections::HashMap, + range_suppressions: Vec, + diagnostics: Vec, + lines: Peekable>>, + line_index: LineIndex, + + start_suppressions_stack: Vec, +} + +impl<'a> SuppressionsParser<'a> { + pub fn new(doc: &'a str) -> Self { + let lines = doc.lines().enumerate().peekable(); + + Self { + file_suppressions: vec![], + line_suppressions: std::collections::HashMap::default(), + range_suppressions: vec![], + diagnostics: vec![], + lines, + line_index: LineIndex::new(doc), + start_suppressions_stack: vec![], + } + } + + pub fn parse(doc: &str) -> Suppressions { + let mut parser = SuppressionsParser::new(doc); + + parser.parse_file_suppressions(); + parser.parse_suppressions(); + parser.handle_unmatched_start_suppressions(); + + Suppressions { + file_suppressions: parser.file_suppressions, + line_suppressions: parser.line_suppressions, + range_suppressions: parser.range_suppressions, + diagnostics: parser.diagnostics, + line_index: parser.line_index, + } + } + + /// Will parse the suppressions at the start of the file. + /// As soon as anything is encountered that's not a `pgt-ignore-all` + /// suppression or an empty line, this will stop. + fn parse_file_suppressions(&mut self) { + while let Some((_, preview)) = self.lines.peek() { + if preview.trim().is_empty() { + self.lines.next(); + continue; + } + + if !preview.trim().starts_with("-- pgt-ignore-all") { + return; + } + + let (idx, line) = self.lines.next().unwrap(); + + let offset = self.line_index.offset_for_line(idx).unwrap(); + + match Suppression::from_line(line, offset) { + Ok(suppr) => self.file_suppressions.push(suppr), + Err(diag) => self.diagnostics.push(diag), + } + } + } + + fn parse_suppressions(&mut self) { + for (idx, line) in self.lines.by_ref() { + if !line.trim().starts_with("-- pgt-ignore") { + continue; + } + + let offset = self.line_index.offset_for_line(idx).unwrap(); + + let suppr = match Suppression::from_line(line, offset) { + Ok(suppr) => suppr, + Err(diag) => { + self.diagnostics.push(diag); + continue; + } + }; + + match suppr.kind { + SuppressionKind::File => { + self.diagnostics.push(SuppressionDiagnostic { + span: suppr.suppression_range, + message: MessageAndDescription::from( + "File suppressions should be at the top of the file.".to_string(), + ), + }); + } + + SuppressionKind::Line => { + self.line_suppressions.insert(idx, suppr); + } + + SuppressionKind::Start => self.start_suppressions_stack.push(suppr), + SuppressionKind::End => { + let matching_start_idx = self + .start_suppressions_stack + .iter() + .enumerate() + .filter_map(|(idx, s)| { + if s.rule_specifier == suppr.rule_specifier { + Some(idx) + } else { + None + } + }) + .next_back(); + + if let Some(start_idx) = matching_start_idx { + let start = self.start_suppressions_stack.remove(start_idx); + + let full_range = TextRange::new( + start.suppression_range.start(), + suppr.suppression_range.end(), + ); + + self.range_suppressions.push(RangeSuppression { + suppressed_range: full_range, + start_suppression: start, + }); + } else { + self.diagnostics.push(SuppressionDiagnostic { + span: suppr.suppression_range, + message: MessageAndDescription::from( + "This end suppression does not have a matching start.".to_string(), + ), + }); + } + } + } + } + } + + /// If we have `pgt-ignore-start` suppressions without matching end tags after parsing the entire file, + /// we'll report diagnostics for those. + fn handle_unmatched_start_suppressions(&mut self) { + let start_suppressions = std::mem::take(&mut self.start_suppressions_stack); + + for suppr in start_suppressions { + self.diagnostics.push(SuppressionDiagnostic { + span: suppr.suppression_range, + message: MessageAndDescription::from( + "This start suppression does not have a matching end.".to_string(), + ), + }); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::suppression::{RuleSpecifier, SuppressionKind}; + + #[test] + fn test_parse_line_suppressions() { + let doc = r#" +SELECT 1; +-- pgt-ignore lint/safety/banDropColumn +SELECT 2; +"#; + let suppressions = SuppressionsParser::parse(doc); + + // Should have a line suppression on line 1 (0-based index) + let suppression = suppressions + .line_suppressions + .get(&2) + .expect("no suppression found"); + + assert_eq!(suppression.kind, SuppressionKind::Line); + assert_eq!( + suppression.rule_specifier, + RuleSpecifier::Rule( + "lint".to_string(), + "safety".to_string(), + "banDropColumn".to_string() + ) + ); + } + + #[test] + fn test_parse_multiple_line_suppressions() { + let doc = r#" +SELECT 1; +-- pgt-ignore lint/safety/banDropColumn +-- pgt-ignore lint/safety/banDropTable +-- pgt-ignore lint/safety/banDropNotNull +"#; + + let suppressions = SuppressionsParser::parse(doc); + + assert_eq!(suppressions.line_suppressions.len(), 3); + + assert_eq!( + suppressions + .line_suppressions + .get(&2) + .unwrap() + .rule_specifier + .rule(), + Some("banDropColumn") + ); + + assert_eq!( + suppressions + .line_suppressions + .get(&3) + .unwrap() + .rule_specifier + .rule(), + Some("banDropTable") + ); + + assert_eq!( + suppressions + .line_suppressions + .get(&4) + .unwrap() + .rule_specifier + .rule(), + Some("banDropNotNull") + ); + } + + #[test] + fn parses_file_level_suppressions() { + let doc = r#" +-- pgt-ignore-all lint +-- pgt-ignore-all typecheck + +SELECT 1; +-- pgt-ignore-all lint/safety +"#; + + let suppressions = SuppressionsParser::parse(doc); + + assert_eq!(suppressions.diagnostics.len(), 1); + assert_eq!(suppressions.file_suppressions.len(), 2); + + assert_eq!( + suppressions.file_suppressions[0].rule_specifier, + RuleSpecifier::Category("lint".to_string()) + ); + assert_eq!( + suppressions.file_suppressions[1].rule_specifier, + RuleSpecifier::Category("typecheck".to_string()) + ); + + assert_eq!( + suppressions.diagnostics[0].message.to_string(), + String::from("File suppressions should be at the top of the file.") + ); + } + + #[test] + fn parses_range_suppressions() { + let doc = r#" +-- pgt-ignore-start lint/safety/banDropTable +drop table users; +drop table auth; +drop table posts; +-- pgt-ignore-end lint/safety/banDropTable +"#; + + let suppressions = SuppressionsParser::parse(doc); + + assert_eq!(suppressions.range_suppressions.len(), 1); + + assert_eq!( + suppressions.range_suppressions[0], + RangeSuppression { + suppressed_range: TextRange::new(1.into(), 141.into()), + start_suppression: Suppression { + kind: SuppressionKind::Start, + rule_specifier: RuleSpecifier::Rule( + "lint".to_string(), + "safety".to_string(), + "banDropTable".to_string() + ), + suppression_range: TextRange::new(1.into(), 45.into()), + explanation: None, + }, + } + ); + } + + #[test] + fn parses_range_suppressions_with_errors() { + let doc = r#" +-- pgt-ignore-start lint/safety/banDropTable +drop table users; +-- pgt-ignore-start lint/safety/banDropTable +drop table auth; +drop table posts; +-- pgt-ignore-end lint/safety/banDropTable +-- pgt-ignore-end lint/safety/banDropColumn +"#; + + let suppressions = SuppressionsParser::parse(doc); + + assert_eq!(suppressions.range_suppressions.len(), 1); + assert_eq!(suppressions.diagnostics.len(), 2); + + // the inner, nested start/end combination is recognized. + assert_eq!( + suppressions.range_suppressions[0], + RangeSuppression { + suppressed_range: TextRange::new(64.into(), 186.into()), + start_suppression: Suppression { + kind: SuppressionKind::Start, + rule_specifier: RuleSpecifier::Rule( + "lint".to_string(), + "safety".to_string(), + "banDropTable".to_string() + ), + suppression_range: TextRange::new(64.into(), 108.into()), + explanation: None, + }, + } + ); + + // the outer end is an error + assert_eq!( + suppressions.diagnostics[0].message.to_string(), + String::from("This end suppression does not have a matching start.") + ); + + // the outer start is an error + assert_eq!( + suppressions.diagnostics[1].message.to_string(), + String::from("This start suppression does not have a matching end.") + ); + } +} diff --git a/crates/pgt_suppressions/src/suppression.rs b/crates/pgt_suppressions/src/suppression.rs new file mode 100644 index 00000000..6ebaf25c --- /dev/null +++ b/crates/pgt_suppressions/src/suppression.rs @@ -0,0 +1,459 @@ +use pgt_analyse::RuleFilter; +use pgt_diagnostics::{Category, Diagnostic, MessageAndDescription}; +use pgt_text_size::{TextRange, TextSize}; + +/// A specialized diagnostic for the typechecker. +/// +/// Type diagnostics are always **errors**. +#[derive(Clone, Debug, Diagnostic, PartialEq)] +#[diagnostic(category = "lint", severity = Warning)] +pub struct SuppressionDiagnostic { + #[location(span)] + pub span: TextRange, + #[description] + #[message] + pub message: MessageAndDescription, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum SuppressionKind { + File, + Line, + Start, + End, +} + +#[derive(Debug, PartialEq, Clone, Eq)] +/// Represents the suppressed rule, as written in the suppression comment. +/// e.g. `lint/safety/banDropColumn`, or `lint/safety`, or just `lint`. +/// The format of a rule specifier string is `(/(/))`. +/// +/// `RuleSpecifier` can only be constructed from a `&str` that matches a valid +/// [pgt_diagnostics::Category]. +pub(crate) enum RuleSpecifier { + Category(String), + Group(String, String), + Rule(String, String, String), +} + +impl RuleSpecifier { + pub(crate) fn category(&self) -> &str { + match self { + RuleSpecifier::Category(rule_category) => rule_category, + RuleSpecifier::Group(rule_category, _) => rule_category, + RuleSpecifier::Rule(rule_category, _, _) => rule_category, + } + } + + pub(crate) fn group(&self) -> Option<&str> { + match self { + RuleSpecifier::Category(_) => None, + RuleSpecifier::Group(_, gr) => Some(gr), + RuleSpecifier::Rule(_, gr, _) => Some(gr), + } + } + + pub(crate) fn rule(&self) -> Option<&str> { + match self { + RuleSpecifier::Rule(_, _, ru) => Some(ru), + _ => None, + } + } + + pub(crate) fn is_disabled(&self, disabled_rules: &[RuleFilter<'_>]) -> bool { + // note: it is not possible to disable entire categories via the config + let group = self.group(); + let rule = self.rule(); + + disabled_rules.iter().any(|r| match r { + RuleFilter::Group(gr) => group.is_some_and(|specifier_group| specifier_group == *gr), + RuleFilter::Rule(gr, ru) => group.is_some_and(|specifier_group| { + rule.is_some_and(|specifier_rule| specifier_group == *gr && specifier_rule == *ru) + }), + }) + } +} + +impl From<&Category> for RuleSpecifier { + fn from(category: &Category) -> Self { + let mut specifiers = category.name().split('/').map(|s| s.to_string()); + + let category_str = specifiers.next(); + let group = specifiers.next(); + let rule = specifiers.next(); + + match (category_str, group, rule) { + (Some(c), Some(g), Some(r)) => RuleSpecifier::Rule(c, g, r), + (Some(c), Some(g), None) => RuleSpecifier::Group(c, g), + (Some(c), None, None) => RuleSpecifier::Category(c), + _ => unreachable!(), + } + } +} + +impl TryFrom<&str> for RuleSpecifier { + type Error = String; + + fn try_from(specifier_str: &str) -> Result { + let cat = specifier_str + .parse::<&Category>() + .map_err(|_| "Invalid rule.".to_string())?; + + Ok(RuleSpecifier::from(cat)) + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct Suppression { + pub(crate) suppression_range: TextRange, + pub(crate) kind: SuppressionKind, + pub(crate) rule_specifier: RuleSpecifier, + #[allow(unused)] + pub(crate) explanation: Option, +} + +impl Suppression { + /// Creates a suppression from a suppression comment line. + /// The line start must match `-- pgt-ignore`, otherwise, this will panic. + /// Leading whitespace is ignored. + pub(crate) fn from_line(line: &str, offset: &TextSize) -> Result { + let start_trimmed = line.trim_ascii_start(); + let leading_whitespace_offset = line.len() - start_trimmed.len(); + let trimmed = start_trimmed.trim_ascii_end(); + + assert!( + start_trimmed.starts_with("-- pgt-ignore"), + "Only try parsing suppressions from lines starting with `-- pgt-ignore`." + ); + + let full_offset = *offset + TextSize::new(leading_whitespace_offset.try_into().unwrap()); + let span = TextRange::new( + full_offset, + pgt_text_size::TextSize::new(trimmed.len().try_into().unwrap()) + full_offset, + ); + + let (line, explanation) = match trimmed.split_once(':') { + Some((suppr, explanation)) => (suppr, Some(explanation.trim())), + None => (trimmed, None), + }; + + let mut parts = line.split_ascii_whitespace(); + + let _ = parts.next(); + let kind = match parts.next().unwrap() { + "pgt-ignore-all" => SuppressionKind::File, + "pgt-ignore-start" => SuppressionKind::Start, + "pgt-ignore-end" => SuppressionKind::End, + "pgt-ignore" => SuppressionKind::Line, + k => { + return Err(SuppressionDiagnostic { + span, + message: MessageAndDescription::from(format!( + "'{}' is not a valid suppression tag.", + k, + )), + }); + } + }; + + let specifier_str = match parts.next() { + Some(it) => it, + None => { + return Err(SuppressionDiagnostic { + span, + message: MessageAndDescription::from( + "You must specify which lints to suppress.".to_string(), + ), + }); + } + }; + + let rule_specifier = + RuleSpecifier::try_from(specifier_str).map_err(|e| SuppressionDiagnostic { + span, + message: MessageAndDescription::from(e), + })?; + + Ok(Self { + rule_specifier, + kind, + suppression_range: span, + explanation: explanation.map(|e| e.to_string()), + }) + } + + pub(crate) fn matches(&self, diagnostic_specifier: &RuleSpecifier) -> bool { + let d_category = diagnostic_specifier.category(); + let d_group = diagnostic_specifier.group(); + let d_rule = diagnostic_specifier.rule(); + + match &self.rule_specifier { + // Check if we suppress the entire category + RuleSpecifier::Category(cat) if cat == d_category => return true, + + // Check if we suppress the category & group + RuleSpecifier::Group(cat, group) => { + if cat == d_category && Some(group.as_str()) == d_group { + return true; + } + } + + // Check if we suppress the category & group & specific rule + RuleSpecifier::Rule(cat, group, rule) => { + if cat == d_category + && Some(group.as_str()) == d_group + && Some(rule.as_str()) == d_rule + { + return true; + } + } + + _ => {} + } + + false + } + + pub(crate) fn to_disabled_diagnostic(&self) -> SuppressionDiagnostic { + SuppressionDiagnostic { + span: self.suppression_range, + message: MessageAndDescription::from( + "This rule has been disabled via the configuration. The suppression has no effect." + .to_string(), + ), + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct RangeSuppression { + pub(crate) suppressed_range: TextRange, + pub(crate) start_suppression: Suppression, +} + +#[cfg(test)] +mod tests { + use super::*; + use pgt_text_size::{TextRange, TextSize}; + + #[test] + fn test_suppression_from_line_rule() { + let line = "-- pgt-ignore lint/safety/banDropColumn: explanation"; + let offset = &TextSize::new(0); + let suppression = Suppression::from_line(line, offset).unwrap(); + + assert_eq!(suppression.kind, SuppressionKind::Line); + assert_eq!( + suppression.rule_specifier, + RuleSpecifier::Rule( + "lint".to_string(), + "safety".to_string(), + "banDropColumn".to_string() + ) + ); + assert_eq!(suppression.explanation.as_deref(), Some("explanation")); + } + + #[test] + fn test_suppression_from_line_group() { + let line = "-- pgt-ignore lint/safety: explanation"; + let offset = &TextSize::new(0); + let suppression = Suppression::from_line(line, offset).unwrap(); + + assert_eq!(suppression.kind, SuppressionKind::Line); + assert_eq!( + suppression.rule_specifier, + RuleSpecifier::Group("lint".to_string(), "safety".to_string()) + ); + assert_eq!(suppression.explanation.as_deref(), Some("explanation")); + } + + #[test] + fn test_suppression_from_line_category() { + let line = "-- pgt-ignore lint"; + let offset = &TextSize::new(0); + let suppression = Suppression::from_line(line, offset).unwrap(); + + assert_eq!(suppression.kind, SuppressionKind::Line); + assert_eq!( + suppression.rule_specifier, + RuleSpecifier::Category("lint".to_string()) + ); + } + + #[test] + fn test_suppression_from_line_category_with_explanation() { + let line = "-- pgt-ignore lint: explanation"; + let offset = &TextSize::new(0); + let suppression = Suppression::from_line(line, offset).unwrap(); + + assert_eq!(suppression.kind, SuppressionKind::Line); + assert_eq!( + suppression.rule_specifier, + RuleSpecifier::Category("lint".to_string()) + ); + assert_eq!(suppression.explanation.as_deref(), Some("explanation")); + } + + #[test] + fn test_suppression_from_line_file_kind() { + let line = "-- pgt-ignore-all lint/safety/banDropColumn: explanation"; + let offset = &TextSize::new(0); + let suppression = Suppression::from_line(line, offset).unwrap(); + + assert_eq!(suppression.kind, SuppressionKind::File); + assert_eq!( + suppression.rule_specifier, + RuleSpecifier::Rule( + "lint".to_string(), + "safety".to_string(), + "banDropColumn".to_string() + ) + ); + assert_eq!(suppression.explanation.as_deref(), Some("explanation")); + } + + #[test] + fn test_suppression_from_line_start_kind() { + let line = "-- pgt-ignore-start lint/safety/banDropColumn: explanation"; + let offset = &TextSize::new(0); + let suppression = Suppression::from_line(line, offset).unwrap(); + + assert_eq!(suppression.kind, SuppressionKind::Start); + assert_eq!( + suppression.rule_specifier, + RuleSpecifier::Rule( + "lint".to_string(), + "safety".to_string(), + "banDropColumn".to_string() + ) + ); + assert_eq!(suppression.explanation.as_deref(), Some("explanation")); + } + + #[test] + fn test_suppression_from_line_end_kind() { + let line = "-- pgt-ignore-end lint/safety/banDropColumn: explanation"; + let offset = &TextSize::new(0); + let suppression = Suppression::from_line(line, offset).unwrap(); + + assert_eq!(suppression.kind, SuppressionKind::End); + assert_eq!( + suppression.rule_specifier, + RuleSpecifier::Rule( + "lint".to_string(), + "safety".to_string(), + "banDropColumn".to_string() + ) + ); + assert_eq!(suppression.explanation.as_deref(), Some("explanation")); + } + + #[test] + fn test_suppression_span_with_offset() { + let line = " \n-- pgt-ignore lint/safety/banDropColumn: explanation"; + let offset = TextSize::new(5); + let suppression = Suppression::from_line(line, &offset).unwrap(); + + let expected_start = offset + TextSize::new(5); + let expected_len = TextSize::new(line.trim_ascii().len() as u32); + + let expected_end = expected_start + expected_len; + let expected_span = TextRange::new(expected_start, expected_end); + + assert_eq!(suppression.suppression_range, expected_span); + } + + #[test] + fn test_suppression_from_line_invalid_tag_and_missing_specifier() { + let lines = vec![ + "-- pgt-ignore-foo lint/safety/banDropColumn: explanation", + "-- pgt-ignore foo lint/safety/banDropColumn: explanation", + "-- pgt-ignore xyz lint/safety/banDropColumn: explanation", + "-- pgt-ignore", + ]; + let offset = &TextSize::new(0); + for line in lines { + let result = Suppression::from_line(line, offset); + assert!(result.is_err(), "Expected error for line: {}", line); + } + } + + #[test] + fn test_suppression_matches() { + let cases = vec![ + // the category works for all groups & rules + ("-- pgt-ignore lint", "lint/safety/banDropNotNull", true), + ("-- pgt-ignore lint", "lint/safety/banDropColumn", true), + // the group works for all rules in that group + ( + "-- pgt-ignore lint/safety", + "lint/safety/banDropColumn", + true, + ), + ("-- pgt-ignore lint", "typecheck", false), + ("-- pgt-ignore lint/safety", "typecheck", false), + // a specific supppression only works for that same rule + ( + "-- pgt-ignore lint/safety/banDropColumn", + "lint/safety/banDropColumn", + true, + ), + ( + "-- pgt-ignore lint/safety/banDropColumn", + "lint/safety/banDropTable", + false, + ), + ]; + + let offset = &TextSize::new(0); + + for (suppr_line, specifier_str, expected) in cases { + let suppression = Suppression::from_line(suppr_line, offset).unwrap(); + let specifier = RuleSpecifier::try_from(specifier_str).unwrap(); + assert_eq!( + suppression.matches(&specifier), + expected, + "Suppression line '{}' vs specifier '{}' should be {}", + suppr_line, + specifier_str, + expected + ); + } + } + + #[test] + fn test_rule_specifier_is_disabled() { + use pgt_analyse::RuleFilter; + + // Group filter disables all rules in that group + let spec = RuleSpecifier::Rule( + "lint".to_string(), + "safety".to_string(), + "banDropColumn".to_string(), + ); + let disabled = vec![RuleFilter::Group("safety")]; + assert!(spec.is_disabled(&disabled)); + + let spec2 = RuleSpecifier::Rule( + "lint".to_string(), + "safety".to_string(), + "banDropColumn".to_string(), + ); + let disabled2 = vec![RuleFilter::Rule("safety", "banDropColumn")]; + assert!(spec2.is_disabled(&disabled2)); + + let disabled3 = vec![RuleFilter::Rule("safety", "otherRule")]; + assert!(!spec2.is_disabled(&disabled3)); + + let disabled4 = vec![RuleFilter::Group("perf")]; + assert!(!spec.is_disabled(&disabled4)); + + // one match is enough + let disabled5 = vec![ + RuleFilter::Group("perf"), + RuleFilter::Rule("safety", "banDropColumn"), + ]; + assert!(spec.is_disabled(&disabled5)); + } +} diff --git a/crates/pgt_workspace/Cargo.toml b/crates/pgt_workspace/Cargo.toml index bfa413e3..6b0cc065 100644 --- a/crates/pgt_workspace/Cargo.toml +++ b/crates/pgt_workspace/Cargo.toml @@ -29,6 +29,7 @@ pgt_lexer = { workspace = true } pgt_query_ext = { workspace = true } pgt_schema_cache = { workspace = true } pgt_statement_splitter = { workspace = true } +pgt_suppressions = { workspace = true } pgt_text_size.workspace = true pgt_typecheck = { workspace = true } rustc-hash = { workspace = true } diff --git a/crates/pgt_workspace/src/workspace/server.rs b/crates/pgt_workspace/src/workspace/server.rs index d0c8d13a..5f2a9ebf 100644 --- a/crates/pgt_workspace/src/workspace/server.rs +++ b/crates/pgt_workspace/src/workspace/server.rs @@ -570,6 +570,26 @@ impl Workspace for WorkspaceServer { }, )); + let suppressions = parser.document_suppressions(); + + let disabled_suppression_errors = + suppressions.get_disabled_diagnostic_suppressions_as_errors(&disabled_rules); + + let unused_suppression_errors = + suppressions.get_unused_suppressions_as_errors(&diagnostics); + + let suppression_errors: Vec = suppressions + .diagnostics + .iter() + .chain(disabled_suppression_errors.iter()) + .chain(unused_suppression_errors.iter()) + .cloned() + .map(Error::from) + .collect::>(); + + diagnostics.retain(|d| !suppressions.is_suppressed(d)); + diagnostics.extend(suppression_errors.into_iter().map(SDiagnostic::new)); + let errors = diagnostics .iter() .filter(|d| d.severity() == Severity::Error || d.severity() == Severity::Fatal) diff --git a/crates/pgt_workspace/src/workspace/server/analyser.rs b/crates/pgt_workspace/src/workspace/server/analyser.rs index 4defc79e..86e3d076 100644 --- a/crates/pgt_workspace/src/workspace/server/analyser.rs +++ b/crates/pgt_workspace/src/workspace/server/analyser.rs @@ -75,6 +75,7 @@ impl<'a, 'b> LintVisitor<'a, 'b> { .as_linter_rules() .map(|rules| rules.as_enabled_rules()) .unwrap_or_default(); + self.enabled_rules.extend(enabled_rules); let disabled_rules = self diff --git a/crates/pgt_workspace/src/workspace/server/change.rs b/crates/pgt_workspace/src/workspace/server/change.rs index cc455134..d4a39aa3 100644 --- a/crates/pgt_workspace/src/workspace/server/change.rs +++ b/crates/pgt_workspace/src/workspace/server/change.rs @@ -1,3 +1,4 @@ +use pgt_suppressions::Suppressions; use pgt_text_size::{TextLen, TextRange, TextSize}; use std::ops::{Add, Sub}; @@ -85,6 +86,7 @@ impl Document { } self.version = change.version; + self.suppressions = Suppressions::from(self.content.as_str()); changes } diff --git a/crates/pgt_workspace/src/workspace/server/document.rs b/crates/pgt_workspace/src/workspace/server/document.rs index 89516b23..28514f51 100644 --- a/crates/pgt_workspace/src/workspace/server/document.rs +++ b/crates/pgt_workspace/src/workspace/server/document.rs @@ -1,4 +1,5 @@ use pgt_diagnostics::{Diagnostic, DiagnosticExt, Severity, serde::Diagnostic as SDiagnostic}; +use pgt_suppressions::Suppressions; use pgt_text_size::{TextRange, TextSize}; use super::statement_identifier::{StatementId, StatementIdGenerator}; @@ -10,6 +11,8 @@ pub(crate) struct Document { pub(crate) version: i32, pub(super) diagnostics: Vec, + pub(super) suppressions: Suppressions, + /// List of statements sorted by range.start() pub(super) positions: Vec, @@ -22,6 +25,8 @@ impl Document { let (ranges, diagnostics) = split_with_diagnostics(&content, None); + let suppressions = Suppressions::from(content.as_str()); + Self { positions: ranges .into_iter() @@ -31,6 +36,7 @@ impl Document { version, diagnostics, id_generator, + suppressions, } } diff --git a/crates/pgt_workspace/src/workspace/server/parsed_document.rs b/crates/pgt_workspace/src/workspace/server/parsed_document.rs index 2b81faba..8b515128 100644 --- a/crates/pgt_workspace/src/workspace/server/parsed_document.rs +++ b/crates/pgt_workspace/src/workspace/server/parsed_document.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use pgt_diagnostics::serde::Diagnostic as SDiagnostic; use pgt_fs::PgTPath; use pgt_query_ext::diagnostics::SyntaxDiagnostic; +use pgt_suppressions::Suppressions; use pgt_text_size::{TextRange, TextSize}; use crate::workspace::ChangeFileParams; @@ -98,6 +99,10 @@ impl ParsedDocument { &self.doc.diagnostics } + pub fn document_suppressions(&self) -> &Suppressions { + &self.doc.suppressions + } + pub fn find<'a, M>(&'a self, id: StatementId, mapper: M) -> Option where M: StatementMapper<'a>,