From b109c772320e6825a01cd65ebac8b52e56f05057 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mikko=20Lepp=C3=A4nen?= Date: Tue, 16 Jun 2026 07:57:12 +0300 Subject: [PATCH] feat(ignore): add invalid-ignore-comment and ignore-without-code rules Malformed ignore comments were silently discarded: a typo'd directive (`# pyrefly: ignoree`), an unterminated `[`, or an empty `[]` produced neither a suppression nor any warning, so a developer never learned the comment was wrong. Separately, a bare `# pyrefly: ignore` suppresses any error on its line, which can silently swallow a later, unrelated error. Turn the ignore-comment parser into a single recognizer with three disjoint outcomes (valid / malformed / not-a-directive) so exactly one fires and a non-directive such as `# type: list[int]`, which never reaches the `ignore` stem, can never be flagged. A malformed result still carries the suppression it produced before, so the new invalid-ignore-comment diagnostic (default `warn`) is purely additive and no existing suppression behavior changes. ignore-without-code is opt-in (default off) and scoped to the Pyrefly tool, the only one whose per-line codes Pyrefly honors. Both kinds are unsuppressable, enforced centrally so an ignore comment cannot silence the lint about itself. Refs: facebook/pyrefly#3752, facebook/pyrefly#3450 --- crates/pyrefly_config/src/error_kind.rs | 26 ++ crates/pyrefly_python/src/ignore.rs | 388 ++++++++++++++++++++---- pyrefly/lib/commands/buck_check.rs | 6 +- pyrefly/lib/commands/check.rs | 8 +- pyrefly/lib/commands/suppress.rs | 11 +- pyrefly/lib/error/collector.rs | 18 +- pyrefly/lib/error/error.rs | 8 +- pyrefly/lib/error/suppress.rs | 8 + pyrefly/lib/lsp/non_wasm/server.rs | 4 +- pyrefly/lib/playground.rs | 4 +- pyrefly/lib/state/errors.rs | 237 +++++++++++++-- test/ignores.md | 67 ++++ website/docs/error-kinds.mdx | 28 ++ 13 files changed, 708 insertions(+), 105 deletions(-) diff --git a/crates/pyrefly_config/src/error_kind.rs b/crates/pyrefly_config/src/error_kind.rs index 0159f978f9..ab51ca533c 100644 --- a/crates/pyrefly_config/src/error_kind.rs +++ b/crates/pyrefly_config/src/error_kind.rs @@ -156,6 +156,11 @@ pub enum ErrorKind { DivisionByZero, /// Explicit usage of `typing.Any` in an annotation. ExplicitAny, + /// A `# pyrefly: ignore` comment suppresses errors without naming a specific + /// error code. Off by default; enable it to require every Pyrefly + /// suppression to list the code(s) it silences, e.g. + /// `# pyrefly: ignore[bad-return]`. + IgnoreWithoutCode, /// Raised when a class implicitly becomes abstract by defining abstract members without /// inheriting from `abc.ABC` or using `abc.ABCMeta`. ImplicitAbstractClass, @@ -214,6 +219,11 @@ pub enum ErrorKind { /// is reported under `BadClassDefinition` or `BadFunctionDefinition` /// instead, both of which default to `error`. InvalidDecorator, + /// A comment looks like a type checker ignore directive but is malformed, so + /// it silently suppresses nothing (or not what was intended). For example + /// `# pyrefly: ignoree` (typo), `# pyrefly: ignore[bad-code` (unterminated + /// bracket), or `# pyrefly: ignore[]` (empty code list). + InvalidIgnoreComment, /// An error caused by incorrect inheritance in a class or type definition. /// e.g. a metaclass that is not a subclass of `type`. InvalidInheritance, @@ -464,6 +474,7 @@ impl ErrorKind { ErrorKind::Deprecated => Severity::Warn, ErrorKind::DivisionByZero => Severity::Warn, ErrorKind::ExplicitAny => Severity::Ignore, + ErrorKind::IgnoreWithoutCode => Severity::Ignore, ErrorKind::ImplicitAbstractClass => Severity::Ignore, ErrorKind::ImplicitAny => Severity::Ignore, ErrorKind::ImplicitAnyAttribute => Severity::Ignore, @@ -474,6 +485,7 @@ impl ErrorKind { ErrorKind::ImplicitlyDefinedAttribute => Severity::Ignore, ErrorKind::IncompatibleComparison => Severity::Ignore, ErrorKind::InvalidDecorator => Severity::Warn, + ErrorKind::InvalidIgnoreComment => Severity::Warn, ErrorKind::MissingOverrideDecorator => Severity::Ignore, ErrorKind::MissingSource => Severity::Ignore, ErrorKind::NameMismatch => Severity::Warn, @@ -512,6 +524,20 @@ impl ErrorKind { matches!(self, ErrorKind::RevealType) } + /// Diagnostics about ignore comments themselves. They cannot be silenced by + /// an ignore comment (only by their configured severity), so they must also + /// be excluded from automatic suppression-comment insertion — writing a + /// `# pyrefly: ignore[...]` for one would produce a comment that can never + /// take effect. + pub fn is_unsuppressable(self) -> bool { + matches!( + self, + ErrorKind::UnusedIgnore + | ErrorKind::InvalidIgnoreComment + | ErrorKind::IgnoreWithoutCode + ) + } + /// A soft error is a warning that should not influence overload selection /// or other type-inference decisions. The type check itself passed, but the /// code pattern is suspicious. diff --git a/crates/pyrefly_python/src/ignore.rs b/crates/pyrefly_python/src/ignore.rs index 25b5683db6..ae46ba3121 100644 --- a/crates/pyrefly_python/src/ignore.rs +++ b/crates/pyrefly_python/src/ignore.rs @@ -180,6 +180,20 @@ impl Tool { pub fn all() -> SmallSet { enum_iterator::all::().collect() } + + /// The lowercase name used in ignore comments, e.g. `pyrefly`. + /// Inverse of [`Tool::from_comment`]; used when rendering diagnostics. + pub fn as_str(self) -> &'static str { + match self { + Tool::Type => "type", + Tool::Pyrefly => "pyrefly", + Tool::Pyre => "pyre", + Tool::Pyright => "pyright", + Tool::Mypy => "mypy", + Tool::Ty => "ty", + Tool::Zuban => "zuban", + } + } } /// A simple lexer that deals with the rules around whitespace. @@ -268,6 +282,61 @@ impl Suppression { } } +#[derive(PartialEq, Debug, Clone, Copy, Hash, Eq)] +pub enum MalformedReason { + /// The directive keyword is not `ignore` (nor `ignore-errors`), e.g. + /// `ignoree`, `ignored`, `ignore1`. + UnknownDirective, + /// A `[` opened an error-code list that was never closed before the end of + /// the comment, e.g. `ignore[no-such-cod`. + UnterminatedBracket, + /// The error-code list is empty, e.g. `ignore[]` or `ignore[ ]`. + EmptyBracket, +} + +/// A comment that reached the `: ignore` stem but whose tail is malformed. +/// Recorded alongside the valid suppressions so the type checker can emit an +/// `invalid-ignore-comment` diagnostic without changing what the comment +/// suppresses underneath. +#[derive(PartialEq, Eq, Debug, Clone)] +pub struct MalformedIgnore { + tool: Tool, + comment_line: LineNumber, + reason: MalformedReason, +} + +impl MalformedIgnore { + pub fn tool(&self) -> Tool { + self.tool + } + + pub fn comment_line(&self) -> LineNumber { + self.comment_line + } + + pub fn reason(&self) -> MalformedReason { + self.reason + } +} + +/// The result of parsing a single `#`-delimited comment fragment. +#[derive(PartialEq, Eq, Debug)] +enum ParsedIgnore { + /// A well-formed suppression directive. + Valid(Suppression), + /// A comment that looks like an ignore directive but is malformed. + /// `suppression` carries the lenient parse when the comment still + /// suppresses something underneath (an unterminated bracket, which we + /// continue to honor for backwards compatibility); it is `None` when the + /// comment suppresses nothing. + Malformed { + suppression: Option, + info: MalformedIgnore, + }, + /// Not an ignore directive at all (`# normal comment`, `# type: list[int]`). + NotADirective, +} + /// Record the position of lines affected by `# type: ignore[valid-type]` suppressions. /// For now we don't record the content of the ignore, but we could. #[derive(Debug, Clone, Default)] @@ -275,17 +344,15 @@ pub struct Ignore { /// The line number here represents the line that the suppression applies to, /// not the line of the suppression comment. ignores: SmallMap>, + /// Comments that look like ignore directives but failed to parse. Used to + /// emit `invalid-ignore-comment` diagnostics; does not affect suppression. + malformed: Vec, } impl Ignore { pub fn new(code: &str) -> Self { - Self { - ignores: Self::parse_ignores(code), - } - } - - fn parse_ignores(code: &str) -> SmallMap> { let mut ignores: SmallMap> = SmallMap::new(); + let mut malformed: Vec = Vec::new(); // If we see a comment on a non-code line, apply it to the next non-comment line. let mut pending = Vec::new(); let mut line = LineNumber::default(); @@ -306,7 +373,18 @@ impl Ignore { } // We know `#` is at the beginning, so the first split is an empty string for x in comments.split('#').skip(1) { - if let Some(supp) = Self::parse_ignore_comment(x, line) { + // A malformed comment is recorded as a diagnostic but still + // routes any lenient suppression exactly as today, keeping + // `invalid-ignore-comment` purely additive. + let supp = match Self::parse_ignore_comment(x, line) { + ParsedIgnore::Valid(supp) => Some(supp), + ParsedIgnore::Malformed { suppression, info } => { + malformed.push(info); + suppression + } + ParsedIgnore::NotADirective => None, + }; + if let Some(supp) = supp { if is_comment_only_line { pending.push(supp); } else { @@ -321,12 +399,16 @@ impl Ignore { .or_default() .append(&mut pending); } - ignores + Self { ignores, malformed } } - /// Given the content of a comment, parse it as a suppression. + /// Given the content of a comment, classify it as a valid suppression, a + /// malformed ignore directive, or not a directive at all. The grammar is a + /// single recognizer with disjoint tails, so exactly one outcome fires: a + /// comment that never reaches the `ignore` stem (e.g. `# type: list[int]`) + /// is always `NotADirective` and can never be flagged as malformed. /// The comment_line parameter indicates which line the comment is on. - fn parse_ignore_comment(l: &str, comment_line: LineNumber) -> Option { + fn parse_ignore_comment(l: &str, comment_line: LineNumber) -> ParsedIgnore { let mut lex = Lexer(l); lex.trim_start(); @@ -339,26 +421,91 @@ impl Ignore { } else if lex.starts_with("pyre-ignore") || lex.starts_with("pyre-fixme") { tool = Some(Tool::Pyre); } - let tool = tool?; + let Some(tool) = tool else { + return ParsedIgnore::NotADirective; + }; - // We have seen `type: ignore` or `pyre-ignore`. Now look for `[code]` or the end. + // We have seen `: ignore` or `pyre-ignore`. Classify the tail. let gap = lex.trim_start(); if lex.starts_with("[") { let rest = lex.rest(); - let inside = rest.split_once(']').map_or(rest, |x| x.0); - return Some(Suppression { - tool, - kind: inside.split(',').map(|x| x.trim().to_owned()).collect(), - comment_line, - }); + match rest.split_once(']') { + Some((inside, _)) => { + // Build the suppression exactly as before so behavior is + // unchanged whether or not the bracket is empty. + let suppression = Suppression { + tool, + kind: inside.split(',').map(|x| x.trim().to_owned()).collect(), + comment_line, + }; + if inside.trim().is_empty() { + // `ignore[]` / `ignore[ ]`: an empty code list is + // malformed, but keep the exact suppression it produced + // before (a blanket for tools whose codes we ignore, a + // no-op for Pyrefly) so the diagnostic stays additive. + ParsedIgnore::Malformed { + suppression: Some(suppression), + info: MalformedIgnore { + tool, + comment_line, + reason: MalformedReason::EmptyBracket, + }, + } + } else { + ParsedIgnore::Valid(suppression) + } + } + None => { + // Unterminated `[`: keep the lenient parse (treat the rest as + // the code list) so suppression behavior is unchanged, but flag it. + ParsedIgnore::Malformed { + suppression: Some(Suppression { + tool, + kind: rest.split(',').map(|x| x.trim().to_owned()).collect(), + comment_line, + }), + info: MalformedIgnore { + tool, + comment_line, + reason: MalformedReason::UnterminatedBracket, + }, + } + } + } } else if gap || lex.word_boundary() { - return Some(Suppression { + // A gap or a word boundary (EOL, punctuation) after `ignore` means a + // blanket suppression: `# type: ignore`, `# type: ignore because ...`. + ParsedIgnore::Valid(Suppression { tool, kind: Vec::new(), comment_line, - }); + }) + } else if (lex.starts_with("-errors") || lex.starts_with("-all-errors")) + && lex.word_boundary() + { + // A complete `ignore-errors` / `ignore-all-errors` directive: a real + // file-level directive handled by `parse_ignore_all`, not a per-line + // suppression. (A trailing identifier char, e.g. `ignore-errorss`, + // falls through to the typo branch below.) + ParsedIgnore::NotADirective + } else { + // The tail continues with an identifier char and is neither a code + // list, a blanket, nor a file-level directive: a keyword typo such as + // `ignoree`, `ignore1`, `ignore_x`. + ParsedIgnore::Malformed { + suppression: None, + info: MalformedIgnore { + tool, + comment_line, + reason: MalformedReason::UnknownDirective, + }, + } } - None + } + + /// Comments that look like ignore directives but failed to parse. + pub fn malformed(&self) -> &[MalformedIgnore] { + &self.malformed } pub fn is_ignored( @@ -539,7 +686,8 @@ mod tests { fn test_parse_ignores() { fn f(x: &str, expect: &[(Tool, u32)]) { assert_eq!( - &Ignore::parse_ignores(x) + &Ignore::new(x) + .ignores .into_iter() .flat_map(|(line, xs)| xs.map(|x| (x.tool, line.get()))) .collect::>(), @@ -608,67 +756,187 @@ x = """ #[test] fn test_parse_ignore_comment() { - fn f(x: &str, tool: Option, kind: &[&str]) { - let dummy_line = LineNumber::default(); + let line = LineNumber::default(); + // A well-formed suppression directive. + let valid = |x: &str, tool, kind: &[&str]| { assert_eq!( - Ignore::parse_ignore_comment(x, dummy_line), - tool.map(|tool| Suppression { + Ignore::parse_ignore_comment(x, line), + ParsedIgnore::Valid(Suppression { tool, kind: kind.map(|x| (*x).to_owned()), - comment_line: dummy_line, + comment_line: line, }), "{x:?}" ); - } + }; + // Not an ignore directive at all. + let not_directive = |x: &str| { + assert_eq!( + Ignore::parse_ignore_comment(x, line), + ParsedIgnore::NotADirective, + "{x:?}" + ); + }; + // A malformed directive with the given reason. `supp_kind` is the lenient + // suppression we keep underneath (or None when we drop it entirely), + // proving the diagnostic is additive over today's suppression behavior. + let malformed = |x: &str, tool, reason, supp_kind: Option<&[&str]>| { + assert_eq!( + Ignore::parse_ignore_comment(x, line), + ParsedIgnore::Malformed { + suppression: supp_kind.map(|kind| Suppression { + tool, + kind: kind.map(|x| (*x).to_owned()), + comment_line: line, + }), + info: MalformedIgnore { + tool, + comment_line: line, + reason, + }, + }, + "{x:?}" + ); + }; - f("ignore: pyrefly", None, &[]); - f("pyrefly: ignore", Some(Tool::Pyrefly), &[]); - f( + not_directive("ignore: pyrefly"); + valid("pyrefly: ignore", Tool::Pyrefly, &[]); + valid( "pyrefly: ignore[bad-return]", - Some(Tool::Pyrefly), + Tool::Pyrefly, &["bad-return"], ); - f("pyrefly: ignore[]", Some(Tool::Pyrefly), &[""]); - f("pyrefly: ignore[bad-]", Some(Tool::Pyrefly), &["bad-"]); + valid("pyrefly: ignore[bad-]", Tool::Pyrefly, &["bad-"]); // Check spacing - f(" type: ignore ", Some(Tool::Type), &[]); - f("type:ignore", Some(Tool::Type), &[]); - f("type :ignore", None, &[]); + valid(" type: ignore ", Tool::Type, &[]); + valid("type:ignore", Tool::Type, &[]); + not_directive("type :ignore"); // Check extras // Mypy rejects that, Pyright accepts it - f("type: ignore because it is wrong", Some(Tool::Type), &[]); - f("type: ignore_none", None, &[]); - f("type: ignore1", None, &[]); - f("type: ignore?", Some(Tool::Type), &[]); + valid("type: ignore because it is wrong", Tool::Type, &[]); + valid("type: ignore?", Tool::Type, &[]); - f("pyright: ignore", Some(Tool::Pyright), &[]); - f( - "pyright: ignore[something]", - Some(Tool::Pyright), - &["something"], - ); + valid("pyright: ignore", Tool::Pyright, &[]); + valid("pyright: ignore[something]", Tool::Pyright, &["something"]); - f("pyre-ignore", Some(Tool::Pyre), &[]); - f("pyre-ignore[7]", Some(Tool::Pyre), &["7"]); - f("pyre-fixme[7]", Some(Tool::Pyre), &["7"]); - f( + valid("pyre-ignore", Tool::Pyre, &[]); + valid("pyre-ignore[7]", Tool::Pyre, &["7"]); + valid("pyre-fixme[7]", Tool::Pyre, &["7"]); + valid( "pyre-fixme[61]: `x` may not be initialized here.", - Some(Tool::Pyre), + Tool::Pyre, &["61"], ); - f("pyre-fixme: core type error", Some(Tool::Pyre), &[]); + valid("pyre-fixme: core type error", Tool::Pyre, &[]); + + valid("zuban: ignore", Tool::Zuban, &[]); + valid("zuban: ignore[something]", Tool::Zuban, &["something"]); + + // (a) keyword typo: a directive that reaches the `ignore` stem but + // continues with an identifier char. Suppresses nothing, as today. + malformed( + "pyrefly: ignoree", + Tool::Pyrefly, + MalformedReason::UnknownDirective, + None, + ); + malformed( + "pyrefly: ignoree[asdfasdf]", + Tool::Pyrefly, + MalformedReason::UnknownDirective, + None, + ); + malformed( + "pyrefly: ignored", + Tool::Pyrefly, + MalformedReason::UnknownDirective, + None, + ); + malformed( + "type: ignore1", + Tool::Type, + MalformedReason::UnknownDirective, + None, + ); + malformed( + "type: ignore_none", + Tool::Type, + MalformedReason::UnknownDirective, + None, + ); - f("zuban: ignore", Some(Tool::Zuban), &[]); - f( - "zuban: ignore[something]", - Some(Tool::Zuban), - &["something"], + // (b) unterminated bracket: keep the lenient parse so suppression is + // unchanged (see test_parse_ignores), but flag the comment. + malformed( + "type: ignore[hello", + Tool::Type, + MalformedReason::UnterminatedBracket, + Some(&["hello"]), + ); + + // (c) empty bracket: flagged, but we keep the exact suppression it + // produced before. For Pyrefly the `[""]` code matches nothing; for tools + // whose codes we ignore (Type, Pyre, ...) it is a blanket suppression, so + // dropping it would silently stop suppressing — hence Some(&[""]). + malformed( + "pyrefly: ignore[]", + Tool::Pyrefly, + MalformedReason::EmptyBracket, + Some(&[""]), + ); + malformed( + "pyrefly: ignore[ ]", + Tool::Pyrefly, + MalformedReason::EmptyBracket, + Some(&[""]), + ); + malformed( + "type: ignore[]", + Tool::Type, + MalformedReason::EmptyBracket, + Some(&[""]), + ); + + // A typo'd file-level directive (`ignore-errorss`) is still a typo. + malformed( + "pyrefly: ignore-errorss", + Tool::Pyrefly, + MalformedReason::UnknownDirective, + None, + ); + + // Real directives and non-directives are never flagged. + not_directive("type: list[int]"); + not_directive("pyrefly: see ticket"); + not_directive("pyrefly: ignore-errors"); + not_directive("pyre-ignore-all-errors"); + } + + #[test] + fn test_malformed_ignores_are_additive() { + // A typo is recorded as malformed but suppresses nothing. + let ig = Ignore::new("x = bad # pyrefly: ignoree"); + assert_eq!(ig.malformed().len(), 1); + assert_eq!( + ig.malformed()[0].reason(), + MalformedReason::UnknownDirective + ); + assert!(ig.is_empty()); + + // An unterminated bracket is flagged yet still suppresses, exactly as before. + let ig = Ignore::new("x = bad # pyrefly: ignore[no-such-cod"); + assert_eq!(ig.malformed().len(), 1); + assert_eq!( + ig.malformed()[0].reason(), + MalformedReason::UnterminatedBracket ); + assert!(!ig.is_empty()); - // For a malformed comment, at least do something with it (works well incrementally) - f("type: ignore[hello", Some(Tool::Type), &["hello"]); + // A non-directive comment is never flagged. + let ig = Ignore::new("y = list[int] # type: list[int]"); + assert!(ig.malformed().is_empty()); } #[test] diff --git a/pyrefly/lib/commands/buck_check.rs b/pyrefly/lib/commands/buck_check.rs index 541142f5ef..022496f1d9 100644 --- a/pyrefly/lib/commands/buck_check.rs +++ b/pyrefly/lib/commands/buck_check.rs @@ -162,12 +162,12 @@ fn compute_errors( let errors = transaction.as_ref().get_errors(&modules_to_check); - // Collect main errors (done once, shared with unused ignore check) + // Collect main errors (done once, shared with the ignore-diagnostics check) let collected = errors.collect_errors(); - let unused = errors.collect_unused_ignore_errors_for_display(&collected); + let ignore_diagnostics = errors.collect_ignore_diagnostics_for_display(&collected); let mut output_errors = collected.ordinary; output_errors.extend(collected.directives); - output_errors.extend(unused.ordinary); + output_errors.extend(ignore_diagnostics.ordinary); output_errors.sort_by_cached_key(|e| { ( e.module().name(), diff --git a/pyrefly/lib/commands/check.rs b/pyrefly/lib/commands/check.rs index e9c6ea7080..ade92dade2 100644 --- a/pyrefly/lib/commands/check.rs +++ b/pyrefly/lib/commands/check.rs @@ -1238,7 +1238,7 @@ impl CheckArgs { let collected = loads.collect_errors(); // Pass pre-collected errors to avoid redundant error collection. - let unused_ignore_errors = loads.collect_unused_ignore_errors_for_display(&collected); + let ignore_diagnostics = loads.collect_ignore_diagnostics_for_display(&collected); let errors = loads.apply_baseline( collected, self.output.baseline.as_deref(), @@ -1263,7 +1263,7 @@ impl CheckArgs { }; let ordinary_errors: Vec<_> = if let Some(only) = &self.output.only { let only = only.iter().collect::>(); - let filtered: Vec<_> = unused_ignore_errors + let filtered: Vec<_> = ignore_diagnostics .ordinary .into_iter() .filter(|e| only.contains(&e.error_kind())) @@ -1272,7 +1272,7 @@ impl CheckArgs { } else { ordinary_errors .into_iter() - .chain(unused_ignore_errors.ordinary) + .chain(ignore_diagnostics.ordinary) .collect() }; @@ -1294,7 +1294,7 @@ impl CheckArgs { let serialized_errors: Vec = ordinary_errors .iter() .filter_map(SerializedError::from_error) - .filter(|e| !e.is_unused_ignore()) + .filter(|e| !e.is_unsuppressable()) .collect(); suppress::suppress_errors(serialized_errors, CommentLocation::LineBefore); } diff --git a/pyrefly/lib/commands/suppress.rs b/pyrefly/lib/commands/suppress.rs index b04a7898a8..4c8b779bcd 100644 --- a/pyrefly/lib/commands/suppress.rs +++ b/pyrefly/lib/commands/suppress.rs @@ -86,12 +86,14 @@ impl SuppressArgs { } else { // Add suppressions mode (existing behavior) let serialized_errors: Vec = if let Some(json_path) = &self.json { - // Parse errors from JSON file, filtering out directives and UnusedIgnore errors + // Parse errors from JSON file, filtering out directives and + // unsuppressable ignore diagnostics (a written comment for them + // could never take effect). let json_content = std::fs::read_to_string(json_path)?; let errors: Vec = serde_json::from_str(&json_content)?; errors .into_iter() - .filter(|e| !e.is_directive() && !e.is_unused_ignore()) + .filter(|e| !e.is_directive() && !e.is_unsuppressable()) .collect() } else { // Run type checking to collect errors @@ -106,12 +108,13 @@ impl SuppressArgs { check_args.run_once(files_to_check, config_finder, upsell, thread_count)?; // Convert to SerializedErrors for all user-visible errors, - // excluding directives (e.g. reveal_type) and UnusedIgnore + // excluding directives (e.g. reveal_type) and unsuppressable + // ignore diagnostics that no written comment could silence. errors .into_iter() .filter(|e| !e.error_kind().is_directive()) .filter_map(|e| SerializedError::from_error(&e)) - .filter(|e| !e.is_unused_ignore()) + .filter(|e| !e.is_unsuppressable()) .collect() }; diff --git a/pyrefly/lib/error/collector.rs b/pyrefly/lib/error/collector.rs index 6f6c2375ca..abb7465861 100644 --- a/pyrefly/lib/error/collector.rs +++ b/pyrefly/lib/error/collector.rs @@ -222,13 +222,19 @@ impl ErrorCollector { ignore_all: &SmallMap, error_config: &ErrorConfig, ) -> bool { + // Diagnostics about ignore comments themselves can never be silenced by a + // comment (see ErrorKind::is_unsuppressable); only their configured + // severity applies. Enforce that once here so every path below — ignore-all, + // per-line, and f-string — is covered uniformly, and so a future + // unsuppressable kind cannot slip through one of them. + if err.error_kind().is_unsuppressable() { + return false; + } // Check whole-file ignore-all directives first. - // UnusedIgnore errors cannot be suppressed to prevent infinite loops. - if err.error_kind() != ErrorKind::UnusedIgnore - && error_config - .enabled_ignores - .iter() - .any(|tool| ignore_all.contains_key(tool)) + if error_config + .enabled_ignores + .iter() + .any(|tool| ignore_all.contains_key(tool)) { return true; } diff --git a/pyrefly/lib/error/error.rs b/pyrefly/lib/error/error.rs index 47cae9b259..7210eb739c 100644 --- a/pyrefly/lib/error/error.rs +++ b/pyrefly/lib/error/error.rs @@ -474,9 +474,11 @@ impl Error { } pub fn is_ignored(&self, enabled_ignores: &SmallSet) -> bool { - // UnusedIgnore errors cannot be suppressed - this prevents infinite loops - // where suppressing an unused-ignore creates another unused-ignore. - if self.error_kind == ErrorKind::UnusedIgnore { + // Ignore-comment diagnostics cannot themselves be silenced by an ignore + // comment; the only knob is their configured severity. For UnusedIgnore + // this also prevents an infinite loop where suppressing an unused-ignore + // would create another unused-ignore. + if self.error_kind.is_unsuppressable() { return false; } // Check both this kind's name and any parent kind's name, so that e.g. diff --git a/pyrefly/lib/error/suppress.rs b/pyrefly/lib/error/suppress.rs index 857d08e9d8..c5d0c8bdf5 100644 --- a/pyrefly/lib/error/suppress.rs +++ b/pyrefly/lib/error/suppress.rs @@ -8,6 +8,7 @@ use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; +use std::str::FromStr; use std::sync::Arc; use std::sync::LazyLock; @@ -98,6 +99,13 @@ impl SerializedError { self.name == ErrorKind::UnusedIgnore.to_name() } + /// Returns true if this error kind cannot be suppressed by an ignore comment + /// (see [`ErrorKind::is_unsuppressable`]). Such errors must be excluded when + /// writing suppression comments, since the comment would never take effect. + pub fn is_unsuppressable(&self) -> bool { + ::from_str(&self.name).is_ok_and(|kind| kind.is_unsuppressable()) + } + /// Returns true if this error is a directive (e.g. reveal_type) that /// should never be suppressed. pub fn is_directive(&self) -> bool { diff --git a/pyrefly/lib/lsp/non_wasm/server.rs b/pyrefly/lib/lsp/non_wasm/server.rs index 83be09f712..dbb64a0067 100644 --- a/pyrefly/lib/lsp/non_wasm/server.rs +++ b/pyrefly/lib/lsp/non_wasm/server.rs @@ -3002,7 +3002,7 @@ impl Server { } for e in transaction .get_errors(handles) - .collect_display_errors_with_unused_ignores() + .collect_display_errors_with_ignore_diagnostics() { if let Some((path, diag)) = self.get_diag_if_shown(&e, &open_files, None) { diags.entry(path.to_owned()).or_default().push(diag); @@ -5492,7 +5492,7 @@ impl Server { let open_files = &self.open_files.read(); for e in transaction .get_errors(once(&handle)) - .collect_display_errors_with_unused_ignores() + .collect_display_errors_with_ignore_diagnostics() { if let Some((_, diag)) = self.get_diag_if_shown(&e, open_files, cell_uri) { items.push(diag); diff --git a/pyrefly/lib/playground.rs b/pyrefly/lib/playground.rs index 662061bc4a..0576441735 100644 --- a/pyrefly/lib/playground.rs +++ b/pyrefly/lib/playground.rs @@ -420,10 +420,10 @@ impl Playground { for (filename, handle) in &self.handles { let errors = transaction.get_errors([handle]); let collected = errors.collect_errors(); - let unused = errors.collect_unused_ignore_errors_for_display(&collected); + let ignore_diagnostics = errors.collect_ignore_diagnostics_for_display(&collected); let mut output_errors = collected.ordinary; output_errors.extend(collected.directives); - output_errors.extend(unused.ordinary); + output_errors.extend(ignore_diagnostics.ordinary); let file_errors = output_errors.into_map(|e| { let range = e.display_range(); Diagnostic { diff --git a/pyrefly/lib/state/errors.rs b/pyrefly/lib/state/errors.rs index e6eb7ecc49..58384e673f 100644 --- a/pyrefly/lib/state/errors.rs +++ b/pyrefly/lib/state/errors.rs @@ -12,6 +12,7 @@ use dupe::Dupe; use pyrefly_config::error_kind::ErrorKind; use pyrefly_config::error_kind::Severity; use pyrefly_python::ignore::Ignore; +use pyrefly_python::ignore::MalformedReason; use pyrefly_python::ignore::Tool; use pyrefly_python::ignore::find_comment_start_in_line; use pyrefly_python::ignore::parse_ignore_all; @@ -319,11 +320,11 @@ impl Errors { Self::merge_display_errors(errors.ordinary, errors.directives) } - pub fn collect_display_errors_with_unused_ignores(&self) -> Vec { + pub fn collect_display_errors_with_ignore_diagnostics(&self) -> Vec { let collected = self.collect_errors(); - let unused = self.collect_unused_ignore_errors_for_display(&collected); + let ignore_diagnostics = self.collect_ignore_diagnostics_for_display(&collected); let mut ordinary = collected.ordinary; - ordinary.extend(unused.ordinary); + ordinary.extend(ignore_diagnostics.ordinary); Self::merge_display_errors(ordinary, collected.directives) } @@ -536,15 +537,87 @@ impl Errors { unused_errors } - /// Collects unused ignore errors for display, respecting severity configuration. - /// Unlike `collect_unused_ignore_errors()`, this applies severity filtering so - /// errors with `Severity::Ignore` are not included in the ordinary results. - /// Accepts pre-collected errors to avoid redundant error collection. - pub fn collect_unused_ignore_errors_for_display( + /// Collects the ignore-comment lint diagnostics that are independent of the + /// type checker's findings: `invalid-ignore-comment` (#3752) for comments + /// that look like an ignore directive but are malformed, and + /// `ignore-without-code` (#3450) for a bare `# pyrefly: ignore` that names no + /// error code. Both are gated on the relevant tool being enabled, and emitted + /// at their default severity; `collect_ignore_diagnostics_for_display` applies + /// the configured per-kind severity. + pub fn collect_ignore_comment_lint_errors(&self) -> Vec { + let mut errors = Vec::new(); + for (load, _, config) in &self.loads { + // Don't emit lints for files we are told never to report errors on. + if load.errors.style() == ErrorStyle::Never { + continue; + } + let module = &load.module_info; + let enabled_ignores = config.enabled_ignores(module.path().as_path()); + let ignore = module.ignore(); + + // #3752: comments that look like an ignore directive but are malformed. + for malformed in ignore.malformed() { + if !enabled_ignores.contains(&malformed.tool()) { + continue; + } + let tool = malformed.tool().as_str(); + let msg = match malformed.reason() { + MalformedReason::UnknownDirective => format!( + "Invalid ignore comment; did you mean `# {tool}: ignore` or `# {tool}: ignore[code]`?" + ), + MalformedReason::UnterminatedBracket => { + format!("Unterminated `[` in `# {tool}: ignore[...]` error-code list") + } + MalformedReason::EmptyBracket => { + format!("Empty error-code list in `# {tool}: ignore[]`") + } + }; + let line_start = module.lined_buffer().line_start(malformed.comment_line()); + let range = TextRange::new(line_start, line_start + TextSize::new(1)); + errors.push(Error::new( + module.dupe(), + range, + msg, + Vec::new(), + ErrorKind::InvalidIgnoreComment, + )); + } + + // #3450: a bare `# pyrefly: ignore` that names no error code. Scoped to + // Pyrefly, the only tool whose per-line codes Pyrefly honors. + if enabled_ignores.contains(&Tool::Pyrefly) { + for (_, suppressions) in ignore.iter() { + for supp in suppressions { + if supp.tool() == Tool::Pyrefly && supp.error_codes().is_empty() { + let line_start = module.lined_buffer().line_start(supp.comment_line()); + let range = TextRange::new(line_start, line_start + TextSize::new(1)); + errors.push(Error::new( + module.dupe(), + range, + "`# pyrefly: ignore` has no error code; specify the code(s) being suppressed, e.g. `# pyrefly: ignore[bad-return]`".to_owned(), + Vec::new(), + ErrorKind::IgnoreWithoutCode, + )); + } + } + } + } + } + errors + } + + /// Collects all ignore-comment diagnostics for display, respecting severity + /// configuration: `unused-ignore`, plus the `invalid-ignore-comment` + /// and `ignore-without-code` lints. Each error is filtered by its own + /// kind's configured severity, so kinds set to `Severity::Ignore` land in + /// `disabled` rather than `ordinary`. Accepts pre-collected errors to avoid + /// redundant error collection. + pub fn collect_ignore_diagnostics_for_display( &self, collected: &CollectedErrors, ) -> CollectedErrors { - let unused_errors = self.collect_unused_ignore_errors(collected); + let mut ignore_errors = self.collect_unused_ignore_errors(collected); + ignore_errors.extend(self.collect_ignore_comment_lint_errors()); let mut result = CollectedErrors::default(); // Build a path-to-config map for O(1) lookup instead of O(loads) per error. @@ -554,18 +627,19 @@ impl Errors { .map(|(load, _, config)| (load.module_info.path(), config)) .collect(); - for error in unused_errors { - if let Some(config) = config_by_path.get(&error.path()) { - let error_config = config.get_error_config(error.path().as_path()); - let severity = error_config - .display_config - .severity(ErrorKind::UnusedIgnore); - match severity { - Severity::Error => result.ordinary.push(error.with_severity(Severity::Error)), - Severity::Warn => result.ordinary.push(error.with_severity(Severity::Warn)), - Severity::Info => result.ordinary.push(error.with_severity(Severity::Info)), - Severity::Ignore => result.disabled.push(error), - } + for error in ignore_errors { + // These diagnostics are generated from `self.loads`, so their path is + // always present in the map built from the same loads. + let config = config_by_path + .get(&error.path()) + .expect("ignore diagnostics are generated from loaded modules"); + let error_config = config.get_error_config(error.path().as_path()); + let severity = error_config.display_config.severity(error.error_kind()); + match severity { + Severity::Error => result.ordinary.push(error.with_severity(Severity::Error)), + Severity::Warn => result.ordinary.push(error.with_severity(Severity::Warn)), + Severity::Info => result.ordinary.push(error.with_severity(Severity::Info)), + Severity::Ignore => result.disabled.push(error), } } @@ -603,6 +677,7 @@ mod tests { use dupe::Dupe; use pyrefly_build::handle::Handle; + use pyrefly_config::error_kind::ErrorKind; use pyrefly_python::module_name::ModuleName; use pyrefly_python::module_path::ModulePath; use pyrefly_python::sys_info::SysInfo; @@ -762,6 +837,126 @@ def g() -> str: assert_eq!(unused.len(), 2); } + #[test] + fn test_invalid_ignore_comment_emitted() { + // A typo'd directive that suppresses nothing is flagged, while the real + // error underneath still fires (the lint is purely additive). + let contents = r#" +x: int = "oops" # pyrefly: ignoree +"#; + let (errors, _tdir) = get_errors(contents); + let lints = errors.collect_ignore_comment_lint_errors(); + assert_eq!(lints.len(), 1); + assert_eq!(lints[0].error_kind(), ErrorKind::InvalidIgnoreComment); + + // The bad-assignment error is not suppressed by the malformed comment. + let ordinary = errors.collect_errors().ordinary; + assert!( + ordinary + .iter() + .any(|e| e.error_kind() == ErrorKind::BadAssignment) + ); + } + + #[test] + fn test_invalid_ignore_comment_variants() { + // Unterminated and empty brackets are flagged; valid forms are not. + let contents = r#" +a: int = "x" # pyrefly: ignore[bad-assignment +b: int = "x" # pyrefly: ignore[] +c: int = "x" # pyrefly: ignore[bad-assignment] +d = list[int] # type: list[int] +"#; + let (errors, _tdir) = get_errors(contents); + let lints = errors.collect_ignore_comment_lint_errors(); + assert_eq!(lints.len(), 2); + assert!( + lints + .iter() + .all(|e| e.error_kind() == ErrorKind::InvalidIgnoreComment) + ); + } + + #[test] + fn test_ignore_without_code_emitted() { + // A bare `# pyrefly: ignore` is flagged; a coded one is not. + let contents = r#" +x: int = "oops" # pyrefly: ignore +y: int = "oops" # pyrefly: ignore[bad-assignment] +"#; + let (errors, _tdir) = get_errors(contents); + let lints = errors.collect_ignore_comment_lint_errors(); + assert_eq!(lints.len(), 1); + assert_eq!(lints[0].error_kind(), ErrorKind::IgnoreWithoutCode); + } + + #[test] + fn test_ignore_without_code_scoped_to_pyrefly() { + // `# type: ignore` is bare but belongs to another tool, so it is not flagged. + let contents = r#" +x: int = "oops" # type: ignore +"#; + let (errors, _tdir) = get_errors(contents); + let lints = errors.collect_ignore_comment_lint_errors(); + assert!(lints.is_empty()); + } + + #[test] + fn test_empty_bracket_ignore_is_additive() { + // `# type: ignore[]` suppressed all errors before (per-line codes are not + // checked for `type`), and must continue to — the new lint only adds a + // diagnostic on top. + let contents = r#" +x: int = "oops" # type: ignore[] +"#; + let (errors, _tdir) = get_errors(contents); + let ordinary = errors.collect_errors().ordinary; + assert!( + !ordinary + .iter() + .any(|e| e.error_kind() == ErrorKind::BadAssignment), + "empty-bracket `# type: ignore[]` must still suppress the error" + ); + let lints = errors.collect_ignore_comment_lint_errors(); + assert_eq!(lints.len(), 1); + assert_eq!(lints[0].error_kind(), ErrorKind::InvalidIgnoreComment); + } + + #[test] + fn test_invalid_ignore_comment_is_unsuppressable() { + // A `# pyrefly: ignore[invalid-ignore-comment]` cannot silence the lint + // about a malformed comment on the same line: ignore-comment diagnostics + // are unsuppressable, so the lint still fires. + let contents = r#" +x = 1 # pyrefly: ignoree # pyrefly: ignore[invalid-ignore-comment] +"#; + let (errors, _tdir) = get_errors(contents); + let lints = errors.collect_ignore_comment_lint_errors(); + assert_eq!(lints.len(), 1); + assert_eq!(lints[0].error_kind(), ErrorKind::InvalidIgnoreComment); + } + + #[test] + fn test_ignore_comment_lints_survive_file_level_ignore_errors() { + // A whole-file `# pyrefly: ignore-errors` suppresses ordinary errors but + // not the ignore-comment diagnostics, which are unsuppressable. + let contents = r#"# pyrefly: ignore-errors +x: int = "oops" # pyrefly: ignoree +"#; + let (errors, _tdir) = get_errors(contents); + assert!( + !errors + .collect_errors() + .ordinary + .iter() + .any(|e| e.error_kind() == ErrorKind::BadAssignment), + "file-level `# pyrefly: ignore-errors` must still suppress the error" + ); + let lints = errors.collect_ignore_comment_lint_errors(); + assert_eq!(lints.len(), 1); + assert_eq!(lints[0].error_kind(), ErrorKind::InvalidIgnoreComment); + } + #[test] fn test_backslash_continuation_ranges_ignores_comment_backslash() { use pyrefly_util::lined_buffer::LineNumber; diff --git a/test/ignores.md b/test/ignores.md index 19682c4ffc..3f83724557 100644 --- a/test/ignores.md +++ b/test/ignores.md @@ -82,3 +82,70 @@ $ echo "enabled-ignores = ['pyright']" > $TMPDIR/enabled_ignores/pyrefly.toml && ERROR */foo.py:3* (glob) [1] ``` + +## A malformed ignore comment is flagged (warn) without changing what it suppresses + +```scrut +$ mkdir $TMPDIR/ig_lints && touch $TMPDIR/ig_lints/pyrefly.toml && \ +> printf 'x: int = "oops" # pyrefly: ignoree\n' > $TMPDIR/ig_lints/typo.py && \ +> $PYREFLY check $TMPDIR/ig_lints/typo.py --min-severity=warn --output-format=min-text + WARN */typo.py:1* (glob) +ERROR */typo.py:1* (glob) +[1] +``` + +## `invalid-ignore-comment` is a warning, so it is hidden at the default min-severity + +```scrut +$ $PYREFLY check $TMPDIR/ig_lints/typo.py --output-format=min-text +ERROR */typo.py:1* (glob) +[1] +``` + +## `# type: list[int]` and valid ignores are never flagged + +```scrut +$ printf 'y = list[int] # type: list[int]\nz: int = "x" # pyrefly: ignore[bad-assignment]\n' \ +> > $TMPDIR/ig_lints/clean.py && \ +> $PYREFLY check $TMPDIR/ig_lints/clean.py --min-severity=warn --output-format=min-text +[0] +``` + +## A bare `# pyrefly: ignore` is not flagged by default + +```scrut +$ printf 'x: int = "x" # pyrefly: ignore\n' > $TMPDIR/ig_lints/bare.py && \ +> $PYREFLY check $TMPDIR/ig_lints/bare.py --min-severity=warn --output-format=min-text +[0] +``` + +## Ignore-comment diagnostics are unsuppressable: file-level `# pyrefly: ignore-errors` does not hide them + +```scrut +$ printf '# pyrefly: ignore-errors\nx: int = "oops" # pyrefly: ignoree\n' \ +> > $TMPDIR/ig_lints/unsupp.py && \ +> $PYREFLY check $TMPDIR/ig_lints/unsupp.py --min-severity=warn --output-format=min-text + WARN */unsupp.py:2* (glob) +[1] +``` + +## ...nor can a `# pyrefly: ignore[invalid-ignore-comment]` on the same line silence it + +```scrut +$ printf 'x = 1 # pyrefly: ignoree # pyrefly: ignore[invalid-ignore-comment]\n' \ +> > $TMPDIR/ig_lints/selfsupp.py && \ +> $PYREFLY check $TMPDIR/ig_lints/selfsupp.py --min-severity=warn --output-format=min-text + WARN */selfsupp.py:1* (glob) +[1] +``` + +## When `ignore-without-code` is enabled, only `# pyrefly: ignore` is flagged + +```scrut +$ printf 'x: int = "x" # pyrefly: ignore\nz: int = "x" # type: ignore\n' \ +> > $TMPDIR/ig_lints/scoped.py && \ +> echo 'errors = {ignore-without-code = true}' > $TMPDIR/ig_lints/pyrefly.toml && \ +> $PYREFLY check $TMPDIR/ig_lints/scoped.py --output-format=min-text +ERROR */scoped.py:1* (glob) +[1] +``` diff --git a/website/docs/error-kinds.mdx b/website/docs/error-kinds.mdx index 27faf58844..8f7815661a 100644 --- a/website/docs/error-kinds.mdx +++ b/website/docs/error-kinds.mdx @@ -489,6 +489,21 @@ def f(x: object) -> object: return x ``` +## ignore-without-code + +Default severity: `ignore` + +A `# pyrefly: ignore` comment suppresses any error on its line without naming the specific error code(s) it silences. This rule is off by default; enable it to require every Pyrefly suppression to be explicit, so a blanket ignore can't silently hide a new, unrelated error introduced later. Only `# pyrefly: ignore` comments are checked, since Pyrefly only honors error codes for its own tool. + +```python +def f() -> int: + return "oops" # pyrefly: ignore # ignore-without-code + +# Fix: name the error code being suppressed. +def g() -> int: + return "oops" # pyrefly: ignore[bad-return] +``` + ## implicit-abstract-class Default severity: `ignore` @@ -783,6 +798,19 @@ Decorator misuse that violates the typing spec — for example, `@dataclass` on function — is reported under `bad-class-definition` or `bad-function-definition` instead, both of which default to `error`. +## invalid-ignore-comment + +Default severity: `warn` + +A comment looks like a type checker ignore directive but is malformed, so it would silently suppress nothing (or not what was intended). Pyrefly recognizes three cases: a typo'd directive keyword (`# pyrefly: ignoree`), an unterminated error-code list (`# pyrefly: ignore[bad-code`), and an empty error-code list (`# pyrefly: ignore[]`). A well-formed comment such as `# type: ignore`, `# pyrefly: ignore[bad-return]`, or `# pyrefly: ignore-errors` is never flagged. + +```python +x: int = "oops" # pyrefly: ignoree # invalid-ignore-comment: did you mean `ignore`? + +# Fix: use a valid directive. +y: int = "oops" # pyrefly: ignore[bad-assignment] +``` + ## invalid-inheritance An error caused by incorrect inheritance in a class or type definition.