Add lint rule for calling chmod with non-octal integers (astral-sh#18541)

Co-authored-by: Micha Reiser <[email protected]>

Author: RazerM

crates/ruff_linter/resources/test/fixtures/ruff/RUF064.py

@@ -0,0 +1,53 @@
+import dbm.gnu
+import dbm.ndbm
+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
+
+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
+
+os.fchmod(0, 256)  # 0o400
+os.fchmod(0, 493)  # 0o755

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::NonOctalPermissions) {
+                ruff::rules::non_octal_permissions(checker, call);
+            }
         }
         Expr::Dict(dict) => {
             if checker.any_enabled(&[

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::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),

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::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"))]

crates/ruff_linter/src/rules/ruff/rules/mod.rs

@@ -29,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::*;
@@ -91,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;

crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs

@@ -0,0 +1,213 @@
+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;
+
+use crate::checkers::ast::Checker;
+use crate::{FixAvailability, Violation};
+
+/// ## What it does
+/// Checks for standard library functions which take a numeric `mode` argument
+/// where a non-octal integer literal is passed.
+///
+/// ## 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
+///
+/// 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("foo", 644)
+/// ```
+///
+/// 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:
+///
+/// ```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 integer literal matches a set of common modes.
+#[derive(ViolationMetadata)]
+pub(crate) struct NonOctalPermissions;
+
+impl Violation for NonOctalPermissions {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        "Non-octal mode".to_string()
+    }
+
+    fn fix_title(&self) -> Option<String> {
+        Some("Replace with octal literal".to_string())
+    }
+}
+
+/// RUF064
+pub(crate) fn non_octal_permissions(checker: &Checker, call: &ExprCall) {
+    let mode_arg = find_func_mode_arg(call, checker.semantic())
+        .or_else(|| find_method_mode_arg(call, checker.semantic()));
+
+    let Some(mode_arg) = mode_arg 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 mut diagnostic = checker.report_diagnostic(NonOctalPermissions, mode_arg.range());
+
+    // 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> {
+    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)?;
+
+    match (type_name.segments(), attr_name) {
+        (
+            ["pathlib", "Path" | "PosixPath" | "WindowsPath"],
+            "chmod" | "lchmod" | "mkdir" | "touch",
+        ) => call.arguments.find_argument_value("mode", 0),
+        _ => None,
+    }
+}
+
+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 None;
+    };
+
+    // 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() {
+        let qualified_name = semantic.resolve_qualified_name(&call.func)?;
+        return Some((qualified_name, attr));
+    }
+
+    // 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 None;
+    };
+
+    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 None;
+    };
+
+    let qualified_name = semantic.resolve_qualified_name(&call.func)?;
+
+    Some((qualified_name, attr))
+}
+
+/// Try to determine whether the integer literal
+fn suggest_fix(mode: u16) -> Option<u16> {
+    // These suggestions are in the form of
+    // <missing `0o` prefix> | <mode as decimal> => <octal>
+    // If <as decimal> 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,
+    }
+}