From 913bce3cd5a653cd6dd97d07889e6ed93272a2be Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 20 Dec 2024 10:35:09 +0100 Subject: [PATCH] Basic support for `type: ignore` comments (#15046) ## Summary This PR adds initial support for `type: ignore`. It doesn't do anything fancy yet like: * Detecting invalid type ignore comments * Detecting type ignore comments that are part of another suppression comment: `# fmt: skip # type: ignore` * Suppressing specific lints `type: ignore [code]` * Detecting unsused type ignore comments * ... The goal is to add this functionality in separate PRs. ## Test Plan --------- Co-authored-by: Carl Meyer Co-authored-by: Alex Waygood --- .../mdtest/suppressions/type-ignore.md | 128 ++++++++++++++++++ crates/red_knot_python_semantic/src/lib.rs | 1 + .../src/suppression.rs | 122 ++++++++++++----- .../src/types/context.rs | 10 ++ crates/red_knot_test/src/lib.rs | 10 +- 5 files changed, 234 insertions(+), 37 deletions(-) create mode 100644 crates/red_knot_python_semantic/resources/mdtest/suppressions/type-ignore.md 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 new file mode 100644 index 0000000000000..23f8ec628fa45 --- /dev/null +++ b/crates/red_knot_python_semantic/resources/mdtest/suppressions/type-ignore.md @@ -0,0 +1,128 @@ +# Suppressing errors with `type: ignore` + +Type check errors can be suppressed by a `type: ignore` comment on the same line as the violation. + +## Simple `type: ignore` + +```py +a = 4 + test # type: ignore +``` + +## Multiline ranges + +A diagnostic with a multiline range can be suppressed by a comment on the same line as the +diagnostic's start or end. This is the same behavior as Mypy's. + +```py +# fmt: off +y = ( + 4 / 0 # type: ignore +) + +y = ( + 4 / # type: ignore + 0 +) + +y = ( + 4 / + 0 # type: ignore +) +``` + +Pyright diverges from this behavior and instead applies a suppression if its range intersects with +the diagnostic range. This can be problematic for nested expressions because a suppression in a +child expression now suppresses errors in the outer expression. + +For example, the `type: ignore` comment in this example suppresses the error of adding `2` to +`"test"` and adding `"other"` to the result of the cast. + +```py path=nested.py +# fmt: off +from typing import cast + +y = ( + cast(int, "test" + + 2 # type: ignore + ) + + "other" # TODO: expected-error[invalid-operator] +) +``` + +Mypy flags the second usage. + +## Before opening parenthesis + +A suppression that applies to all errors before the opening parenthesis. + +```py +a: Test = ( # type: ignore + Test() # error: [unresolved-reference] +) # fmt: skip +``` + +## Multiline string + +```py +a: int = 4 +a = """ + This is a multiline string and the suppression is at its end +""" # type: ignore +``` + +## Line continuations + +Suppressions after a line continuation apply to all previous lines. + +```py +# fmt: off +a = test \ + + 2 # type: ignore + +a = test \ + + a \ + + 2 # type: ignore +``` + +## Codes + +Mypy supports `type: ignore[code]`. Red Knot doesn't understand mypy's rule names. Therefore, ignore +the codes and suppress all errors. + +```py +a = test # type: ignore[name-defined] +``` + +## Nested comments + +TODO: We should support this for better interopability with other suppression comments. + +```py +# fmt: off +# TODO this error should be suppressed +# error: [unresolved-reference] +a = test \ + + 2 # fmt: skip # type: ignore + +a = test \ + + 2 # type: ignore # fmt: skip +``` + +## Misspelled `type: ignore` + +```py +# error: [unresolved-reference] +a = test + 2 # type: ignoree +``` + +## Invalid - ignore on opening parentheses + +`type: ignore` comments after an opening parentheses suppress any type errors inside the parentheses +in Pyright. Neither Ruff, nor mypy support this and neither does Red Knot. + +```py +# fmt: off +a = ( # type: ignore + test + 4 # error: [unresolved-reference] +) +``` diff --git a/crates/red_knot_python_semantic/src/lib.rs b/crates/red_knot_python_semantic/src/lib.rs index 324d2756b8c55..2199163554bb2 100644 --- a/crates/red_knot_python_semantic/src/lib.rs +++ b/crates/red_knot_python_semantic/src/lib.rs @@ -22,6 +22,7 @@ pub mod semantic_index; mod semantic_model; pub(crate) mod site_packages; mod stdlib; +mod suppression; pub(crate) mod symbol; pub mod types; mod unpack; diff --git a/crates/red_knot_python_semantic/src/suppression.rs b/crates/red_knot_python_semantic/src/suppression.rs index c87cbfc8bc7e0..69e266f87cb21 100644 --- a/crates/red_knot_python_semantic/src/suppression.rs +++ b/crates/red_knot_python_semantic/src/suppression.rs @@ -1,50 +1,104 @@ -use salsa; +use ruff_python_parser::TokenKind; +use ruff_source_file::LineRanges; +use ruff_text_size::{Ranged, TextRange}; -use ruff_db::{files::File, parsed::comment_ranges, source::source_text}; -use ruff_index::{newtype_index, IndexVec}; +use ruff_db::{files::File, parsed::parsed_module, source::source_text}; use crate::{lint::LintId, Db}; #[salsa::tracked(return_ref)] -pub(crate) fn suppressions(db: &dyn Db, file: File) -> IndexVec { - let comments = comment_ranges(db.upcast(), file); +pub(crate) fn suppressions(db: &dyn Db, file: File) -> Suppressions { let source = source_text(db.upcast(), file); + let parsed = parsed_module(db.upcast(), file); - let mut suppressions = IndexVec::default(); - - for range in comments { - let text = &source[range]; - - if text.starts_with("# type: ignore") { - suppressions.push(Suppression { - target: None, - kind: SuppressionKind::TypeIgnore, - }); - } else if text.starts_with("# knot: ignore") { - suppressions.push(Suppression { - target: None, - kind: SuppressionKind::KnotIgnore, - }); + // TODO: Support `type: ignore` comments at the + // [start of the file](https://typing.readthedocs.io/en/latest/spec/directives.html#type-ignore-comments). + let mut suppressions = Vec::default(); + let mut line_start = source.bom_start_offset(); + + for token in parsed.tokens() { + match token.kind() { + TokenKind::Comment => { + let text = &source[token.range()]; + + let suppressed_range = TextRange::new(line_start, token.end()); + + if text.strip_prefix("# type: ignore").is_some_and(|suffix| { + suffix.is_empty() + || suffix.starts_with(char::is_whitespace) + || suffix.starts_with('[') + }) { + suppressions.push(Suppression { suppressed_range }); + } + } + TokenKind::Newline | TokenKind::NonLogicalNewline => { + line_start = token.end(); + } + _ => {} } } - suppressions + Suppressions { suppressions } } -#[newtype_index] -pub(crate) struct SuppressionIndex; - -#[derive(Clone, Debug, Eq, PartialEq, Hash)] -pub(crate) struct Suppression { - target: Option, - kind: SuppressionKind, +/// The suppression comments of a single file. +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) struct Suppressions { + /// The suppressions sorted by the suppressed range. + suppressions: Vec, } -#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)] -pub(crate) enum SuppressionKind { - /// A `type: ignore` comment - TypeIgnore, +impl Suppressions { + /// Finds a suppression for the specified lint. + /// + /// Returns the first matching suppression if more than one suppression apply to `range` and `id`. + /// + /// Returns `None` if the lint isn't suppressed. + pub(crate) fn find_suppression(&self, range: TextRange, _id: LintId) -> Option<&Suppression> { + // TODO(micha): + // * Test if the suppression suppresses the passed lint + self.for_range(range).next() + } - /// A `knot: ignore` comment - KnotIgnore, + /// Returns all suppression comments that apply for `range`. + /// + /// A suppression applies for the given range if it contains the range's + /// start or end offset. This means the suppression is on the same line + /// as the diagnostic's start or end. + fn for_range(&self, range: TextRange) -> impl Iterator + '_ { + // First find the index of the suppression comment that ends right before the range + // starts. This allows us to skip suppressions that are not relevant for the range. + let end_offset = self + .suppressions + .binary_search_by_key(&range.start(), |suppression| { + suppression.suppressed_range.end() + }) + .unwrap_or_else(|index| index); + + // From here, search the remaining suppression comments for one that + // contains the range's start or end offset. Stop the search + // as soon as the suppression's range and the range no longer overlap. + self.suppressions[end_offset..] + .iter() + // Stop searching if the suppression starts after the range we're looking for. + .take_while(move |suppression| range.end() >= suppression.suppressed_range.start()) + .filter(move |suppression| { + // Don't use intersect to avoid that suppressions on inner-expression + // ignore errors for outer expressions + suppression.suppressed_range.contains(range.start()) + || suppression.suppressed_range.contains(range.end()) + }) + } +} + +/// A `type: ignore` or `knot: ignore` suppression comment. +#[derive(Clone, Debug, Eq, PartialEq, Hash)] +pub(crate) struct Suppression { + /// The range for which this suppression applies. + /// Most of the time, this is the range of the comment's line. + /// However, there are few cases where the range gets expanded to + /// cover multiple lines: + /// * multiline strings: `expr + """multiline\nstring""" # type: ignore` + /// * line continuations: `expr \ + "test" # type: ignore` + suppressed_range: TextRange, } diff --git a/crates/red_knot_python_semantic/src/types/context.rs b/crates/red_knot_python_semantic/src/types/context.rs index efd0c651bee7c..9c98fdd6504b9 100644 --- a/crates/red_knot_python_semantic/src/types/context.rs +++ b/crates/red_knot_python_semantic/src/types/context.rs @@ -10,6 +10,7 @@ use ruff_text_size::Ranged; use crate::{ lint::{LintId, LintMetadata}, + suppression::suppressions, Db, }; @@ -74,6 +75,15 @@ impl<'db> InferContext<'db> { return; }; + let suppressions = suppressions(self.db, self.file); + + if suppressions + .find_suppression(node.range(), LintId::of(lint)) + .is_some() + { + return; + } + self.report_diagnostic(node, DiagnosticId::Lint(lint.name()), severity, message); } diff --git a/crates/red_knot_test/src/lib.rs b/crates/red_knot_test/src/lib.rs index f2a7639e29ec7..eaa59e6c283d9 100644 --- a/crates/red_knot_test/src/lib.rs +++ b/crates/red_knot_test/src/lib.rs @@ -97,7 +97,11 @@ fn run_test(db: &mut db::Db, test: &parser::MarkdownTest) -> Result<(), Failures let test_files: Vec<_> = test .files() - .map(|embedded| { + .filter_map(|embedded| { + if embedded.lang == "ignore" { + return None; + } + assert!( matches!(embedded.lang, "py" | "pyi"), "Non-Python files not supported yet." @@ -106,10 +110,10 @@ fn run_test(db: &mut db::Db, test: &parser::MarkdownTest) -> Result<(), Failures db.write_file(&full_path, embedded.code).unwrap(); let file = system_path_to_file(db, full_path).unwrap(); - TestFile { + Some(TestFile { file, backtick_offset: embedded.md_offset, - } + }) }) .collect();