Skip to content

Commit

Permalink
Basic support for type: ignore comments (#15046)
Browse files Browse the repository at this point in the history
## 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 <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
  • Loading branch information
3 people authored Dec 20, 2024
1 parent 6195c02 commit 913bce3
Show file tree
Hide file tree
Showing 5 changed files with 234 additions and 37 deletions.
Original file line number Diff line number Diff line change
@@ -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]
)
```
1 change: 1 addition & 0 deletions crates/red_knot_python_semantic/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
122 changes: 88 additions & 34 deletions crates/red_knot_python_semantic/src/suppression.rs
Original file line number Diff line number Diff line change
@@ -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<SuppressionIndex, Suppression> {
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<LintId>,
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<Suppression>,
}

#[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<Item = &Suppression> + '_ {
// 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,
}
10 changes: 10 additions & 0 deletions crates/red_knot_python_semantic/src/types/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use ruff_text_size::Ranged;

use crate::{
lint::{LintId, LintMetadata},
suppression::suppressions,
Db,
};

Expand Down Expand Up @@ -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);
}

Expand Down
10 changes: 7 additions & 3 deletions crates/red_knot_test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand All @@ -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();

Expand Down

0 comments on commit 913bce3

Please sign in to comment.