Locate suppressions at their comment's # in pyrefly coverage (#4030)

Summary:
`Suppression` now records the string-aware comment offset already known at parse time, so `pyrefly coverage report` no longer points a suppression at a `#` inside a string literal.

Fixes #4022

Pull Request resolved: #4030

Test Plan: Regression tests added

Reviewed By: stroxler

Differential Revision: D111306424

Pulled By: NathanTempest

fbshipit-source-id: 8cac5492c90d200fc19436c769bc12c09ccdf080

Author: jorenham

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<Suppression> {
+    /// `comment_line` and `comment_offset` locate the `#` starting the comment.
+    fn parse_ignore_comment(
+        l: &str,
+        comment_line: LineNumber,
+        comment_offset: usize,
+    ) -> Option<Suppression> {
         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,20 +520,22 @@ 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;
         }
 
         let mut lex = Lexer(trimmed);
         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::<Vec<_>>(),
+                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<Tool>, 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::<Vec<_>>(),
                 "{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::<Vec<_>>(),
                 "{x:?}"

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<ReportSuppression> {
-    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
 }
 

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
 }

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