From 57fc6d54841feca45fcc3eaca71c9d3591ff1bc8 Mon Sep 17 00:00:00 2001 From: Michael Fortunato Date: Sun, 17 Aug 2025 14:26:02 -0700 Subject: [PATCH] feat(code-action): add ignore comment to suppress error This PR address #675. It works by 1. By gathering all the errors whose range contains the current cursor. 2. Finding the column position of the first non-whitespace character of the line the cursor is on. Call this `column offset`. 3. Inserting an ignore comment on the line above the cursor; the comment is preceded by `column offset` single-spaces to match the prior indentation. In my tests I have found this simple scheme to be surprisingly robust. My first attempt used the ast exclusively, but the ast does not contain comments, nor a way to insert _comments_ into the tree, meaning, just for this one use case of programmatically adding ignore comments, I cannot update the document using the ast, and I found myself having to calculate whitespace offsets in my first ast only attempt. It is also worth pointing out that some LSPs might not _want_ to provide this kind of code action--they want to encourage the user to address the underlying cause, not ignore it. But because Pyrefly is new, there are many false positives and so I think this will be a welcome addition. Plus, the suppression comments in this code action reference specific error codes, so the user in the future can address the lints precisely. Future work is needed to address different types of ignore comments and a code action to - [ ] Add all ignore comments to a module - [ ] Add a top-level ignore comment to a module Both should be relatively straightforward to do by using the utility functions I added in `pyrefly/lib/state/ide.rs` Let me know your thoughts on this. Thanks! --- crates/pyrefly_python/src/ignore.rs | 16 +++- pyrefly/lib/state/ide.rs | 66 +++++++++++++ pyrefly/lib/state/lsp.rs | 12 +++ pyrefly/lib/test/lsp/code_actions.rs | 138 +++++++++++++++++++++++++-- 4 files changed, 224 insertions(+), 8 deletions(-) diff --git a/crates/pyrefly_python/src/ignore.rs b/crates/pyrefly_python/src/ignore.rs index 45accc4e7..fe7d40347 100644 --- a/crates/pyrefly_python/src/ignore.rs +++ b/crates/pyrefly_python/src/ignore.rs @@ -283,15 +283,24 @@ impl Ignore { false } - /// Get all pyrefly ignores. + #[inline] + /// Get pyrefly ignores that are global, and do not have an error code. + pub fn get_pyrefly_all_ignores(&self) -> SmallSet { + self._get_pyrefly_ignores(true) + } + + #[inline] pub fn get_pyrefly_ignores(&self) -> SmallSet { + self._get_pyrefly_ignores(false) + } + fn _get_pyrefly_ignores(&self, only_all: bool) -> SmallSet { self.ignores .iter() .filter(|ignore| { ignore .1 .iter() - .any(|s| s.tool == Tool::Pyrefly && s.kind.is_empty()) + .any(|s| s.tool == Tool::Pyrefly && (!only_all || s.kind.is_empty())) }) .map(|(line, _)| *line) .collect() @@ -306,6 +315,8 @@ mod tests { #[test] fn test_parse_ignores() { + // TODO: The second component of `expect` (the error code) + // is not actually tested. fn f(x: &str, expect: &[(Tool, u32)]) { assert_eq!( &Ignore::parse_ignores(x) @@ -325,6 +336,7 @@ mod tests { "code # mypy: ignore\n# pyre-fixme\nmore code", &[(Tool::Mypy, 1), (Tool::Pyre, 3)], ); + f("# pyrefly: ignore[bad-]\n", &[(Tool::Pyrefly, 2)]); f( "# type: ignore\n# mypy: ignore\n# bad\n\ncode", &[(Tool::Any, 4), (Tool::Mypy, 4)], diff --git a/pyrefly/lib/state/ide.rs b/pyrefly/lib/state/ide.rs index 04744be42..ad62f463f 100644 --- a/pyrefly/lib/state/ide.rs +++ b/pyrefly/lib/state/ide.rs @@ -5,6 +5,8 @@ * LICENSE file in the root directory of this source tree. */ +use dupe::Dupe; +use pyrefly_python::module::Module; use pyrefly_python::module_name::ModuleName; use pyrefly_python::short_identifier::ShortIdentifier; use pyrefly_python::symbol_kind::SymbolKind; @@ -23,6 +25,7 @@ use crate::binding::binding::Key; use crate::binding::bindings::Bindings; use crate::binding::narrow::identifier_and_chain_for_expr; use crate::binding::narrow::identifier_and_chain_prefix_for_expr; +use crate::error::error::Error; use crate::export::exports::Export; use crate::state::handle::Handle; @@ -186,3 +189,66 @@ pub fn insert_import_edit( ); (position, insert_text) } + +pub fn create_ignore_code_action( + error: &Error, + module_info: &Module, +) -> Option<(String, Module, TextRange, String)> { + let ignore_lines_in_module = module_info.ignore().get_pyrefly_ignores(); + let start_line = error.display_range().start.line; + + if ignore_lines_in_module.contains(&start_line) { + create_add_to_existing_ignore_action(error, module_info) + } else { + create_new_ignore_action(error, module_info) + } +} + +fn create_add_to_existing_ignore_action( + error: &Error, + module_info: &Module, +) -> Option<(String, Module, TextRange, String)> { + let dec = error.display_range().start.line.decrement()?; + let suppression_comment = module_info + .lined_buffer() + .content_in_line_range(dec, dec) + .trim_end(); + let bracket_pos = suppression_comment.rfind(']')?; + let row_offset = module_info.lined_buffer().line_start(dec); + let text_range = TextRange::new( + row_offset + TextSize::from(bracket_pos as u32), + row_offset + TextSize::from((bracket_pos + 1) as u32), + ); + + Some(( + format!("Add {} to above ignore comment", error.error_kind()), + module_info.dupe(), + text_range, + format!(", {}]", error.error_kind().to_name()), + )) +} + +fn create_new_ignore_action( + error: &Error, + module_info: &Module, +) -> Option<(String, Module, TextRange, String)> { + let start = error.range().start(); + let display_pos = module_info.display_pos(start); + let start = module_info.lined_buffer().line_start(display_pos.line); + let line = module_info + .lined_buffer() + .content_in_line_range(display_pos.line, display_pos.line); + let offset = line.find(|c: char| !c.is_whitespace()).unwrap_or(0); + let leading_indentation = " ".repeat(offset as usize); + + Some(( + format!("Suppress {} with ignore comment", error.error_kind()), + module_info.dupe(), + TextRange::new(start, start), + format!( + "{}# pyrefly: ignore[{}]\n", + leading_indentation, + error.error_kind().to_name() + ), + )) +} diff --git a/pyrefly/lib/state/lsp.rs b/pyrefly/lib/state/lsp.rs index 8a47a7b44..a4418610f 100644 --- a/pyrefly/lib/state/lsp.rs +++ b/pyrefly/lib/state/lsp.rs @@ -69,6 +69,7 @@ use crate::export::exports::ExportLocation; use crate::graph::index::Idx; use crate::state::handle::Handle; use crate::state::ide::IntermediateDefinition; +use crate::state::ide::create_ignore_code_action; use crate::state::ide::insert_import_edit; use crate::state::ide::key_to_intermediate_definition; use crate::state::require::Require; @@ -1424,6 +1425,17 @@ impl<'a> Transaction<'a> { _ => {} } } + code_actions.extend( + self.get_errors(vec![handle]) + .collect_errors() + .shown + .iter() + // NOTE: For these suppressions the cursor needs to literally + // be on the error range, instead of just the line. This + // is typical with LSPs but worth noting. + .filter(|error| error.range().contains_range(range)) + .filter_map(|error| create_ignore_code_action(error, &module_info)), + ); code_actions.sort_by(|(title1, _, _, _), (title2, _, _, _)| title1.cmp(title2)); Some(code_actions) } diff --git a/pyrefly/lib/test/lsp/code_actions.rs b/pyrefly/lib/test/lsp/code_actions.rs index a35660581..8fdd824b0 100644 --- a/pyrefly/lib/test/lsp/code_actions.rs +++ b/pyrefly/lib/test/lsp/code_actions.rs @@ -25,12 +25,16 @@ fn apply_patch(info: &ModuleInfo, range: TextRange, patch: String) -> (String, S (before, after) } -fn get_test_report(state: &State, handle: &Handle, position: TextSize) -> String { +fn get_test_report_insert_import(state: &State, handle: &Handle, position: TextSize) -> String { let mut report = "Code Actions Results:\n".to_owned(); let transaction = state.transaction(); for (title, info, range, patch) in transaction .local_quickfix_code_actions(handle, TextRange::new(position, position)) - .unwrap_or_default() + .into_iter() + .flatten() + // NOTE: We do NOT patch buffer with ignore comment code action that is + // tested separately + .filter(|(title, _, _, _)| !title.contains("ignore")) { let (before, after) = apply_patch(&info, range, patch); report.push_str("# Title: "); @@ -45,6 +49,128 @@ fn get_test_report(state: &State, handle: &Handle, position: TextSize) -> String report } +fn get_test_report_ignore_code_action( + state: &State, + handle: &Handle, + position: TextSize, +) -> String { + let mut report = "Code Actions Results:\n".to_owned(); + let transaction = state.transaction(); + for (title, info, range, patch) in transaction + .local_quickfix_code_actions(handle, TextRange::new(position, position)) + .into_iter() + .flatten() + // NOTE: We ONLY patch buffer with ignore comment code action so + // we can test it separately + .filter(|(title, _, _, _)| title.contains("ignore")) + { + let (before, after) = apply_patch(&info, range, patch); + report.push_str("# Title: "); + report.push_str(&title); + report.push('\n'); + report.push_str("\n## Before:\n"); + report.push_str(&before); + report.push_str("\n## After:\n"); + report.push_str(&after); + report.push('\n'); + } + report +} + +#[test] +fn add_ignore_comment() { + let report = get_batched_lsp_operations_report_allow_error( + &[ + ("a", "my_export = 3\nanother_thing = 4"), + ("b", "from a import another_thing\nmy_export\n# ^"), + ( + "c", + r#" +def func(x: int) -> int: + pass +func(y) +# ^"#, + ), + ( + "d", + r#" +def func(x: int) -> int: +# ^ + pass +func(y)"#, + ), + ], + get_test_report_ignore_code_action, + ); + assert_eq!( + r#" +# a.py + +# b.py +2 | my_export + ^ +Code Actions Results: +# Title: Suppress UnknownName with ignore comment + +## Before: +from a import another_thing +my_export +# ^ +## After: +from a import another_thing +# pyrefly: ignore[unknown-name] +my_export +# ^ + + + +# c.py +4 | func(y) + ^ +Code Actions Results: +# Title: Suppress UnknownName with ignore comment + +## Before: + +def func(x: int) -> int: + pass +func(y) +# ^ +## After: + +def func(x: int) -> int: + pass +# pyrefly: ignore[unknown-name] +func(y) +# ^ + + + +# d.py +2 | def func(x: int) -> int: + ^ +Code Actions Results: +# Title: Suppress BadReturn with ignore comment + +## Before: + +def func(x: int) -> int: +# ^ + pass +func(y) +## After: + +# pyrefly: ignore[bad-return] +def func(x: int) -> int: +# ^ + pass +func(y) +"# + .trim(), + report.trim() + ); +} + #[test] fn basic_test() { let report = get_batched_lsp_operations_report_allow_error( @@ -54,7 +180,7 @@ fn basic_test() { ("c", "my_export\n# ^"), ("d", "my_export = 3\n"), ], - get_test_report, + get_test_report_insert_import, ); // We should suggest imports from both a and d, but not b. assert_eq!( @@ -102,7 +228,7 @@ fn insertion_test_comments() { ("a", "my_export = 3\n"), ("b", "# i am a comment\nmy_export\n# ^"), ], - get_test_report, + get_test_report_insert_import, ); // We will insert the import after a comment, which might not be the intended target of the // comment. This is not ideal, but we cannot do much better without sophisticated comment @@ -139,7 +265,7 @@ fn insertion_test_existing_imports() { ("a", "my_export = 3\n"), ("b", "from typing import List\nmy_export\n# ^"), ], - get_test_report, + get_test_report_insert_import, ); // Insert before all imports. This might not adhere to existing import sorting code style. assert_eq!( @@ -174,7 +300,7 @@ fn insertion_test_duplicate_imports() { ("a", "my_export = 3\nanother_thing = 4"), ("b", "from a import another_thing\nmy_export\n# ^"), ], - get_test_report, + get_test_report_insert_import, ); // The insertion won't attempt to merge imports from the same module. // It's not illegal, but it would be nice if we do merge.