From dafbc30994d76de1176c768a79da853ec43acfe5 Mon Sep 17 00:00:00 2001 From: Ben King <9087625+benfdking@users.noreply.github.com> Date: Fri, 4 Oct 2024 15:02:28 -0700 Subject: [PATCH] feat: implementing noqa directives --- crates/cli/tests/ui/LT01_noqa.sql | 1 + crates/cli/tests/ui/LT01_noqa.stderr | 2 + crates/lib-core/src/errors.rs | 8 +- crates/lib/src/core/linter/core.rs | 61 ++- crates/lib/src/core/linter/linted_file.rs | 8 +- crates/lib/src/core/rules.rs | 1 + crates/lib/src/core/rules/noqa.rs | 517 ++++++++++++++++++++++ editors/code/src/browser.ts | 14 +- 8 files changed, 580 insertions(+), 32 deletions(-) create mode 100644 crates/cli/tests/ui/LT01_noqa.sql create mode 100644 crates/cli/tests/ui/LT01_noqa.stderr create mode 100644 crates/lib/src/core/rules/noqa.rs diff --git a/crates/cli/tests/ui/LT01_noqa.sql b/crates/cli/tests/ui/LT01_noqa.sql new file mode 100644 index 00000000..b4a0bce2 --- /dev/null +++ b/crates/cli/tests/ui/LT01_noqa.sql @@ -0,0 +1 @@ +SELECT 1; --noqa: disable=LT01 diff --git a/crates/cli/tests/ui/LT01_noqa.stderr b/crates/cli/tests/ui/LT01_noqa.stderr new file mode 100644 index 00000000..b6303578 --- /dev/null +++ b/crates/cli/tests/ui/LT01_noqa.stderr @@ -0,0 +1,2 @@ +The linter processed 1 file(s). +All Finished diff --git a/crates/lib-core/src/errors.rs b/crates/lib-core/src/errors.rs index 59b332d8..31521e7d 100644 --- a/crates/lib-core/src/errors.rs +++ b/crates/lib-core/src/errors.rs @@ -28,7 +28,7 @@ pub struct SQLBaseError { pub source_slice: Range, } -#[derive(Debug, PartialEq, Clone)] +#[derive(Debug, PartialEq, Clone, Default)] pub struct ErrorStructRule { pub name: &'static str, pub code: &'static str, @@ -126,6 +126,12 @@ impl From for SQLBaseError { } } +impl From for SQLLintError { + fn from(value: SQLBaseError) -> Self { + Self { base: value } + } +} + #[derive(Debug, PartialEq, Clone)] pub struct SQLTemplaterError {} diff --git a/crates/lib/src/core/linter/core.rs b/crates/lib/src/core/linter/core.rs index c708b092..34e58419 100644 --- a/crates/lib/src/core/linter/core.rs +++ b/crates/lib/src/core/linter/core.rs @@ -10,7 +10,7 @@ use rayon::iter::{IntoParallelRefIterator as _, ParallelIterator as _}; use smol_str::{SmolStr, ToSmolStr}; use sqruff_lib_core::dialects::base::Dialect; use sqruff_lib_core::errors::{ - SQLFluffUserError, SQLLexError, SQLLintError, SQLParseError, SqlError, + SQLBaseError, SQLFluffUserError, SQLLexError, SQLLintError, SQLParseError, SqlError, }; use sqruff_lib_core::helpers; use sqruff_lib_core::linter::compute_anchor_edit_info; @@ -28,6 +28,7 @@ use crate::core::linter::common::{ParsedString, RenderedFile}; use crate::core::linter::linted_file::LintedFile; use crate::core::linter::linting_result::LintingResult; use crate::core::rules::base::{ErasedRule, LintPhase, RulePack}; +use crate::core::rules::noqa::IgnoreMask; use crate::rules::get_ruleset; use crate::templaters::placeholder::PlaceholderTemplater; use crate::templaters::raw::RawTemplater; @@ -187,25 +188,38 @@ impl Linter { ) -> LintedFile { let mut violations = parsed_string.violations; - let (tree, initial_linting_errors) = parsed_string - .tree - .map(|tree| self.lint_fix_parsed(tables, tree, &parsed_string.templated_file, fix)) - .unzip(); + let (patches, ignore_mask, initial_linting_errors) = + parsed_string + .tree + .map_or((Vec::new(), None, Vec::new()), |erased_segment| { + let (tree, ignore_mask, initial_linting_errors) = self.lint_fix_parsed( + tables, + erased_segment, + &parsed_string.templated_file, + fix, + ); + let patches = tree.iter_patches(&parsed_string.templated_file); + (patches, ignore_mask, initial_linting_errors) + }); + violations.extend(initial_linting_errors.into_iter().map_into()); - violations.extend( - initial_linting_errors - .unwrap_or_default() - .into_iter() - .map(Into::into), - ); + // Filter violations with ignore mask + let violations = violations + .into_iter() + .filter(|violation| { + ignore_mask + .as_ref() + .map_or(true, |ignore_mask| !ignore_mask.is_masked(violation)) + }) + .collect(); + // TODO Need to error out unused noqas let linted_file = LintedFile { path: parsed_string.filename, - patches: tree.map_or(Vec::new(), |tree| { - tree.iter_patches(&parsed_string.templated_file) - }), + patches, templated_file: parsed_string.templated_file, violations, + ignore_mask, }; if let Some(formatter) = &self.formatter { @@ -221,7 +235,7 @@ impl Linter { mut tree: ErasedSegment, templated_file: &TemplatedFile, fix: bool, - ) -> (ErasedSegment, Vec) { + ) -> (ErasedSegment, Option, Vec) { let mut tmp; let mut initial_linting_errors = Vec::new(); let phases: &[_] = if fix { @@ -235,6 +249,21 @@ impl Linter { // If we are fixing then we want to loop up to the runaway_limit, otherwise just // once for linting. let loop_limit = if fix { 10 } else { 1 }; + // Look for comment segments which might indicate lines to ignore. + let (ignore_mask, violations): (Option, Vec) = { + let disable_noqa = self + .config + .get("disable_noqa", "core") + .as_bool() + .unwrap_or(false); + if disable_noqa { + (None, Vec::new()) + } else { + let (ignore_mask, errors) = IgnoreMask::from_tree(&tree); + (Some(ignore_mask), errors) + } + }; + initial_linting_errors.extend(violations.into_iter().map_into()); for phase in phases { let mut rules_this_phase = if phases.len() > 1 { @@ -327,7 +356,7 @@ impl Linter { } } - (tree, initial_linting_errors) + (tree, ignore_mask, initial_linting_errors) } /// Template the file. diff --git a/crates/lib/src/core/linter/linted_file.rs b/crates/lib/src/core/linter/linted_file.rs index 4bafa3d5..21f7a4cc 100644 --- a/crates/lib/src/core/linter/linted_file.rs +++ b/crates/lib/src/core/linter/linted_file.rs @@ -1,5 +1,6 @@ use std::ops::Range; +use crate::core::rules::noqa::IgnoreMask; use itertools::Itertools; use rustc_hash::FxHashSet; use sqruff_lib_core::errors::SQLBaseError; @@ -12,16 +13,13 @@ pub struct LintedFile { pub patches: Vec, pub templated_file: TemplatedFile, pub violations: Vec, + pub ignore_mask: Option, } impl LintedFile { #[allow(unused_variables)] pub fn get_violations(&self, fixable: Option) -> Vec { - self.violations - .clone() - .into_iter() - .map(Into::into) - .collect_vec() + self.violations.clone().into_iter().map_into().collect_vec() } /// Use patches and raw file to fix the source file. diff --git a/crates/lib/src/core/rules.rs b/crates/lib/src/core/rules.rs index f2b27e53..e6b47a56 100644 --- a/crates/lib/src/core/rules.rs +++ b/crates/lib/src/core/rules.rs @@ -1,4 +1,5 @@ pub mod base; pub mod context; pub mod crawlers; +pub mod noqa; pub mod reference; diff --git a/crates/lib/src/core/rules/noqa.rs b/crates/lib/src/core/rules/noqa.rs new file mode 100644 index 00000000..dee603e8 --- /dev/null +++ b/crates/lib/src/core/rules/noqa.rs @@ -0,0 +1,517 @@ +use ahash::HashSet; +use fancy_regex::Regex; +use sqruff_lib_core::dialects::syntax::{SyntaxKind, SyntaxSet}; +use sqruff_lib_core::errors::SQLBaseError; +use sqruff_lib_core::parser::segments::base::ErasedSegment; +use std::str::FromStr; + +#[derive(Eq, PartialEq, Debug)] +struct NoQADirective { + line_no: usize, + line_pos: usize, + raw_string: String, + // Can be Enable, Disable or None. + // TODO Make the None clearer to actually what it is + action: Option, + // This could be able to be a perfect map because we should know all the rules upfront. + // TODO Make it clearer what None means + rules: Option>, + // TODO Introduce a method for used because want to be able to return all unused noqas + // used: bool +} + +#[derive(Debug, Default)] +pub struct IgnoreMask { + ignore_list: Vec, +} + +const NOQA_PREFIX: &str = "noqa"; + +#[derive(Eq, PartialEq, Debug, strum_macros::EnumString)] +#[strum(serialize_all = "lowercase")] +enum IgnoreAction { + Enable, + Disable, +} + +impl IgnoreMask { + /// Extract ignore mask entries from a comment segment + fn extract_ignore_from_comment( + comment: ErasedSegment, + ) -> Result, SQLBaseError> { + // Trim any whitespace + let mut comment_content = comment.raw().trim(); + // If we have leading or trailing block comment markers, also strip them. + // NOTE: We need to strip block comment markers from the start + // to ensure that noqa directives in the following form are followed: + // /* noqa: disable=all */ + if comment_content.ends_with("*/") { + comment_content = &comment_content[..comment_content.len() - 2].trim_end(); + } + if comment_content.starts_with("/*") { + comment_content = &comment_content[2..].trim_start(); + } + let (line_no, line_pos) = comment + .get_position_marker() + .ok_or(SQLBaseError { + fatal: true, + ignore: false, + warning: false, + line_no: 0, + line_pos: 0, + description: "Could not get position marker".to_string(), + rule: None, + source_slice: Default::default(), + })? + .source_position(); + IgnoreMask::parse_noqa(comment_content, line_no, line_pos) + } + + /// Extract ignore mask entries from a comment string. + fn parse_noqa( + original_comment: &str, + line_no: usize, + line_pos: usize, + ) -> Result, SQLBaseError> { + // Comment lines can also have noqa e.g. + // --dafhsdkfwdiruweksdkjdaffldfsdlfjksd -- noqa: LT05 + // Therefore extract last possible inline ignore. + let comment = original_comment.split("--").last(); + if let Some(comment) = comment { + let comment = comment.trim(); + if comment.starts_with(NOQA_PREFIX) { + if comment.trim() == NOQA_PREFIX { + return Ok(Some(NoQADirective { + line_no, + line_pos, + raw_string: comment.to_string(), + action: None, + rules: None, + })); + } + let comment = &comment[4..]; + if !comment.starts_with(":") { + return Err(SQLBaseError { + fatal: true, + ignore: false, + warning: false, + line_no, + line_pos, + description: "Malformed 'noqa' section. Expected 'noqa: [,...]" + .to_string(), + rule: None, + // TODO: Add source slice + source_slice: Default::default(), + }); + } + let comment = comment[1..].trim(); + + let mut action: Option = None; + let mut rule_part: Option<&str> = None; + + if let Some(position) = comment.find("=") { + let (action_part, rule_part_parsed) = comment.split_at(position); + action = Some(IgnoreAction::from_str(action_part.trim()).map_err( + |err| SQLBaseError { + fatal: true, + ignore: false, + warning: false, + line_no, + line_pos, + description: format!("Malformed 'noqa' section. Expected --noqa: disable=[rules]|all or --noqa: enable=[rules]|all : {}", err), + rule: None, + // TODO Add source slice + source_slice: Default::default(), + } + )?); + rule_part = Some(rule_part_parsed[1..].trim()); + } else { + rule_part = Some(comment); + if ["disable", "enable"].contains(&comment) { + return Err(SQLBaseError { + fatal: true, + ignore: false, + warning: false, + line_no, + line_pos, + description: "Malformed 'noqa' section. Expected --noqa: disable=[rules]|all or --noqa: enable=[rules]|all".to_string(), + rule: None, + // TODO Add source slice + source_slice: Default::default(), + }); + } + } + + return if rule_part == Some("all") { + Ok(Some(NoQADirective { + line_no, + line_pos, + raw_string: original_comment.to_string(), + action, + rules: None, + })) + } else { + // TODO HERE SHOULD MAKE SURE EVERY RULE MAKES SENSE + let rules: HashSet<_> = rule_part + .unwrap() + .split(",") + .map(|rule| rule.trim().to_string()) + .filter(|rule| !rule.is_empty()) + .collect(); + if rules.is_empty() { + Ok(Some(NoQADirective { + line_no, + line_pos, + raw_string: original_comment.to_string(), + action, + rules: None, + })) + } else { + Ok(Some(NoQADirective { + line_no, + line_pos, + raw_string: original_comment.to_string(), + action, + rules: Some(rules), + })) + } + }; + } + } + Ok(None) + } + + // TODO See if need to implement reference_map + pub fn from_tree(tree: &ErasedSegment) -> (IgnoreMask, Vec) { + let mut ignore_list: Vec = vec![]; + let mut violations: Vec = vec![]; + + for comment in tree.recursive_crawl( + const { + &SyntaxSet::new(&[ + SyntaxKind::Comment, + SyntaxKind::InlineComment, + SyntaxKind::BlockComment, + ]) + }, + false, + &SyntaxSet::new(&[]), + false, + ) { + let ignore_entry = IgnoreMask::extract_ignore_from_comment(comment); + if let Err(err) = ignore_entry { + violations.push(err); + } else if let Ok(Some(ignore_entry)) = ignore_entry { + ignore_list.push(ignore_entry); + } + } + + (IgnoreMask { ignore_list }, violations) + } + + /// Look for inline ignore comments and return NoQaDirectives. + /// + /// Very similar to .from_tree(), but can be run on raw source + /// (i.e. does not require the code to have parsed successfully). + fn from_source(source: &str, inline_comment_regex: &Regex) -> (IgnoreMask, Vec) { + let mut ignore_list: Vec = vec![]; + let mut violations: Vec = vec![]; + for (idx, line) in source.lines().enumerate() { + if let Ok(Some(match_)) = inline_comment_regex.find(line) { + let ignore_entry = IgnoreMask::parse_noqa( + &line[match_.start()..match_.end()], + idx + 1, + match_.start(), + ); + match ignore_entry { + Ok(Some(entry)) => ignore_list.push(entry), + Ok(None) => {} + Err(err) => violations.push(err), + } + } + } + (IgnoreMask { ignore_list }, violations) + } + + /// Filter a list of violations based on this single line noqa. + /// The "ignore" list is assumed to ONLY contain NoQaDirectives with action = None. + fn is_masked_violation_single_line( + directives: &[NoQADirective], + violation: &SQLBaseError, + ) -> bool { + todo!() + } + + /// is_masked returns true if the IgnoreMask masks the violation + pub fn is_masked(&self, violation: &SQLBaseError) -> bool { + let ignore_on_specific_line = self + .ignore_list + .iter() + .filter(|ignore| { + ignore.action.is_none() || matches!(ignore.action, Some(IgnoreAction::Disable)) + }) + .filter(|ignore| ignore.line_no == violation.line_no) + .collect::>(); + + for ignore in ignore_on_specific_line { + match (&violation.rule, &ignore.rules) { + (Some(violation_rule), Some(ignore_rules)) => { + if ignore_rules.contains(violation_rule.code) { + return true; + } + } + _ => {} + } + } + false + } + + // def ignore_masked_violations( + // self, violations: List[SQLBaseError] + // ) -> List[SQLBaseError]: + // """Remove any violations specified by ignore_mask. + // + // This involves two steps: + // 1. Filter out violations affected by single-line "noqa" directives. + // 2. Filter out violations affected by disable/enable "noqa" directives. + // """ + // ignore_specific = [ignore for ignore in self._ignore_list if not ignore.action] + // ignore_range = [ignore for ignore in self._ignore_list if ignore.action] + // violations = self._ignore_masked_violations_single_line( + // violations, ignore_specific + // ) + // violations = self._ignore_masked_violations_line_range(violations, ignore_range) + // return violations +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::core::config::FluffConfig; + use crate::core::linter::core::Linter; + use crate::core::rules::noqa::NoQADirective; + use sqruff_lib_core::errors::ErrorStructRule; + use std::collections::BTreeSet; + + #[test] + fn test_is_masked() { + let error = SQLBaseError { + fatal: false, + ignore: false, + warning: false, + line_no: 2, + line_pos: 11, + description: "Implicit/explicit aliasing of columns.".to_string(), + rule: Some(ErrorStructRule { + name: "aliasing.column", + code: "AL02", + }), + source_slice: Default::default(), + }; + let mask = IgnoreMask { + ignore_list: vec![NoQADirective { + line_no: 2, + line_pos: 13, + raw_string: "--noqa: disable=AL02".to_string(), + action: Some(IgnoreAction::Disable), + rules: Some(["AL02".to_string()].into_iter().collect()), + }], + }; + let not_mask_wrong_line = IgnoreMask { + ignore_list: vec![NoQADirective { + line_no: 3, + line_pos: 13, + raw_string: "--noqa: disable=AL02".to_string(), + action: Some(IgnoreAction::Disable), + rules: Some(["AL02".to_string()].into_iter().collect()), + }], + }; + let not_mask_wrong_rule = IgnoreMask { + ignore_list: vec![NoQADirective { + line_no: 3, + line_pos: 13, + raw_string: "--noqa: disable=AL03".to_string(), + action: Some(IgnoreAction::Disable), + rules: Some(["AL03".to_string()].into_iter().collect()), + }], + }; + + assert!(!not_mask_wrong_line.is_masked(&error)); + assert!(!not_mask_wrong_rule.is_masked(&error)); + assert!(mask.is_masked(&error)); + } + + #[test] + fn test_parse_noqa() { + let test_cases = vec![ + ("", Ok(None)), + ( + "noqa", + Ok(Some(NoQADirective { + line_no: 0, + line_pos: 0, + raw_string: "noqa".to_string(), + action: None, + rules: None, + })), + ), + ("noqa?", Err("")), + ( + "noqa:", + Ok(Some(NoQADirective { + line_no: 0, + line_pos: 0, + raw_string: "noqa:".to_string(), + action: None, + rules: None, + })), + ), + ( + "noqa:LT01,LT02", + Ok(Some(NoQADirective { + line_no: 0, + line_pos: 0, + raw_string: "noqa:LT01,LT02".to_string(), + action: None, + rules: Some( + ["LT01".to_string(), "LT02".to_string()] + .into_iter() + .collect(), + ), + })), + ), + ( + "noqa: enable=LT01", + Ok(Some(NoQADirective { + line_no: 0, + line_pos: 0, + raw_string: "noqa: enable=LT01".to_string(), + action: Some(IgnoreAction::Enable), + rules: Some(["LT01".to_string()].into_iter().collect()), + })), + ), + ( + "noqa: disable=CP01", + Ok(Some(NoQADirective { + line_no: 0, + line_pos: 0, + raw_string: "noqa: disable=CP01".to_string(), + action: Some(IgnoreAction::Disable), + rules: Some(["CP01".to_string()].into_iter().collect()), + })), + ), + ( + "noqa: disable=all", + Ok(Some(NoQADirective { + line_no: 0, + line_pos: 0, + raw_string: "noqa: disable=all".to_string(), + action: Some(IgnoreAction::Disable), + rules: None, + })), + ), + ("noqa: disable", Err("")), + ( + "Inline comment before inline ignore -- noqa:LT01,LT02", + Ok(Some(NoQADirective { + line_no: 0, + line_pos: 0, + raw_string: "Inline comment before inline ignore -- noqa:LT01,LT02".to_string(), + action: None, + rules: Some( + ["LT01".to_string(), "LT02".to_string()] + .into_iter() + .collect(), + ), + })), + ), + // TODO Eventually think about GLOB expansion of rules + // ("noqa:L04*", Some(NoQADirective { line_no: 0, line_pos: 0, raw_string: "noqa:L04*".to_string(), action: None, rules: Some(HashSet::from(["AM04".to_string(), "CP04".to_string(), "CV04".to_string(), "CV05".to_string(), "JJ01".to_string(), "LT01".to_string(), "LT10".to_string(), "ST02".to_string(), "ST03".to_string(), "ST05".to_string()])) })), + // ("noqa:L002", Some(NoQADirective { line_no: 0, line_pos: 0, raw_string: "noqa:L002".to_string(), action: None, rules: Some(HashSet::from(["LT02".to_string()])) })), + // ("noqa:L00*", Some(NoQADirective { line_no: 0, line_pos: 0, raw_string: "noqa:L00*".to_string(), action: None, rules: Some(HashSet::from(["LT01".to_string(), "LT02".to_string(), "LT03".to_string(), "LT12".to_string()])) })), + // TODO Implement these as well + // ("noqa:capitalisation.keywords", Some(NoQADirective { line_no: 0, line_pos: 0, raw_string: "noqa:capitalisation.keywords".to_string(), action: None, rules: Some(HashSet::from(["CP01".to_string()])) })), + // ("noqa:capitalisation", Some(NoQADirective { line_no: 0, line_pos: 0, raw_string: "noqa:capitalisation".to_string(), action: None, rules: Some(HashSet::from(["CP01".to_string(), "CP02".to_string(), "CP03".to_string(), "CP04".to_string(), "CP05".to_string()])) })), + ]; + + for (input, expected) in test_cases { + let result = IgnoreMask::parse_noqa(input, 0, 0); + match expected { + Ok(ref ok) => assert_eq!(result.unwrap(), expected.unwrap()), + Err(_) => { + assert!(result.is_err()); + assert!(result.err().unwrap().fatal); + } + } + } + } + + #[test] + /// Test "noqa" feature at the higher "Linter" level. + fn test_linter_single_noqa() { + let linter = Linter::new( + FluffConfig::from_source( + r#" +dialect: bigquery, +rules: AL02 +"#, + ), + None, + None, + ); + + let sql = r#"SELECT + col_a a, + col_b b --noqa: disable=AL02 +FROM foo +"#; + + let result = linter.lint_string(sql, None, false); + let violations = result.get_violations(None); + + assert_eq!(violations.len(), 1); + assert_eq!( + violations.iter().map(|v| v.line_no).collect::>(), + [2].iter().cloned().collect::>() + ); + } + + #[test] + /// Test "noqa" feature at the higher "Linter" level and turn off noqa + fn test_linter_noqa_but_disabled() { + let linter_without_disabled = Linter::new( + FluffConfig::from_source( + r#" +[sqruff] +dialect = bigquery +rules = AL02 +"#, + ), + None, + None, + ); + let linter_with_disabled = Linter::new( + FluffConfig::from_source( + r#" +[sqruff] +dialect = bigquery +rules = AL02 +disable_noqa = True +"#, + ), + None, + None, + ); + + let sql = r#"SELECT + col_a a, + col_b b --noqa: disable=AL02 +FROM foo +"#; + let result_with_disabled = linter_with_disabled.lint_string(sql, None, false); + let result_without_disabled = linter_without_disabled.lint_string(sql, None, false); + + assert_eq!(result_without_disabled.get_violations(None).len(), 1); + assert_eq!(result_with_disabled.get_violations(None).len(), 2); + } +} diff --git a/editors/code/src/browser.ts b/editors/code/src/browser.ts index be5dcff9..1e1e4ba7 100644 --- a/editors/code/src/browser.ts +++ b/editors/code/src/browser.ts @@ -28,19 +28,13 @@ export function activate(context: vscode.ExtensionContext) { ]; for (const fileEvent of fileEvents) { - fileEvent.onDidChange(() => { - cl.sendRequest("changeConfig"); - }); - fileEvent.onDidCreate(() => { - cl.sendRequest("changeConfig"); - }); - fileEvent.onDidDelete(() => { - cl.sendRequest("deleteConfig"); - }); + fileEvent.onDidChange(() => cl.sendRequest("changeConfig")); + fileEvent.onDidCreate(() => cl.sendRequest("changeConfig")); + fileEvent.onDidDelete(() => cl.sendRequest("deleteConfig")); } cl.onRequest("loadConfig", async () => { - if (vscode.workspace.workspaceFolders === undefined) { + if (!vscode.workspace.workspaceFolders) { return ""; }