From 624e60efaa79b63a8e94ca974d8d423e998d41be Mon Sep 17 00:00:00 2001 From: jorenham Date: Thu, 2 Jul 2026 17:10:00 +0200 Subject: [PATCH] Locate suppressions at their comment's `#` in `pyrefly coverage` --- crates/pyrefly_python/src/ignore.rs | 73 ++++++++++++++----- pyrefly/lib/commands/coverage/collect.rs | 28 ++----- .../test_files/suppressions.expected.json | 33 +++++++-- .../test/coverage/test_files/suppressions.py | 1 + 4 files changed, 90 insertions(+), 45 deletions(-) diff --git a/crates/pyrefly_python/src/ignore.rs b/crates/pyrefly_python/src/ignore.rs index fdac074e72..1f7f6c895c 100644 --- a/crates/pyrefly_python/src/ignore.rs +++ b/crates/pyrefly_python/src/ignore.rs @@ -250,15 +250,18 @@ pub struct Suppression { /// This may differ from the line the suppression applies to /// (e.g., when the comment is on the line above). comment_line: LineNumber, + /// Byte offset within `comment_line` of the `#` that starts the comment. + comment_offset: usize, } impl Suppression { /// A blanket suppression for `tool` that matches every error code. - fn blanket(tool: Tool, comment_line: LineNumber) -> Self { + fn blanket(tool: Tool, comment_line: LineNumber, comment_offset: usize) -> Self { Self { tool, kind: Vec::new(), comment_line, + comment_offset, } } @@ -267,6 +270,11 @@ impl Suppression { self.comment_line } + /// Returns the byte offset of the comment's `#` within `comment_line`. + pub fn comment_offset(&self) -> usize { + self.comment_offset + } + /// Returns the error codes that this suppression applies to. /// An empty slice means the suppression applies to all error codes. pub fn error_codes(&self) -> &[String] { @@ -304,20 +312,18 @@ impl Ignore { for (idx, line_str) in code.lines().enumerate() { let (comment_start, new_state) = find_comment_start(line_str, in_triple_quote); in_triple_quote = new_state; - let comments = if let Some(comment_start) = comment_start { - &line_str[comment_start..] - } else { - "" - }; let is_comment_only_line = comment_start .is_some_and(|comment_start| line_str[..comment_start].trim_start().is_empty()); line = LineNumber::from_zero_indexed(idx as u32); if !pending.is_empty() && (line_str.is_empty() || !is_comment_only_line) { ignores.entry(line).or_default().append(&mut pending); } - // 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) { + let Some(comment_start) = comment_start else { + continue; + }; + // We know `#` is at `comment_start`, so the first split is an empty string + for x in line_str[comment_start..].split('#').skip(1) { + if let Some(supp) = Self::parse_ignore_comment(x, line, comment_start) { if is_comment_only_line { pending.push(supp); } else { @@ -336,8 +342,12 @@ impl Ignore { } /// Given the content of a comment, parse it as a suppression. - /// The comment_line parameter indicates which line the comment is on. - fn parse_ignore_comment(l: &str, comment_line: LineNumber) -> Option { + /// `comment_line` and `comment_offset` locate the `#` starting the comment. + fn parse_ignore_comment( + l: &str, + comment_line: LineNumber, + comment_offset: usize, + ) -> Option { let mut lex = Lexer(l); lex.trim_start(); @@ -361,9 +371,10 @@ impl Ignore { tool, kind: parse_error_codes(inside), comment_line, + comment_offset, }); } else if gap || lex.word_boundary() { - return Some(Suppression::blanket(tool, comment_line)); + return Some(Suppression::blanket(tool, comment_line, comment_offset)); } None } @@ -509,10 +520,10 @@ pub fn parse_ignore_all( break; } - if let Some((tool, prev_line)) = prev_ignore { + if let Some((tool, prev_line, prev_offset)) = prev_ignore { // The previous `# type: ignore` was followed by another comment or // blank line, so it is a whole-file suppression. - res.push(Suppression::blanket(tool, prev_line)); + res.push(Suppression::blanket(tool, prev_line, prev_offset)); prev_ignore = None; } @@ -520,9 +531,11 @@ pub fn parse_ignore_all( if !lex.starts_with("#") { continue; } + // `trimmed` starts with `#`, so its offset is the line's leading whitespace. + let comment_offset = raw_line.len() - raw_line.trim_start().len(); lex.trim_start(); if lex.starts_with("pyre-ignore-all-errors") { - res.push(Suppression::blanket(Tool::Pyre, line)); + res.push(Suppression::blanket(Tool::Pyre, line, comment_offset)); } else if let Some(tool) = lex.starts_with_tool() { lex.trim_start(); if lex.starts_with("ignore-errors") { @@ -554,12 +567,13 @@ pub fn parse_ignore_all( tool, kind, comment_line: line, + comment_offset, }); } } else if !seen_docstring && lex.starts_with("ignore") && lex.blank() { // After a docstring, bare `# type: ignore` is not recognized // as an ignore-all directive. - prev_ignore = Some((tool, line)); + prev_ignore = Some((tool, line, comment_offset)); } } } @@ -648,16 +662,39 @@ x = """ f("x = '''''' # pyrefly: ignore", &[(Tool::Pyrefly, 1)]); } + #[test] + fn test_suppression_comment_offset() { + fn f(x: &str, expect: &[(u32, usize)]) { + assert_eq!( + &Ignore::parse_ignores(x) + .into_iter() + .flat_map(|(_, xs)| xs.map(|x| (x.comment_line.get(), x.comment_offset))) + .collect::>(), + expect, + "{x:?}" + ); + } + + f("x = 1 # type: ignore", &[(1, 7)]); + // Not the `#` inside the string literal + f(r##"x: str = "#hash" # type: ignore"##, &[(1, 18)]); + // Line starts inside a triple-quoted string that closes mid-line + f("x = \"\"\"\n#fake\"\"\" # type: ignore", &[(2, 9)]); + // A comment above code keeps its own line and offset + f(" # type: ignore\nx = 1", &[(1, 2)]); + } + #[test] fn test_parse_ignore_comment() { fn f(x: &str, tool: Option, kind: &[&str]) { let dummy_line = LineNumber::default(); assert_eq!( - Ignore::parse_ignore_comment(x, dummy_line), + Ignore::parse_ignore_comment(x, dummy_line, 0), tool.map(|tool| Suppression { tool, kind: kind.map(|x| (*x).to_owned()), comment_line: dummy_line, + comment_offset: 0, }), "{x:?}" ); @@ -750,6 +787,7 @@ x = """ tool: x.0, kind: x.2.iter().map(|x| (*x).to_owned()).collect(), comment_line: LineNumber::new(x.1).unwrap(), + comment_offset: 0, }) .collect::>(), "{x:?}" @@ -837,6 +875,7 @@ x = """ tool: x.0, kind: x.2.iter().map(|x| (*x).to_owned()).collect(), comment_line: LineNumber::new(x.1).unwrap(), + comment_offset: 0, }) .collect::>(), "{x:?}" diff --git a/pyrefly/lib/commands/coverage/collect.rs b/pyrefly/lib/commands/coverage/collect.rs index 1fe61d8d09..486af9abdc 100644 --- a/pyrefly/lib/commands/coverage/collect.rs +++ b/pyrefly/lib/commands/coverage/collect.rs @@ -16,7 +16,6 @@ use pyrefly_config::error_kind::ErrorKind; use pyrefly_config::finder::ConfigFinder; use pyrefly_graph::index::Idx; use pyrefly_python::dunder; -use pyrefly_python::ignore::Ignore; use pyrefly_python::module::Module; use pyrefly_python::module_name::ModuleName; use pyrefly_python::module_path::ModuleStyle; @@ -35,6 +34,7 @@ use ruff_python_ast::Parameters; use ruff_python_ast::name::Name; use ruff_text_size::Ranged; use ruff_text_size::TextRange; +use ruff_text_size::TextSize; use starlark_map::Hashed; use starlark_map::small_map::SmallMap; use starlark_map::small_set::SmallSet; @@ -106,36 +106,20 @@ fn range_to_location(module: &Module, range: TextRange) -> Location { } } -/// Parse type-ignore suppressions from source using the multi-tool parser from `ignore.rs`. +/// Collect the module's type-ignore suppressions, each located at the `#` starting its comment. fn parse_suppressions(module: &Module) -> Vec { - let source = module.lined_buffer().contents(); - let ignore = Ignore::new(source); let mut suppressions = Vec::new(); - let lines: Vec<&str> = source.lines().collect(); - - for (_line_number, supps) in ignore.iter() { + for (_, supps) in module.ignore().iter() { for supp in supps { - let comment_line_num = supp.comment_line().get() as usize; - let column = comment_line_num - .checked_sub(1) - .and_then(|idx| lines.get(idx)) - .and_then(|line| line.find('#')) - .map(|c| c + 1); - let Some(column) = column else { - continue; - }; - + let offset = module.lined_buffer().line_start(supp.comment_line()) + + TextSize::try_from(supp.comment_offset()).unwrap(); suppressions.push(ReportSuppression { kind: supp.tool(), codes: supp.error_codes().to_vec(), - location: Location { - line: comment_line_num, - column, - }, + location: range_to_location(module, TextRange::empty(offset)), }); } } - suppressions } diff --git a/pyrefly/lib/test/coverage/test_files/suppressions.expected.json b/pyrefly/lib/test/coverage/test_files/suppressions.expected.json index 16ebd8fbb8..1c6a0d2355 100644 --- a/pyrefly/lib/test/coverage/test_files/suppressions.expected.json +++ b/pyrefly/lib/test/coverage/test_files/suppressions.expected.json @@ -4,9 +4,10 @@ "names": [ "test.x", "test.y", - "test.z" + "test.z", + "test.s" ], - "line_count": 9, + "line_count": 10, "symbol_reports": [ { "kind": "attr", @@ -43,6 +44,18 @@ "line": 8, "column": 1 } + }, + { + "kind": "attr", + "name": "test.s", + "n_typable": 1, + "n_typed": 1, + "n_any": 0, + "n_untyped": 0, + "location": { + "line": 9, + "column": 1 + } } ], "type_ignores": [ @@ -66,10 +79,18 @@ "line": 8, "column": 8 } + }, + { + "kind": "type", + "codes": [], + "location": { + "line": 9, + "column": 19 + } } ], - "n_typable": 0, - "n_typed": 0, + "n_typable": 1, + "n_typed": 1, "n_any": 0, "n_untyped": 0, "coverage": 100.0, @@ -79,7 +100,7 @@ "n_function_params": 0, "n_method_params": 0, "n_classes": 0, - "n_attrs": 3, + "n_attrs": 4, "n_properties": 0, - "n_type_ignores": 2 + "n_type_ignores": 3 } diff --git a/pyrefly/lib/test/coverage/test_files/suppressions.py b/pyrefly/lib/test/coverage/test_files/suppressions.py index 13e2809fc7..a8527e2330 100644 --- a/pyrefly/lib/test/coverage/test_files/suppressions.py +++ b/pyrefly/lib/test/coverage/test_files/suppressions.py @@ -6,3 +6,4 @@ x = 1 # pyrefly: ignore[error-code] y = 2 z = 3 # pyrefly: ignore[code1, code2] +s: str = "#hash" # type: ignore