Skip to content

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

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.

Sign up for GitHub

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

Open
jorenham wants to merge 1 commit into
base: main
Choose a base branch
from
+90 −45
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 56 additions & 17 deletions crates/pyrefly_python/src/ignore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand All @@ -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] {
Expand Down Expand Up @@ -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 {
Expand All @@ -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();

Expand All @@ -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
}
Expand Down Expand Up @@ -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") {
Expand Down Expand Up @@ -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));
}
}
}
Expand Down Expand Up @@ -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:?}"
);
Expand Down Expand Up @@ -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:?}"
Expand Down Expand Up @@ -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:?}"
Expand Down
28 changes: 6 additions & 22 deletions pyrefly/lib/commands/coverage/collect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
}

Expand Down
33 changes: 27 additions & 6 deletions pyrefly/lib/test/coverage/test_files/suppressions.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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": [
Expand All @@ -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,
Expand All @@ -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
}
1 change: 1 addition & 0 deletions pyrefly/lib/test/coverage/test_files/suppressions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
x = 1 # pyrefly: ignore[error-code]
y = 2
z = 3 # pyrefly: ignore[code1, code2]
s: str = "#hash" # type: ignore
Loading