-
Notifications
You must be signed in to change notification settings - Fork 2k
Support file-level type: ignore comments
#15081
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,80 +1,31 @@ | ||
| use ruff_db::{files::File, parsed::parsed_module, source::source_text}; | ||
| use ruff_python_parser::TokenKind; | ||
| use ruff_python_trivia::Cursor; | ||
| use ruff_source_file::LineRanges; | ||
| use ruff_text_size::{Ranged, TextRange, TextSize}; | ||
| use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; | ||
| use smallvec::{smallvec, SmallVec}; | ||
|
|
||
| use crate::lint::LintRegistry; | ||
| use crate::{lint::LintId, Db}; | ||
|
|
||
| #[salsa::tracked(return_ref)] | ||
| 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 source = source_text(db.upcast(), file); | ||
|
|
||
| let lints = db.lint_registry(); | ||
|
|
||
| // 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(); | ||
| let mut builder = SuppressionsBuilder::new(&source, db.lint_registry()); | ||
| let mut line_start = TextSize::default(); | ||
|
|
||
| for token in parsed.tokens() { | ||
| if !token.kind().is_trivia() { | ||
| builder.set_seen_non_trivia_token(); | ||
| } | ||
|
|
||
| match token.kind() { | ||
| TokenKind::Comment => { | ||
| let parser = SuppressionParser::new(&source, token.range()); | ||
| let suppressed_range = TextRange::new(line_start, token.range().end()); | ||
|
|
||
| for comment in parser { | ||
| match comment.codes { | ||
| // `type: ignore` | ||
| None => { | ||
| suppressions.push(Suppression { | ||
| target: SuppressionTarget::All, | ||
| comment_range: comment.range, | ||
| range: comment.range, | ||
| suppressed_range, | ||
| }); | ||
| } | ||
|
|
||
| // `type: ignore[..]` | ||
| // The suppression applies to all lints if it is a `type: ignore` | ||
| // comment. `type: ignore` apply to all lints for better mypy compatibility. | ||
| Some(_) if comment.kind.is_type_ignore() => { | ||
| suppressions.push(Suppression { | ||
| target: SuppressionTarget::All, | ||
| comment_range: comment.range, | ||
| range: comment.range, | ||
| suppressed_range, | ||
| }); | ||
| } | ||
|
|
||
| // `knot: ignore[a, b]` | ||
| Some(codes) => { | ||
| for code in &codes { | ||
| match lints.get(&source[*code]) { | ||
| Ok(lint) => { | ||
| let range = if codes.len() == 1 { | ||
| comment.range | ||
| } else { | ||
| *code | ||
| }; | ||
|
|
||
| suppressions.push(Suppression { | ||
| target: SuppressionTarget::Lint(lint), | ||
| range, | ||
| comment_range: comment.range, | ||
| suppressed_range, | ||
| }); | ||
| } | ||
| Err(error) => { | ||
| tracing::debug!("Invalid suppression: {error}"); | ||
| // TODO(micha): Handle invalid lint codes | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| builder.add_comment(comment, line_start); | ||
| } | ||
| } | ||
| TokenKind::Newline | TokenKind::NonLogicalNewline => { | ||
|
|
@@ -84,34 +35,46 @@ pub(crate) fn suppressions(db: &dyn Db, file: File) -> Suppressions { | |
| } | ||
| } | ||
|
|
||
| Suppressions { suppressions } | ||
| builder.finish() | ||
| } | ||
|
|
||
| /// The suppression of a single file. | ||
| /// The suppressions of a single file. | ||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub(crate) struct Suppressions { | ||
| /// The suppressions sorted by the suppressed range. | ||
| /// Suppressions that apply to the entire file. | ||
| /// | ||
| /// The suppressions are sorted by [`Suppression::comment_range`] and the [`Suppression::suppressed_range`] | ||
| /// spans the entire file. | ||
| /// | ||
| /// For now, this is limited to `type: ignore` comments. | ||
| file: Vec<Suppression>, | ||
|
|
||
| /// Suppressions that apply to a specific line (or lines). | ||
| /// | ||
| /// It's possible that multiple suppressions apply for the same range. | ||
| suppressions: Vec<Suppression>, | ||
| /// Comments with multiple codes create multiple [`Suppression`]s that all share the same [`Suppression::comment_range`]. | ||
| /// | ||
| /// The suppressions are sorted by [`Suppression::range`] (which implies [`Suppression::comment_range`]). | ||
| line: Vec<Suppression>, | ||
| } | ||
|
|
||
| impl Suppressions { | ||
| pub(crate) fn find_suppression(&self, range: TextRange, id: LintId) -> Option<&Suppression> { | ||
| self.for_range(range) | ||
| self.file | ||
| .iter() | ||
| .chain(self.line_suppressions(range)) | ||
| .find(|suppression| suppression.matches(id)) | ||
| } | ||
|
|
||
| /// Returns all suppression comments that apply for `range`. | ||
| /// Returns the line-level suppressions 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> + '_ { | ||
| fn line_suppressions(&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 | ||
| .line | ||
| .binary_search_by_key(&range.start(), |suppression| { | ||
| suppression.suppressed_range.end() | ||
| }) | ||
|
|
@@ -120,7 +83,7 @@ impl Suppressions { | |
| // 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..] | ||
| self.line[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()) | ||
|
|
@@ -177,6 +140,116 @@ enum SuppressionTarget { | |
| Lint(LintId), | ||
| } | ||
|
|
||
| struct SuppressionsBuilder<'a> { | ||
| lint_registry: &'a LintRegistry, | ||
| source: &'a str, | ||
|
|
||
| /// `type: ignore` comments at the top of the file before any non-trivia code apply to the entire file. | ||
| /// This boolean tracks if there has been any non trivia token. | ||
| seen_non_trivia_token: bool, | ||
|
|
||
| line: Vec<Suppression>, | ||
| file: Vec<Suppression>, | ||
| } | ||
|
|
||
| impl<'a> SuppressionsBuilder<'a> { | ||
| fn new(source: &'a str, lint_registry: &'a LintRegistry) -> Self { | ||
| Self { | ||
| source, | ||
| lint_registry, | ||
| seen_non_trivia_token: false, | ||
| line: Vec::new(), | ||
| file: Vec::new(), | ||
| } | ||
| } | ||
|
|
||
| fn set_seen_non_trivia_token(&mut self) { | ||
| self.seen_non_trivia_token = true; | ||
| } | ||
|
|
||
| fn finish(mut self) -> Suppressions { | ||
| self.line.shrink_to_fit(); | ||
| self.file.shrink_to_fit(); | ||
|
|
||
| Suppressions { | ||
| file: self.file, | ||
| line: self.line, | ||
| } | ||
| } | ||
|
|
||
| fn add_comment(&mut self, comment: SuppressionComment, line_start: TextSize) { | ||
| let (suppressions, suppressed_range) = | ||
| // `type: ignore` comments at the start of the file apply to the entire range. | ||
| // > A # type: ignore comment on a line by itself at the top of a file, before any docstrings, | ||
| // > imports, or other executable code, silences all errors in the file. | ||
| // > Blank lines and other comments, such as shebang lines and coding cookies, | ||
| // > may precede the # type: ignore comment. | ||
| // > https://typing.readthedocs.io/en/latest/spec/directives.html#type-ignore-comments | ||
| if comment.kind.is_type_ignore() && !self.seen_non_trivia_token { | ||
| ( | ||
| &mut self.file, | ||
| TextRange::new(0.into(), self.source.text_len()), | ||
| ) | ||
| } else { | ||
| ( | ||
| &mut self.line, | ||
| TextRange::new(line_start, comment.range.end()), | ||
| ) | ||
| }; | ||
|
Comment on lines
+181
to
+198
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only new code in this block. Everything else was extracted from the |
||
|
|
||
| match comment.codes { | ||
| // `type: ignore` | ||
| None => { | ||
| suppressions.push(Suppression { | ||
| target: SuppressionTarget::All, | ||
| comment_range: comment.range, | ||
| range: comment.range, | ||
| suppressed_range, | ||
| }); | ||
| } | ||
|
|
||
| // `type: ignore[..]` | ||
| // The suppression applies to all lints if it is a `type: ignore` | ||
| // comment. `type: ignore` apply to all lints for better mypy compatibility. | ||
| Some(_) if comment.kind.is_type_ignore() => { | ||
| suppressions.push(Suppression { | ||
| target: SuppressionTarget::All, | ||
| comment_range: comment.range, | ||
| range: comment.range, | ||
| suppressed_range, | ||
| }); | ||
| } | ||
|
|
||
| // `knot: ignore[a, b]` | ||
| Some(codes) => { | ||
| for code_range in &codes { | ||
| let code = &self.source[*code_range]; | ||
| match self.lint_registry.get(code) { | ||
| Ok(lint) => { | ||
| let range = if codes.len() == 1 { | ||
| comment.range | ||
| } else { | ||
| *code_range | ||
| }; | ||
|
|
||
| suppressions.push(Suppression { | ||
| target: SuppressionTarget::Lint(lint), | ||
| range, | ||
| comment_range: comment.range, | ||
| suppressed_range, | ||
| }); | ||
| } | ||
| Err(error) => { | ||
| tracing::debug!("Invalid suppression: {error}"); | ||
| // TODO(micha): Handle invalid lint codes | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| struct SuppressionParser<'src> { | ||
| cursor: Cursor<'src>, | ||
| range: TextRange, | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this emit a diagnostic? Or not necessary, because it just won't work and that will be obvious from other diagnostics in the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets flagged by the
unused-ignore-commentrulehttps://github.com/astral-sh/ruff/blob/micha/unused-ignore-comment/crates/red_knot_python_semantic/resources/mdtest/suppressions/type-ignore.md#invalid-own-line-suppression