From c0f67a5d43afc8f97ead1eb0a93f09c1f3d275be Mon Sep 17 00:00:00 2001 From: Nitish Agarwal <1592163+nitishagar@users.noreply.github.com> Date: Mon, 18 May 2026 16:50:20 +0530 Subject: [PATCH] suppress: avoid orphaning foreign linter pragmas When the line above a target already contains a non-pyrefly linter pragma (noqa, nosemgrep, pyright:, pylint:, etc.), `pyrefly suppress` was inserting a `# pyrefly: ignore [...]` line between them, which silently disabled the foreign pragma. Detect that case and append inline at the end of the target line instead. Fixes #3373 --- pyrefly/lib/error/suppress.rs | 170 ++++++++++++++++++++++++++++++++-- 1 file changed, 162 insertions(+), 8 deletions(-) diff --git a/pyrefly/lib/error/suppress.rs b/pyrefly/lib/error/suppress.rs index 276c02d06c..f8820e6019 100644 --- a/pyrefly/lib/error/suppress.rs +++ b/pyrefly/lib/error/suppress.rs @@ -160,6 +160,41 @@ fn read_and_validate_file(path: &Path) -> anyhow::Result<(String, ModModule)> { } } +/// Linter pragma prefixes (lowercase) that, when present on the line directly +/// above a target line, indicate we should NOT insert a new pyrefly suppression +/// line between them — the foreign pragma applies to the *next* line and a +/// pyrefly comment inserted between them would silently disable it. See #3373. +const FOREIGN_LINTER_PRAGMAS: &[&str] = &[ + "noqa", + "nosemgrep", + "nosec", + "type: ignore", + "pyright:", + "pylint:", + "ruff:", + "flake8:", +]; + +/// Returns true if `line` contains a comment that begins with a known +/// non-pyrefly linter pragma. Excludes pyrefly's own comments — those are +/// handled by the existing merge path. +fn has_foreign_linter_pragma(line: &str) -> bool { + let Some(start) = find_comment_start_in_line(line) else { + return false; + }; + // Skip the `#` and any whitespace. + let body = line[start + 1..].trim_start(); + let body_lower = body.to_lowercase(); + // Exclude pyrefly comments — handled elsewhere. + if body_lower.starts_with("pyrefly:") + || body_lower.starts_with("pyre:") + || body_lower.starts_with("pyre-") + { + return false; + } + FOREIGN_LINTER_PRAGMAS.iter().any(|p| body_lower.starts_with(p)) +} + /// Extracts error codes from an existing pyrefly ignore comment. /// Returns Some(Vec) if the line contains a valid ignore comment, None otherwise. /// Uses string-aware parsing to avoid matching inside string literals. @@ -376,14 +411,27 @@ fn add_suppressions( buf.push_str(error_comment); buf.push_str(line_ending); } else { - // Add suppression line above the error line - buf.push_str(get_indentation(line)); - buf.push_str(error_comment); - buf.push_str(line_ending); - - // Write the current line as-is - buf.push_str(line); - buf.push_str(line_ending); + // Bug fix #3373: don't insert between a foreign linter pragma and its target + // line — that would silently disable the foreign pragma. Append inline instead. + let force_inline = idx > 0 + && !lines_to_skip.contains(&(idx - 1)) + && has_foreign_linter_pragma(lines[idx - 1]); + + if force_inline { + buf.push_str(line); + buf.push_str(" "); + buf.push_str(error_comment); + buf.push_str(line_ending); + } else { + // Add suppression line above the error line + buf.push_str(get_indentation(line)); + buf.push_str(error_comment); + buf.push_str(line_ending); + + // Write the current line as-is + buf.push_str(line); + buf.push_str(line_ending); + } } } else { // No error on this line, write as-is @@ -2080,4 +2128,110 @@ def foo(x: tuple) -> None: "#, ); } + + #[test] + fn foreign_pragma_detection() { + assert!(has_foreign_linter_pragma("# noqa: F821")); + assert!(has_foreign_linter_pragma(" # noqa")); + assert!(has_foreign_linter_pragma("# nosemgrep: rules.foo")); + assert!(has_foreign_linter_pragma("x = 1 # noqa")); // trailing comment counts + assert!(has_foreign_linter_pragma("# type: ignore")); + assert!(has_foreign_linter_pragma("# pyright: ignore[reportFoo]")); + assert!(has_foreign_linter_pragma("# pylint: disable=W")); + assert!(!has_foreign_linter_pragma("# pyrefly: ignore [bad-return]")); + assert!(!has_foreign_linter_pragma("# pyre-fixme[1]")); + assert!(!has_foreign_linter_pragma("# regular comment")); + assert!(!has_foreign_linter_pragma("x = '# noqa'")); // string, not comment + assert!(!has_foreign_linter_pragma("")); + } + + #[test] + fn test_add_suppressions_preserves_noqa_pragma() { + // Bug #3373: # noqa above the target must not be orphaned. + assert_suppress_errors( + r#" +def foo(): + # noqa: F821 + return undefined_variable +"#, + r#" +def foo(): + # noqa: F821 + return undefined_variable # pyrefly: ignore [unknown-name] +"#, + ); + } + + #[test] + fn test_add_suppressions_preserves_nosemgrep_pragma() { + // The exact scenario from issue #3373. + assert_suppress_errors( + r#" +import subprocess +def run_command(cmd): + # nosemgrep: python.lang.security.audit.subprocess-shell-true.subprocess-shell-true + result = subprocess.run(cmd, shell=True, foo="bar") + return result.returncode +"#, + r#" +import subprocess +def run_command(cmd): + # nosemgrep: python.lang.security.audit.subprocess-shell-true.subprocess-shell-true + result = subprocess.run(cmd, shell=True, foo="bar") # pyrefly: ignore [no-matching-overload] + return result.returncode +"#, + ); + } + + #[test] + fn test_add_suppressions_preserves_pyright_pragma() { + assert_suppress_errors( + r#" +def foo(): + # pyright: ignore[reportFoo] + bar() +"#, + r#" +def foo(): + # pyright: ignore[reportFoo] + bar() # pyrefly: ignore [unknown-name] +"#, + ); + } + + #[test] + fn test_add_suppressions_plain_comment_above_still_inserts_above() { + // Sanity: a plain comment is NOT a foreign pragma, behavior unchanged. + assert_suppress_errors( + r#" +def foo() -> int: + # comment + return "" +"#, + r#" +def foo() -> int: + # comment + # pyrefly: ignore [bad-return] + return "" +"#, + ); + } + + #[test] + fn test_add_suppressions_pragma_string_not_comment() { + // # noqa appearing inside a string literal must NOT trigger inline behavior. + assert_suppress_errors( + r##" +def foo() -> int: + msg = "# noqa: do not match" + return "" +"##, + r##" +def foo() -> int: + msg = "# noqa: do not match" + # pyrefly: ignore [bad-return] + return "" +"##, + ); + } }