From 8d327087efd09ae55007910ab8e66e2f27f91cfe Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 23 Dec 2024 11:38:10 +0100 Subject: [PATCH] Add `invalid-ignore-comment` rule (#15094) --- .../mdtest/suppressions/knot-ignore.md | 10 + .../mdtest/suppressions/type-ignore.md | 1 + crates/red_knot_python_semantic/src/lib.rs | 3 +- .../src/suppression.rs | 199 +++++++++++++++--- 4 files changed, 188 insertions(+), 25 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/suppressions/knot-ignore.md b/crates/red_knot_python_semantic/resources/mdtest/suppressions/knot-ignore.md index fea52e14e4168..f4bc7612f9c09 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/suppressions/knot-ignore.md +++ b/crates/red_knot_python_semantic/resources/mdtest/suppressions/knot-ignore.md @@ -117,6 +117,7 @@ a = 10 / 0 # knot: ignore[division-by-zero,] ```py # error: [division-by-zero] +# error: [invalid-ignore-comment] "Invalid `knot: ignore` comment: expected a alphanumeric character or `-` or `_` as code" a = 10 / 0 # knot: ignore[*-*] ``` @@ -138,9 +139,18 @@ future. ```py # error: [unresolved-reference] +# error: [invalid-ignore-comment] "Invalid `knot: ignore` comment: expected a comma separating the rule codes" a = x / 0 # knot: ignore[division-by-zero unresolved-reference] ``` +## Missing closing bracket + +```py +# error: [unresolved-reference] "Name `x` used when not defined" +# error: [invalid-ignore-comment] "Invalid `knot: ignore` comment: expected a comma separating the rule codes" +a = x / 2 # knot: ignore[unresolved-reference +``` + ## Empty codes An empty codes array suppresses no-diagnostics and is always useless diff --git a/crates/red_knot_python_semantic/resources/mdtest/suppressions/type-ignore.md b/crates/red_knot_python_semantic/resources/mdtest/suppressions/type-ignore.md index 380d5c34d5d5a..ba7e1f30ad68a 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/suppressions/type-ignore.md +++ b/crates/red_knot_python_semantic/resources/mdtest/suppressions/type-ignore.md @@ -110,6 +110,7 @@ a = test \ ```py # error: [unresolved-reference] +# error: [invalid-ignore-comment] a = test + 2 # type: ignoree ``` diff --git a/crates/red_knot_python_semantic/src/lib.rs b/crates/red_knot_python_semantic/src/lib.rs index 797b1cc4410de..f65c54cfb7264 100644 --- a/crates/red_knot_python_semantic/src/lib.rs +++ b/crates/red_knot_python_semantic/src/lib.rs @@ -3,7 +3,7 @@ use std::hash::BuildHasherDefault; use rustc_hash::FxHasher; use crate::lint::{LintRegistry, LintRegistryBuilder}; -use crate::suppression::{UNKNOWN_RULE, UNUSED_IGNORE_COMMENT}; +use crate::suppression::{INVALID_IGNORE_COMMENT, UNKNOWN_RULE, UNUSED_IGNORE_COMMENT}; pub use db::Db; pub use module_name::ModuleName; pub use module_resolver::{resolve_module, system_module_search_paths, KnownModule, Module}; @@ -50,4 +50,5 @@ pub fn register_lints(registry: &mut LintRegistryBuilder) { types::register_lints(registry); registry.register_lint(&UNUSED_IGNORE_COMMENT); registry.register_lint(&UNKNOWN_RULE); + registry.register_lint(&INVALID_IGNORE_COMMENT); } diff --git a/crates/red_knot_python_semantic/src/suppression.rs b/crates/red_knot_python_semantic/src/suppression.rs index f987c1993cd8b..6816e07f0965e 100644 --- a/crates/red_knot_python_semantic/src/suppression.rs +++ b/crates/red_knot_python_semantic/src/suppression.rs @@ -7,8 +7,10 @@ use ruff_python_parser::TokenKind; use ruff_python_trivia::Cursor; use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; use smallvec::{smallvec, SmallVec}; +use std::error::Error; use std::fmt; use std::fmt::Formatter; +use thiserror::Error; declare_lint! { /// ## What it does @@ -60,6 +62,30 @@ declare_lint! { } } +declare_lint! { + /// ## What it does + /// Checks for `type: ignore` and `knot: ignore` comments that are syntactically incorrect. + /// + /// ## Why is this bad? + /// A syntactically incorrect ignore comment is probably a mistake and is useless. + /// + /// ## Examples + /// ```py + /// a = 20 / 0 # type: ignoree + /// ``` + /// + /// Use instead: + /// + /// ```py + /// a = 20 / 0 # type: ignore + /// ``` + pub(crate) static INVALID_IGNORE_COMMENT = { + summary: "detects ignore comments that use invalid syntax", + status: LintStatus::preview("1.0.0"), + default_level: Level::Warn, + } +} + #[salsa::tracked(return_ref)] pub(crate) fn suppressions(db: &dyn Db, file: File) -> Suppressions { let parsed = parsed_module(db.upcast(), file); @@ -78,7 +104,23 @@ pub(crate) fn suppressions(db: &dyn Db, file: File) -> Suppressions { let parser = SuppressionParser::new(&source, token.range()); for comment in parser { - builder.add_comment(&comment, TextRange::new(line_start, token.end())); + match comment { + Ok(comment) => { + builder.add_comment(&comment, TextRange::new(line_start, token.end())); + } + Err(error) => match error.kind { + ParseErrorKind::NotASuppression + | ParseErrorKind::CommentWithoutHash => { + // Skip non suppression comments and comments that miss a hash (shouldn't ever happen) + } + ParseErrorKind::NoWhitespaceAfterIgnore(kind) + | ParseErrorKind::CodesMissingComma(kind) + | ParseErrorKind::InvalidCode(kind) + | ParseErrorKind::CodesMissingClosingBracket(kind) => { + builder.add_invalid_comment(kind, error); + } + }, + } } } TokenKind::Newline | TokenKind::NonLogicalNewline => { @@ -95,6 +137,7 @@ pub(crate) fn check_suppressions(db: &dyn Db, file: File, diagnostics: &mut Type let mut context = CheckSuppressionsContext::new(db, file, diagnostics); check_unknown_rule(&mut context); + check_invalid_suppression(&mut context); check_unused_suppressions(&mut context); } @@ -124,6 +167,24 @@ fn check_unknown_rule(context: &mut CheckSuppressionsContext) { } } +fn check_invalid_suppression(context: &mut CheckSuppressionsContext) { + if context.is_lint_disabled(&INVALID_IGNORE_COMMENT) { + return; + } + + for invalid in &context.suppressions.invalid { + context.report_lint( + &INVALID_IGNORE_COMMENT, + invalid.error.range, + format_args!( + "Invalid `{kind}` comment: {reason}", + kind = invalid.kind, + reason = &invalid.error + ), + ); + } +} + /// Checks for unused suppression comments in `file` and /// adds diagnostic for each of them to `diagnostics`. /// @@ -246,6 +307,7 @@ impl<'a> CheckSuppressionsContext<'a> { let Some(severity) = self.db.rule_selection().severity(LintId::of(lint)) else { return; }; + self.diagnostics.push(TypeCheckDiagnostic { id: DiagnosticId::Lint(lint.name()), message: message.to_string(), @@ -276,6 +338,9 @@ pub(crate) struct Suppressions { /// Suppressions with lint codes that are unknown. unknown: Vec, + + /// Suppressions that are syntactically invalid. + invalid: Vec, } impl Suppressions { @@ -421,6 +486,7 @@ struct SuppressionsBuilder<'a> { line: Vec, file: SmallVec<[Suppression; 1]>, unknown: Vec, + invalid: Vec, } impl<'a> SuppressionsBuilder<'a> { @@ -432,6 +498,7 @@ impl<'a> SuppressionsBuilder<'a> { line: Vec::new(), file: SmallVec::new(), unknown: Vec::new(), + invalid: Vec::new(), } } @@ -443,11 +510,13 @@ impl<'a> SuppressionsBuilder<'a> { self.line.shrink_to_fit(); self.file.shrink_to_fit(); self.unknown.shrink_to_fit(); + self.invalid.shrink_to_fit(); Suppressions { file: self.file, line: self.line, unknown: self.unknown, + invalid: self.invalid, } } @@ -540,6 +609,10 @@ impl<'a> SuppressionsBuilder<'a> { } } } + + fn add_invalid_comment(&mut self, kind: SuppressionKind, error: ParseError) { + self.invalid.push(InvalidSuppression { kind, error }); + } } /// Suppression for an unknown lint rule. @@ -554,6 +627,12 @@ struct UnknownSuppression { reason: GetLintError, } +#[derive(Debug, PartialEq, Eq)] +struct InvalidSuppression { + kind: SuppressionKind, + error: ParseError, +} + struct SuppressionParser<'src> { cursor: Cursor<'src>, range: TextRange, @@ -566,36 +645,41 @@ impl<'src> SuppressionParser<'src> { Self { cursor, range } } - fn parse_comment(&mut self) -> Option { + fn parse_comment(&mut self) -> Result { let comment_start = self.offset(); self.cursor.start_token(); if !self.cursor.eat_char('#') { - return None; + return self.syntax_error(ParseErrorKind::CommentWithoutHash); } self.eat_whitespace(); // type: ignore[code] // ^^^^^^^^^^^^ - let kind = self.eat_kind()?; + let Some(kind) = self.eat_kind() else { + return Err(ParseError::new( + ParseErrorKind::NotASuppression, + TextRange::new(comment_start, self.offset()), + )); + }; let has_trailing_whitespace = self.eat_whitespace(); // type: ignore[code1, code2] // ^^^^^^ - let codes = self.eat_codes(); + let codes = self.eat_codes(kind)?; if self.cursor.is_eof() || codes.is_some() || has_trailing_whitespace { // Consume the comment until its end or until the next "sub-comment" starts. self.cursor.eat_while(|c| c != '#'); - Some(SuppressionComment { + Ok(SuppressionComment { kind, codes, range: TextRange::at(comment_start, self.cursor.token_len()), }) } else { - None + self.syntax_error(ParseErrorKind::NoWhitespaceAfterIgnore(kind)) } } @@ -627,28 +711,31 @@ impl<'src> SuppressionParser<'src> { Some(kind) } - fn eat_codes(&mut self) -> Option> { + fn eat_codes( + &mut self, + kind: SuppressionKind, + ) -> Result>, ParseError> { if !self.cursor.eat_char('[') { - return None; + return Ok(None); } let mut codes: SmallVec<[TextRange; 2]> = smallvec![]; loop { if self.cursor.is_eof() { - return None; + return self.syntax_error(ParseErrorKind::CodesMissingClosingBracket(kind)); } self.eat_whitespace(); // `knot: ignore[]` or `knot: ignore[a,]` if self.cursor.eat_char(']') { - break Some(codes); + break Ok(Some(codes)); } let code_start = self.offset(); if !self.eat_word() { - return None; + return self.syntax_error(ParseErrorKind::InvalidCode(kind)); } codes.push(TextRange::new(code_start, self.offset())); @@ -659,10 +746,10 @@ impl<'src> SuppressionParser<'src> { self.eat_whitespace(); if self.cursor.eat_char(']') { - break Some(codes); + break Ok(Some(codes)); } // `knot: ignore[a b] - return None; + return self.syntax_error(ParseErrorKind::CodesMissingComma(kind)); } } } @@ -686,25 +773,35 @@ impl<'src> SuppressionParser<'src> { } } + fn syntax_error(&self, kind: ParseErrorKind) -> Result { + let len = if self.cursor.is_eof() { + TextSize::default() + } else { + self.cursor.first().text_len() + }; + + Err(ParseError::new(kind, TextRange::at(self.offset(), len))) + } + fn offset(&self) -> TextSize { self.range.start() + self.range.len() - self.cursor.text_len() } } impl Iterator for SuppressionParser<'_> { - type Item = SuppressionComment; + type Item = Result; fn next(&mut self) -> Option { - loop { - if self.cursor.is_eof() { - return None; - } + if self.cursor.is_eof() { + return None; + } - if let Some(suppression) = self.parse_comment() { - return Some(suppression); + match self.parse_comment() { + Ok(result) => Some(Ok(result)), + Err(error) => { + self.cursor.eat_while(|c| c != '#'); + Some(Err(error)) } - - self.cursor.eat_while(|c| c != '#'); } } } @@ -761,6 +858,58 @@ impl fmt::Display for SuppressionKind { } } +#[derive(Debug, Eq, PartialEq, Clone)] +struct ParseError { + kind: ParseErrorKind, + + /// The position/range at which the parse error occurred. + range: TextRange, +} + +impl ParseError { + fn new(kind: ParseErrorKind, range: TextRange) -> Self { + Self { kind, range } + } +} + +impl fmt::Display for ParseError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + self.kind.fmt(f) + } +} + +impl Error for ParseError {} + +#[derive(Debug, Eq, PartialEq, Clone, Error)] +enum ParseErrorKind { + /// The comment isn't a suppression comment. + #[error("not a suppression comment")] + NotASuppression, + + #[error("the comment doesn't start with a `#`")] + CommentWithoutHash, + + /// A valid suppression `type: ignore` but it misses a whitespaces after the `ignore` keyword. + /// + /// ```py + /// type: ignoree + /// ``` + #[error("no whitespace after `ignore`")] + NoWhitespaceAfterIgnore(SuppressionKind), + + /// Missing comma between two codes + #[error("expected a comma separating the rule codes")] + CodesMissingComma(SuppressionKind), + + /// `knot: ignore[*.*]` + #[error("expected a alphanumeric character or `-` or `_` as code")] + InvalidCode(SuppressionKind), + + /// `knot: ignore[a, b` + #[error("expected a closing bracket")] + CodesMissingClosingBracket(SuppressionKind), +} + #[cfg(test)] mod tests { use crate::suppression::{SuppressionComment, SuppressionParser}; @@ -964,7 +1113,9 @@ mod tests { for comment in SuppressionParser::new( self.source, TextRange::new(0.into(), self.source.text_len()), - ) { + ) + .flatten() + { list.entry(&comment.debug(self.source)); }