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!

Author: michaelfortunato

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<LineNumber> {
+        self._get_pyrefly_ignores(true)
+    }
+
+    #[inline]
     pub fn get_pyrefly_ignores(&self) -> SmallSet<LineNumber> {
+        self._get_pyrefly_ignores(false)
+    }
+    fn _get_pyrefly_ignores(&self, only_all: bool) -> SmallSet<LineNumber> {
         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)],

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()
+        ),
+    ))
+}

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;
@@ -1425,6 +1426,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)
     }

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.