From 8a8b4c6c21e9314dc3712e93f8e050ba490b0dbf Mon Sep 17 00:00:00 2001 From: Julian Date: Wed, 25 Jun 2025 09:12:01 +0200 Subject: [PATCH 01/20] new crate --- Cargo.lock | 4 ++++ crates/pgt_suppressions/Cargo.toml | 14 ++++++++++++++ crates/pgt_suppressions/src/lib.rs | 14 ++++++++++++++ 3 files changed, 32 insertions(+) create mode 100644 crates/pgt_suppressions/Cargo.toml create mode 100644 crates/pgt_suppressions/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 41f807d1..1eec7fc2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2807,6 +2807,10 @@ dependencies = [ "regex", ] +[[package]] +name = "pgt_suppressions" +version = "0.1.0" + [[package]] name = "pgt_test_macros" version = "0.0.0" diff --git a/crates/pgt_suppressions/Cargo.toml b/crates/pgt_suppressions/Cargo.toml new file mode 100644 index 00000000..bbf29e5e --- /dev/null +++ b/crates/pgt_suppressions/Cargo.toml @@ -0,0 +1,14 @@ + +[package] +authors.workspace = true +categories.workspace = true +description = "" +edition.workspace = true +homepage.workspace = true +keywords.workspace = true +license.workspace = true +name = "pgt_suppressions" +repository.workspace = true +version = "0.0.0" + + diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs new file mode 100644 index 00000000..b93cf3ff --- /dev/null +++ b/crates/pgt_suppressions/src/lib.rs @@ -0,0 +1,14 @@ +pub fn add(left: u64, right: u64) -> u64 { + left + right +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn it_works() { + let result = add(2, 2); + assert_eq!(result, 4); + } +} From 3a0bd0347c624d01f499b517af3351781866cb6a Mon Sep 17 00:00:00 2001 From: Julian Date: Mon, 30 Jun 2025 18:51:17 +0200 Subject: [PATCH 02/20] feat: suppressions --- Cargo.lock | 8 +- Cargo.toml | 1 + crates/pgt_analyse/src/categories.rs | 21 + crates/pgt_diagnostics/src/location.rs | 4 +- crates/pgt_suppressions/Cargo.toml | 5 + crates/pgt_suppressions/src/lib.rs | 601 +++++++++++++++++- crates/pgt_suppressions/src/line_index.rs | 37 ++ crates/pgt_workspace/Cargo.toml | 1 + crates/pgt_workspace/src/workspace/server.rs | 13 + .../src/workspace/server/change.rs | 2 + .../src/workspace/server/document.rs | 6 + .../src/workspace/server/parsed_document.rs | 5 + 12 files changed, 692 insertions(+), 12 deletions(-) create mode 100644 crates/pgt_suppressions/src/line_index.rs diff --git a/Cargo.lock b/Cargo.lock index 1eec7fc2..0ebf8a45 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2809,7 +2809,12 @@ dependencies = [ [[package]] name = "pgt_suppressions" -version = "0.1.0" +version = "0.0.0" +dependencies = [ + "pgt_analyse", + "pgt_diagnostics", + "pgt_text_size", +] [[package]] name = "pgt_test_macros" @@ -2913,6 +2918,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 fe00d7ca..404f3aa4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,6 +76,7 @@ pgt_query_ext_codegen = { path = "./crates/pgt_query_ext_codegen", version 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_treesitter_queries = { path = "./crates/pgt_treesitter_queries", 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 index bbf29e5e..bc7264c0 100644 --- a/crates/pgt_suppressions/Cargo.toml +++ b/crates/pgt_suppressions/Cargo.toml @@ -11,4 +11,9 @@ name = "pgt_suppressions" repository.workspace = true version = "0.0.0" +[dependencies] +pgt_text_size = {workspace = true } +pgt_diagnostics = {workspace = true } +pgt_analyse = {workspace = true } + diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index b93cf3ff..e0baa645 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -1,14 +1,597 @@ -pub fn add(left: u64, right: u64) -> u64 { - left + right +use std::{ + iter::{Enumerate, Peekable}, + str::Lines, +}; + +use pgt_analyse::RuleCategory; +use pgt_diagnostics::{Diagnostic, Location, MessageAndDescription}; +use pgt_text_size::{TextRange, TextSize}; + +pub mod line_index; + +use line_index::LineIndex; + +/// A specialized diagnostic for the typechecker. +/// +/// Type diagnostics are always **errors**. +#[derive(Clone, Debug, Diagnostic)] +#[diagnostic(category = "lint", severity = Warning)] +pub struct SuppressionDiagnostic { + #[location(span)] + span: TextRange, + #[description] + #[message] + message: MessageAndDescription, +} + +#[derive(Debug)] +pub enum SuppressionKind { + File, + Line, + Start, + End, +} + +#[derive(Debug, PartialEq)] +enum RuleSpecifier { + Category(RuleCategory), + Group(RuleCategory, String), + Rule(RuleCategory, String, String), +} + +impl RuleSpecifier { + fn category(&self) -> &RuleCategory { + match self { + RuleSpecifier::Category(rule_category) => rule_category, + RuleSpecifier::Group(rule_category, _) => rule_category, + RuleSpecifier::Rule(rule_category, _, _) => rule_category, + } + } + + fn group(&self) -> Option<&str> { + match self { + RuleSpecifier::Category(_) => None, + RuleSpecifier::Group(_, gr) => Some(gr), + RuleSpecifier::Rule(_, gr, _) => Some(gr), + } + } + + fn rule(&self) -> Option<&str> { + match self { + RuleSpecifier::Rule(_, _, ru) => Some(ru), + _ => None, + } + } } -#[cfg(test)] -mod tests { - use super::*; +impl TryFrom<&str> for RuleSpecifier { + type Error = String; + + fn try_from(specifier_str: &str) -> Result { + let mut specifiers = specifier_str.split('/').map(|s| s.to_string()); + + let rule_category: RuleCategory = specifiers.next().unwrap().try_into()?; - #[test] - fn it_works() { - let result = add(2, 2); - assert_eq!(result, 4); + let group = specifiers.next(); + let rule = specifiers.next(); + + match (group, rule) { + (Some(g), Some(r)) => Ok(RuleSpecifier::Rule(rule_category, g, r)), + (Some(g), None) => Ok(RuleSpecifier::Group(rule_category, g)), + (None, None) => Ok(RuleSpecifier::Category(rule_category)), + _ => unreachable!(), + } } } + +#[derive(Debug)] +pub struct Suppression { + suppression_range: TextRange, + /// `None` means that all categories are suppressed + kind: SuppressionKind, + rule_specifier: RuleSpecifier, + explanation: Option, +} + +impl Suppression { + 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(format!( + "You must specify which lints to suppress." + )), + }); + } + }; + + 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()), + }) + } + + 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 + } +} + +#[derive(Debug)] +pub struct RangeSuppression { + suppressed_range: TextRange, + start_suppression: Suppression, +} + +type Line = usize; + +#[derive(Debug, Default)] +pub struct Suppressions { + file_suppressions: Vec, + line_suppressions: std::collections::HashMap, + range_suppressions: Vec, + pub diagnostics: Vec, + line_index: LineIndex, +} + +impl Suppressions { + pub fn is_suppressed(&self, diagnostic: &D) -> bool { + let location = diagnostic.location(); + + diagnostic + .category() + .map(|c| match RuleSpecifier::try_from(c.name()) { + Ok(specifier) => { + self.by_file_suppression(&specifier) + || self.by_range_suppression(location, &specifier) + || self.by_line_suppression(location, &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, location: Location<'_>, specifier: &RuleSpecifier) -> bool { + location + .span + .and_then(|span| self.line_index.line_for_offset(span.start())) + .filter(|line_no| *line_no > 0) + .is_some_and(|mut line_no| { + let mut eligible_suppressions = vec![]; + + // one-for-one, we're checking the lines above a diagnostic location + // until there are no more diagnostics + line_no -= 1; + while let Some(suppr) = self.line_suppressions.get(&line_no) { + eligible_suppressions.push(suppr); + line_no -= 1; + } + + eligible_suppressions.iter().any(|s| s.matches(specifier)) + }) + } + + fn by_range_suppression(&self, location: Location<'_>, specifier: &RuleSpecifier) -> bool { + self.range_suppressions.iter().any(|range_suppr| { + range_suppr.start_suppression.matches(specifier) + && location + .span + .is_some_and(|sp| range_suppr.suppressed_range.contains_range(sp)) + }) + } +} + +#[derive(Debug)] +pub 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: LineIndex::new(doc), + } + } + + /// Will parse the suppressions at the start of the file. + /// As soon as anything is encountered that's not a `pgt-ignore-all` + /// suppression, this will stop. + fn parse_file_suppressions(&mut self) { + while let Some((_, preview)) = self.lines.peek() { + 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) { + while let Some((idx, line)) = self.lines.next() { + 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(format!( + "File suppressions should be at the top of the file." + )), + }); + } + + 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_mut() + .rev() + .position(|start| start.rule_specifier == suppr.rule_specifier); + + 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(format!( + "This end suppression does not have a matching start." + )), + }); + } + } + } + } + } + + /// 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::replace(&mut self.start_suppressions_stack, vec![]); + + for suppr in start_suppressions { + self.diagnostics.push(SuppressionDiagnostic { + span: suppr.suppression_range, + message: MessageAndDescription::from(format!( + "This start suppression does not have a matching end." + )), + }); + } + } +} + +// #[cfg(test)] +// mod tests { +// use super::*; + +// fn parse(doc: &str) -> Suppressions { +// SuppressionsParser::parse(doc) +// } + +// #[test] +// fn test_ignore_with_extra_colons_in_explanation() { +// let doc = "// pgt-ignore lint/safety: reason: with: colons"; +// let sups = parse(doc); +// let suppr = sups.line_suppressions.values().next().unwrap(); +// assert_eq!(suppr.explanation, Some("reason: with: colons")); +// } + +// #[test] +// fn test_ignore_with_trailing_whitespace() { +// let doc = "// pgt-ignore lint/safety "; +// let sups = parse(doc); +// assert_eq!(sups.line_suppressions.len(), 1); +// assert!(sups.diagnostics.is_empty()); +// } + +// #[test] +// fn test_ignore_with_leading_whitespace() { +// let doc = " // pgt-ignore lint/safety"; +// let sups = parse(doc); +// assert_eq!(sups.line_suppressions.len(), 1); +// assert!(sups.diagnostics.is_empty()); +// } + +// #[test] +// fn test_multiple_unmatched_ends() { +// let doc = r#" +// // pgt-ignore-end lint/safety +// // pgt-ignore-end lint/performance +// "#; +// let sups = parse(doc); +// assert_eq!(sups.diagnostics.len(), 2); +// for diag in sups.diagnostics { +// assert!( +// diag.message +// .to_string() +// .contains("does not have a matching start") +// ); +// } +// } + +// #[test] +// fn test_multiple_unmatched_starts() { +// let doc = r#" +// // pgt-ignore-start lint/safety +// // pgt-ignore-start lint/performance +// "#; +// let sups = parse(doc); +// assert_eq!(sups.diagnostics.len(), 2); +// for diag in sups.diagnostics { +// assert!( +// diag.message +// .to_string() +// .contains("does not have a matching end") +// ); +// } +// } + +// #[test] +// fn test_ignore_with_invalid_tag_and_valid_tag() { +// let doc = r#" +// // pgt-ignore-foo lint/safety +// // pgt-ignore lint/safety +// "#; +// let sups = parse(doc); +// assert_eq!(sups.diagnostics.len(), 1); +// assert_eq!(sups.line_suppressions.len(), 1); +// } + +// #[test] +// fn test_ignore_with_missing_category_and_valid_tag() { +// let doc = r#" +// // pgt-ignore +// // pgt-ignore lint/safety +// "#; +// let sups = parse(doc); +// assert_eq!(sups.diagnostics.len(), 1); +// assert_eq!(sups.line_suppressions.len(), 1); +// } + +// #[test] +// fn test_ignore_with_group_and_rule_and_explanation() { +// let doc = "// pgt-ignore lint/safety/banDropColumn: explanation"; +// let sups = parse(doc); +// let suppr = sups.line_suppressions.values().next().unwrap(); +// assert_eq!(suppr.explanation, Some("explanation")); +// match suppr.rule_filter { +// Some(RuleFilter::Rule(group, rule)) => { +// assert_eq!(group, "safety"); +// assert_eq!(rule, "banDropColumn"); +// } +// _ => panic!("Expected RuleFilter::Rule"), +// } +// } + +// #[test] +// fn test_ignore_with_group_only_and_explanation() { +// let doc = "// pgt-ignore lint/safety: explanation"; +// let sups = parse(doc); +// let suppr = sups.line_suppressions.values().next().unwrap(); +// assert_eq!(suppr.explanation, Some("explanation")); +// match suppr.rule_filter { +// Some(RuleFilter::Group(group)) => { +// assert_eq!(group, "safety"); +// } +// _ => panic!("Expected RuleFilter::Group"), +// } +// } + +// #[test] +// fn test_ignore_with_no_group_or_rule_and_explanation() { +// let doc = "// pgt-ignore lint: explanation"; +// let sups = parse(doc); +// let suppr = sups.line_suppressions.values().next().unwrap(); +// assert_eq!(suppr.explanation, Some("explanation")); +// assert!(suppr.rule_filter.is_none()); +// } + +// #[test] +// fn test_ignore_with_empty_explanation() { +// let doc = "// pgt-ignore lint/safety:"; +// let sups = parse(doc); +// let suppr = sups.line_suppressions.values().next().unwrap(); +// assert_eq!(suppr.explanation, Some("")); +// } + +// #[test] +// fn test_ignore_with_multiple_colons_and_spaces() { +// let doc = "// pgt-ignore lint/safety: explanation: with spaces "; +// let sups = parse(doc); +// let suppr = sups.line_suppressions.values().next().unwrap(); +// assert_eq!(suppr.explanation, Some("explanation: with spaces")); +// } + +// #[test] +// fn test_ignore_with_invalid_category() { +// let doc = "// pgt-ignore foo/safety"; +// let sups = parse(doc); +// assert_eq!(sups.line_suppressions.len(), 0); +// assert_eq!(sups.diagnostics.len(), 1); +// let diag = &sups.diagnostics[0]; +// assert_eq!(diag.message.to_string(), "Invalid Rule Category: foo"); +// } + +// #[test] +// fn test_ignore_with_missing_specifier() { +// let doc = "// pgt-ignore"; +// let sups = parse(doc); +// assert_eq!(sups.line_suppressions.len(), 0); +// assert_eq!(sups.diagnostics.len(), 1); +// let diag = &sups.diagnostics[0]; +// assert!( +// diag.message +// .to_string() +// .contains("must specify which lints to suppress") +// || diag.message.to_string().contains("must specify") +// ); +// } + +// #[test] +// fn test_range_suppression_basic() { +// let doc = r#" +// // pgt-ignore-start lint/safety/banDropColumn: start explanation +// SELECT * FROM foo; +// // pgt-ignore-end lint/safety/banDropColumn: end explanation +// "#; +// let sups = parse(doc); +// // Should have one range suppression +// assert_eq!(sups.range_suppressions.len(), 1); +// let range = &sups.range_suppressions[0]; +// assert_eq!(range.rule_category, RuleCategory::Lint); +// assert_eq!( +// range.rule_filter, +// Some(RuleFilter::Rule("safety", "banDropColumn")) +// ); +// assert_eq!(range.explanation, Some("start explanation")); +// // The start and end suppressions should be present and correct +// assert_eq!( +// range.start_suppression.explanation, +// Some("start explanation") +// ); +// assert_eq!(range.end_suppression.explanation, Some("end explanation")); +// } +// } diff --git a/crates/pgt_suppressions/src/line_index.rs b/crates/pgt_suppressions/src/line_index.rs new file mode 100644 index 00000000..e916f7c5 --- /dev/null +++ b/crates/pgt_suppressions/src/line_index.rs @@ -0,0 +1,37 @@ +use pgt_text_size::TextSize; + +#[derive(Debug, Default)] +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.iter().skip(idx).next() + } + + pub fn line_for_offset(&self, offset: TextSize) -> Option { + self.line_offset + .iter() + .enumerate() + .find(|(_, line_offset)| **line_offset >= offset) + .map(|(i, _)| i) + } +} diff --git a/crates/pgt_workspace/Cargo.toml b/crates/pgt_workspace/Cargo.toml index bfa413e3..022c93e3 100644 --- a/crates/pgt_workspace/Cargo.toml +++ b/crates/pgt_workspace/Cargo.toml @@ -24,6 +24,7 @@ pgt_completions = { workspace = true } pgt_configuration = { workspace = true } pgt_console = { workspace = true } pgt_diagnostics = { workspace = true } +pgt_suppressions = { workspace = true } pgt_fs = { workspace = true, features = ["serde"] } pgt_lexer = { workspace = true } pgt_query_ext = { workspace = true } diff --git a/crates/pgt_workspace/src/workspace/server.rs b/crates/pgt_workspace/src/workspace/server.rs index d0c8d13a..a2379b14 100644 --- a/crates/pgt_workspace/src/workspace/server.rs +++ b/crates/pgt_workspace/src/workspace/server.rs @@ -570,6 +570,19 @@ impl Workspace for WorkspaceServer { }, )); + let suppressions = parser.document_suppressions(); + + diagnostics.retain(|d| !suppressions.is_suppressed(d)); + + let suppression_errors: Vec = suppressions + .diagnostics + .clone() + .into_iter() + .map(Error::from) + .collect::>(); + + diagnostics.extend(suppression_errors.into_iter().map(|e| SDiagnostic::new(e))); + let errors = diagnostics .iter() .filter(|d| d.severity() == Severity::Error || d.severity() == Severity::Fatal) diff --git a/crates/pgt_workspace/src/workspace/server/change.rs b/crates/pgt_workspace/src/workspace/server/change.rs index 62e3da03..849fbae5 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::SuppressionsParser; use pgt_text_size::{TextLen, TextRange, TextSize}; use std::ops::{Add, Sub}; @@ -85,6 +86,7 @@ impl Document { } self.version = change.version; + self.suppressions = SuppressionsParser::parse(&self.content); changes } diff --git a/crates/pgt_workspace/src/workspace/server/document.rs b/crates/pgt_workspace/src/workspace/server/document.rs index ed0ca40f..37b5ee83 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, SuppressionsParser}; 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 = SuppressionsParser::parse(&content); + 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>, From 4285e6a1f9d96fdf6af7c33d7f93cb6271b89ea7 Mon Sep 17 00:00:00 2001 From: Julian Date: Tue, 1 Jul 2025 19:26:39 +0200 Subject: [PATCH 03/20] ok --- Cargo.lock | 1 + crates/pgt_suppressions/Cargo.toml | 1 + crates/pgt_suppressions/src/lib.rs | 278 +++++------------- crates/pgt_suppressions/src/line_index.rs | 2 +- crates/pgt_workspace/src/workspace/server.rs | 5 +- .../src/workspace/server/analyser.rs | 43 +++ 6 files changed, 128 insertions(+), 202 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0ebf8a45..e662fd6c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2814,6 +2814,7 @@ dependencies = [ "pgt_analyse", "pgt_diagnostics", "pgt_text_size", + "tracing", ] [[package]] diff --git a/crates/pgt_suppressions/Cargo.toml b/crates/pgt_suppressions/Cargo.toml index bc7264c0..b85b7a90 100644 --- a/crates/pgt_suppressions/Cargo.toml +++ b/crates/pgt_suppressions/Cargo.toml @@ -15,5 +15,6 @@ version = "0.0.0" pgt_text_size = {workspace = true } pgt_diagnostics = {workspace = true } pgt_analyse = {workspace = true } +tracing = { workspace = true } diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index e0baa645..0a0b9f75 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -3,7 +3,7 @@ use std::{ str::Lines, }; -use pgt_analyse::RuleCategory; +use pgt_analyse::{RuleCategory, RuleFilter}; use pgt_diagnostics::{Diagnostic, Location, MessageAndDescription}; use pgt_text_size::{TextRange, TextSize}; @@ -24,7 +24,7 @@ pub struct SuppressionDiagnostic { message: MessageAndDescription, } -#[derive(Debug)] +#[derive(Debug, Clone)] pub enum SuppressionKind { File, Line, @@ -32,7 +32,7 @@ pub enum SuppressionKind { End, } -#[derive(Debug, PartialEq)] +#[derive(Debug, PartialEq, Clone)] enum RuleSpecifier { Category(RuleCategory), Group(RuleCategory, String), @@ -62,6 +62,21 @@ impl RuleSpecifier { _ => None, } } + + 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(); + + tracing::error!("group and rule: {:#?} {:#?}", group, 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 TryFrom<&str> for RuleSpecifier { @@ -84,12 +99,12 @@ impl TryFrom<&str> for RuleSpecifier { } } -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct Suppression { suppression_range: TextRange, - /// `None` means that all categories are suppressed kind: SuppressionKind, rule_specifier: RuleSpecifier, + #[allow(unused)] explanation: Option, } @@ -191,9 +206,18 @@ impl Suppression { false } + + fn to_disabled_diagnostic(self) -> SuppressionDiagnostic { + SuppressionDiagnostic { + span: self.suppression_range, + message: MessageAndDescription::from(format!( + "This rule has been disabled via the configuration. The suppression has no effect." + )), + } + } } -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct RangeSuppression { suppressed_range: TextRange, start_suppression: Suppression, @@ -201,7 +225,7 @@ pub struct RangeSuppression { type Line = usize; -#[derive(Debug, Default)] +#[derive(Debug, Default, Clone)] pub struct Suppressions { file_suppressions: Vec, line_suppressions: std::collections::HashMap, @@ -211,6 +235,53 @@ pub struct Suppressions { } impl Suppressions { + pub fn considering_disabled_rules(mut self, disabled_rules: &[RuleFilter<'_>]) -> Self { + tracing::error!("disabled rules: {:#?}", disabled_rules); + + { + let (enabled, disabled) = self + .file_suppressions + .into_iter() + .partition(|s| !s.rule_specifier.is_disabled(disabled_rules)); + + self.file_suppressions = enabled; + + for suppr in disabled { + self.diagnostics.push(suppr.to_disabled_diagnostic()); + } + } + + { + let (enabled, disabled) = self + .line_suppressions + .into_iter() + .partition(|(_, s)| !s.rule_specifier.is_disabled(disabled_rules)); + + self.line_suppressions = enabled; + + for (_, suppr) in disabled { + self.diagnostics.push(suppr.to_disabled_diagnostic()); + } + } + + { + let (enabled, disabled) = self.range_suppressions.into_iter().partition(|s| { + !s.start_suppression + .rule_specifier + .is_disabled(disabled_rules) + }); + + self.range_suppressions = enabled; + + for range_suppr in disabled { + self.diagnostics + .push(range_suppr.start_suppression.to_disabled_diagnostic()); + } + } + + self + } + pub fn is_suppressed(&self, diagnostic: &D) -> bool { let location = diagnostic.location(); @@ -402,196 +473,3 @@ impl<'a> SuppressionsParser<'a> { } } } - -// #[cfg(test)] -// mod tests { -// use super::*; - -// fn parse(doc: &str) -> Suppressions { -// SuppressionsParser::parse(doc) -// } - -// #[test] -// fn test_ignore_with_extra_colons_in_explanation() { -// let doc = "// pgt-ignore lint/safety: reason: with: colons"; -// let sups = parse(doc); -// let suppr = sups.line_suppressions.values().next().unwrap(); -// assert_eq!(suppr.explanation, Some("reason: with: colons")); -// } - -// #[test] -// fn test_ignore_with_trailing_whitespace() { -// let doc = "// pgt-ignore lint/safety "; -// let sups = parse(doc); -// assert_eq!(sups.line_suppressions.len(), 1); -// assert!(sups.diagnostics.is_empty()); -// } - -// #[test] -// fn test_ignore_with_leading_whitespace() { -// let doc = " // pgt-ignore lint/safety"; -// let sups = parse(doc); -// assert_eq!(sups.line_suppressions.len(), 1); -// assert!(sups.diagnostics.is_empty()); -// } - -// #[test] -// fn test_multiple_unmatched_ends() { -// let doc = r#" -// // pgt-ignore-end lint/safety -// // pgt-ignore-end lint/performance -// "#; -// let sups = parse(doc); -// assert_eq!(sups.diagnostics.len(), 2); -// for diag in sups.diagnostics { -// assert!( -// diag.message -// .to_string() -// .contains("does not have a matching start") -// ); -// } -// } - -// #[test] -// fn test_multiple_unmatched_starts() { -// let doc = r#" -// // pgt-ignore-start lint/safety -// // pgt-ignore-start lint/performance -// "#; -// let sups = parse(doc); -// assert_eq!(sups.diagnostics.len(), 2); -// for diag in sups.diagnostics { -// assert!( -// diag.message -// .to_string() -// .contains("does not have a matching end") -// ); -// } -// } - -// #[test] -// fn test_ignore_with_invalid_tag_and_valid_tag() { -// let doc = r#" -// // pgt-ignore-foo lint/safety -// // pgt-ignore lint/safety -// "#; -// let sups = parse(doc); -// assert_eq!(sups.diagnostics.len(), 1); -// assert_eq!(sups.line_suppressions.len(), 1); -// } - -// #[test] -// fn test_ignore_with_missing_category_and_valid_tag() { -// let doc = r#" -// // pgt-ignore -// // pgt-ignore lint/safety -// "#; -// let sups = parse(doc); -// assert_eq!(sups.diagnostics.len(), 1); -// assert_eq!(sups.line_suppressions.len(), 1); -// } - -// #[test] -// fn test_ignore_with_group_and_rule_and_explanation() { -// let doc = "// pgt-ignore lint/safety/banDropColumn: explanation"; -// let sups = parse(doc); -// let suppr = sups.line_suppressions.values().next().unwrap(); -// assert_eq!(suppr.explanation, Some("explanation")); -// match suppr.rule_filter { -// Some(RuleFilter::Rule(group, rule)) => { -// assert_eq!(group, "safety"); -// assert_eq!(rule, "banDropColumn"); -// } -// _ => panic!("Expected RuleFilter::Rule"), -// } -// } - -// #[test] -// fn test_ignore_with_group_only_and_explanation() { -// let doc = "// pgt-ignore lint/safety: explanation"; -// let sups = parse(doc); -// let suppr = sups.line_suppressions.values().next().unwrap(); -// assert_eq!(suppr.explanation, Some("explanation")); -// match suppr.rule_filter { -// Some(RuleFilter::Group(group)) => { -// assert_eq!(group, "safety"); -// } -// _ => panic!("Expected RuleFilter::Group"), -// } -// } - -// #[test] -// fn test_ignore_with_no_group_or_rule_and_explanation() { -// let doc = "// pgt-ignore lint: explanation"; -// let sups = parse(doc); -// let suppr = sups.line_suppressions.values().next().unwrap(); -// assert_eq!(suppr.explanation, Some("explanation")); -// assert!(suppr.rule_filter.is_none()); -// } - -// #[test] -// fn test_ignore_with_empty_explanation() { -// let doc = "// pgt-ignore lint/safety:"; -// let sups = parse(doc); -// let suppr = sups.line_suppressions.values().next().unwrap(); -// assert_eq!(suppr.explanation, Some("")); -// } - -// #[test] -// fn test_ignore_with_multiple_colons_and_spaces() { -// let doc = "// pgt-ignore lint/safety: explanation: with spaces "; -// let sups = parse(doc); -// let suppr = sups.line_suppressions.values().next().unwrap(); -// assert_eq!(suppr.explanation, Some("explanation: with spaces")); -// } - -// #[test] -// fn test_ignore_with_invalid_category() { -// let doc = "// pgt-ignore foo/safety"; -// let sups = parse(doc); -// assert_eq!(sups.line_suppressions.len(), 0); -// assert_eq!(sups.diagnostics.len(), 1); -// let diag = &sups.diagnostics[0]; -// assert_eq!(diag.message.to_string(), "Invalid Rule Category: foo"); -// } - -// #[test] -// fn test_ignore_with_missing_specifier() { -// let doc = "// pgt-ignore"; -// let sups = parse(doc); -// assert_eq!(sups.line_suppressions.len(), 0); -// assert_eq!(sups.diagnostics.len(), 1); -// let diag = &sups.diagnostics[0]; -// assert!( -// diag.message -// .to_string() -// .contains("must specify which lints to suppress") -// || diag.message.to_string().contains("must specify") -// ); -// } - -// #[test] -// fn test_range_suppression_basic() { -// let doc = r#" -// // pgt-ignore-start lint/safety/banDropColumn: start explanation -// SELECT * FROM foo; -// // pgt-ignore-end lint/safety/banDropColumn: end explanation -// "#; -// let sups = parse(doc); -// // Should have one range suppression -// assert_eq!(sups.range_suppressions.len(), 1); -// let range = &sups.range_suppressions[0]; -// assert_eq!(range.rule_category, RuleCategory::Lint); -// assert_eq!( -// range.rule_filter, -// Some(RuleFilter::Rule("safety", "banDropColumn")) -// ); -// assert_eq!(range.explanation, Some("start explanation")); -// // The start and end suppressions should be present and correct -// assert_eq!( -// range.start_suppression.explanation, -// Some("start explanation") -// ); -// assert_eq!(range.end_suppression.explanation, Some("end explanation")); -// } -// } diff --git a/crates/pgt_suppressions/src/line_index.rs b/crates/pgt_suppressions/src/line_index.rs index e916f7c5..f2d30dab 100644 --- a/crates/pgt_suppressions/src/line_index.rs +++ b/crates/pgt_suppressions/src/line_index.rs @@ -1,6 +1,6 @@ use pgt_text_size::TextSize; -#[derive(Debug, Default)] +#[derive(Debug, Default, Clone)] pub(crate) struct LineIndex { line_offset: Vec, } diff --git a/crates/pgt_workspace/src/workspace/server.rs b/crates/pgt_workspace/src/workspace/server.rs index a2379b14..b41753c5 100644 --- a/crates/pgt_workspace/src/workspace/server.rs +++ b/crates/pgt_workspace/src/workspace/server.rs @@ -570,7 +570,10 @@ impl Workspace for WorkspaceServer { }, )); - let suppressions = parser.document_suppressions(); + let suppressions = parser + .document_suppressions() + .clone() + .considering_disabled_rules(&disabled_rules); diagnostics.retain(|d| !suppressions.is_suppressed(d)); diff --git a/crates/pgt_workspace/src/workspace/server/analyser.rs b/crates/pgt_workspace/src/workspace/server/analyser.rs index d4b08ba1..42af7580 100644 --- a/crates/pgt_workspace/src/workspace/server/analyser.rs +++ b/crates/pgt_workspace/src/workspace/server/analyser.rs @@ -68,14 +68,19 @@ impl<'a, 'b> LintVisitor<'a, 'b> { fn finish(mut self) -> (FxHashSet>, FxHashSet>) { let has_only_filter = !self.only.is_empty(); + if !has_only_filter { + println!("does not have only filter!"); + let enabled_rules = self .settings .as_linter_rules() .map(|rules| rules.as_enabled_rules()) .unwrap_or_default(); + self.enabled_rules.extend(enabled_rules); } + (self.enabled_rules, self.disabled_rules) } @@ -127,3 +132,41 @@ impl RegistryVisitor for LintVisitor<'_, '_> { self.push_rule::() } } + +#[cfg(test)] +mod tests { + use pgt_configuration::{RuleConfiguration, Rules, analyser::Safety}; + + use crate::{ + settings::{LinterSettings, Settings}, + workspace::server::analyser::AnalyserVisitorBuilder, + }; + + #[test] + fn identifies_disabled_rules() { + let settings = Settings { + linter: LinterSettings { + rules: Some(Rules { + recommended: Some(true), + safety: Some(Safety { + ban_drop_column: Some(RuleConfiguration::Plain( + pgt_configuration::RulePlainConfiguration::Off, + )), + ..Default::default() + }), + ..Default::default() + }), + + ..Default::default() + }, + ..Default::default() + }; + + let (enabled_rules, disabled_rules) = AnalyserVisitorBuilder::new(&settings) + .with_linter_rules(&[], &[]) + .finish(); + + println!("{:#?}", enabled_rules); + println!("{:#?}", disabled_rules); + } +} From f33bc67dcdce2e18388edaad39ef751ff4e732f9 Mon Sep 17 00:00:00 2001 From: Julian Date: Tue, 1 Jul 2025 19:37:20 +0200 Subject: [PATCH 04/20] ok --- .../src/analyser/linter/rules.rs | 8 +++++++ xtask/codegen/src/generate_configuration.rs | 22 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/crates/pgt_configuration/src/analyser/linter/rules.rs b/crates/pgt_configuration/src/analyser/linter/rules.rs index 8db5a0ab..d45199b0 100644 --- a/crates/pgt_configuration/src/analyser/linter/rules.rs +++ b/crates/pgt_configuration/src/analyser/linter/rules.rs @@ -121,6 +121,14 @@ impl Rules { } enabled_rules.difference(&disabled_rules).copied().collect() } + #[doc = r" It returns the disabled rules by configuration."] + pub fn as_disabled_rules(&self) -> FxHashSet> { + let mut disabled_rules = FxHashSet::default(); + if let Some(group) = self.safety.as_ref() { + disabled_rules.extend(&group.get_disabled_rules()); + } + disabled_rules + } } #[derive(Clone, Debug, Default, Deserialize, Eq, Merge, PartialEq, Serialize)] #[cfg_attr(feature = "schema", derive(JsonSchema))] diff --git a/xtask/codegen/src/generate_configuration.rs b/xtask/codegen/src/generate_configuration.rs index 3799f19b..661f44b5 100644 --- a/xtask/codegen/src/generate_configuration.rs +++ b/xtask/codegen/src/generate_configuration.rs @@ -61,6 +61,8 @@ fn generate_for_groups( let mut group_idents = Vec::with_capacity(groups.len()); let mut group_strings = Vec::with_capacity(groups.len()); let mut group_as_default_rules = Vec::with_capacity(groups.len()); + let mut group_as_disabled_rules = Vec::with_capacity(groups.len()); + for (group, rules) in groups { let group_pascal_ident = quote::format_ident!("{}", &Case::Pascal.convert(group)); let group_ident = quote::format_ident!("{}", group); @@ -95,6 +97,12 @@ fn generate_for_groups( } }); + group_as_disabled_rules.push(quote! { + if let Some(group) = self.#group_ident.as_ref() { + disabled_rules.extend(&group.get_disabled_rules()); + } + }); + group_pascal_idents.push(group_pascal_ident); group_idents.push(group_ident); group_strings.push(Literal::string(group)); @@ -246,6 +254,13 @@ fn generate_for_groups( #( #group_as_default_rules )* enabled_rules } + + /// It returns the disabled rules by configuration. + pub fn as_disabled_rules(&self) -> FxHashSet> { + let mut disabled_rules = FxHashSet::default(); + #( #group_as_disabled_rules )* + disabled_rules + } } #( #struct_groups )* @@ -358,6 +373,13 @@ fn generate_for_groups( enabled_rules.difference(&disabled_rules).copied().collect() } + + /// It returns the disabled rules by configuration. + pub fn as_disabled_rules(&self) -> FxHashSet> { + let mut disabled_rules = FxHashSet::default(); + #( #group_as_disabled_rules )* + disabled_rules + } } #( #struct_groups )* From bc704c1b6fba77d6db2259dc2094a145089fcc42 Mon Sep 17 00:00:00 2001 From: Julian Date: Tue, 1 Jul 2025 19:45:56 +0200 Subject: [PATCH 05/20] fix(analyser): recognize disabled rules --- .../src/workspace/server/analyser.rs | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/crates/pgt_workspace/src/workspace/server/analyser.rs b/crates/pgt_workspace/src/workspace/server/analyser.rs index d4b08ba1..4defc79e 100644 --- a/crates/pgt_workspace/src/workspace/server/analyser.rs +++ b/crates/pgt_workspace/src/workspace/server/analyser.rs @@ -68,6 +68,7 @@ impl<'a, 'b> LintVisitor<'a, 'b> { fn finish(mut self) -> (FxHashSet>, FxHashSet>) { let has_only_filter = !self.only.is_empty(); + if !has_only_filter { let enabled_rules = self .settings @@ -75,7 +76,15 @@ impl<'a, 'b> LintVisitor<'a, 'b> { .map(|rules| rules.as_enabled_rules()) .unwrap_or_default(); self.enabled_rules.extend(enabled_rules); + + let disabled_rules = self + .settings + .as_linter_rules() + .map(|rules| rules.as_disabled_rules()) + .unwrap_or_default(); + self.disabled_rules.extend(disabled_rules); } + (self.enabled_rules, self.disabled_rules) } @@ -127,3 +136,42 @@ impl RegistryVisitor for LintVisitor<'_, '_> { self.push_rule::() } } + +#[cfg(test)] +mod tests { + use pgt_analyse::RuleFilter; + use pgt_configuration::{RuleConfiguration, Rules, analyser::Safety}; + + use crate::{ + settings::{LinterSettings, Settings}, + workspace::server::analyser::AnalyserVisitorBuilder, + }; + + #[test] + fn recognizes_disabled_rules() { + let settings = Settings { + linter: LinterSettings { + rules: Some(Rules { + safety: Some(Safety { + ban_drop_column: Some(RuleConfiguration::Plain( + pgt_configuration::RulePlainConfiguration::Off, + )), + ..Default::default() + }), + ..Default::default() + }), + ..Default::default() + }, + ..Default::default() + }; + + let (_, disabled_rules) = AnalyserVisitorBuilder::new(&settings) + .with_linter_rules(&[], &[]) + .finish(); + + assert_eq!( + disabled_rules, + vec![RuleFilter::Rule("safety", "banDropColumn")] + ) + } +} From 9a793a8d0a141db9c4bd699538d5ecc0cb4a8743 Mon Sep 17 00:00:00 2001 From: Julian Date: Thu, 3 Jul 2025 09:38:44 +0200 Subject: [PATCH 06/20] ok --- crates/pgt_suppressions/src/lib.rs | 114 ++++++++++++++---- crates/pgt_workspace/src/workspace/server.rs | 4 +- .../src/workspace/server/analyser.rs | 2 - 3 files changed, 92 insertions(+), 28 deletions(-) diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index 0a0b9f75..01b9bc51 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -1,10 +1,11 @@ use std::{ + collections::HashMap, iter::{Enumerate, Peekable}, str::Lines, }; use pgt_analyse::{RuleCategory, RuleFilter}; -use pgt_diagnostics::{Diagnostic, Location, MessageAndDescription}; +use pgt_diagnostics::{Diagnostic, MessageAndDescription}; use pgt_text_size::{TextRange, TextSize}; pub mod line_index; @@ -68,8 +69,6 @@ impl RuleSpecifier { let group = self.group(); let rule = self.rule(); - tracing::error!("group and rule: {:#?} {:#?}", group, 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| { @@ -235,9 +234,8 @@ pub struct Suppressions { } impl Suppressions { + #[must_use] pub fn considering_disabled_rules(mut self, disabled_rules: &[RuleFilter<'_>]) -> Self { - tracing::error!("disabled rules: {:#?}", disabled_rules); - { let (enabled, disabled) = self .file_suppressions @@ -282,16 +280,64 @@ impl Suppressions { self } - pub fn is_suppressed(&self, diagnostic: &D) -> bool { - let location = diagnostic.location(); + #[must_use] + pub fn with_unused_suppressions_as_errors(mut self, diagnostics: &[D]) -> Self { + 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]); + } + } + for (line, suppr) in &self.line_suppressions { + let mut expected_diagnostic_line = line + 1; + while let Some(_) = self.line_suppressions.get(&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 { + self.diagnostics.push(SuppressionDiagnostic { + span: suppr.suppression_range, + message: MessageAndDescription::from(format!( + "This suppression has no effect." + )), + }) + } + } + + self + } + + 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(location, &specifier) - || self.by_line_suppression(location, &specifier) + || self.by_range_suppression(diagnostic, &specifier) + || self.by_line_suppression(diagnostic, &specifier) } Err(_) => false, }) @@ -302,33 +348,53 @@ impl Suppressions { self.file_suppressions.iter().any(|s| s.matches(specifier)) } - fn by_line_suppression(&self, location: Location<'_>, specifier: &RuleSpecifier) -> bool { - location + 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) - .is_some_and(|mut line_no| { - let mut eligible_suppressions = vec![]; + .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 diagnostics line_no -= 1; while let Some(suppr) = self.line_suppressions.get(&line_no) { - eligible_suppressions.push(suppr); + eligible.push(suppr); line_no -= 1; } - eligible_suppressions.iter().any(|s| s.matches(specifier)) + eligible }) - } - - fn by_range_suppression(&self, location: Location<'_>, specifier: &RuleSpecifier) -> bool { - self.range_suppressions.iter().any(|range_suppr| { - range_suppr.start_suppression.matches(specifier) - && location - .span - .is_some_and(|sp| range_suppr.suppressed_range.contains_range(sp)) - }) + .unwrap_or(vec![]) } } diff --git a/crates/pgt_workspace/src/workspace/server.rs b/crates/pgt_workspace/src/workspace/server.rs index b41753c5..1a80fa7d 100644 --- a/crates/pgt_workspace/src/workspace/server.rs +++ b/crates/pgt_workspace/src/workspace/server.rs @@ -573,13 +573,13 @@ impl Workspace for WorkspaceServer { let suppressions = parser .document_suppressions() .clone() - .considering_disabled_rules(&disabled_rules); + .considering_disabled_rules(&disabled_rules) + .with_unused_suppressions_as_errors(&diagnostics); diagnostics.retain(|d| !suppressions.is_suppressed(d)); let suppression_errors: Vec = suppressions .diagnostics - .clone() .into_iter() .map(Error::from) .collect::>(); diff --git a/crates/pgt_workspace/src/workspace/server/analyser.rs b/crates/pgt_workspace/src/workspace/server/analyser.rs index 831087ef..86e3d076 100644 --- a/crates/pgt_workspace/src/workspace/server/analyser.rs +++ b/crates/pgt_workspace/src/workspace/server/analyser.rs @@ -70,8 +70,6 @@ impl<'a, 'b> LintVisitor<'a, 'b> { let has_only_filter = !self.only.is_empty(); if !has_only_filter { - println!("does not have only filter!"); - let enabled_rules = self .settings .as_linter_rules() From 3b15864aa1cf7027e3041c4267c71d2936a43f0b Mon Sep 17 00:00:00 2001 From: Julian Date: Thu, 3 Jul 2025 09:45:12 +0200 Subject: [PATCH 07/20] format --- Cargo.toml | 2 +- crates/pgt_suppressions/Cargo.toml | 10 ++++------ crates/pgt_workspace/Cargo.toml | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 404f3aa4..74221d0a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,7 +76,7 @@ pgt_query_ext_codegen = { path = "./crates/pgt_query_ext_codegen", version 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_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_treesitter_queries = { path = "./crates/pgt_treesitter_queries", version = "0.0.0" } diff --git a/crates/pgt_suppressions/Cargo.toml b/crates/pgt_suppressions/Cargo.toml index b85b7a90..54064c6c 100644 --- a/crates/pgt_suppressions/Cargo.toml +++ b/crates/pgt_suppressions/Cargo.toml @@ -12,9 +12,7 @@ repository.workspace = true version = "0.0.0" [dependencies] -pgt_text_size = {workspace = true } -pgt_diagnostics = {workspace = true } -pgt_analyse = {workspace = true } -tracing = { workspace = true } - - +pgt_analyse = { workspace = true } +pgt_diagnostics = { workspace = true } +pgt_text_size = { workspace = true } +tracing = { workspace = true } diff --git a/crates/pgt_workspace/Cargo.toml b/crates/pgt_workspace/Cargo.toml index 022c93e3..6b0cc065 100644 --- a/crates/pgt_workspace/Cargo.toml +++ b/crates/pgt_workspace/Cargo.toml @@ -24,12 +24,12 @@ pgt_completions = { workspace = true } pgt_configuration = { workspace = true } pgt_console = { workspace = true } pgt_diagnostics = { workspace = true } -pgt_suppressions = { workspace = true } pgt_fs = { workspace = true, features = ["serde"] } 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 } From 4fa6de091ccef8ff73a37d02520bd716cc45b807 Mon Sep 17 00:00:00 2001 From: Julian Date: Thu, 3 Jul 2025 09:48:03 +0200 Subject: [PATCH 08/20] linting --- crates/pgt_suppressions/src/lib.rs | 46 +++++++++++--------- crates/pgt_suppressions/src/line_index.rs | 2 +- crates/pgt_workspace/src/workspace/server.rs | 2 +- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index 01b9bc51..765a458d 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -153,9 +153,9 @@ impl Suppression { None => { return Err(SuppressionDiagnostic { span, - message: MessageAndDescription::from(format!( - "You must specify which lints to suppress." - )), + message: MessageAndDescription::from( + "You must specify which lints to suppress.".to_string(), + ), }); } }; @@ -209,9 +209,10 @@ impl Suppression { fn to_disabled_diagnostic(self) -> SuppressionDiagnostic { SuppressionDiagnostic { span: self.suppression_range, - message: MessageAndDescription::from(format!( + message: MessageAndDescription::from( "This rule has been disabled via the configuration. The suppression has no effect." - )), + .to_string(), + ), } } } @@ -300,7 +301,10 @@ impl Suppressions { for (line, suppr) in &self.line_suppressions { let mut expected_diagnostic_line = line + 1; - while let Some(_) = self.line_suppressions.get(&expected_diagnostic_line) { + while self + .line_suppressions + .contains_key(&expected_diagnostic_line) + { expected_diagnostic_line += 1; } @@ -320,9 +324,9 @@ impl Suppressions { } else { self.diagnostics.push(SuppressionDiagnostic { span: suppr.suppression_range, - message: MessageAndDescription::from(format!( - "This suppression has no effect." - )), + message: MessageAndDescription::from( + "This suppression has no effect.".to_string(), + ), }) } } @@ -394,7 +398,7 @@ impl Suppressions { eligible }) - .unwrap_or(vec![]) + .unwrap_or_default() } } @@ -462,7 +466,7 @@ impl<'a> SuppressionsParser<'a> { } fn parse_suppressions(&mut self) { - while let Some((idx, line)) = self.lines.next() { + for (idx, line) in self.lines.by_ref() { if !line.trim().starts_with("-- pgt-ignore") { continue; } @@ -481,9 +485,9 @@ impl<'a> SuppressionsParser<'a> { SuppressionKind::File => { self.diagnostics.push(SuppressionDiagnostic { span: suppr.suppression_range, - message: MessageAndDescription::from(format!( - "File suppressions should be at the top of the file." - )), + message: MessageAndDescription::from( + "File suppressions should be at the top of the file.".to_string(), + ), }); } @@ -514,9 +518,9 @@ impl<'a> SuppressionsParser<'a> { } else { self.diagnostics.push(SuppressionDiagnostic { span: suppr.suppression_range, - message: MessageAndDescription::from(format!( - "This end suppression does not have a matching start." - )), + message: MessageAndDescription::from( + "This end suppression does not have a matching start.".to_string(), + ), }); } } @@ -527,14 +531,14 @@ impl<'a> SuppressionsParser<'a> { /// 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::replace(&mut self.start_suppressions_stack, vec![]); + 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(format!( - "This start suppression does not have a matching end." - )), + message: MessageAndDescription::from( + "This start suppression does not have a matching end.".to_string(), + ), }); } } diff --git a/crates/pgt_suppressions/src/line_index.rs b/crates/pgt_suppressions/src/line_index.rs index f2d30dab..7d7356c6 100644 --- a/crates/pgt_suppressions/src/line_index.rs +++ b/crates/pgt_suppressions/src/line_index.rs @@ -24,7 +24,7 @@ impl LineIndex { } pub fn offset_for_line(&self, idx: usize) -> Option<&pgt_text_size::TextSize> { - self.line_offset.iter().skip(idx).next() + self.line_offset.get(idx) } pub fn line_for_offset(&self, offset: TextSize) -> Option { diff --git a/crates/pgt_workspace/src/workspace/server.rs b/crates/pgt_workspace/src/workspace/server.rs index 1a80fa7d..89a1e05f 100644 --- a/crates/pgt_workspace/src/workspace/server.rs +++ b/crates/pgt_workspace/src/workspace/server.rs @@ -584,7 +584,7 @@ impl Workspace for WorkspaceServer { .map(Error::from) .collect::>(); - diagnostics.extend(suppression_errors.into_iter().map(|e| SDiagnostic::new(e))); + diagnostics.extend(suppression_errors.into_iter().map(SDiagnostic::new)); let errors = diagnostics .iter() From 792d14ca30024eda80038e26f990652bcf837360 Mon Sep 17 00:00:00 2001 From: Julian Date: Thu, 3 Jul 2025 09:50:10 +0200 Subject: [PATCH 09/20] comment, rename --- crates/pgt_suppressions/src/lib.rs | 4 +++- crates/pgt_workspace/src/workspace/server.rs | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index 765a458d..6dc8a0f7 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -235,8 +235,10 @@ pub struct Suppressions { } impl Suppressions { + /// Some diagnostics can be turned off via the configuration. + /// This will mark suppressions that try to suppress these disabled diagnostics as errors. #[must_use] - pub fn considering_disabled_rules(mut self, disabled_rules: &[RuleFilter<'_>]) -> Self { + pub fn with_disabled_rules(mut self, disabled_rules: &[RuleFilter<'_>]) -> Self { { let (enabled, disabled) = self .file_suppressions diff --git a/crates/pgt_workspace/src/workspace/server.rs b/crates/pgt_workspace/src/workspace/server.rs index 89a1e05f..2be99204 100644 --- a/crates/pgt_workspace/src/workspace/server.rs +++ b/crates/pgt_workspace/src/workspace/server.rs @@ -573,7 +573,7 @@ impl Workspace for WorkspaceServer { let suppressions = parser .document_suppressions() .clone() - .considering_disabled_rules(&disabled_rules) + .with_disabled_rules(&disabled_rules) .with_unused_suppressions_as_errors(&diagnostics); diagnostics.retain(|d| !suppressions.is_suppressed(d)); From 7d79543eb318b374e3029d2b9ec96cc58b10ed3a Mon Sep 17 00:00:00 2001 From: Julian Date: Fri, 4 Jul 2025 09:56:08 +0200 Subject: [PATCH 10/20] clean up --- crates/pgt_suppressions/src/lib.rs | 378 +----------------- crates/pgt_suppressions/src/parser.rs | 155 +++++++ crates/pgt_suppressions/src/suppression.rs | 214 ++++++++++ .../src/workspace/server/change.rs | 4 +- .../src/workspace/server/document.rs | 4 +- 5 files changed, 392 insertions(+), 363 deletions(-) create mode 100644 crates/pgt_suppressions/src/parser.rs create mode 100644 crates/pgt_suppressions/src/suppression.rs diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index 6dc8a0f7..dcf98cc2 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -1,227 +1,18 @@ -use std::{ - collections::HashMap, - iter::{Enumerate, Peekable}, - str::Lines, -}; +use std::collections::HashMap; +pub mod parser; +pub mod suppression; -use pgt_analyse::{RuleCategory, RuleFilter}; +use pgt_analyse::RuleFilter; use pgt_diagnostics::{Diagnostic, MessageAndDescription}; -use pgt_text_size::{TextRange, TextSize}; pub mod line_index; use line_index::LineIndex; -/// A specialized diagnostic for the typechecker. -/// -/// Type diagnostics are always **errors**. -#[derive(Clone, Debug, Diagnostic)] -#[diagnostic(category = "lint", severity = Warning)] -pub struct SuppressionDiagnostic { - #[location(span)] - span: TextRange, - #[description] - #[message] - message: MessageAndDescription, -} - -#[derive(Debug, Clone)] -pub enum SuppressionKind { - File, - Line, - Start, - End, -} - -#[derive(Debug, PartialEq, Clone)] -enum RuleSpecifier { - Category(RuleCategory), - Group(RuleCategory, String), - Rule(RuleCategory, String, String), -} - -impl RuleSpecifier { - fn category(&self) -> &RuleCategory { - match self { - RuleSpecifier::Category(rule_category) => rule_category, - RuleSpecifier::Group(rule_category, _) => rule_category, - RuleSpecifier::Rule(rule_category, _, _) => rule_category, - } - } - - fn group(&self) -> Option<&str> { - match self { - RuleSpecifier::Category(_) => None, - RuleSpecifier::Group(_, gr) => Some(gr), - RuleSpecifier::Rule(_, gr, _) => Some(gr), - } - } - - fn rule(&self) -> Option<&str> { - match self { - RuleSpecifier::Rule(_, _, ru) => Some(ru), - _ => None, - } - } - - 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 TryFrom<&str> for RuleSpecifier { - type Error = String; - - fn try_from(specifier_str: &str) -> Result { - let mut specifiers = specifier_str.split('/').map(|s| s.to_string()); - - let rule_category: RuleCategory = specifiers.next().unwrap().try_into()?; - - let group = specifiers.next(); - let rule = specifiers.next(); - - match (group, rule) { - (Some(g), Some(r)) => Ok(RuleSpecifier::Rule(rule_category, g, r)), - (Some(g), None) => Ok(RuleSpecifier::Group(rule_category, g)), - (None, None) => Ok(RuleSpecifier::Category(rule_category)), - _ => unreachable!(), - } - } -} - -#[derive(Debug, Clone)] -pub struct Suppression { - suppression_range: TextRange, - kind: SuppressionKind, - rule_specifier: RuleSpecifier, - #[allow(unused)] - explanation: Option, -} - -impl Suppression { - 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()), - }) - } - - 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 - } - - 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)] -pub struct RangeSuppression { - suppressed_range: TextRange, - start_suppression: Suppression, -} +use crate::{ + parser::SuppressionsParser, + suppression::{RangeSuppression, RuleSpecifier, Suppression, SuppressionDiagnostic}, +}; type Line = usize; @@ -234,6 +25,17 @@ pub struct Suppressions { 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. @@ -403,145 +205,3 @@ impl Suppressions { .unwrap_or_default() } } - -#[derive(Debug)] -pub 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: LineIndex::new(doc), - } - } - - /// Will parse the suppressions at the start of the file. - /// As soon as anything is encountered that's not a `pgt-ignore-all` - /// suppression, this will stop. - fn parse_file_suppressions(&mut self) { - while let Some((_, preview)) = self.lines.peek() { - 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_mut() - .rev() - .position(|start| start.rule_specifier == suppr.rule_specifier); - - 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(), - ), - }); - } - } -} diff --git a/crates/pgt_suppressions/src/parser.rs b/crates/pgt_suppressions/src/parser.rs new file mode 100644 index 00000000..4e1eee43 --- /dev/null +++ b/crates/pgt_suppressions/src/parser.rs @@ -0,0 +1,155 @@ +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: LineIndex::new(doc), + } + } + + /// Will parse the suppressions at the start of the file. + /// As soon as anything is encountered that's not a `pgt-ignore-all` + /// suppression, this will stop. + fn parse_file_suppressions(&mut self) { + while let Some((_, preview)) = self.lines.peek() { + 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_mut() + .rev() + .position(|start| start.rule_specifier == suppr.rule_specifier); + + 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(), + ), + }); + } + } +} diff --git a/crates/pgt_suppressions/src/suppression.rs b/crates/pgt_suppressions/src/suppression.rs new file mode 100644 index 00000000..321ccba3 --- /dev/null +++ b/crates/pgt_suppressions/src/suppression.rs @@ -0,0 +1,214 @@ +use pgt_analyse::{RuleCategory, RuleFilter}; +use pgt_diagnostics::{Diagnostic, MessageAndDescription}; +use pgt_text_size::{TextRange, TextSize}; + +/// A specialized diagnostic for the typechecker. +/// +/// Type diagnostics are always **errors**. +#[derive(Clone, Debug, Diagnostic)] +#[diagnostic(category = "lint", severity = Warning)] +pub struct SuppressionDiagnostic { + #[location(span)] + pub span: TextRange, + #[description] + #[message] + pub message: MessageAndDescription, +} + +#[derive(Debug, Clone)] +pub(crate) enum SuppressionKind { + File, + Line, + Start, + End, +} + +#[derive(Debug, PartialEq, Clone)] +pub(crate) enum RuleSpecifier { + Category(RuleCategory), + Group(RuleCategory, String), + Rule(RuleCategory, String, String), +} + +impl RuleSpecifier { + fn category(&self) -> &RuleCategory { + match self { + RuleSpecifier::Category(rule_category) => rule_category, + RuleSpecifier::Group(rule_category, _) => rule_category, + RuleSpecifier::Rule(rule_category, _, _) => rule_category, + } + } + + fn group(&self) -> Option<&str> { + match self { + RuleSpecifier::Category(_) => None, + RuleSpecifier::Group(_, gr) => Some(gr), + RuleSpecifier::Rule(_, gr, _) => Some(gr), + } + } + + 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 TryFrom<&str> for RuleSpecifier { + type Error = String; + + fn try_from(specifier_str: &str) -> Result { + let mut specifiers = specifier_str.split('/').map(|s| s.to_string()); + + let rule_category: RuleCategory = specifiers.next().unwrap().try_into()?; + + let group = specifiers.next(); + let rule = specifiers.next(); + + match (group, rule) { + (Some(g), Some(r)) => Ok(RuleSpecifier::Rule(rule_category, g, r)), + (Some(g), None) => Ok(RuleSpecifier::Group(rule_category, g)), + (None, None) => Ok(RuleSpecifier::Category(rule_category)), + _ => unreachable!(), + } + } +} + +#[derive(Debug, Clone)] +pub(crate) struct Suppression { + pub(crate) suppression_range: TextRange, + pub(crate) kind: SuppressionKind, + pub(crate) rule_specifier: RuleSpecifier, + #[allow(unused)] + explanation: Option, +} + +impl Suppression { + 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)] +pub(crate) struct RangeSuppression { + pub(crate) suppressed_range: TextRange, + pub(crate) start_suppression: Suppression, +} diff --git a/crates/pgt_workspace/src/workspace/server/change.rs b/crates/pgt_workspace/src/workspace/server/change.rs index 849fbae5..4b4669cf 100644 --- a/crates/pgt_workspace/src/workspace/server/change.rs +++ b/crates/pgt_workspace/src/workspace/server/change.rs @@ -1,4 +1,4 @@ -use pgt_suppressions::SuppressionsParser; +use pgt_suppressions::Suppressions; use pgt_text_size::{TextLen, TextRange, TextSize}; use std::ops::{Add, Sub}; @@ -86,7 +86,7 @@ impl Document { } self.version = change.version; - self.suppressions = SuppressionsParser::parse(&self.content); + 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 37b5ee83..ef6f1581 100644 --- a/crates/pgt_workspace/src/workspace/server/document.rs +++ b/crates/pgt_workspace/src/workspace/server/document.rs @@ -1,5 +1,5 @@ use pgt_diagnostics::{Diagnostic, DiagnosticExt, Severity, serde::Diagnostic as SDiagnostic}; -use pgt_suppressions::{Suppressions, SuppressionsParser}; +use pgt_suppressions::Suppressions; use pgt_text_size::{TextRange, TextSize}; use super::statement_identifier::{StatementId, StatementIdGenerator}; @@ -25,7 +25,7 @@ impl Document { let (ranges, diagnostics) = split_with_diagnostics(&content, None); - let suppressions = SuppressionsParser::parse(&content); + let suppressions = Suppressions::from(content.as_str()); Self { positions: ranges From eab166f26749eb13ecbcc7300e5c8dc581868050 Mon Sep 17 00:00:00 2001 From: Julian Date: Fri, 4 Jul 2025 16:23:43 +0200 Subject: [PATCH 11/20] add tests --- crates/pgt_suppressions/src/lib.rs | 135 +++++++++++ crates/pgt_suppressions/src/line_index.rs | 10 +- crates/pgt_suppressions/src/parser.rs | 208 ++++++++++++++++- crates/pgt_suppressions/src/suppression.rs | 248 ++++++++++++++++++++- 4 files changed, 586 insertions(+), 15 deletions(-) diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index dcf98cc2..2e9c3f68 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -205,3 +205,138 @@ impl Suppressions { .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(89.into(), 98.into()), + })); + } + + #[test] + fn correctly_suppresses_with_multiple_line_diagnostics() { + let doc = r#" + select 2; + + -- pgt-ignore lint + -- pgt-ignore action + 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) + .with_disabled_rules(&[pgt_analyse::RuleFilter::Group("safety")]); + + assert!(!suppressions.is_suppressed(&TestDiagnostic { + span: TextRange::new(89.into(), 98.into()), + })); + + assert_eq!(suppressions.diagnostics.len(), 1); + + assert_eq!( + suppressions.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).with_unused_suppressions_as_errors(&diagnostics); + + assert_eq!(suppressions.diagnostics.len(), 1); + + assert_eq!( + suppressions.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 index 7d7356c6..e6f07260 100644 --- a/crates/pgt_suppressions/src/line_index.rs +++ b/crates/pgt_suppressions/src/line_index.rs @@ -31,7 +31,13 @@ impl LineIndex { self.line_offset .iter() .enumerate() - .find(|(_, line_offset)| **line_offset >= offset) - .map(|(i, _)| i) + .filter_map(|(i, line_offset)| { + if offset >= *line_offset { + Some(i) + } else { + None + } + }) + .last() } } diff --git a/crates/pgt_suppressions/src/parser.rs b/crates/pgt_suppressions/src/parser.rs index 4e1eee43..43ac08eb 100644 --- a/crates/pgt_suppressions/src/parser.rs +++ b/crates/pgt_suppressions/src/parser.rs @@ -51,7 +51,7 @@ impl<'a> SuppressionsParser<'a> { line_suppressions: parser.line_suppressions, range_suppressions: parser.range_suppressions, diagnostics: parser.diagnostics, - line_index: LineIndex::new(doc), + line_index: parser.line_index, } } @@ -60,6 +60,11 @@ impl<'a> SuppressionsParser<'a> { /// suppression, 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; } @@ -109,9 +114,16 @@ impl<'a> SuppressionsParser<'a> { SuppressionKind::End => { let matching_start_idx = self .start_suppressions_stack - .iter_mut() - .rev() - .position(|start| start.rule_specifier == suppr.rule_specifier); + .iter() + .enumerate() + .filter_map(|(idx, s)| { + if s.rule_specifier == suppr.rule_specifier { + Some(idx) + } else { + None + } + }) + .last(); if let Some(start_idx) = matching_start_idx { let start = self.start_suppressions_stack.remove(start_idx); @@ -153,3 +165,191 @@ impl<'a> SuppressionsParser<'a> { } } } + +#[cfg(test)] +mod tests { + use pgt_analyse::RuleCategory; + + 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( + RuleCategory::Lint, + "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/banDropSomething +"#; + + 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("banDropSomething") + ); + } + + #[test] + fn parses_file_level_suppressions() { + let doc = r#" +-- pgt-ignore-all lint +-- pgt-ignore-all action + +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(RuleCategory::Lint) + ); + assert_eq!( + suppressions.file_suppressions[1].rule_specifier, + RuleSpecifier::Category(RuleCategory::Action) + ); + + 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( + RuleCategory::Lint, + "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( + RuleCategory::Lint, + "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 index 321ccba3..a572ee31 100644 --- a/crates/pgt_suppressions/src/suppression.rs +++ b/crates/pgt_suppressions/src/suppression.rs @@ -5,7 +5,7 @@ use pgt_text_size::{TextRange, TextSize}; /// A specialized diagnostic for the typechecker. /// /// Type diagnostics are always **errors**. -#[derive(Clone, Debug, Diagnostic)] +#[derive(Clone, Debug, Diagnostic, PartialEq)] #[diagnostic(category = "lint", severity = Warning)] pub struct SuppressionDiagnostic { #[location(span)] @@ -15,7 +15,7 @@ pub struct SuppressionDiagnostic { pub message: MessageAndDescription, } -#[derive(Debug, Clone)] +#[derive(Debug, Clone, PartialEq, Eq)] pub(crate) enum SuppressionKind { File, Line, @@ -23,7 +23,7 @@ pub(crate) enum SuppressionKind { End, } -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Clone, Eq)] pub(crate) enum RuleSpecifier { Category(RuleCategory), Group(RuleCategory, String), @@ -31,7 +31,7 @@ pub(crate) enum RuleSpecifier { } impl RuleSpecifier { - fn category(&self) -> &RuleCategory { + pub(crate) fn category(&self) -> &RuleCategory { match self { RuleSpecifier::Category(rule_category) => rule_category, RuleSpecifier::Group(rule_category, _) => rule_category, @@ -39,7 +39,7 @@ impl RuleSpecifier { } } - fn group(&self) -> Option<&str> { + pub(crate) fn group(&self) -> Option<&str> { match self { RuleSpecifier::Category(_) => None, RuleSpecifier::Group(_, gr) => Some(gr), @@ -47,7 +47,7 @@ impl RuleSpecifier { } } - fn rule(&self) -> Option<&str> { + pub(crate) fn rule(&self) -> Option<&str> { match self { RuleSpecifier::Rule(_, _, ru) => Some(ru), _ => None, @@ -88,13 +88,13 @@ impl TryFrom<&str> for RuleSpecifier { } } -#[derive(Debug, Clone)] +#[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)] - explanation: Option, + pub(crate) explanation: Option, } impl Suppression { @@ -207,8 +207,238 @@ impl Suppression { } } -#[derive(Debug, Clone)] +#[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( + RuleCategory::Lint, + "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(RuleCategory::Lint, "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(RuleCategory::Lint) + ); + } + + #[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(RuleCategory::Lint) + ); + 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( + RuleCategory::Lint, + "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( + RuleCategory::Lint, + "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( + RuleCategory::Lint, + "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/somethingElse/aRule", 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/safety", + "lint/somethingElse/aRule", + 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( + RuleCategory::Lint, + "safety".to_string(), + "banDropColumn".to_string(), + ); + let disabled = vec![RuleFilter::Group("safety")]; + assert!(spec.is_disabled(&disabled)); + + let spec2 = RuleSpecifier::Rule( + RuleCategory::Lint, + "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)); + } +} From f60383f36413182a1377893689dd5494ac2b7860 Mon Sep 17 00:00:00 2001 From: Julian Date: Fri, 4 Jul 2025 16:26:31 +0200 Subject: [PATCH 12/20] Ok --- crates/pgt_suppressions/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index 2e9c3f68..02b86c4c 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -249,7 +249,7 @@ mod tests { let suppressions = super::Suppressions::from(doc); assert!(suppressions.is_suppressed(&TestDiagnostic { - span: TextRange::new(89.into(), 98.into()), + span: TextRange::new(67.into(), 76.into()), })); } From 647fd36ecd515e735dc28b299240f882d69fa9c8 Mon Sep 17 00:00:00 2001 From: Julian Date: Fri, 4 Jul 2025 16:28:42 +0200 Subject: [PATCH 13/20] ok --- crates/pgt_suppressions/src/line_index.rs | 2 +- crates/pgt_suppressions/src/parser.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/pgt_suppressions/src/line_index.rs b/crates/pgt_suppressions/src/line_index.rs index e6f07260..16af72dd 100644 --- a/crates/pgt_suppressions/src/line_index.rs +++ b/crates/pgt_suppressions/src/line_index.rs @@ -38,6 +38,6 @@ impl LineIndex { None } }) - .last() + .next_back() } } diff --git a/crates/pgt_suppressions/src/parser.rs b/crates/pgt_suppressions/src/parser.rs index 43ac08eb..8bd19826 100644 --- a/crates/pgt_suppressions/src/parser.rs +++ b/crates/pgt_suppressions/src/parser.rs @@ -123,7 +123,7 @@ impl<'a> SuppressionsParser<'a> { None } }) - .last(); + .next_back(); if let Some(start_idx) = matching_start_idx { let start = self.start_suppressions_stack.remove(start_idx); From f4cb0ec6c7767fba597dc5baead3a30abaedf4ae Mon Sep 17 00:00:00 2001 From: Julian Domke <68325451+juleswritescode@users.noreply.github.com> Date: Sat, 5 Jul 2025 09:45:52 +0200 Subject: [PATCH 14/20] Update crates/pgt_suppressions/src/lib.rs --- crates/pgt_suppressions/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index 02b86c4c..792ba42e 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -193,7 +193,7 @@ impl Suppressions { let mut eligible = vec![]; // one-for-one, we're checking the lines above a diagnostic location - // until there are no more diagnostics + // until there are no more suppressions line_no -= 1; while let Some(suppr) = self.line_suppressions.get(&line_no) { eligible.push(suppr); From d957bce61cdc057bdcc4763adce0c27b917b0eb6 Mon Sep 17 00:00:00 2001 From: Julian Date: Sat, 5 Jul 2025 10:00:14 +0200 Subject: [PATCH 15/20] no clone --- crates/pgt_suppressions/Cargo.toml | 2 +- crates/pgt_suppressions/src/lib.rs | 83 +++++++++++--------- crates/pgt_suppressions/src/parser.rs | 2 +- crates/pgt_suppressions/src/suppression.rs | 9 ++- crates/pgt_workspace/src/workspace/server.rs | 18 +++-- 5 files changed, 68 insertions(+), 46 deletions(-) diff --git a/crates/pgt_suppressions/Cargo.toml b/crates/pgt_suppressions/Cargo.toml index 54064c6c..ee723b3b 100644 --- a/crates/pgt_suppressions/Cargo.toml +++ b/crates/pgt_suppressions/Cargo.toml @@ -2,7 +2,7 @@ [package] authors.workspace = true categories.workspace = true -description = "" +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 diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index 02b86c4c..e58515eb 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -39,54 +39,55 @@ impl From for Suppressions { impl Suppressions { /// Some diagnostics can be turned off via the configuration. /// This will mark suppressions that try to suppress these disabled diagnostics as errors. - #[must_use] - pub fn with_disabled_rules(mut self, disabled_rules: &[RuleFilter<'_>]) -> Self { + pub fn get_disabled_diagnostic_suppressions_as_errors( + &self, + disabled_rules: &[RuleFilter<'_>], + ) -> Vec { + let mut diagnostics = vec![]; + { - let (enabled, disabled) = self + let disabled = self .file_suppressions - .into_iter() - .partition(|s| !s.rule_specifier.is_disabled(disabled_rules)); - - self.file_suppressions = enabled; + .iter() + .filter(|s| s.rule_specifier.is_disabled(disabled_rules)); for suppr in disabled { - self.diagnostics.push(suppr.to_disabled_diagnostic()); + diagnostics.push(suppr.to_disabled_diagnostic()); } } { - let (enabled, disabled) = self + let disabled = self .line_suppressions - .into_iter() - .partition(|(_, s)| !s.rule_specifier.is_disabled(disabled_rules)); - - self.line_suppressions = enabled; + .iter() + .filter(|(_, s)| s.rule_specifier.is_disabled(disabled_rules)); for (_, suppr) in disabled { - self.diagnostics.push(suppr.to_disabled_diagnostic()); + diagnostics.push(suppr.to_disabled_diagnostic()); } } { - let (enabled, disabled) = self.range_suppressions.into_iter().partition(|s| { - !s.start_suppression + let disabled = self.range_suppressions.iter().filter(|s| { + s.start_suppression .rule_specifier .is_disabled(disabled_rules) }); - self.range_suppressions = enabled; - for range_suppr in disabled { - self.diagnostics - .push(range_suppr.start_suppression.to_disabled_diagnostic()); + diagnostics.push(range_suppr.start_suppression.to_disabled_diagnostic()); } } - self + diagnostics } - #[must_use] - pub fn with_unused_suppressions_as_errors(mut self, diagnostics: &[D]) -> Self { + 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 @@ -103,6 +104,14 @@ impl Suppressions { } } + // 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 @@ -126,7 +135,7 @@ impl Suppressions { { continue; } else { - self.diagnostics.push(SuppressionDiagnostic { + results.push(SuppressionDiagnostic { span: suppr.suppression_range, message: MessageAndDescription::from( "This suppression has no effect.".to_string(), @@ -135,7 +144,7 @@ impl Suppressions { } } - self + results } pub fn is_suppressed(&self, diagnostic: &D) -> bool { @@ -296,17 +305,18 @@ mod tests { select 1; "#; - let suppressions = super::Suppressions::from(doc) - .with_disabled_rules(&[pgt_analyse::RuleFilter::Group("safety")]); + let suppressions = super::Suppressions::from(doc); - assert!(!suppressions.is_suppressed(&TestDiagnostic { - span: TextRange::new(89.into(), 98.into()), - })); + let disabled_diagnostics = suppressions.get_disabled_diagnostic_suppressions_as_errors(&[ + pgt_analyse::RuleFilter::Group("safety"), + ]); - assert_eq!(suppressions.diagnostics.len(), 1); + println!("{:?}", disabled_diagnostics); + + assert_eq!(disabled_diagnostics.len(), 1); assert_eq!( - suppressions.diagnostics[0], + 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()) @@ -326,13 +336,14 @@ mod tests { // no diagnostics let diagnostics: Vec = vec![]; - let suppressions = - super::Suppressions::from(doc).with_unused_suppressions_as_errors(&diagnostics); + let suppressions = super::Suppressions::from(doc); + + let unused_diagnostics = suppressions.get_unused_suppressions_as_errors(&diagnostics); - assert_eq!(suppressions.diagnostics.len(), 1); + assert_eq!(unused_diagnostics.len(), 1); assert_eq!( - suppressions.diagnostics[0], + 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/parser.rs b/crates/pgt_suppressions/src/parser.rs index 8bd19826..eaf71d97 100644 --- a/crates/pgt_suppressions/src/parser.rs +++ b/crates/pgt_suppressions/src/parser.rs @@ -57,7 +57,7 @@ impl<'a> SuppressionsParser<'a> { /// Will parse the suppressions at the start of the file. /// As soon as anything is encountered that's not a `pgt-ignore-all` - /// suppression, this will stop. + /// 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() { diff --git a/crates/pgt_suppressions/src/suppression.rs b/crates/pgt_suppressions/src/suppression.rs index a572ee31..3efc10eb 100644 --- a/crates/pgt_suppressions/src/suppression.rs +++ b/crates/pgt_suppressions/src/suppression.rs @@ -24,6 +24,10 @@ pub(crate) enum SuppressionKind { } #[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 `(/(/))`. +/// pub(crate) enum RuleSpecifier { Category(RuleCategory), Group(RuleCategory, String), @@ -98,6 +102,9 @@ pub(crate) struct Suppression { } 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(); @@ -196,7 +203,7 @@ impl Suppression { false } - pub(crate) fn to_disabled_diagnostic(self) -> SuppressionDiagnostic { + pub(crate) fn to_disabled_diagnostic(&self) -> SuppressionDiagnostic { SuppressionDiagnostic { span: self.suppression_range, message: MessageAndDescription::from( diff --git a/crates/pgt_workspace/src/workspace/server.rs b/crates/pgt_workspace/src/workspace/server.rs index 2be99204..5f2a9ebf 100644 --- a/crates/pgt_workspace/src/workspace/server.rs +++ b/crates/pgt_workspace/src/workspace/server.rs @@ -570,20 +570,24 @@ impl Workspace for WorkspaceServer { }, )); - let suppressions = parser - .document_suppressions() - .clone() - .with_disabled_rules(&disabled_rules) - .with_unused_suppressions_as_errors(&diagnostics); + let suppressions = parser.document_suppressions(); - diagnostics.retain(|d| !suppressions.is_suppressed(d)); + 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 - .into_iter() + .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 From b8e00b5e314d0ab7ad0ceb935d65c82d5ed81c79 Mon Sep 17 00:00:00 2001 From: Julian Domke <68325451+juleswritescode@users.noreply.github.com> Date: Sat, 5 Jul 2025 10:07:30 +0200 Subject: [PATCH 16/20] Update crates/pgt_suppressions/src/lib.rs --- crates/pgt_suppressions/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index 9e5a554d..f7b4bbab 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -106,7 +106,7 @@ impl Suppressions { // Users may use many suppressions for a single diagnostic, like so: // ``` - // -- pgt ignore lint/safety/banDropTable + // -- pgt-ignore lint/safety/banDropTable // -- pgt ignore lint/safety/banDropColumn // // ``` From 0d77e52d153dbd27ab28cfb0d3f7831a8b3955e5 Mon Sep 17 00:00:00 2001 From: Julian Domke <68325451+juleswritescode@users.noreply.github.com> Date: Sat, 5 Jul 2025 10:07:56 +0200 Subject: [PATCH 17/20] Update crates/pgt_suppressions/src/lib.rs --- crates/pgt_suppressions/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index f7b4bbab..cc3c5378 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -107,7 +107,7 @@ impl Suppressions { // Users may use many suppressions for a single diagnostic, like so: // ``` // -- pgt-ignore lint/safety/banDropTable - // -- pgt ignore lint/safety/banDropColumn + // -- pgt-ignore lint/safety/banDropColumn // // ``` // So to find a matching diagnostic for any suppression, we're moving From 4b73bedf8471746c34cbda80c865f6e34cfb2d2b Mon Sep 17 00:00:00 2001 From: Julian Domke <68325451+juleswritescode@users.noreply.github.com> Date: Sat, 5 Jul 2025 10:09:24 +0200 Subject: [PATCH 18/20] Update crates/pgt_suppressions/src/lib.rs --- crates/pgt_suppressions/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index cc3c5378..03281b99 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -311,8 +311,6 @@ mod tests { pgt_analyse::RuleFilter::Group("safety"), ]); - println!("{:?}", disabled_diagnostics); - assert_eq!(disabled_diagnostics.len(), 1); assert_eq!( From 59aa3f1ddab53c17296a4a1dd33968c16d87438b Mon Sep 17 00:00:00 2001 From: Julian Domke <68325451+juleswritescode@users.noreply.github.com> Date: Sat, 5 Jul 2025 14:44:54 +0200 Subject: [PATCH 19/20] feat(suppressions): suppress all kinds of diagnostics (#442) --- crates/pgt_suppressions/src/suppression.rs | 63 +++++++++++++--------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/crates/pgt_suppressions/src/suppression.rs b/crates/pgt_suppressions/src/suppression.rs index 3efc10eb..c1d4e5c3 100644 --- a/crates/pgt_suppressions/src/suppression.rs +++ b/crates/pgt_suppressions/src/suppression.rs @@ -1,5 +1,5 @@ -use pgt_analyse::{RuleCategory, RuleFilter}; -use pgt_diagnostics::{Diagnostic, MessageAndDescription}; +use pgt_analyse::RuleFilter; +use pgt_diagnostics::{Category, Diagnostic, MessageAndDescription}; use pgt_text_size::{TextRange, TextSize}; /// A specialized diagnostic for the typechecker. @@ -28,14 +28,16 @@ pub(crate) enum SuppressionKind { /// 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(RuleCategory), - Group(RuleCategory, String), - Rule(RuleCategory, String, String), + Category(String), + Group(String, String), + Rule(String, String, String), } impl RuleSpecifier { - pub(crate) fn category(&self) -> &RuleCategory { + pub(crate) fn category(&self) -> &str { match self { RuleSpecifier::Category(rule_category) => rule_category, RuleSpecifier::Group(rule_category, _) => rule_category, @@ -72,26 +74,35 @@ impl RuleSpecifier { } } -impl TryFrom<&str> for RuleSpecifier { - type Error = String; - - fn try_from(specifier_str: &str) -> Result { - let mut specifiers = specifier_str.split('/').map(|s| s.to_string()); - - let rule_category: RuleCategory = specifiers.next().unwrap().try_into()?; +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 (group, rule) { - (Some(g), Some(r)) => Ok(RuleSpecifier::Rule(rule_category, g, r)), - (Some(g), None) => Ok(RuleSpecifier::Group(rule_category, g)), - (None, None) => Ok(RuleSpecifier::Category(rule_category)), + 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, @@ -235,7 +246,7 @@ mod tests { assert_eq!( suppression.rule_specifier, RuleSpecifier::Rule( - RuleCategory::Lint, + "lint".to_string(), "safety".to_string(), "banDropColumn".to_string() ) @@ -252,7 +263,7 @@ mod tests { assert_eq!(suppression.kind, SuppressionKind::Line); assert_eq!( suppression.rule_specifier, - RuleSpecifier::Group(RuleCategory::Lint, "safety".to_string()) + RuleSpecifier::Group("lint".to_string(), "safety".to_string()) ); assert_eq!(suppression.explanation.as_deref(), Some("explanation")); } @@ -266,7 +277,7 @@ mod tests { assert_eq!(suppression.kind, SuppressionKind::Line); assert_eq!( suppression.rule_specifier, - RuleSpecifier::Category(RuleCategory::Lint) + RuleSpecifier::Category("lint".to_string()) ); } @@ -279,7 +290,7 @@ mod tests { assert_eq!(suppression.kind, SuppressionKind::Line); assert_eq!( suppression.rule_specifier, - RuleSpecifier::Category(RuleCategory::Lint) + RuleSpecifier::Category("lint".to_string()) ); assert_eq!(suppression.explanation.as_deref(), Some("explanation")); } @@ -294,7 +305,7 @@ mod tests { assert_eq!( suppression.rule_specifier, RuleSpecifier::Rule( - RuleCategory::Lint, + "lint".to_string(), "safety".to_string(), "banDropColumn".to_string() ) @@ -312,7 +323,7 @@ mod tests { assert_eq!( suppression.rule_specifier, RuleSpecifier::Rule( - RuleCategory::Lint, + "lint".to_string(), "safety".to_string(), "banDropColumn".to_string() ) @@ -330,7 +341,7 @@ mod tests { assert_eq!( suppression.rule_specifier, RuleSpecifier::Rule( - RuleCategory::Lint, + "lint".to_string(), "safety".to_string(), "banDropColumn".to_string() ) @@ -420,7 +431,7 @@ mod tests { // Group filter disables all rules in that group let spec = RuleSpecifier::Rule( - RuleCategory::Lint, + "lint".to_string(), "safety".to_string(), "banDropColumn".to_string(), ); @@ -428,7 +439,7 @@ mod tests { assert!(spec.is_disabled(&disabled)); let spec2 = RuleSpecifier::Rule( - RuleCategory::Lint, + "lint".to_string(), "safety".to_string(), "banDropColumn".to_string(), ); From 8d1bd8b03cb39832a66f45a9054328009dd59ee3 Mon Sep 17 00:00:00 2001 From: Julian Date: Sat, 5 Jul 2025 15:01:07 +0200 Subject: [PATCH 20/20] fix tests --- crates/pgt_suppressions/src/lib.rs | 2 +- crates/pgt_suppressions/src/parser.rs | 18 ++++++++---------- crates/pgt_suppressions/src/suppression.rs | 9 +++------ 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/crates/pgt_suppressions/src/lib.rs b/crates/pgt_suppressions/src/lib.rs index 03281b99..2577ea41 100644 --- a/crates/pgt_suppressions/src/lib.rs +++ b/crates/pgt_suppressions/src/lib.rs @@ -268,7 +268,7 @@ mod tests { select 2; -- pgt-ignore lint - -- pgt-ignore action + -- pgt-ignore syntax select 1; "#; diff --git a/crates/pgt_suppressions/src/parser.rs b/crates/pgt_suppressions/src/parser.rs index eaf71d97..663e52fe 100644 --- a/crates/pgt_suppressions/src/parser.rs +++ b/crates/pgt_suppressions/src/parser.rs @@ -168,8 +168,6 @@ impl<'a> SuppressionsParser<'a> { #[cfg(test)] mod tests { - use pgt_analyse::RuleCategory; - use super::*; use crate::suppression::{RuleSpecifier, SuppressionKind}; @@ -192,7 +190,7 @@ SELECT 2; assert_eq!( suppression.rule_specifier, RuleSpecifier::Rule( - RuleCategory::Lint, + "lint".to_string(), "safety".to_string(), "banDropColumn".to_string() ) @@ -205,7 +203,7 @@ SELECT 2; SELECT 1; -- pgt-ignore lint/safety/banDropColumn -- pgt-ignore lint/safety/banDropTable --- pgt-ignore lint/safety/banDropSomething +-- pgt-ignore lint/safety/banDropNotNull "#; let suppressions = SuppressionsParser::parse(doc); @@ -239,7 +237,7 @@ SELECT 1; .unwrap() .rule_specifier .rule(), - Some("banDropSomething") + Some("banDropNotNull") ); } @@ -247,7 +245,7 @@ SELECT 1; fn parses_file_level_suppressions() { let doc = r#" -- pgt-ignore-all lint --- pgt-ignore-all action +-- pgt-ignore-all typecheck SELECT 1; -- pgt-ignore-all lint/safety @@ -260,11 +258,11 @@ SELECT 1; assert_eq!( suppressions.file_suppressions[0].rule_specifier, - RuleSpecifier::Category(RuleCategory::Lint) + RuleSpecifier::Category("lint".to_string()) ); assert_eq!( suppressions.file_suppressions[1].rule_specifier, - RuleSpecifier::Category(RuleCategory::Action) + RuleSpecifier::Category("typecheck".to_string()) ); assert_eq!( @@ -294,7 +292,7 @@ drop table posts; start_suppression: Suppression { kind: SuppressionKind::Start, rule_specifier: RuleSpecifier::Rule( - RuleCategory::Lint, + "lint".to_string(), "safety".to_string(), "banDropTable".to_string() ), @@ -330,7 +328,7 @@ drop table posts; start_suppression: Suppression { kind: SuppressionKind::Start, rule_specifier: RuleSpecifier::Rule( - RuleCategory::Lint, + "lint".to_string(), "safety".to_string(), "banDropTable".to_string() ), diff --git a/crates/pgt_suppressions/src/suppression.rs b/crates/pgt_suppressions/src/suppression.rs index c1d4e5c3..6ebaf25c 100644 --- a/crates/pgt_suppressions/src/suppression.rs +++ b/crates/pgt_suppressions/src/suppression.rs @@ -383,7 +383,7 @@ mod tests { fn test_suppression_matches() { let cases = vec![ // the category works for all groups & rules - ("-- pgt-ignore lint", "lint/somethingElse/aRule", true), + ("-- pgt-ignore lint", "lint/safety/banDropNotNull", true), ("-- pgt-ignore lint", "lint/safety/banDropColumn", true), // the group works for all rules in that group ( @@ -391,11 +391,8 @@ mod tests { "lint/safety/banDropColumn", true, ), - ( - "-- pgt-ignore lint/safety", - "lint/somethingElse/aRule", - false, - ), + ("-- 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",