diff --git a/crates/ruff_linter/resources/mdtest/ruff/noqa-comments.md b/crates/ruff_linter/resources/mdtest/ruff/noqa-comments.md new file mode 100644 index 0000000000000..c84e2a85680d9 --- /dev/null +++ b/crates/ruff_linter/resources/mdtest/ruff/noqa-comments.md @@ -0,0 +1,399 @@ +# `noqa-comments` (`RUF105`) + +```toml +[lint] +preview = true +select = ["noqa-comments", "F401", "F402", "F403"] +``` + +## File-level comments + +### Single code + +```py +# snapshot: noqa-comments +# ruff: noqa: F401 +import math +``` + +```snapshot +error[RUF105]: `ruff: noqa` comment used instead of `ruff:file-ignore` + --> src/mdtest_snippet.py:2:1 + | +2 | # ruff: noqa: F401 + | ^^^^^^^^^^^^^^^^^^ + | +help: Use `ruff:file-ignore` instead + | +1 | # snapshot: noqa-comments + - # ruff: noqa: F401 +2 + # ruff:file-ignore[F401] +3 | import math + | +``` + +### Multiple codes + +```py +# snapshot: noqa-comments +# ruff: noqa: F401, F402, F403 +import math +import os +from module import * +for os in []: + pass +``` + +```snapshot +error[RUF105]: `ruff: noqa` comment used instead of `ruff:file-ignore` + --> src/mdtest_snippet.py:2:1 + | +2 | # ruff: noqa: F401, F402, F403 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Use `ruff:file-ignore` instead + | +1 | # snapshot: noqa-comments + - # ruff: noqa: F401, F402, F403 +2 + # ruff:file-ignore[F401, F402, F403] +3 | import math + | +``` + +### Multiple codes followed by a reason + +```py +# snapshot: noqa-comments +# ruff: noqa: F401, F402, F403 for some reason +import math +import os +from module import * +for os in []: + pass +``` + +```snapshot +error[RUF105]: `ruff: noqa` comment used instead of `ruff:file-ignore` + --> src/mdtest_snippet.py:2:1 + | +2 | # ruff: noqa: F401, F402, F403 for some reason + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Use `ruff:file-ignore` instead + | +1 | # snapshot: noqa-comments + - # ruff: noqa: F401, F402, F403 for some reason +2 + # ruff:file-ignore[F401, F402, F403] for some reason +3 | import math + | +``` + +### Multiple codes followed by a nested (pragma) comment + +```py +# snapshot: noqa-comments +# ruff: noqa: F401, F402, F403 # fmt:skip +import math +import os +from module import * +for os in []: + pass +``` + +```snapshot +error[RUF105]: `ruff: noqa` comment used instead of `ruff:file-ignore` + --> src/mdtest_snippet.py:2:1 + | +2 | # ruff: noqa: F401, F402, F403 # fmt:skip + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Use `ruff:file-ignore` instead + | +1 | # snapshot: noqa-comments + - # ruff: noqa: F401, F402, F403 # fmt:skip +2 + # ruff:file-ignore[F401, F402, F403] # fmt:skip +3 | import math + | +``` + +### Unknown codes still receive a diagnostic + +In case the unknown code is a typo rather than an intentionally external code, we emit both +`invalid-rule-code` and `noqa-comments`: + +```toml +[lint] +preview = true +select = ["noqa-comments", "unused-noqa", "invalid-rule-code", "F401"] +``` + +```py +# error: [invalid-rule-code] +# snapshot: noqa-comments +import math # noqa: F401, UNK001 +``` + +```snapshot +error[RUF105]: `noqa` comment used instead of `ruff:ignore` + --> src/mdtest_snippet.py:3:14 + | +3 | import math # noqa: F401, UNK001 + | ^^^^^^^^^^^^^^^^^^^^ + | +help: Use `ruff:ignore` instead + | +2 | # snapshot: noqa-comments + - import math # noqa: F401, UNK001 +3 + import math # ruff:ignore[F401, UNK001] + | +``` + +### External codes + +```toml +[lint] +preview = true +select = ["noqa-comments", "unused-noqa", "invalid-rule-code", "F401"] +external = ["EXT"] +``` + +If all of the codes are marked `external`, no diagnostic is emitted: + +```py +# error: [unused-import] +import math # noqa: EXT001, EXT002 +``` + +However, if only some of the codes are `external`, a diagnostic is emitted without an autofix. In +this case, the external codes likely need to remain in a `noqa` comment, while the codes known by +Ruff could potentially move into a `ruff:ignore` comment. + +```py +# snapshot: noqa-comments +import math # noqa: F401, EXT001 +``` + +```snapshot +error[RUF105]: `noqa` comment used instead of `ruff:ignore` + --> src/mdtest_snippet.py:4:14 + | +4 | import math # noqa: F401, EXT001 + | ^^^^^^^^^^^^^^^^^^^^ + | +help: Use `ruff:ignore` instead +``` + +### Any unmatched code disables the fix + +This leaves an unused `noqa` comment to be cleaned up by `RUF100` instead, which can be especially +important in the case of a standalone `noqa` comment, which has no effect (in almost all cases), but +could become an effectful own-line `ruff:ignore` comment if `RUF105` applied. + +```py +# snapshot: noqa-comments +# ruff: noqa: F401, F402 +import math +``` + +```snapshot +error[RUF105]: `ruff: noqa` comment used instead of `ruff:file-ignore` + --> src/mdtest_snippet.py:2:1 + | +2 | # ruff: noqa: F401, F402 + | ^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: Use `ruff:file-ignore` instead +``` + +### Flake8 comments are ignored + +```py +# flake8: noqa: F401 +import math +``` + +## Inline comments + +### Basic + +```py +# snapshot: noqa-comments +import math # noqa: F401 +``` + +```snapshot +error[RUF105]: `noqa` comment used instead of `ruff:ignore` + --> src/mdtest_snippet.py:2:14 + | +2 | import math # noqa: F401 + | ^^^^^^^^^^^^ + | +help: Use `ruff:ignore` instead + | +1 | # snapshot: noqa-comments + - import math # noqa: F401 +2 + import math # ruff:ignore[F401] + | +``` + +### One unmatched code + +Just like the file-level version above, this disables the autofix but not the rule. + +```py +# snapshot: noqa-comments +import os # noqa: F401, F402 +``` + +```snapshot +error[RUF105]: `noqa` comment used instead of `ruff:ignore` + --> src/mdtest_snippet.py:2:12 + | +2 | import os # noqa: F401, F402 + | ^^^^^^^^^^^^^^^^^^ + | +help: Use `ruff:ignore` instead +``` + +### Nested pragma comment before the directive + +```py +# snapshot: noqa-comments +import math # fmt:skip # noqa: F401 +``` + +```snapshot +error[RUF105]: `noqa` comment used instead of `ruff:ignore` + --> src/mdtest_snippet.py:2:25 + | +2 | import math # fmt:skip # noqa: F401 + | ^^^^^^^^^^^^ + | +help: Use `ruff:ignore` instead + | +1 | # snapshot: noqa-comments + - import math # fmt:skip # noqa: F401 +2 + import math # fmt:skip # ruff:ignore[F401] + | +``` + +## Blanket comments + +### Inline + +For inline comments, `RUF105` flags blanket comments and offers a fix containing the codes that are +actually suppressed: + +```py +# snapshot: noqa-comments +import math # noqa +``` + +```snapshot +error[RUF105]: `noqa` comment used instead of `ruff:ignore` + --> src/mdtest_snippet.py:2:14 + | +2 | import math # noqa + | ^^^^^^ + | +help: Use `ruff:ignore` instead + | +1 | # snapshot: noqa-comments + - import math # noqa +2 + import math # ruff:ignore[F401] +3 | # snapshot: noqa-comments + | +``` + +Multiple diagnostics on the same line don't cause duplicate codes in the final comment: + +```py +# snapshot: noqa-comments +import foo, bar # noqa +``` + +```snapshot +error[RUF105]: `noqa` comment used instead of `ruff:ignore` + --> src/mdtest_snippet.py:4:18 + | +4 | import foo, bar # noqa + | ^^^^^^ + | +help: Use `ruff:ignore` instead + | +3 | # snapshot: noqa-comments + - import foo, bar # noqa +4 + import foo, bar # ruff:ignore[F401] + | +``` + +### File-level + +For file-level comments, only a diagnostic is emitted, without a fix: + +```py +# snapshot: noqa-comments +# ruff: noqa +import math +``` + +```snapshot +error[RUF105]: `ruff: noqa` comment used instead of `ruff:file-ignore` + --> src/mdtest_snippet.py:2:1 + | +2 | # ruff: noqa + | ^^^^^^^^^^^^ + | +help: Use `ruff:file-ignore` instead +``` + +## Inline self-suppression + +```toml +[lint] +preview = true +select = ["noqa-comments", "unused-noqa", "F401"] +``` + +It should be possible to suppress `RUF105` with a `noqa` comment: + +```py +value = 1 # noqa: RUF105 +``` + +But a suppression for `RUF100` should not prevent the rule from firing: + +```py +# error: [noqa-comments] +import math # noqa: RUF100, F401 +``` + +## Suppression with `ruff:ignore` + +```toml +[lint] +preview = true +select = ["noqa-comments", "unused-noqa", "F401"] +``` + +### Inline suppression + +```py +import math # noqa: F401 # ruff:ignore[RUF105] +``` + +### Standalone suppression + +```py +# ruff:ignore[RUF105] +# ruff: noqa: F401 +import math +``` + +### File-level suppression + +```py +# ruff:file-ignore[RUF105] +# ruff: noqa: F401 +import math +``` diff --git a/crates/ruff_linter/resources/mdtest/suppression/ignore.md b/crates/ruff_linter/resources/mdtest/suppression/ignore.md index 5c7b5f0d01617..9eed7d355901e 100644 --- a/crates/ruff_linter/resources/mdtest/suppression/ignore.md +++ b/crates/ruff_linter/resources/mdtest/suppression/ignore.md @@ -668,7 +668,7 @@ import foo ```toml [lint] preview = true -select = ["F401", "RUF10", "FIX002"] +select = ["F401", "RUF100", "FIX002"] ``` Nested suppression comments on a comment-only line are treated as trailing on the comment itself and diff --git a/crates/ruff_linter/src/checkers/noqa.rs b/crates/ruff_linter/src/checkers/noqa.rs index fbd82a6ed83a3..afd190c66c04a 100644 --- a/crates/ruff_linter/src/checkers/noqa.rs +++ b/crates/ruff_linter/src/checkers/noqa.rs @@ -117,15 +117,14 @@ pub(crate) fn check_noqa( } } - // Diagnostics for unused/invalid range suppressions - suppressions.check_suppressions(context, locator); - - // Enforce that the noqa directive was actually used (RUF100), unless RUF100 was itself - // suppressed. - if context.is_rule_enabled(Rule::UnusedNOQA) + // Only migrate directives that don't require RUF100 cleanup first. + let check_unused_noqa = context.is_rule_enabled(Rule::UnusedNOQA) && analyze_directives - && !exemption.includes(Rule::UnusedNOQA) - { + && !exemption.includes(Rule::UnusedNOQA); + let check_noqa_comment = + context.is_rule_enabled(Rule::NoqaComments) && !exemption.enumerates(Rule::NoqaComments); + + if check_unused_noqa || check_noqa_comment { let directives = noqa_directives .lines() .iter() @@ -138,36 +137,59 @@ pub(crate) fn check_noqa( ); for (directive, matches, is_file_level) in directives { match directive { - Directive::All(directive) => { - if matches.is_empty() { - let edit = delete_comment(directive.range(), locator); + Directive::All(all) => { + if check_unused_noqa && matches.is_empty() { + let edit = delete_comment(all.range(), locator); let mut diagnostic = context.report_diagnostic( UnusedNOQA { codes: None, kind: ruff::rules::UnusedNOQAKind::Noqa, }, - directive.range(), + all.range(), ); diagnostic.add_primary_tag(ruff_db::diagnostic::DiagnosticTag::Unnecessary); diagnostic.set_fix(Fix::safe_edit(edit)); + } else if check_noqa_comment { + ruff::rules::noqa_comments( + context, + locator, + is_file_level, + matches.is_empty(), + directive, + matches, + suppressions, + ); } } - Directive::Codes(directive) => { + Directive::Codes(codes) => { let mut disabled_codes = vec![]; let mut duplicated_codes = vec![]; - let mut unknown_codes = vec![]; let mut unmatched_codes = vec![]; let mut valid_codes = vec![]; let mut seen_codes = FxHashSet::default(); let mut self_ignore = false; - for original_code in directive.iter().map(Code::as_str) { + let mut suppress_noqa_comment = false; + for original_code in codes.iter().map(Code::as_str) { let code = get_redirect_target(original_code).unwrap_or(original_code); - if Rule::UnusedNOQA.noqa_code() == code { - self_ignore = true; - break; - } - if seen_codes.insert(original_code) { + if Rule::UnusedNOQA.noqa_code() == code { + self_ignore = true; + if context.is_rule_enabled(Rule::UnusedNOQA) { + valid_codes.push(original_code); + } else { + disabled_codes.push(original_code); + } + continue; + } + + if context.is_rule_enabled(Rule::NoqaComments) + && Rule::NoqaComments.noqa_code() == code + { + suppress_noqa_comment = true; + valid_codes.push(original_code); + continue; + } + let is_code_used = if is_file_level { context.iter().any(|diag| { diag.secondary_code().is_some_and(|noqa| *noqa == code) @@ -187,26 +209,21 @@ pub(crate) fn check_noqa( } else { disabled_codes.push(original_code); } - } else { - unknown_codes.push(original_code); } } else { duplicated_codes.push(original_code); } } - if self_ignore { - continue; - } - - if !(disabled_codes.is_empty() + let has_unused_codes = !(disabled_codes.is_empty() && duplicated_codes.is_empty() - && unmatched_codes.is_empty()) - { + && unmatched_codes.is_empty()); + + if check_unused_noqa && !self_ignore && has_unused_codes { let edit = if valid_codes.is_empty() { - delete_comment(directive.range(), locator) + delete_comment(codes.range(), locator) } else { - let original_text = locator.slice(directive.range()); + let original_text = locator.slice(codes.range()); let prefix = if is_file_level { if original_text.contains("flake8") { "# flake8: noqa: " @@ -218,7 +235,7 @@ pub(crate) fn check_noqa( }; Edit::range_replacement( format!("{}{}", prefix, valid_codes.join(", ")), - directive.range(), + codes.range(), ) }; let mut diagnostic = context.report_diagnostic( @@ -230,16 +247,29 @@ pub(crate) fn check_noqa( }), kind: ruff::rules::UnusedNOQAKind::Noqa, }, - directive.range(), + codes.range(), ); diagnostic.add_primary_tag(ruff_db::diagnostic::DiagnosticTag::Unnecessary); diagnostic.set_fix(Fix::safe_edit(edit)); + } else if check_noqa_comment && !suppress_noqa_comment { + ruff::rules::noqa_comments( + context, + locator, + is_file_level, + has_unused_codes, + directive, + matches, + suppressions, + ); } } } } } + // Diagnostics for unused/invalid range suppressions + suppressions.check_suppressions(context, locator); + if context.is_rule_enabled(Rule::RedirectedNOQA) && !exemption.includes(Rule::RedirectedNOQA) { ruff::rules::redirected_noqa(context, &noqa_directives); ruff::rules::redirected_file_noqa(context, &file_noqa_directives); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 4a6490eaa3d53..a51f84a442399 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1089,6 +1089,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "102") => rules::ruff::rules::InvalidRuleCode, (Ruff, "103") => rules::ruff::rules::InvalidSuppressionComment, (Ruff, "104") => rules::ruff::rules::UnmatchedSuppressionComment, + (Ruff, "105") => rules::ruff::rules::NoqaComments, (Ruff, "200") => rules::ruff::rules::InvalidPyprojectToml, #[cfg(any(feature = "test-rules", test))] diff --git a/crates/ruff_linter/src/noqa.rs b/crates/ruff_linter/src/noqa.rs index eb645456d833e..f7fe29b14b900 100644 --- a/crates/ruff_linter/src/noqa.rs +++ b/crates/ruff_linter/src/noqa.rs @@ -65,6 +65,15 @@ pub(crate) enum Directive<'a> { Codes(Codes<'a>), } +impl Ranged for Directive<'_> { + fn range(&self) -> TextRange { + match self { + Directive::All(all) => all.range(), + Directive::Codes(codes) => codes.range(), + } + } +} + #[derive(Debug)] pub(crate) struct All { range: TextRange, @@ -122,6 +131,10 @@ impl Codes<'_> { self.iter() .any(|code| *needle == get_redirect_target(code.as_str()).unwrap_or(code.as_str())) } + + pub(crate) fn len(&self) -> usize { + self.codes.len() + } } impl Ranged for Codes<'_> { @@ -1355,7 +1368,7 @@ mod tests { use crate::rules::pycodestyle::rules::{AmbiguousVariableName, UselessSemicolon}; use crate::rules::pyflakes::rules::UnusedVariable; use crate::rules::pyupgrade::rules::PrintfStringFormatting; - use crate::settings::{LinterSettings, flags, types::PreviewMode}; + use crate::settings::{LinterSettings, flags}; use crate::source_kind::SourceKind; use crate::suppression::Suppressions; use crate::test::{print_messages, test_contents}; @@ -1417,18 +1430,8 @@ mod tests { fn add_suppressions_in( source: &str, suppression_kind: SuppressionKind, - preview: PreviewMode, + settings: &LinterSettings, ) -> Result { - let settings = LinterSettings { - preview, - ..LinterSettings::for_rules([ - Rule::MissingTypeFunctionArgument, - Rule::MissingReturnTypeUndocumentedPublicFunction, - Rule::UnsortedImports, - Rule::UnusedFunctionArgument, - Rule::UndocumentedPublicFunction, - ]) - }; let path = Path::new(""); let source_map = SourceMap::default(); let source_kind = SourceKind::from_source_code( @@ -1437,7 +1440,7 @@ mod tests { )? .ok_or_else(|| anyhow!("test file should be Python"))?; - let (count, fixed) = add_suppressions(path, &source_kind, &settings, suppression_kind); + let (count, fixed) = add_suppressions(path, &source_kind, settings, suppression_kind); let plural = if count == 1 { "" } else { "s" }; let mut output = String::new(); writeln!( @@ -1447,7 +1450,7 @@ mod tests { let source_kind = source_kind.updated(fixed, &source_map); let (second_count, fixed) = - add_suppressions(path, &source_kind, &settings, suppression_kind); + add_suppressions(path, &source_kind, settings, suppression_kind); if second_count > 0 { writeln!( output, @@ -1456,7 +1459,7 @@ mod tests { } let source_kind = source_kind.updated(fixed, &source_map); - let (diagnostics, _) = test_contents(&source_kind, path, &settings); + let (diagnostics, _) = test_contents(&source_kind, path, settings); if !diagnostics.is_empty() { writeln!( output, @@ -3054,7 +3057,12 @@ mod tests { pass "#, SuppressionKind::Noqa, - PreviewMode::Disabled, + &LinterSettings::for_rules([ + Rule::MissingTypeFunctionArgument, + Rule::MissingReturnTypeUndocumentedPublicFunction, + Rule::UnusedFunctionArgument, + Rule::UndocumentedPublicFunction, + ]), )?, @" Added 1 suppression @@ -3079,7 +3087,13 @@ mod tests { pass "#, SuppressionKind::Noqa, - PreviewMode::Enabled, + &LinterSettings::for_rules([ + Rule::MissingTypeFunctionArgument, + Rule::MissingReturnTypeUndocumentedPublicFunction, + Rule::UnusedFunctionArgument, + Rule::UndocumentedPublicFunction, + ]) + .with_preview_mode(), )?, @" Added 1 suppression @@ -3104,7 +3118,13 @@ mod tests { pass "#, SuppressionKind::Ignore, - PreviewMode::Enabled, + &LinterSettings::for_rules([ + Rule::MissingTypeFunctionArgument, + Rule::MissingReturnTypeUndocumentedPublicFunction, + Rule::UnusedFunctionArgument, + Rule::UndocumentedPublicFunction, + ]) + .with_preview_mode(), )?, @" Added 1 suppression @@ -3120,6 +3140,56 @@ mod tests { Ok(()) } + #[test] + fn add_noqa_ruf105() -> Result<()> { + let settings = + LinterSettings::for_rules([Rule::NoqaComments, Rule::UnusedImport]).with_preview_mode(); + + assert_snapshot!( + add_suppressions_in( + "import math # noqa: F401", + SuppressionKind::Noqa, + &settings, + )?, + @" + Added 1 suppression + + ## Fixed source + + ```py + import math # noqa: F401, RUF105 + + ``` + " + ); + Ok(()) + } + + #[test] + fn add_ignore_ruf105() -> Result<()> { + let settings = + LinterSettings::for_rules([Rule::NoqaComments, Rule::UnusedImport]).with_preview_mode(); + + assert_snapshot!( + add_suppressions_in( + "import math # noqa: F401", + SuppressionKind::Ignore, + &settings, + )?, + @" + Added 1 suppression + + ## Fixed source + + ```py + import math # noqa: F401 # ruff:ignore[noqa-comments] + + ``` + " + ); + Ok(()) + } + #[test] fn add_ignore_to_existing_ignore() -> Result<()> { assert_snapshot!( @@ -3129,7 +3199,13 @@ mod tests { pass "#, SuppressionKind::Ignore, - PreviewMode::Enabled, + &LinterSettings::for_rules([ + Rule::MissingTypeFunctionArgument, + Rule::MissingReturnTypeUndocumentedPublicFunction, + Rule::UnusedFunctionArgument, + Rule::UndocumentedPublicFunction, + ]) + .with_preview_mode(), )?, @" Added 1 suppression @@ -3154,7 +3230,12 @@ mod tests { pass "#, SuppressionKind::Ignore, - PreviewMode::Enabled, + &LinterSettings::for_rules([ + Rule::MissingTypeFunctionArgument, + Rule::MissingReturnTypeUndocumentedPublicFunction, + Rule::UndocumentedPublicFunction, + ]) + .with_preview_mode(), )?, @" Added 1 suppression @@ -3180,7 +3261,7 @@ mod tests { import a "#, SuppressionKind::Ignore, - PreviewMode::Enabled, + &LinterSettings::for_rules([Rule::UnsortedImports]).with_preview_mode(), )?, @" Added 1 suppression @@ -3208,7 +3289,11 @@ mod tests { return x "#, SuppressionKind::Ignore, - PreviewMode::Enabled, + &LinterSettings::for_rules([ + Rule::MissingTypeFunctionArgument, + Rule::MissingReturnTypeUndocumentedPublicFunction, + ]) + .with_preview_mode(), )?, @r#" Added 1 suppression diff --git a/crates/ruff_linter/src/registry.rs b/crates/ruff_linter/src/registry.rs index 766eee98529f9..5afccdda5a6e3 100644 --- a/crates/ruff_linter/src/registry.rs +++ b/crates/ruff_linter/src/registry.rs @@ -250,7 +250,9 @@ impl Rule { pub const fn lint_source(&self) -> LintSource { match self { Rule::InvalidPyprojectToml => LintSource::PyprojectToml, - Rule::BlanketNOQA | Rule::RedirectedNOQA | Rule::UnusedNOQA => LintSource::Noqa, + Rule::BlanketNOQA | Rule::NoqaComments | Rule::RedirectedNOQA | Rule::UnusedNOQA => { + LintSource::Noqa + } Rule::BidirectionalUnicode | Rule::BlankLineWithWhitespace | Rule::DocLineTooLong diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 09e55de4f7b82..e470647b0f007 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -40,6 +40,7 @@ pub(crate) use never_union::*; pub(crate) use non_empty_init_module::*; pub(crate) use non_octal_permissions::*; pub(crate) use none_not_at_end_of_union::*; +pub(crate) use noqa_comments::*; pub(crate) use os_path_commonprefix::*; pub(crate) use parenthesize_chained_operators::*; pub(crate) use post_init_default::*; @@ -118,6 +119,7 @@ mod never_union; mod non_empty_init_module; mod non_octal_permissions; mod none_not_at_end_of_union; +mod noqa_comments; mod os_path_commonprefix; mod parenthesize_chained_operators; mod post_init_default; diff --git a/crates/ruff_linter/src/rules/ruff/rules/noqa_comments.rs b/crates/ruff_linter/src/rules/ruff/rules/noqa_comments.rs new file mode 100644 index 0000000000000..c255d5ea53cf1 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/noqa_comments.rs @@ -0,0 +1,201 @@ +use itertools::Itertools; + +use ruff_diagnostics::{Edit, Fix}; +use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_text_size::{Ranged, TextRange}; + +use crate::{ + FixAvailability, Locator, Violation, checkers::ast::LintContext, codes::Rule, noqa::Directive, + suppression::Suppressions, +}; + +/// ## What it does +/// +/// Checks for the use of `noqa` comments instead of Ruff-specific `ruff:ignore` comments. +/// +/// ## Why is this bad? +/// +/// `ruff:ignore` comments allow the use of rule names instead of codes and can be used in more +/// places than `noqa` comments. +/// +/// Note that this is an opinionated, stylistic rule. `noqa` comments may be needed for backwards +/// compatibility with other tools. You should also feel free to disable this rule if you simply +/// prefer `noqa` comments. +/// +/// ## Example +/// +/// ```python +/// import os # noqa: F401 +/// ``` +/// +/// Use instead: +/// ```python +/// import os # ruff:ignore[F401] +/// ``` +/// +/// Or if you prefer the own-line form: +/// +/// ```python +/// # ruff:ignore[unused-import] +/// import os +/// ``` +/// +/// ## Options +/// +/// This rule will flag `noqa` comments containing rule codes that are unknown to Ruff, even if they +/// are valid for other tools. You can tell Ruff to ignore such codes by configuring the list of +/// known "external" rule codes with the following option: +/// +/// - `lint.external` +/// +/// Ruff will still emit a diagnostic without a fix if `external` and known codes are present in the +/// same `noqa` comment, assuming that only the `external` codes need to remain in the `noqa` +/// comment. +/// +/// ## See also +/// +/// This rule avoids offering a fix if any of the rule codes in a `noqa` comment are unused. See +/// `unused-noqa` for a rule that will remove these and allow the remaining codes to be moved into a +/// `ruff:ignore` comment. +#[derive(ViolationMetadata)] +#[violation_metadata(preview_since = "NEXT_RUFF_VERSION")] +pub(crate) struct NoqaComments { + file_level: bool, +} + +impl Violation for NoqaComments { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + if !self.file_level { + "`noqa` comment used instead of `ruff:ignore`".to_string() + } else { + "`ruff: noqa` comment used instead of `ruff:file-ignore`".to_string() + } + } + + fn fix_title(&self) -> Option { + Some(if self.file_level { + "Use `ruff:file-ignore` instead".to_string() + } else { + "Use `ruff:ignore` instead".to_string() + }) + } +} + +/// RUF105 +pub(crate) fn noqa_comments( + context: &LintContext, + locator: &Locator, + file_level: bool, + has_unused_codes: bool, + directive: &Directive, + matches: &[Rule], + suppressions: &Suppressions, +) { + let codes = Codes::from_directive(directive, matches); + + let range = codes.range; + + if file_level && locator.slice(range).contains("flake8") { + return; + } + + let has_external_codes = if let CodesKind::Codes(codes) = codes.kind { + let external_codes = codes + .iter() + .filter(|code| { + context + .settings() + .external + .iter() + .any(|prefix| code.as_str().starts_with(prefix)) + }) + .count(); + + // Avoid a diagnostic if all of the codes are external. + if external_codes == codes.len() { + return; + } + + external_codes > 0 + } else { + false + }; + + if suppressions.check_rule(Rule::NoqaComments, range, None) { + return; + } + + let mut diagnostic = context.report_diagnostic(NoqaComments { file_level }, range); + + // If some codes are external, return without a fix. + if has_external_codes { + return; + } + + // Similarly, return without a fix if any unused codes are present. This avoids potentially + // activating an unused `noqa` comment on its own line like: + // + // ```py + // # noqa: F401 + // import math + // ``` + // + // by converting it to a valid `ruff:ignore` comment. + if has_unused_codes { + return; + } + + let edit = Edit::range_replacement( + format!( + "# ruff:{action}[{codes}]", + action = if file_level { "file-ignore" } else { "ignore" }, + ), + codes.range, + ); + diagnostic.set_fix(Fix::safe_edit(edit)); +} + +struct Codes<'a> { + kind: CodesKind<'a>, + range: TextRange, +} + +enum CodesKind<'a> { + Codes(&'a crate::noqa::Codes<'a>), + Rules(&'a [Rule]), +} + +impl<'a> Codes<'a> { + fn from_directive(directive: &'a Directive, matches: &'a [Rule]) -> Self { + let kind = match directive { + Directive::All(_) => CodesKind::Rules(matches), + Directive::Codes(codes) => CodesKind::Codes(codes), + }; + + Self { + kind, + range: directive.range(), + } + } +} + +impl std::fmt::Display for Codes<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self.kind { + CodesKind::Codes(codes) => write!(f, "{}", codes.iter().join(", ")), + CodesKind::Rules(rules) => write!( + f, + "{}", + rules + .iter() + .map(Rule::noqa_code) + .sorted() + .dedup() + .join(", ") + ), + } + } +} diff --git a/crates/ruff_linter/src/suppression.rs b/crates/ruff_linter/src/suppression.rs index d6f2d9059c9ed..85c2d7b0f12df 100644 --- a/crates/ruff_linter/src/suppression.rs +++ b/crates/ruff_linter/src/suppression.rs @@ -345,22 +345,49 @@ impl Suppressions { return false; }; + self.check_suppression( + diagnostic.secondary_code(), + diagnostic.name(), + range, + diagnostic.parent(), + ) + } + + /// Check whether a rule is suppressed at the given range and mark the suppression as used. + pub(crate) fn check_rule( + &self, + rule: Rule, + range: TextRange, + parent: Option, + ) -> bool { + self.check_suppression(Some(&rule.noqa_code()), rule.name().as_str(), range, parent) + } + + /// Check whether the given rule code or name corresponds to a valid suppression comment at + /// `range` itself or the `parent` offset. + fn check_suppression( + &self, + code: Option<&C>, + name: &str, + range: TextRange, + parent: Option, + ) -> bool + where + C: for<'a> PartialEq<&'a str>, + { for suppression in &self.valid { let suppression_code = get_redirect_target(suppression.code.as_str()).unwrap_or(suppression.code.as_str()); - let code_matches = diagnostic - .secondary_code() - .is_some_and(|code| *code == suppression_code); - - let name_matches = is_human_readable_names_enabled(self.preview) - && diagnostic.name() == suppression_code; + let code_matches = code.is_some_and(|code| code == &suppression_code); + let name_matches = + is_human_readable_names_enabled(self.preview) && name == suppression_code; if !code_matches && !name_matches { continue; } - if suppression.applies_to_diagnostic(range, diagnostic.parent()) { + if suppression.applies_to_diagnostic(range, parent) { suppression.used.set(true); return true; } diff --git a/python/ruff-ecosystem/ruff_ecosystem/check.py b/python/ruff-ecosystem/ruff_ecosystem/check.py index 0775e7bb61846..78fae8f1628ad 100644 --- a/python/ruff-ecosystem/ruff_ecosystem/check.py +++ b/python/ruff-ecosystem/ruff_ecosystem/check.py @@ -565,9 +565,10 @@ async def ruff_check( if proc.returncode != 0: raise ToolError(err.decode("utf8")) - # Strip summary lines so the diff is only diagnostic lines - return [ + # Strip summary lines so the diff is only diagnostic lines. Also sort the lines so that + # reordering isn't presented as an addition/deletion pair. + return sorted( line for line in result.decode("utf8").splitlines() if not CHECK_SUMMARY_LINE_RE.match(line) - ] + ) diff --git a/ruff.schema.json b/ruff.schema.json index 3c844e66bbfd8..2507c70894ca6 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -4265,6 +4265,7 @@ "RUF102", "RUF103", "RUF104", + "RUF105", "RUF2", "RUF20", "RUF200", @@ -5037,6 +5038,7 @@ "none-not-at-end-of-union", "nonlocal-and-global", "nonlocal-without-binding", + "noqa-comments", "not-in-test", "not-is-test", "numeric-literal-too-long",