From 83cf848a8a3063eb7de797b14cc4230a32421a11 Mon Sep 17 00:00:00 2001 From: Frazer McLean Date: Sun, 8 Jun 2025 00:04:19 +0200 Subject: [PATCH 1/6] Add lint rule for calling chmod with non-octal integers Resolves #18464 --- .../resources/test/fixtures/ruff/RUF064.py | 13 ++ .../src/checkers/ast/analyze/expression.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/ruff/mod.rs | 1 + .../src/rules/ruff/rules/chmod_non_octal.rs | 177 ++++++++++++++++++ .../ruff_linter/src/rules/ruff/rules/mod.rs | 2 + ..._rules__ruff__tests__RUF064_RUF064.py.snap | 105 +++++++++++ ruff.schema.json | 3 +- 8 files changed, 304 insertions(+), 1 deletion(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py create mode 100644 crates/ruff_linter/src/rules/ruff/rules/chmod_non_octal.rs create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py new file mode 100644 index 0000000000000..014558925c10f --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py @@ -0,0 +1,13 @@ +import os +from pathlib import Path + +os.chmod("foo", 444) # Error +os.chmod("foo", 0o444) # OK +os.chmod("foo", 7777) # Error +os.chmod("foo", 10000) # Error +os.chmod("foo", 99999) # Error +Path("bar").chmod(755) # Error +Path("bar").chmod(0o755) # OK +path = Path("bar") +path.chmod(755) # Error +path.chmod(0o755) # OK diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 377a50cc396a4..67cfc4d630e32 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1205,6 +1205,9 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) { if checker.enabled(Rule::FromisoformatReplaceZ) { refurb::rules::fromisoformat_replace_z(checker, call); } + if checker.enabled(Rule::ChmodNonOctal) { + ruff::rules::chmod_non_octal(checker, call); + } } Expr::Dict(dict) => { if checker.any_enabled(&[ diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 80fcc0708f378..e29cde1651f3d 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1028,6 +1028,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "059") => (RuleGroup::Preview, rules::ruff::rules::UnusedUnpackedVariable), (Ruff, "060") => (RuleGroup::Preview, rules::ruff::rules::InEmptyCollection), (Ruff, "061") => (RuleGroup::Preview, rules::ruff::rules::LegacyFormPytestRaises), + (Ruff, "064") => (RuleGroup::Preview, rules::ruff::rules::ChmodNonOctal), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA), (Ruff, "102") => (RuleGroup::Preview, rules::ruff::rules::InvalidRuleCode), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 5873a8d1ed9ca..add2e12a6feab 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -109,6 +109,7 @@ mod tests { #[test_case(Rule::LegacyFormPytestRaises, Path::new("RUF061_raises.py"))] #[test_case(Rule::LegacyFormPytestRaises, Path::new("RUF061_warns.py"))] #[test_case(Rule::LegacyFormPytestRaises, Path::new("RUF061_deprecated_call.py"))] + #[test_case(Rule::ChmodNonOctal, Path::new("RUF064.py"))] #[test_case(Rule::RedirectedNOQA, Path::new("RUF101_0.py"))] #[test_case(Rule::RedirectedNOQA, Path::new("RUF101_1.py"))] #[test_case(Rule::InvalidRuleCode, Path::new("RUF102.py"))] diff --git a/crates/ruff_linter/src/rules/ruff/rules/chmod_non_octal.rs b/crates/ruff_linter/src/rules/ruff/rules/chmod_non_octal.rs new file mode 100644 index 0000000000000..97dc51463c7c5 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/chmod_non_octal.rs @@ -0,0 +1,177 @@ +use ruff_diagnostics::{Applicability, Edit, Fix}; +use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_ast::{self as ast, Expr, ExprCall}; +use ruff_python_semantic::{SemanticModel, analyze}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; +use crate::{FixAvailability, Violation}; + +/// ## What it does +/// Checks for `chmod` calls which use non-octal integer literals. +/// +/// ## Why is this bad? +/// +/// Numeric modes are made up of one to four octal digits. Converting a non-octal +/// integer to octal may not be the mode the author intended. +/// +/// ## Example +/// +/// ```python +/// os.chmod("foo", 644) +/// ``` +/// +/// Use instead: +/// +/// ```python +/// os.chmod("foo", 0o644) +/// ``` +/// +/// ## Fix safety +/// +/// This rule's fix is marked as unsafe because it changes runtime behavior. +/// +/// ## Fix availability +/// +/// A fix is only available if the existing digits could make up a valid octal literal. +#[derive(ViolationMetadata)] +pub(crate) struct ChmodNonOctal { + reason: Reason, +} + +#[derive(Debug, Clone, Copy)] +enum Reason { + Decimal { found: u16, suggested: u16 }, + Invalid, +} + +impl Violation for ChmodNonOctal { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + match self.reason { + Reason::Decimal { found, suggested } => { + format!("Non-octal mode `{found}`, did you mean `{suggested:#o}`?") + } + Reason::Invalid => "Non-octal mode".to_string(), + } + } + + fn fix_title(&self) -> Option { + Some("Replace with octal literal".to_string()) + } +} + +/// RUF064 +pub(crate) fn chmod_non_octal(checker: &Checker, call: &ExprCall) { + let mode_arg_index = if is_os_chmod(&call.func, checker.semantic()) { + 1 + } else if is_pathlib_chmod(&call.func, checker.semantic()) { + 0 + } else { + return; + }; + + let Some(mode_arg) = call.arguments.find_argument_value("mode", mode_arg_index) else { + return; + }; + + let Expr::NumberLiteral(ast::ExprNumberLiteral { + value: ast::Number::Int(int), + .. + }) = mode_arg + else { + return; + }; + + let mode_literal = &checker.locator().contents()[mode_arg.range()]; + + if mode_literal.starts_with("0o") || mode_literal.starts_with("0O") || mode_literal == "0" { + return; + } + let mode = int.as_u16(); + + let reason = match (mode_literal.starts_with('0'), mode) { + (true, _) => Reason::Invalid, + (false, Some(found)) => match u16::from_str_radix(&found.to_string(), 8) { + Ok(suggested) if suggested <= 0o7777 => Reason::Decimal { found, suggested }, + _ => Reason::Invalid, + }, + _ => Reason::Invalid, + }; + + let mut diagnostic = checker.report_diagnostic(ChmodNonOctal { reason }, mode_arg.range()); + if let Reason::Decimal { suggested, .. } = reason { + let edit = Edit::range_replacement(format!("{suggested:#o}"), mode_arg.range()); + diagnostic.set_fix(Fix::applicable_edit(edit, Applicability::Unsafe)); + } +} + +/// Returns `true` if an expression resolves to `os.chmod`, `os.fchmod`, or +/// `os.lchmod`. +fn is_os_chmod(func: &Expr, semantic: &SemanticModel) -> bool { + let Some(qualified_name) = semantic.resolve_qualified_name(func) else { + return false; + }; + + matches!( + qualified_name.segments(), + ["os", "chmod" | "fchmod" | "lchmod"] + ) +} + +/// Returns `true` if an expression resolves to a `chmod` or `lchmod` call +/// to any concrete `pathlib.Path` class. +fn is_pathlib_chmod(func: &Expr, semantic: &SemanticModel) -> bool { + let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func else { + return false; + }; + + if attr != "chmod" && attr != "lchmod" { + return false; + } + + // First: is this an inlined call to `pathlib.Path.chmod`? + // ```python + // from pathlib import Path + // Path("foo").chmod(0o644) + // ``` + if let Expr::Call(call) = value.as_ref() { + if is_concrete_pathlib_path_call(semantic, &call.func) { + return true; + } + } + + // Second, is this a call to `pathlib.Path.chmod` via a variable? + // ```python + // from pathlib import Path + // path = Path("foo") + // path.chmod() + // ``` + let Expr::Name(name) = value.as_ref() else { + return false; + }; + + let Some(binding_id) = semantic.resolve_name(name) else { + return false; + }; + + let binding = semantic.binding(binding_id); + + let Some(Expr::Call(call)) = analyze::typing::find_binding_value(binding, semantic) else { + return false; + }; + + is_concrete_pathlib_path_call(semantic, &call.func) +} + +fn is_concrete_pathlib_path_call(semantic: &SemanticModel, func: &Expr) -> bool { + let Some(qualified_name) = semantic.resolve_qualified_name(func) else { + return false; + }; + matches!( + qualified_name.segments(), + ["pathlib", "Path" | "PosixPath" | "WindowsPath"] + ) +} diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index a8344ad9874d5..e8c6862fcfe01 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -2,6 +2,7 @@ pub(crate) use ambiguous_unicode_character::*; pub(crate) use assert_with_print_message::*; pub(crate) use assignment_in_assert::*; pub(crate) use asyncio_dangling_task::*; +pub(crate) use chmod_non_octal::*; pub(crate) use class_with_mixed_type_vars::*; pub(crate) use collection_literal_concatenation::*; pub(crate) use dataclass_enum::*; @@ -62,6 +63,7 @@ mod ambiguous_unicode_character; mod assert_with_print_message; mod assignment_in_assert; mod asyncio_dangling_task; +mod chmod_non_octal; mod class_with_mixed_type_vars; mod collection_literal_concatenation; mod confusables; diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap new file mode 100644 index 0000000000000..75cc7f7e4676c --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap @@ -0,0 +1,105 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF064.py:4:17: RUF064 [*] Non-octal mode `444`, did you mean `0o444`? + | +2 | from pathlib import Path +3 | +4 | os.chmod("foo", 444) # Error + | ^^^ RUF064 +5 | os.chmod("foo", 0o444) # OK +6 | os.chmod("foo", 7777) # Error + | + = help: Replace with octal literal + +ℹ Unsafe fix +1 1 | import os +2 2 | from pathlib import Path +3 3 | +4 |-os.chmod("foo", 444) # Error + 4 |+os.chmod("foo", 0o444) # Error +5 5 | os.chmod("foo", 0o444) # OK +6 6 | os.chmod("foo", 7777) # Error +7 7 | os.chmod("foo", 10000) # Error + +RUF064.py:6:17: RUF064 [*] Non-octal mode `7777`, did you mean `0o7777`? + | +4 | os.chmod("foo", 444) # Error +5 | os.chmod("foo", 0o444) # OK +6 | os.chmod("foo", 7777) # Error + | ^^^^ RUF064 +7 | os.chmod("foo", 10000) # Error +8 | os.chmod("foo", 99999) # Error + | + = help: Replace with octal literal + +ℹ Unsafe fix +3 3 | +4 4 | os.chmod("foo", 444) # Error +5 5 | os.chmod("foo", 0o444) # OK +6 |-os.chmod("foo", 7777) # Error + 6 |+os.chmod("foo", 0o7777) # Error +7 7 | os.chmod("foo", 10000) # Error +8 8 | os.chmod("foo", 99999) # Error +9 9 | Path("bar").chmod(755) # Error + +RUF064.py:7:17: RUF064 Non-octal mode + | +5 | os.chmod("foo", 0o444) # OK +6 | os.chmod("foo", 7777) # Error +7 | os.chmod("foo", 10000) # Error + | ^^^^^ RUF064 +8 | os.chmod("foo", 99999) # Error +9 | Path("bar").chmod(755) # Error + | + = help: Replace with octal literal + +RUF064.py:8:17: RUF064 Non-octal mode + | + 6 | os.chmod("foo", 7777) # Error + 7 | os.chmod("foo", 10000) # Error + 8 | os.chmod("foo", 99999) # Error + | ^^^^^ RUF064 + 9 | Path("bar").chmod(755) # Error +10 | Path("bar").chmod(0o755) # OK + | + = help: Replace with octal literal + +RUF064.py:9:19: RUF064 [*] Non-octal mode `755`, did you mean `0o755`? + | + 7 | os.chmod("foo", 10000) # Error + 8 | os.chmod("foo", 99999) # Error + 9 | Path("bar").chmod(755) # Error + | ^^^ RUF064 +10 | Path("bar").chmod(0o755) # OK +11 | path = Path("bar") + | + = help: Replace with octal literal + +ℹ Unsafe fix +6 6 | os.chmod("foo", 7777) # Error +7 7 | os.chmod("foo", 10000) # Error +8 8 | os.chmod("foo", 99999) # Error +9 |-Path("bar").chmod(755) # Error + 9 |+Path("bar").chmod(0o755) # Error +10 10 | Path("bar").chmod(0o755) # OK +11 11 | path = Path("bar") +12 12 | path.chmod(755) # Error + +RUF064.py:12:12: RUF064 [*] Non-octal mode `755`, did you mean `0o755`? + | +10 | Path("bar").chmod(0o755) # OK +11 | path = Path("bar") +12 | path.chmod(755) # Error + | ^^^ RUF064 +13 | path.chmod(0o755) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +9 9 | Path("bar").chmod(755) # Error +10 10 | Path("bar").chmod(0o755) # OK +11 11 | path = Path("bar") +12 |-path.chmod(755) # Error + 12 |+path.chmod(0o755) # Error +13 13 | path.chmod(0o755) # OK diff --git a/ruff.schema.json b/ruff.schema.json index e8c6c30e853bd..b0b1aeb1a5712 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -4040,6 +4040,7 @@ "RUF06", "RUF060", "RUF061", + "RUF064", "RUF1", "RUF10", "RUF100", @@ -4359,4 +4360,4 @@ ] } } -} \ No newline at end of file +} From 775f3f39c8c93009892d155555211b20fe5d5a38 Mon Sep 17 00:00:00 2001 From: Frazer McLean Date: Mon, 9 Jun 2025 21:57:41 +0200 Subject: [PATCH 2/6] Rename to NonOctalPermissions --- .../src/checkers/ast/analyze/expression.rs | 4 ++-- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/ruff/mod.rs | 2 +- crates/ruff_linter/src/rules/ruff/rules/mod.rs | 4 ++-- ...{chmod_non_octal.rs => non_octal_permissions.rs} | 13 +++++++------ 5 files changed, 13 insertions(+), 12 deletions(-) rename crates/ruff_linter/src/rules/ruff/rules/{chmod_non_octal.rs => non_octal_permissions.rs} (92%) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 67cfc4d630e32..09f1f4f4e9fb8 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -1205,8 +1205,8 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) { if checker.enabled(Rule::FromisoformatReplaceZ) { refurb::rules::fromisoformat_replace_z(checker, call); } - if checker.enabled(Rule::ChmodNonOctal) { - ruff::rules::chmod_non_octal(checker, call); + if checker.enabled(Rule::NonOctalPermissions) { + ruff::rules::non_octal_permissions(checker, call); } } Expr::Dict(dict) => { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index e29cde1651f3d..144006b07db24 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -1028,7 +1028,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "059") => (RuleGroup::Preview, rules::ruff::rules::UnusedUnpackedVariable), (Ruff, "060") => (RuleGroup::Preview, rules::ruff::rules::InEmptyCollection), (Ruff, "061") => (RuleGroup::Preview, rules::ruff::rules::LegacyFormPytestRaises), - (Ruff, "064") => (RuleGroup::Preview, rules::ruff::rules::ChmodNonOctal), + (Ruff, "064") => (RuleGroup::Preview, rules::ruff::rules::NonOctalPermissions), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA), (Ruff, "102") => (RuleGroup::Preview, rules::ruff::rules::InvalidRuleCode), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index add2e12a6feab..d9901d6b17515 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -109,7 +109,7 @@ mod tests { #[test_case(Rule::LegacyFormPytestRaises, Path::new("RUF061_raises.py"))] #[test_case(Rule::LegacyFormPytestRaises, Path::new("RUF061_warns.py"))] #[test_case(Rule::LegacyFormPytestRaises, Path::new("RUF061_deprecated_call.py"))] - #[test_case(Rule::ChmodNonOctal, Path::new("RUF064.py"))] + #[test_case(Rule::NonOctalPermissions, Path::new("RUF064.py"))] #[test_case(Rule::RedirectedNOQA, Path::new("RUF101_0.py"))] #[test_case(Rule::RedirectedNOQA, Path::new("RUF101_1.py"))] #[test_case(Rule::InvalidRuleCode, Path::new("RUF102.py"))] diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index e8c6862fcfe01..39df796762818 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -2,7 +2,6 @@ pub(crate) use ambiguous_unicode_character::*; pub(crate) use assert_with_print_message::*; pub(crate) use assignment_in_assert::*; pub(crate) use asyncio_dangling_task::*; -pub(crate) use chmod_non_octal::*; pub(crate) use class_with_mixed_type_vars::*; pub(crate) use collection_literal_concatenation::*; pub(crate) use dataclass_enum::*; @@ -30,6 +29,7 @@ pub(crate) use mutable_dataclass_default::*; pub(crate) use mutable_fromkeys_value::*; pub(crate) use needless_else::*; pub(crate) use never_union::*; +pub(crate) use non_octal_permissions::*; pub(crate) use none_not_at_end_of_union::*; pub(crate) use parenthesize_chained_operators::*; pub(crate) use post_init_default::*; @@ -63,7 +63,6 @@ mod ambiguous_unicode_character; mod assert_with_print_message; mod assignment_in_assert; mod asyncio_dangling_task; -mod chmod_non_octal; mod class_with_mixed_type_vars; mod collection_literal_concatenation; mod confusables; @@ -93,6 +92,7 @@ mod mutable_dataclass_default; mod mutable_fromkeys_value; mod needless_else; mod never_union; +mod non_octal_permissions; mod none_not_at_end_of_union; mod parenthesize_chained_operators; mod post_init_default; diff --git a/crates/ruff_linter/src/rules/ruff/rules/chmod_non_octal.rs b/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs similarity index 92% rename from crates/ruff_linter/src/rules/ruff/rules/chmod_non_octal.rs rename to crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs index 97dc51463c7c5..92737ff241799 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/chmod_non_octal.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs @@ -1,4 +1,4 @@ -use ruff_diagnostics::{Applicability, Edit, Fix}; +use ruff_diagnostics::{Edit, Fix}; use ruff_macros::{ViolationMetadata, derive_message_formats}; use ruff_python_ast::{self as ast, Expr, ExprCall}; use ruff_python_semantic::{SemanticModel, analyze}; @@ -35,7 +35,7 @@ use crate::{FixAvailability, Violation}; /// /// A fix is only available if the existing digits could make up a valid octal literal. #[derive(ViolationMetadata)] -pub(crate) struct ChmodNonOctal { +pub(crate) struct NonOctalPermissions { reason: Reason, } @@ -45,7 +45,7 @@ enum Reason { Invalid, } -impl Violation for ChmodNonOctal { +impl Violation for NonOctalPermissions { const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; #[derive_message_formats] @@ -64,7 +64,7 @@ impl Violation for ChmodNonOctal { } /// RUF064 -pub(crate) fn chmod_non_octal(checker: &Checker, call: &ExprCall) { +pub(crate) fn non_octal_permissions(checker: &Checker, call: &ExprCall) { let mode_arg_index = if is_os_chmod(&call.func, checker.semantic()) { 1 } else if is_pathlib_chmod(&call.func, checker.semantic()) { @@ -101,10 +101,11 @@ pub(crate) fn chmod_non_octal(checker: &Checker, call: &ExprCall) { _ => Reason::Invalid, }; - let mut diagnostic = checker.report_diagnostic(ChmodNonOctal { reason }, mode_arg.range()); + let mut diagnostic = + checker.report_diagnostic(NonOctalPermissions { reason }, mode_arg.range()); if let Reason::Decimal { suggested, .. } = reason { let edit = Edit::range_replacement(format!("{suggested:#o}"), mode_arg.range()); - diagnostic.set_fix(Fix::applicable_edit(edit, Applicability::Unsafe)); + diagnostic.set_fix(Fix::unsafe_edit(edit)); } } From fe335401fb9563c4ed6d7f85d66e28addbd609ce Mon Sep 17 00:00:00 2001 From: Frazer McLean Date: Wed, 11 Jun 2025 00:03:22 +0200 Subject: [PATCH 3/6] Expand to all standard library functions which take a numeric mode --- .../resources/test/fixtures/ruff/RUF064.py | 37 ++ .../rules/ruff/rules/non_octal_permissions.rs | 125 +++--- ..._rules__ruff__tests__RUF064_RUF064.py.snap | 371 ++++++++++++++---- 3 files changed, 386 insertions(+), 147 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py index 014558925c10f..f861a3096881b 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py @@ -1,3 +1,5 @@ +import dbm.gnu +import dbm.ndbm import os from pathlib import Path @@ -6,8 +8,43 @@ os.chmod("foo", 7777) # Error os.chmod("foo", 10000) # Error os.chmod("foo", 99999) # Error + +os.umask(777) # Error +os.umask(0o777) # OK + +os.fchmod(0, 400) # Error +os.fchmod(0, 0o400) # OK + +os.lchmod("foo", 755) # Error +os.lchmod("foo", 0o755) # OK + +os.mkdir("foo", 600) # Error +os.mkdir("foo", 0o600) # OK + +os.makedirs("foo", 644) # Error +os.makedirs("foo", 0o644) # OK + +os.mkfifo("foo", 640) # Error +os.mkfifo("foo", 0o640) # OK + +os.mknod("foo", 660) # Error +os.mknod("foo", 0o660) # OK + +os.open("foo", os.O_CREAT, 644) # Error +os.open("foo", os.O_CREAT, 0o644) # OK + Path("bar").chmod(755) # Error Path("bar").chmod(0o755) # OK + path = Path("bar") path.chmod(755) # Error path.chmod(0o755) # OK + +dbm.open("db", "r", 600) # Error +dbm.open("db", "r", 0o600) # OK + +dbm.gnu.open("db", "r", 600) # Error +dbm.gnu.open("db", "r", 0o600) # OK + +dbm.ndbm.open("db", "r", 600) # Error +dbm.ndbm.open("db", "r", 0o600) # OK diff --git a/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs b/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs index 92737ff241799..8a552d90f3b6c 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs @@ -1,5 +1,6 @@ use ruff_diagnostics::{Edit, Fix}; use ruff_macros::{ViolationMetadata, derive_message_formats}; +use ruff_python_ast::name::QualifiedName; use ruff_python_ast::{self as ast, Expr, ExprCall}; use ruff_python_semantic::{SemanticModel, analyze}; use ruff_text_size::Ranged; @@ -8,7 +9,8 @@ use crate::checkers::ast::Checker; use crate::{FixAvailability, Violation}; /// ## What it does -/// Checks for `chmod` calls which use non-octal integer literals. +/// Checks for standard library functions which take a numeric `mode` argument +/// where a non-octal integer literal is passed. /// /// ## Why is this bad? /// @@ -35,27 +37,14 @@ use crate::{FixAvailability, Violation}; /// /// A fix is only available if the existing digits could make up a valid octal literal. #[derive(ViolationMetadata)] -pub(crate) struct NonOctalPermissions { - reason: Reason, -} - -#[derive(Debug, Clone, Copy)] -enum Reason { - Decimal { found: u16, suggested: u16 }, - Invalid, -} +pub(crate) struct NonOctalPermissions; impl Violation for NonOctalPermissions { const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; #[derive_message_formats] fn message(&self) -> String { - match self.reason { - Reason::Decimal { found, suggested } => { - format!("Non-octal mode `{found}`, did you mean `{suggested:#o}`?") - } - Reason::Invalid => "Non-octal mode".to_string(), - } + "Non-octal mode".to_string() } fn fix_title(&self) -> Option { @@ -65,15 +54,10 @@ impl Violation for NonOctalPermissions { /// RUF064 pub(crate) fn non_octal_permissions(checker: &Checker, call: &ExprCall) { - let mode_arg_index = if is_os_chmod(&call.func, checker.semantic()) { - 1 - } else if is_pathlib_chmod(&call.func, checker.semantic()) { - 0 - } else { - return; - }; + let mode_arg = find_func_mode_arg(call, checker.semantic()) + .or_else(|| find_method_mode_arg(call, checker.semantic())); - let Some(mode_arg) = call.arguments.find_argument_value("mode", mode_arg_index) else { + let Some(mode_arg) = mode_arg else { return; }; @@ -92,87 +76,88 @@ pub(crate) fn non_octal_permissions(checker: &Checker, call: &ExprCall) { } let mode = int.as_u16(); - let reason = match (mode_literal.starts_with('0'), mode) { - (true, _) => Reason::Invalid, + let suggested = match (mode_literal.starts_with('0'), mode) { + (true, _) => None, (false, Some(found)) => match u16::from_str_radix(&found.to_string(), 8) { - Ok(suggested) if suggested <= 0o7777 => Reason::Decimal { found, suggested }, - _ => Reason::Invalid, + Ok(suggested) if suggested <= 0o7777 => Some(suggested), + _ => None, }, - _ => Reason::Invalid, + _ => None, }; - let mut diagnostic = - checker.report_diagnostic(NonOctalPermissions { reason }, mode_arg.range()); - if let Reason::Decimal { suggested, .. } = reason { + let mut diagnostic = checker.report_diagnostic(NonOctalPermissions, mode_arg.range()); + if let Some(suggested) = suggested { let edit = Edit::range_replacement(format!("{suggested:#o}"), mode_arg.range()); diagnostic.set_fix(Fix::unsafe_edit(edit)); } } -/// Returns `true` if an expression resolves to `os.chmod`, `os.fchmod`, or -/// `os.lchmod`. -fn is_os_chmod(func: &Expr, semantic: &SemanticModel) -> bool { - let Some(qualified_name) = semantic.resolve_qualified_name(func) else { - return false; - }; +fn find_func_mode_arg<'a>(call: &'a ExprCall, semantic: &SemanticModel) -> Option<&'a Expr> { + let qualified_name = semantic.resolve_qualified_name(&call.func)?; + + match qualified_name.segments() { + ["os", "umask"] => call.arguments.find_argument_value("mode", 0), + [ + "os", + "chmod" | "fchmod" | "lchmod" | "mkdir" | "makedirs" | "mkfifo" | "mknod", + ] => call.arguments.find_argument_value("mode", 1), + ["os", "open"] => call.arguments.find_argument_value("mode", 2), + ["dbm", "open"] | ["dbm", "gnu" | "ndbm", "open"] => { + call.arguments.find_argument_value("mode", 2) + } + _ => None, + } +} + +fn find_method_mode_arg<'a>(call: &'a ExprCall, semantic: &SemanticModel) -> Option<&'a Expr> { + let (type_name, attr_name) = resolve_method_call(&call.func, semantic)?; - matches!( - qualified_name.segments(), - ["os", "chmod" | "fchmod" | "lchmod"] - ) + match (type_name.segments(), attr_name) { + ( + ["pathlib", "Path" | "PosixPath" | "WindowsPath"], + "chmod" | "lchmod" | "mkdir" | "touch", + ) => call.arguments.find_argument_value("mode", 0), + _ => None, + } } -/// Returns `true` if an expression resolves to a `chmod` or `lchmod` call -/// to any concrete `pathlib.Path` class. -fn is_pathlib_chmod(func: &Expr, semantic: &SemanticModel) -> bool { +fn resolve_method_call<'a>( + func: &'a Expr, + semantic: &'a SemanticModel, +) -> Option<(QualifiedName<'a>, &'a str)> { let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func else { - return false; + return None; }; - if attr != "chmod" && attr != "lchmod" { - return false; - } - - // First: is this an inlined call to `pathlib.Path.chmod`? + // First: is this an inlined call like `pathlib.Path.chmod`? // ```python // from pathlib import Path // Path("foo").chmod(0o644) // ``` if let Expr::Call(call) = value.as_ref() { - if is_concrete_pathlib_path_call(semantic, &call.func) { - return true; - } + let qualified_name = semantic.resolve_qualified_name(&call.func)?; + return Some((qualified_name, attr)); } - // Second, is this a call to `pathlib.Path.chmod` via a variable? + // Second, is this a call like `pathlib.Path.chmod` via a variable? // ```python // from pathlib import Path // path = Path("foo") // path.chmod() // ``` let Expr::Name(name) = value.as_ref() else { - return false; + return None; }; - let Some(binding_id) = semantic.resolve_name(name) else { - return false; - }; + let binding_id = semantic.resolve_name(name)?; let binding = semantic.binding(binding_id); let Some(Expr::Call(call)) = analyze::typing::find_binding_value(binding, semantic) else { - return false; + return None; }; - is_concrete_pathlib_path_call(semantic, &call.func) -} + let qualified_name = semantic.resolve_qualified_name(&call.func)?; -fn is_concrete_pathlib_path_call(semantic: &SemanticModel, func: &Expr) -> bool { - let Some(qualified_name) = semantic.resolve_qualified_name(func) else { - return false; - }; - matches!( - qualified_name.segments(), - ["pathlib", "Path" | "PosixPath" | "WindowsPath"] - ) + Some((qualified_name, attr)) } diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap index 75cc7f7e4676c..13bb21451c228 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap @@ -1,105 +1,322 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs --- -RUF064.py:4:17: RUF064 [*] Non-octal mode `444`, did you mean `0o444`? +RUF064.py:6:17: RUF064 [*] Non-octal mode | -2 | from pathlib import Path -3 | -4 | os.chmod("foo", 444) # Error +4 | from pathlib import Path +5 | +6 | os.chmod("foo", 444) # Error | ^^^ RUF064 -5 | os.chmod("foo", 0o444) # OK -6 | os.chmod("foo", 7777) # Error +7 | os.chmod("foo", 0o444) # OK +8 | os.chmod("foo", 7777) # Error | = help: Replace with octal literal ℹ Unsafe fix -1 1 | import os -2 2 | from pathlib import Path -3 3 | -4 |-os.chmod("foo", 444) # Error - 4 |+os.chmod("foo", 0o444) # Error -5 5 | os.chmod("foo", 0o444) # OK -6 6 | os.chmod("foo", 7777) # Error -7 7 | os.chmod("foo", 10000) # Error - -RUF064.py:6:17: RUF064 [*] Non-octal mode `7777`, did you mean `0o7777`? - | -4 | os.chmod("foo", 444) # Error -5 | os.chmod("foo", 0o444) # OK -6 | os.chmod("foo", 7777) # Error - | ^^^^ RUF064 -7 | os.chmod("foo", 10000) # Error -8 | os.chmod("foo", 99999) # Error - | - = help: Replace with octal literal +3 3 | import os +4 4 | from pathlib import Path +5 5 | +6 |-os.chmod("foo", 444) # Error + 6 |+os.chmod("foo", 0o444) # Error +7 7 | os.chmod("foo", 0o444) # OK +8 8 | os.chmod("foo", 7777) # Error +9 9 | os.chmod("foo", 10000) # Error + +RUF064.py:8:17: RUF064 [*] Non-octal mode + | + 6 | os.chmod("foo", 444) # Error + 7 | os.chmod("foo", 0o444) # OK + 8 | os.chmod("foo", 7777) # Error + | ^^^^ RUF064 + 9 | os.chmod("foo", 10000) # Error +10 | os.chmod("foo", 99999) # Error + | + = help: Replace with octal literal ℹ Unsafe fix -3 3 | -4 4 | os.chmod("foo", 444) # Error -5 5 | os.chmod("foo", 0o444) # OK -6 |-os.chmod("foo", 7777) # Error - 6 |+os.chmod("foo", 0o7777) # Error -7 7 | os.chmod("foo", 10000) # Error -8 8 | os.chmod("foo", 99999) # Error -9 9 | Path("bar").chmod(755) # Error - -RUF064.py:7:17: RUF064 Non-octal mode - | -5 | os.chmod("foo", 0o444) # OK -6 | os.chmod("foo", 7777) # Error -7 | os.chmod("foo", 10000) # Error - | ^^^^^ RUF064 -8 | os.chmod("foo", 99999) # Error -9 | Path("bar").chmod(755) # Error - | - = help: Replace with octal literal +5 5 | +6 6 | os.chmod("foo", 444) # Error +7 7 | os.chmod("foo", 0o444) # OK +8 |-os.chmod("foo", 7777) # Error + 8 |+os.chmod("foo", 0o7777) # Error +9 9 | os.chmod("foo", 10000) # Error +10 10 | os.chmod("foo", 99999) # Error +11 11 | -RUF064.py:8:17: RUF064 Non-octal mode +RUF064.py:9:17: RUF064 Non-octal mode | - 6 | os.chmod("foo", 7777) # Error - 7 | os.chmod("foo", 10000) # Error - 8 | os.chmod("foo", 99999) # Error + 7 | os.chmod("foo", 0o444) # OK + 8 | os.chmod("foo", 7777) # Error + 9 | os.chmod("foo", 10000) # Error | ^^^^^ RUF064 - 9 | Path("bar").chmod(755) # Error -10 | Path("bar").chmod(0o755) # OK +10 | os.chmod("foo", 99999) # Error + | + = help: Replace with octal literal + +RUF064.py:10:17: RUF064 Non-octal mode + | + 8 | os.chmod("foo", 7777) # Error + 9 | os.chmod("foo", 10000) # Error +10 | os.chmod("foo", 99999) # Error + | ^^^^^ RUF064 +11 | +12 | os.umask(777) # Error + | + = help: Replace with octal literal + +RUF064.py:12:10: RUF064 [*] Non-octal mode + | +10 | os.chmod("foo", 99999) # Error +11 | +12 | os.umask(777) # Error + | ^^^ RUF064 +13 | os.umask(0o777) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +9 9 | os.chmod("foo", 10000) # Error +10 10 | os.chmod("foo", 99999) # Error +11 11 | +12 |-os.umask(777) # Error + 12 |+os.umask(0o777) # Error +13 13 | os.umask(0o777) # OK +14 14 | +15 15 | os.fchmod(0, 400) # Error + +RUF064.py:15:14: RUF064 [*] Non-octal mode + | +13 | os.umask(0o777) # OK +14 | +15 | os.fchmod(0, 400) # Error + | ^^^ RUF064 +16 | os.fchmod(0, 0o400) # OK | = help: Replace with octal literal -RUF064.py:9:19: RUF064 [*] Non-octal mode `755`, did you mean `0o755`? +ℹ Unsafe fix +12 12 | os.umask(777) # Error +13 13 | os.umask(0o777) # OK +14 14 | +15 |-os.fchmod(0, 400) # Error + 15 |+os.fchmod(0, 0o400) # Error +16 16 | os.fchmod(0, 0o400) # OK +17 17 | +18 18 | os.lchmod("foo", 755) # Error + +RUF064.py:18:18: RUF064 [*] Non-octal mode | - 7 | os.chmod("foo", 10000) # Error - 8 | os.chmod("foo", 99999) # Error - 9 | Path("bar").chmod(755) # Error +16 | os.fchmod(0, 0o400) # OK +17 | +18 | os.lchmod("foo", 755) # Error + | ^^^ RUF064 +19 | os.lchmod("foo", 0o755) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +15 15 | os.fchmod(0, 400) # Error +16 16 | os.fchmod(0, 0o400) # OK +17 17 | +18 |-os.lchmod("foo", 755) # Error + 18 |+os.lchmod("foo", 0o755) # Error +19 19 | os.lchmod("foo", 0o755) # OK +20 20 | +21 21 | os.mkdir("foo", 600) # Error + +RUF064.py:21:17: RUF064 [*] Non-octal mode + | +19 | os.lchmod("foo", 0o755) # OK +20 | +21 | os.mkdir("foo", 600) # Error + | ^^^ RUF064 +22 | os.mkdir("foo", 0o600) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +18 18 | os.lchmod("foo", 755) # Error +19 19 | os.lchmod("foo", 0o755) # OK +20 20 | +21 |-os.mkdir("foo", 600) # Error + 21 |+os.mkdir("foo", 0o600) # Error +22 22 | os.mkdir("foo", 0o600) # OK +23 23 | +24 24 | os.makedirs("foo", 644) # Error + +RUF064.py:24:20: RUF064 [*] Non-octal mode + | +22 | os.mkdir("foo", 0o600) # OK +23 | +24 | os.makedirs("foo", 644) # Error + | ^^^ RUF064 +25 | os.makedirs("foo", 0o644) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +21 21 | os.mkdir("foo", 600) # Error +22 22 | os.mkdir("foo", 0o600) # OK +23 23 | +24 |-os.makedirs("foo", 644) # Error + 24 |+os.makedirs("foo", 0o644) # Error +25 25 | os.makedirs("foo", 0o644) # OK +26 26 | +27 27 | os.mkfifo("foo", 640) # Error + +RUF064.py:27:18: RUF064 [*] Non-octal mode + | +25 | os.makedirs("foo", 0o644) # OK +26 | +27 | os.mkfifo("foo", 640) # Error + | ^^^ RUF064 +28 | os.mkfifo("foo", 0o640) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +24 24 | os.makedirs("foo", 644) # Error +25 25 | os.makedirs("foo", 0o644) # OK +26 26 | +27 |-os.mkfifo("foo", 640) # Error + 27 |+os.mkfifo("foo", 0o640) # Error +28 28 | os.mkfifo("foo", 0o640) # OK +29 29 | +30 30 | os.mknod("foo", 660) # Error + +RUF064.py:30:17: RUF064 [*] Non-octal mode + | +28 | os.mkfifo("foo", 0o640) # OK +29 | +30 | os.mknod("foo", 660) # Error + | ^^^ RUF064 +31 | os.mknod("foo", 0o660) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +27 27 | os.mkfifo("foo", 640) # Error +28 28 | os.mkfifo("foo", 0o640) # OK +29 29 | +30 |-os.mknod("foo", 660) # Error + 30 |+os.mknod("foo", 0o660) # Error +31 31 | os.mknod("foo", 0o660) # OK +32 32 | +33 33 | os.open("foo", os.O_CREAT, 644) # Error + +RUF064.py:33:28: RUF064 [*] Non-octal mode + | +31 | os.mknod("foo", 0o660) # OK +32 | +33 | os.open("foo", os.O_CREAT, 644) # Error + | ^^^ RUF064 +34 | os.open("foo", os.O_CREAT, 0o644) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +30 30 | os.mknod("foo", 660) # Error +31 31 | os.mknod("foo", 0o660) # OK +32 32 | +33 |-os.open("foo", os.O_CREAT, 644) # Error + 33 |+os.open("foo", os.O_CREAT, 0o644) # Error +34 34 | os.open("foo", os.O_CREAT, 0o644) # OK +35 35 | +36 36 | Path("bar").chmod(755) # Error + +RUF064.py:36:19: RUF064 [*] Non-octal mode + | +34 | os.open("foo", os.O_CREAT, 0o644) # OK +35 | +36 | Path("bar").chmod(755) # Error | ^^^ RUF064 -10 | Path("bar").chmod(0o755) # OK -11 | path = Path("bar") +37 | Path("bar").chmod(0o755) # OK | = help: Replace with octal literal ℹ Unsafe fix -6 6 | os.chmod("foo", 7777) # Error -7 7 | os.chmod("foo", 10000) # Error -8 8 | os.chmod("foo", 99999) # Error -9 |-Path("bar").chmod(755) # Error - 9 |+Path("bar").chmod(0o755) # Error -10 10 | Path("bar").chmod(0o755) # OK -11 11 | path = Path("bar") -12 12 | path.chmod(755) # Error - -RUF064.py:12:12: RUF064 [*] Non-octal mode `755`, did you mean `0o755`? - | -10 | Path("bar").chmod(0o755) # OK -11 | path = Path("bar") -12 | path.chmod(755) # Error +33 33 | os.open("foo", os.O_CREAT, 644) # Error +34 34 | os.open("foo", os.O_CREAT, 0o644) # OK +35 35 | +36 |-Path("bar").chmod(755) # Error + 36 |+Path("bar").chmod(0o755) # Error +37 37 | Path("bar").chmod(0o755) # OK +38 38 | +39 39 | path = Path("bar") + +RUF064.py:40:12: RUF064 [*] Non-octal mode + | +39 | path = Path("bar") +40 | path.chmod(755) # Error | ^^^ RUF064 -13 | path.chmod(0o755) # OK +41 | path.chmod(0o755) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +37 37 | Path("bar").chmod(0o755) # OK +38 38 | +39 39 | path = Path("bar") +40 |-path.chmod(755) # Error + 40 |+path.chmod(0o755) # Error +41 41 | path.chmod(0o755) # OK +42 42 | +43 43 | dbm.open("db", "r", 600) # Error + +RUF064.py:43:21: RUF064 [*] Non-octal mode + | +41 | path.chmod(0o755) # OK +42 | +43 | dbm.open("db", "r", 600) # Error + | ^^^ RUF064 +44 | dbm.open("db", "r", 0o600) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +40 40 | path.chmod(755) # Error +41 41 | path.chmod(0o755) # OK +42 42 | +43 |-dbm.open("db", "r", 600) # Error + 43 |+dbm.open("db", "r", 0o600) # Error +44 44 | dbm.open("db", "r", 0o600) # OK +45 45 | +46 46 | dbm.gnu.open("db", "r", 600) # Error + +RUF064.py:46:25: RUF064 [*] Non-octal mode + | +44 | dbm.open("db", "r", 0o600) # OK +45 | +46 | dbm.gnu.open("db", "r", 600) # Error + | ^^^ RUF064 +47 | dbm.gnu.open("db", "r", 0o600) # OK + | + = help: Replace with octal literal + +ℹ Unsafe fix +43 43 | dbm.open("db", "r", 600) # Error +44 44 | dbm.open("db", "r", 0o600) # OK +45 45 | +46 |-dbm.gnu.open("db", "r", 600) # Error + 46 |+dbm.gnu.open("db", "r", 0o600) # Error +47 47 | dbm.gnu.open("db", "r", 0o600) # OK +48 48 | +49 49 | dbm.ndbm.open("db", "r", 600) # Error + +RUF064.py:49:26: RUF064 [*] Non-octal mode + | +47 | dbm.gnu.open("db", "r", 0o600) # OK +48 | +49 | dbm.ndbm.open("db", "r", 600) # Error + | ^^^ RUF064 +50 | dbm.ndbm.open("db", "r", 0o600) # OK | = help: Replace with octal literal ℹ Unsafe fix -9 9 | Path("bar").chmod(755) # Error -10 10 | Path("bar").chmod(0o755) # OK -11 11 | path = Path("bar") -12 |-path.chmod(755) # Error - 12 |+path.chmod(0o755) # Error -13 13 | path.chmod(0o755) # OK +46 46 | dbm.gnu.open("db", "r", 600) # Error +47 47 | dbm.gnu.open("db", "r", 0o600) # OK +48 48 | +49 |-dbm.ndbm.open("db", "r", 600) # Error + 49 |+dbm.ndbm.open("db", "r", 0o600) # Error +50 50 | dbm.ndbm.open("db", "r", 0o600) # OK From 464457c4c9ac50de6081346e4a31674cb38e8f6c Mon Sep 17 00:00:00 2001 From: Frazer McLean Date: Wed, 11 Jun 2025 00:03:39 +0200 Subject: [PATCH 4/6] Explain more in fix safety section --- .../src/rules/ruff/rules/non_octal_permissions.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs b/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs index 8a552d90f3b6c..51506c5051377 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs @@ -33,6 +33,20 @@ use crate::{FixAvailability, Violation}; /// /// This rule's fix is marked as unsafe because it changes runtime behavior. /// +/// Consider these examples: +/// +/// ```python +/// os.chmod("foo", 400) +/// os.chmod("bar", 256) +/// ``` +/// +/// `400` corresponds to `0o620` (`u=rw,g=w,o=`). If the intention was `0o400` +/// (`u=r,go=`), the fix can be accepted safely, fixing a permissions issue. +/// +/// `256` corresponds to `0o400` (`u=r,go=`). It is unlikely that `0o256` +/// (`u=w,g=rx,o=rw`) was the intention here and so the fix should not be +/// accepted. It is recommended to change this case to `0o400` manually. +/// /// ## Fix availability /// /// A fix is only available if the existing digits could make up a valid octal literal. From df60a38246d517ff509031061f1a2788f9613242 Mon Sep 17 00:00:00 2001 From: Frazer McLean Date: Wed, 18 Jun 2025 22:09:44 +0200 Subject: [PATCH 5/6] Only suggest fixes for common cases --- .../resources/test/fixtures/ruff/RUF064.py | 3 + .../rules/ruff/rules/non_octal_permissions.rs | 82 +++++++++++++------ ..._rules__ruff__tests__RUF064_RUF064.py.snap | 47 ++++++++--- 3 files changed, 98 insertions(+), 34 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py index f861a3096881b..4f565ae15aee8 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py @@ -48,3 +48,6 @@ dbm.ndbm.open("db", "r", 600) # Error dbm.ndbm.open("db", "r", 0o600) # OK + +os.fchmod(0, 256) # 0o400 +os.fchmod(0, 493) # 0o755 diff --git a/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs b/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs index 51506c5051377..90e70f9201587 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs @@ -31,25 +31,35 @@ use crate::{FixAvailability, Violation}; /// /// ## Fix safety /// -/// This rule's fix is marked as unsafe because it changes runtime behavior. -/// -/// Consider these examples: +/// There are two categories of fix, the first of which is where it looks like +/// the author intended to use an octal literal but the `0o` prefix is missing: /// /// ```python /// os.chmod("foo", 400) -/// os.chmod("bar", 256) +/// os.chmod("foo", 644) /// ``` /// -/// `400` corresponds to `0o620` (`u=rw,g=w,o=`). If the intention was `0o400` -/// (`u=r,go=`), the fix can be accepted safely, fixing a permissions issue. +/// This class of fix changes runtime behaviour. In the first case, `400` +/// corresponds to `0o620` (`u=rw,g=w,o=`). As this mode is not deemed likely, +/// it is changed to `0o400` (`u=r,go=`). Similarly, `644` corresponds to +/// `0o1204` (`u=ws,g=,o=r`) and is changed to `0o644` (`u=rw,go=r`). +/// +/// The second category is decimal literals which are recognised as likely valid +/// but in decimal form: /// -/// `256` corresponds to `0o400` (`u=r,go=`). It is unlikely that `0o256` -/// (`u=w,g=rx,o=rw`) was the intention here and so the fix should not be -/// accepted. It is recommended to change this case to `0o400` manually. +/// ```python +/// os.chmod("foo", 256) +/// os.chmod("foo", 493) +/// ``` +/// +/// `256` corresponds to `0o400` (`u=r,go=`) and `493` corresponds to `0o755` +/// (`u=rwx,go=rx`). Both of these fixes keep runtime behavior unchanged. If the +/// original code really intended to use `0o256` (`u=w,g=rx,o=rw`) instead of +/// `256`, this fix should not be accepted. /// /// ## Fix availability /// -/// A fix is only available if the existing digits could make up a valid octal literal. +/// A fix is only available if the integer literal matches a set of common modes. #[derive(ViolationMetadata)] pub(crate) struct NonOctalPermissions; @@ -88,22 +98,20 @@ pub(crate) fn non_octal_permissions(checker: &Checker, call: &ExprCall) { if mode_literal.starts_with("0o") || mode_literal.starts_with("0O") || mode_literal == "0" { return; } - let mode = int.as_u16(); - - let suggested = match (mode_literal.starts_with('0'), mode) { - (true, _) => None, - (false, Some(found)) => match u16::from_str_radix(&found.to_string(), 8) { - Ok(suggested) if suggested <= 0o7777 => Some(suggested), - _ => None, - }, - _ => None, - }; let mut diagnostic = checker.report_diagnostic(NonOctalPermissions, mode_arg.range()); - if let Some(suggested) = suggested { - let edit = Edit::range_replacement(format!("{suggested:#o}"), mode_arg.range()); - diagnostic.set_fix(Fix::unsafe_edit(edit)); + + // Don't suggest a fix for 0x or 0b literals. + if mode_literal.starts_with('0') { + return; } + + let Some(suggested) = int.as_u16().and_then(suggest_fix) else { + return; + }; + + let edit = Edit::range_replacement(format!("{suggested:#o}"), mode_arg.range()); + diagnostic.set_fix(Fix::unsafe_edit(edit)); } fn find_func_mode_arg<'a>(call: &'a ExprCall, semantic: &SemanticModel) -> Option<&'a Expr> { @@ -175,3 +183,31 @@ fn resolve_method_call<'a>( Some((qualified_name, attr)) } + +/// Try to determine whether the integer literal +fn suggest_fix(mode: u16) -> Option { + // These suggestions are in the form of + // | => + // If could theoretically be a valid octal literal, the + // comment explains why it's deemed unlikely to be intentional. + match mode { + 400 | 256 => Some(0o400), // -w-r-xrw-, group/other > user unlikely + 440 | 288 => Some(0o440), + 444 | 292 => Some(0o444), + 600 | 384 => Some(0o600), + 640 | 416 => Some(0o640), // r----xrw-, other > user unlikely + 644 | 420 => Some(0o644), // r---w----, group write but not read unlikely + 660 | 432 => Some(0o660), // r---wx-w-, write but not read unlikely + 664 | 436 => Some(0o664), // r---wxrw-, other > user unlikely + 666 | 438 => Some(0o666), + 700 | 448 => Some(0o700), + 744 | 484 => Some(0o744), + 750 | 488 => Some(0o750), + 755 | 493 => Some(0o755), + 770 | 504 => Some(0o770), // r-x---r--, other > group unlikely + 775 | 509 => Some(0o775), + 776 | 510 => Some(0o776), // r-x--x---, seems unlikely + 777 | 511 => Some(0o777), // r-x--x--x, seems unlikely + _ => None, + } +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap index 13bb21451c228..e42b87b266f15 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF064_RUF064.py.snap @@ -22,7 +22,7 @@ RUF064.py:6:17: RUF064 [*] Non-octal mode 8 8 | os.chmod("foo", 7777) # Error 9 9 | os.chmod("foo", 10000) # Error -RUF064.py:8:17: RUF064 [*] Non-octal mode +RUF064.py:8:17: RUF064 Non-octal mode | 6 | os.chmod("foo", 444) # Error 7 | os.chmod("foo", 0o444) # OK @@ -33,16 +33,6 @@ RUF064.py:8:17: RUF064 [*] Non-octal mode | = help: Replace with octal literal -ℹ Unsafe fix -5 5 | -6 6 | os.chmod("foo", 444) # Error -7 7 | os.chmod("foo", 0o444) # OK -8 |-os.chmod("foo", 7777) # Error - 8 |+os.chmod("foo", 0o7777) # Error -9 9 | os.chmod("foo", 10000) # Error -10 10 | os.chmod("foo", 99999) # Error -11 11 | - RUF064.py:9:17: RUF064 Non-octal mode | 7 | os.chmod("foo", 0o444) # OK @@ -320,3 +310,38 @@ RUF064.py:49:26: RUF064 [*] Non-octal mode 49 |-dbm.ndbm.open("db", "r", 600) # Error 49 |+dbm.ndbm.open("db", "r", 0o600) # Error 50 50 | dbm.ndbm.open("db", "r", 0o600) # OK +51 51 | +52 52 | os.fchmod(0, 256) # 0o400 + +RUF064.py:52:14: RUF064 [*] Non-octal mode + | +50 | dbm.ndbm.open("db", "r", 0o600) # OK +51 | +52 | os.fchmod(0, 256) # 0o400 + | ^^^ RUF064 +53 | os.fchmod(0, 493) # 0o755 + | + = help: Replace with octal literal + +ℹ Unsafe fix +49 49 | dbm.ndbm.open("db", "r", 600) # Error +50 50 | dbm.ndbm.open("db", "r", 0o600) # OK +51 51 | +52 |-os.fchmod(0, 256) # 0o400 + 52 |+os.fchmod(0, 0o400) # 0o400 +53 53 | os.fchmod(0, 493) # 0o755 + +RUF064.py:53:14: RUF064 [*] Non-octal mode + | +52 | os.fchmod(0, 256) # 0o400 +53 | os.fchmod(0, 493) # 0o755 + | ^^^ RUF064 + | + = help: Replace with octal literal + +ℹ Unsafe fix +50 50 | dbm.ndbm.open("db", "r", 0o600) # OK +51 51 | +52 52 | os.fchmod(0, 256) # 0o400 +53 |-os.fchmod(0, 493) # 0o755 + 53 |+os.fchmod(0, 0o755) # 0o755 From 259c01cf11ae861dcc3979258bce33f4b335e24a Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 19 Jun 2025 11:23:11 +0200 Subject: [PATCH 6/6] Update schema --- ruff.schema.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ruff.schema.json b/ruff.schema.json index b0b1aeb1a5712..ae2a8d4b1e86e 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -4360,4 +4360,4 @@ ] } } -} +} \ No newline at end of file