diff --git a/pyrefly/lib/commands/check.rs b/pyrefly/lib/commands/check.rs index e54c05cf77..c83f93f03a 100644 --- a/pyrefly/lib/commands/check.rs +++ b/pyrefly/lib/commands/check.rs @@ -233,6 +233,7 @@ impl SnippetCheckArgs { suppress_errors: false, expectations: false, remove_unused_ignores: false, + remove_unused_type_ignores: false, }, }; let (status, check_result) = @@ -420,6 +421,9 @@ struct BehaviorArgs { /// Remove unused ignores from the input files. #[arg(long)] remove_unused_ignores: bool, + /// Remove unused `# type: ignore` comments in addition to unused Pyrefly ignores. + #[arg(long)] + remove_unused_type_ignores: bool, } fn write_errors_to_file( @@ -1315,11 +1319,14 @@ impl CheckArgs { .collect(); suppress::suppress_errors(serialized_errors, CommentLocation::LineBefore); } - if self.behavior.remove_unused_ignores { + if self.behavior.remove_unused_ignores || self.behavior.remove_unused_type_ignores { // TODO: Deprecate this in favor of `pyrefly suppress` let collected = loads.collect_errors(); let unused_errors = loads.collect_unused_ignore_errors(&collected); - suppress::remove_unused_ignores(unused_errors); + suppress::remove_unused_ignores( + unused_errors, + self.behavior.remove_unused_type_ignores, + ); } // We update the baseline file if requested, after reporting any new diff --git a/pyrefly/lib/commands/suppress.rs b/pyrefly/lib/commands/suppress.rs index b04a7898a8..428b51b16e 100644 --- a/pyrefly/lib/commands/suppress.rs +++ b/pyrefly/lib/commands/suppress.rs @@ -39,6 +39,10 @@ pub struct SuppressArgs { #[arg(long)] remove_unused: bool, + /// Remove unused `# type: ignore` comments in addition to unused Pyrefly ignores. + #[arg(long)] + remove_unused_type_ignores: bool, + /// Where to place suppression comments: on the line before the error /// (`line-before`, the default) or on the same line (`same-line`). #[arg(long, default_value = "line-before")] @@ -51,19 +55,22 @@ impl SuppressArgs { wrapper: Option, thread_count: ThreadCount, ) -> anyhow::Result { - if self.remove_unused { + if self.remove_unused || self.remove_unused_type_ignores { // Remove unused ignores mode let unused_errors: Vec = if let Some(json_path) = &self.json { - // Parse errors from JSON file, filtering for UnusedIgnore errors only + // Parse errors from JSON file, filtering for unused suppression errors only. let json_content = std::fs::read_to_string(json_path)?; let errors: Vec = serde_json::from_str(&json_content)?; errors .into_iter() - .filter(|e| e.is_unused_ignore()) + .filter(|e| { + e.is_unused_ignore() + || (self.remove_unused_type_ignores && e.is_unused_type_ignore()) + }) .collect() } else { - // Delegate to `check --remove-unused-ignores`, which calls - // collect_unused_ignore_errors directly (bypassing severity + // Delegate to `check --remove-unused-[type-]ignores`, which + // collects unused ignore errors directly (bypassing severity // filtering) and handles removal in one step. self.config_override.validate()?; let (files_to_check, config_finder, upsell) = self @@ -71,18 +78,26 @@ impl SuppressArgs { .clone() .resolve(self.config_override.clone(), wrapper.clone())?; + let remove_unused_flag = if self.remove_unused_type_ignores { + "--remove-unused-type-ignores" + } else { + "--remove-unused-ignores" + }; let check_args = CheckArgs::parse_from([ "check", "--output-format", "omit-errors", - "--remove-unused-ignores", + remove_unused_flag, ]); check_args.run_once(files_to_check, config_finder, upsell, thread_count)?; return Ok(CommandExitStatus::Success); }; // Remove unused ignores (JSON path only) - suppress::remove_unused_ignores_from_serialized(unused_errors); + suppress::remove_unused_ignores_from_serialized( + unused_errors, + self.remove_unused_type_ignores, + ); } else { // Add suppressions mode (existing behavior) let serialized_errors: Vec = if let Some(json_path) = &self.json { diff --git a/pyrefly/lib/error/suppress.rs b/pyrefly/lib/error/suppress.rs index 857d08e9d8..986aea356d 100644 --- a/pyrefly/lib/error/suppress.rs +++ b/pyrefly/lib/error/suppress.rs @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +use std::borrow::Cow; use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; @@ -38,13 +39,17 @@ use crate::state::errors::sorted_backslash_continuation_ranges; use crate::state::errors::sorted_bracketed_continuation_ranges; use crate::state::errors::sorted_multi_line_string_ranges; -/// Regex to match pyrefly/type/pyre ignore comments with optional error codes and trailing text. -/// Consumes all non-`#` characters after the ignore pattern, so trailing comment text is -/// removed, but a separate `# ...` comment is preserved -/// (e.g., "# pyrefly: ignore [x] # other" -> "# other"). -static IGNORE_COMMENT_REGEX: LazyLock = LazyLock::new(|| { +/// Regexes to match ignore comments with optional error codes and trailing text. +/// Each consumes all non-`#` characters after its ignore pattern, so removing one +/// suppression preserves any other pragma later on the same line. +static PYREFLY_IGNORE_COMMENT_REGEX: LazyLock = LazyLock::new(|| { + Regex::new(r"#\s*pyrefly:\s*ignore\s*(\[[^\]]*\])?\s*(?:;\s*)?[^#]*").unwrap() +}); +static TYPE_IGNORE_COMMENT_REGEX: LazyLock = + LazyLock::new(|| Regex::new(r"#\s*type:\s*ignore\s*(\[[^\]]*\])?\s*(?:;\s*)?[^#]*").unwrap()); +static PYRE_IGNORE_COMMENT_REGEX: LazyLock = LazyLock::new(|| { Regex::new( - r"#\s*pyrefly:\s*ignore\s*(\[[^\]]*\])?\s*(?:;\s*)?[^#]*|#\s*type:\s*ignore\s*(\[[^\]]*\])?\s*(?:;\s*)?[^#]*|#\s*pyre-(?:fixme|ignore)\s*(\[[^\]]*\])?\s*(?:;\s*)?[^#]*|#\s*pyre:\s*ignore\s*(\[[^\]]*\])?\s*(?:;\s*)?[^#]*", + r"#\s*pyre-(?:fixme|ignore)\s*(\[[^\]]*\])?\s*(?:;\s*)?[^#]*|#\s*pyre:\s*ignore\s*(\[[^\]]*\])?\s*(?:;\s*)?[^#]*", ) .unwrap() }); @@ -98,6 +103,11 @@ impl SerializedError { self.name == ErrorKind::UnusedIgnore.to_name() } + /// Returns true if this error is an UnusedTypeIgnore error. + pub fn is_unused_type_ignore(&self) -> bool { + self.name == ErrorKind::UnusedTypeIgnore.to_name() + } + /// Returns true if this error is a directive (e.g. reveal_type) that /// should never be suppressed. pub fn is_directive(&self) -> bool { @@ -519,8 +529,8 @@ fn update_ignore_comment_with_used_codes( // If there are no used codes, remove the entire comment if used_codes.is_empty() { - if IGNORE_COMMENT_REGEX.is_match(comment_part) { - let new_comment = IGNORE_COMMENT_REGEX.replace_all(comment_part, ""); + if PYREFLY_IGNORE_COMMENT_REGEX.is_match(comment_part) { + let new_comment = PYREFLY_IGNORE_COMMENT_REGEX.replace(comment_part, ""); let result = format!("{}{}", code_part, new_comment); return Some(result.trim_end().to_owned()); } @@ -553,20 +563,27 @@ fn update_ignore_comment_with_used_codes( /// Takes a list of UnusedIgnore errors (from collect_unused_ignore_errors) and uses /// the error location and message to determine what to remove: /// - "Unused `# pyrefly: ignore` comment" -> remove entire comment +/// - "Unused `# type: ignore` comment" -> remove entire comment when opted in /// - "Unused `# pyrefly: ignore` comment for code(s): X, Y" -> remove entire comment /// - "Unused error code(s) in `# pyrefly: ignore`: X, Y" -> remove only those codes -pub fn remove_unused_ignores(unused_ignore_errors: Vec) -> usize { +pub fn remove_unused_ignores( + unused_ignore_errors: Vec, + remove_unused_type_ignores: bool, +) -> usize { let serialized: Vec = unused_ignore_errors .iter() .filter_map(SerializedError::from_error) .collect(); - remove_unused_ignores_from_serialized(serialized) + remove_unused_ignores_from_serialized(serialized, remove_unused_type_ignores) } /// Removes unused ignore comments from source files using SerializedError. /// This is similar to remove_unused_ignores but works with SerializedError instead of Error, /// allowing it to be used with errors parsed from JSON. -pub fn remove_unused_ignores_from_serialized(unused_ignore_errors: Vec) -> usize { +pub fn remove_unused_ignores_from_serialized( + unused_ignore_errors: Vec, + remove_unused_type_ignores: bool, +) -> usize { if unused_ignore_errors.is_empty() { return 0; } @@ -574,6 +591,11 @@ pub fn remove_unused_ignores_from_serialized(unused_ignore_errors: Vec> = SmallMap::new(); for error in &unused_ignore_errors { + if !(error.is_unused_ignore() + || remove_unused_type_ignores && error.is_unused_type_ignore()) + { + continue; + } errors_by_path .entry(error.path.clone()) .or_default() @@ -583,10 +605,11 @@ pub fn remove_unused_ignores_from_serialized(unused_ignore_errors: Vec = SmallMap::new(); for (path, path_errors) in &errors_by_path { - // Build a map from line number to the error - let mut line_errors: SmallMap = SmallMap::new(); + // Build a map from line number to its errors. Multiple unused suppressions + // may appear on the same line and must each be handled independently. + let mut line_errors: SmallMap> = SmallMap::new(); for error in path_errors { - line_errors.insert(error.line, *error); + line_errors.entry(error.line).or_default().push(*error); } if let Ok((file, _ast)) = read_and_validate_file(path) { @@ -596,62 +619,71 @@ pub fn remove_unused_ignores_from_serialized(unused_ignore_errors: Vec = codes_part - .split(", ") - .map(|s| s.trim().to_owned()) + if let Some(errors) = line_errors.get(&idx) { + let mut updated_line = Cow::Borrowed(*line); + + for error in errors { + let msg = &error.message; + + if msg.starts_with("Unused error code(s)") { + // Partially unused - extract codes from message and remove only those. + // Message format: "Unused error code(s) in `# pyrefly: ignore`: code1, code2" + if let Some(codes_part) = msg.split(": ").last() { + let unused_codes: SmallSet = codes_part + .split(", ") + .map(|s| s.trim().to_owned()) + .collect(); + + if let Some(existing_codes) = parse_ignore_comment(&updated_line) { + let used_codes: SmallSet = existing_codes + .into_iter() + .filter(|c| !unused_codes.contains(c)) .collect(); - if let Some(existing_codes) = parse_ignore_comment(line) { - let used_codes: SmallSet = existing_codes - .into_iter() - .filter(|c| !unused_codes.contains(c)) - .collect(); - - if let Some(updated) = update_ignore_comment_with_used_codes( - line, - &used_codes, - &unused_codes, - ) { - unused_count += 1; - if !updated.trim().is_empty() { - buf.push_str(&updated); - buf.push_str(line_ending); - } - continue; - } + if let Some(updated) = update_ignore_comment_with_used_codes( + &updated_line, + &used_codes, + &unused_codes, + ) { + updated_line = Cow::Owned(updated); } } } + continue; } + + let ignore_regex = if error.is_unused_type_ignore() { + &*TYPE_IGNORE_COMMENT_REGEX + } else if msg.starts_with("Unused `# pyrefly: ignore` comment") { + &*PYREFLY_IGNORE_COMMENT_REGEX + } else if msg.starts_with("Unused pyre-fixme comment") { + &*PYRE_IGNORE_COMMENT_REGEX + } else { + continue; + }; + + // Use string-aware comment detection instead of raw regex. + let Some(comment_start) = find_comment_start_in_line(&updated_line) else { + continue; + }; + let comment_part = &updated_line[comment_start..]; + if let Cow::Owned(new_comment) = ignore_regex.replace(comment_part, "") { + let code_part = &updated_line[..comment_start]; + updated_line = Cow::Owned( + format!("{}{}", code_part, new_comment) + .trim_end() + .to_owned(), + ); + } + } + + if let Cow::Owned(updated_line) = updated_line { + unused_count += 1; + if !updated_line.trim().is_empty() { + buf.push_str(&updated_line); + buf.push_str(line_ending); + } + continue; } } buf.push_str(line); @@ -734,7 +766,7 @@ mod tests { let (errors, tdir) = get_errors(before); let collected = errors.collect_errors(); let unused_errors = errors.collect_unused_ignore_errors(&collected); - let removals = suppress::remove_unused_ignores(unused_errors); + let removals = suppress::remove_unused_ignores(unused_errors, false); let got_file = fs_anyhow::read_to_string(&get_path(&tdir)).unwrap(); assert_eq!(after, got_file); assert_eq!(removals, expected_removals); @@ -751,7 +783,7 @@ mod tests { let (errors, tdir) = get_errors(before); let collected = errors.collect_errors(); let unused_errors = errors.collect_unused_ignore_errors(&collected); - let removals = suppress::remove_unused_ignores(unused_errors); + let removals = suppress::remove_unused_ignores(unused_errors, false); let got_file = fs_anyhow::read_to_string(&get_path(&tdir)).unwrap(); assert_eq!(after, got_file); assert_eq!(removals, expected_removals); @@ -1140,6 +1172,21 @@ def f() -> int: assert_remove_ignores(input, output, 2); } + #[test] + fn test_remove_unused_type_ignore_when_enabled() { + let input = "a = 1 # pyrefly: ignore\nb = 2 # type: ignore\n"; + let want = "a = 1\nb = 2\n"; + let (errors, tdir) = get_errors(input); + let collected = errors.collect_errors(); + let unused_errors = errors.collect_unused_ignore_errors(&collected); + + let removals = suppress::remove_unused_ignores(unused_errors, true); + + let got_file = fs_anyhow::read_to_string(&get_path(&tdir)).unwrap(); + assert_eq!(want, got_file); + assert_eq!(removals, 2); + } + #[test] fn test_errors_deduped() { let file_contents = r#" @@ -1436,10 +1483,26 @@ def f() -> int: // Helper function to test remove_unused_ignores_from_serialized fn assert_remove_ignores_from_serialized( + file_content: &str, + serialized_errors: Vec, + expected_content: &str, + expected_removals: usize, + ) { + assert_remove_ignores_from_serialized_with_flag( + file_content, + serialized_errors, + expected_content, + expected_removals, + false, + ); + } + + fn assert_remove_ignores_from_serialized_with_flag( file_content: &str, mut serialized_errors: Vec, expected_content: &str, expected_removals: usize, + remove_unused_type_ignores: bool, ) { let tdir = tempfile::tempdir().unwrap(); let path = get_path(&tdir); @@ -1450,13 +1513,71 @@ def f() -> int: error.path = path.clone(); } - let removals = suppress::remove_unused_ignores_from_serialized(serialized_errors); + let removals = suppress::remove_unused_ignores_from_serialized( + serialized_errors, + remove_unused_type_ignores, + ); let got_file = fs_anyhow::read_to_string(&path).unwrap(); assert_eq!(expected_content, got_file); assert_eq!(removals, expected_removals); } + #[test] + fn test_used_type_ignore_preserved_when_removing_pyrefly_ignore() { + // An unused Pyrefly suppression must not remove a still-used type suppression + // from the same line when type-ignore removal was not requested. + let input = "x = 1 # pyrefly: ignore # type: ignore\n"; + let want = "x = 1 # type: ignore\n"; + let errors = vec![SerializedError { + path: PathBuf::from("test.py"), + line: 0, + name: "unused-ignore".to_owned(), + message: "Unused `# pyrefly: ignore` comment".to_owned(), + }]; + assert_remove_ignores_from_serialized_with_flag(input, errors, want, 1, false); + } + + #[test] + fn test_used_pyrefly_ignore_preserved_when_removing_type_ignore() { + // An unused type suppression must not remove a still-used Pyrefly suppression + // from the same line when type-ignore removal was requested. + let input = "x = 1 # type: ignore # pyrefly: ignore\n"; + let want = "x = 1 # pyrefly: ignore\n"; + let errors = vec![SerializedError { + path: PathBuf::from("test.py"), + line: 0, + name: "unused-type-ignore".to_owned(), + message: "Unused `# type: ignore` comment".to_owned(), + }]; + assert_remove_ignores_from_serialized_with_flag(input, errors, want, 1, true); + } + + #[test] + fn test_remove_multiple_unused_suppressions_from_same_line() { + let want = "x = 1\n"; + for input in [ + "x = 1 # pyrefly: ignore # type: ignore\n", + "x = 1 # type: ignore # pyrefly: ignore\n", + ] { + let errors = vec![ + SerializedError { + path: PathBuf::from("test.py"), + line: 0, + name: "unused-ignore".to_owned(), + message: "Unused `# pyrefly: ignore` comment".to_owned(), + }, + SerializedError { + path: PathBuf::from("test.py"), + line: 0, + name: "unused-type-ignore".to_owned(), + message: "Unused `# type: ignore` comment".to_owned(), + }, + ]; + assert_remove_ignores_from_serialized_with_flag(input, errors, want, 1, true); + } + } + #[test] fn test_remove_unused_ignores_from_serialized_blanket() { let input = r#"def g() -> str: @@ -1560,7 +1681,7 @@ def g() -> str: }, ]; - let removals = suppress::remove_unused_ignores_from_serialized(errors); + let removals = suppress::remove_unused_ignores_from_serialized(errors, false); assert_eq!(fs_anyhow::read_to_string(&path1).unwrap(), "x = 1\n"); assert_eq!(fs_anyhow::read_to_string(&path2).unwrap(), "y = 2\n"); @@ -1570,7 +1691,7 @@ def g() -> str: #[test] fn test_remove_unused_ignores_from_serialized_empty_list() { let errors: Vec = vec![]; - let removals = suppress::remove_unused_ignores_from_serialized(errors); + let removals = suppress::remove_unused_ignores_from_serialized(errors, false); assert_eq!(removals, 0); } @@ -2074,7 +2195,7 @@ build_query( name: "unused-ignore".to_owned(), message: "Unused pyre-fixme comment".to_owned(), }]; - let removals = suppress::remove_unused_ignores_from_serialized(errors); + let removals = suppress::remove_unused_ignores_from_serialized(errors, false); let got = fs_anyhow::read_to_string(&path).unwrap(); assert_eq!(want, got); assert_eq!(removals, 1); diff --git a/test/suppress.md b/test/suppress.md index 86588336b1..39e777a83b 100644 --- a/test/suppress.md +++ b/test/suppress.md @@ -1,5 +1,37 @@ # Tests for `--suppress-errors` +## `suppress --remove-unused` preserves unused `# type: ignore` comments by default + +```scrut +$ mkdir $TMPDIR/suppress_remove_unused_default && \ +> printf 'a = 1 # pyrefly: ignore\nb = 2 # type: ignore\n' > $TMPDIR/suppress_remove_unused_default/main.py && \ +> : > $TMPDIR/suppress_remove_unused_default/pyrefly.toml && \ +> $PYREFLY suppress $TMPDIR/suppress_remove_unused_default/main.py \ +> --config $TMPDIR/suppress_remove_unused_default/pyrefly.toml \ +> --remove-unused \ +> >/dev/null 2>/dev/null && \ +> cat $TMPDIR/suppress_remove_unused_default/main.py +a = 1 +b = 2 # type: ignore +[0] +``` + +## `suppress --remove-unused-type-ignores` also removes unused `# type: ignore` comments + +```scrut +$ mkdir $TMPDIR/suppress_remove_unused_type_ignores && \ +> printf 'a = 1 # pyrefly: ignore\nb = 2 # type: ignore\n' > $TMPDIR/suppress_remove_unused_type_ignores/main.py && \ +> : > $TMPDIR/suppress_remove_unused_type_ignores/pyrefly.toml && \ +> $PYREFLY suppress $TMPDIR/suppress_remove_unused_type_ignores/main.py \ +> --config $TMPDIR/suppress_remove_unused_type_ignores/pyrefly.toml \ +> --remove-unused-type-ignores \ +> >/dev/null 2>/dev/null && \ +> cat $TMPDIR/suppress_remove_unused_type_ignores/main.py +a = 1 +b = 2 +[0] +``` + ## `--suppress-errors` should not rewrite warnings hidden by `--min-severity` Use an explicit empty config so the repro does not depend on any ancestor diff --git a/website/docs/error-suppressions.mdx b/website/docs/error-suppressions.mdx index aad0cfa36d..f09acc0b6f 100644 --- a/website/docs/error-suppressions.mdx +++ b/website/docs/error-suppressions.mdx @@ -156,10 +156,12 @@ Repeat the steps above until you get a clean formatting run and a clean type che This will add ` # pyrefly: ignore` comments to your code that will enable you to silence errors, and come back and fix them at a later date. This can make the process of upgrading a large codebase much more manageable. +By default, `--remove-unused` preserves `# type: ignore` comments because they may be shared with other type checkers. To remove unused `# type: ignore` comments as well, use `pyrefly suppress --remove-unused-type-ignores` instead. + :::tip If your project uses other tools that place suppression comments on the line before the error (e.g. other type checkers or linters), use `pyrefly suppress --comment-location=same-line` in step 1 to avoid conflicts. ::: :::note -`pyrefly suppress` is equivalent to `pyrefly check --suppress-errors`, and `pyrefly suppress --remove-unused` is equivalent to `pyrefly check --remove-unused-ignores`. +`pyrefly suppress` is equivalent to `pyrefly check --suppress-errors`, `pyrefly suppress --remove-unused` is equivalent to `pyrefly check --remove-unused-ignores`, and `pyrefly suppress --remove-unused-type-ignores` is equivalent to `pyrefly check --remove-unused-type-ignores`. :::