Add lint rule for calling chmod with non-octal integers by RazerM · Pull Request #18541 · astral-sh/ruff

RazerM · 2025-06-07T22:26:51Z

Resolves #18464

Summary

Adds a new lint rule to find cases where chmod was called with a decimal integer, e.g. os.chmod(400) looks like it would set u=r,go= permissions but actually sets u=rw,g=w,o=. The intent was probably 0o400.

Test Plan

cargo test

github-actions · 2025-06-08T03:42:29Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+1 -0 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

apache/airflow (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ devel-common/src/tests_common/test_utils/gcp_system_helpers.py:175:32: RUF064 Non-octal mode

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF064	1	1	0	0	0

MichaReiser · 2025-06-09T17:26:05Z

Related clippy rule https://rust-lang.github.io/rust-clippy/master/index.html#non_octal_unix_permissions

MichaReiser

Thank you.

This mostly looks good. My main feedback is that I don't think we should offer a fix here.

MichaReiser · 2025-06-09T17:28:08Z

crates/ruff_linter/src/codes.rs

        (Ruff, "058") => (RuleGroup::Preview, rules::ruff::rules::StarmapZip),
        (Ruff, "059") => (RuleGroup::Preview, rules::ruff::rules::UnusedUnpackedVariable),
        (Ruff, "060") => (RuleGroup::Preview, rules::ruff::rules::InEmptyCollection),
+        (Ruff, "064") => (RuleGroup::Preview, rules::ruff::rules::ChmodNonOctal),


I do like clippy's naming because it leaves the door open to lint for other cases than just chmod (if there are any)

I'm definitely open to naming suggestions. My only gripe with the phrase "unix permissions" is that this API is available on Windows in Python (albeit only the 0o400 and 0o200 flags do something)

I think NonOctalPermissions would be fine (just dropping the unix part from clippy's name). As Micha said, it looks like many of the os methods take a mode argument, so we could reuse a more generally-named rule for those too.

MichaReiser · 2025-06-09T17:30:18Z

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

+    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));


I don't think we should offer a fix here. The main promise of the rule is that the code is probably incorrect and just rewriting the code from decimal to octal doesn't fix the root problem -> That the code is probably just incorrect.

I'd be okay if we allowlisted some common chmods and offered rewriting them but I don't think we should offer rewriting in general.

MichaReiser · 2025-06-09T17:31:01Z

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

+    fn message(&self) -> String {
+        match self.reason {
+            Reason::Decimal { found, suggested } => {
+                format!("Non-octal mode `{found}`, did you mean `{suggested:#o}`?")


See my comment below. I don't think we should offer a suggestion here because the promise of the rule is that we believe that the used decimal permissions are actually wrong and simply rewriting to octal doesn't fix that.

MichaReiser · 2025-06-09T18:23:54Z

@ntBre reached out to me on Discord and I think I could be persuaded that we should offer a fix (but I would remove the suggestion from the lint message, given that the fix will show the mode).

I mainly focused on existing (working) code. The fix would be incorrect in those cases and is probably going to break the user code. This seems fine, given that the fix is marked as unsafe.

What I think is the main promise of the rule is that it catches an incorrect permission when you're writing new code. Providing a fix seems very useful in those cases. And I think that should work well because we do show unsafe fixes in the IDE as code actions that users can apply manually.

RazerM · 2025-06-09T19:34:36Z

I also hemmed and hawed about whether to include a fix, and whether it should be limited to certain common cases (e.g. 644 -> 0o644).

A public code search is interesting.

There definitely are cases where a correct decimal integer is used, and often but not always there is a comment of the equivalent octal. This may stem back to code bases that supported Python 2.5 and Python 3 (octal syntax 0o came in 2.6).

E.g. the pytest test suite contains chmod(256) which is equivalent to 0o400. Our suggested fix would be wrong.

MichaReiser

Okay, I think we can go with a fix and see whether users find it useful.

I've a few smaller remarks.

MichaReiser · 2025-06-10T10:56:57Z

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

+///
+/// ## Fix safety
+///
+/// This rule's fix is marked as unsafe because it changes runtime behavior.


It would be great if we could be more concrete here. I think the airflow example that you mentioned would be a great example

MichaReiser · 2025-06-10T10:57:36Z

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

+        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));


Suggested change

diagnostic.set_fix(Fix::applicable_edit(edit, Applicability::Unsafe));

diagnostic.set_fix(Fix::unsafe_edit(edit));

MichaReiser · 2025-06-10T10:58:06Z

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

+    fn message(&self) -> String {
+        match self.reason {
+            Reason::Decimal { found, suggested } => {
+                format!("Non-octal mode `{found}`, did you mean `{suggested:#o}`?")


I'd remove the suggested as it is also visible in the fix

RazerM · 2025-06-10T22:14:06Z

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

+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        "Non-octal mode".to_string()


I like the idea but I think it's confusing in the fix message because it gives the impression that u=rw, ... was the non octal mode, which it wasn't.

I think this is a case where sub diagnostics (@ntBre is working on this) would be very useful. You could then add two hints (needs better wording but roughly):

info: 0oxxx (256) is u=rw, g=w, o= info: 0o256 is u=....

But our diagnostics don't support this today :(

Added to #17203!

RazerM · 2025-06-10T22:57:20Z

I'm tempted to make the fix logic this:

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--x, seems unlikely
        777 | 511 => Some(0o777), // r-x--x--x, seems unlikely
        _ => None
    }
}

MichaReiser · 2025-06-11T06:42:59Z

I'm tempted to make the fix logic this:

I like it!

Resolves astral-sh#18464

MichaReiser

Awesome, thank you

* main: (68 commits) Unify `OldDiagnostic` and `Message` (#18391) [`pylint`] Detect more exotic NaN literals in `PLW0177` (#18630) [`flake8-async`] Mark autofix for `ASYNC115` as unsafe if the call expression contains comments (#18753) [`flake8-bugbear`] Mark autofix for `B004` as unsafe if the `hasattr` call expr contains comments (#18755) Enforce `pytest` import for decorators (#18779) [`flake8-comprehension`] Mark autofix for `C420` as unsafe if there's comments inside the dict comprehension (#18768) [flake8-async] fix detection for large integer sleep durations in `ASYNC116` rule (#18767) Update dependency ruff to v0.12.0 (#18790) Update taiki-e/install-action action to v2.53.2 (#18789) Add lint rule for calling chmod with non-octal integers (#18541) Mark `RET501` fix unsafe if comments are inside (#18780) Use `LintContext::report_diagnostic_if_enabled` in `check_tokens` (#18769) [UP008]: use `super()`, not `__super__` in error messages (#18743) Use Depot Windows runners for `cargo test` (#18754) Run ty benchmarks when `ruff_benchmark` changes (#18758) Disallow newlines in format specifiers of single quoted f- or t-strings (#18708) [ty] Add more benchmarks (#18714) [ty] Anchor all exclude patterns (#18685) Include changelog reference for other major versions (#18745) Use updated pre-commit id (#18718) ...

## Summary  Part of #17203, comment: #18541 (comment) ## Test Plan  updated snapshot --------- Signed-off-by: Bhuminjay <bhuminjaysoni@gmail.com> Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>

ntBre self-requested a review June 8, 2025 03:35

ntBre added the rule Implementing or modifying a lint rule label Jun 8, 2025

MichaReiser reviewed Jun 9, 2025

View reviewed changes

MichaReiser reviewed Jun 10, 2025

View reviewed changes

RazerM force-pushed the feature/chmod-non-octal branch 2 times, most recently from 23dcc33 to 3bebfc1 Compare June 10, 2025 22:10

RazerM commented Jun 10, 2025

View reviewed changes

ntBre mentioned this pull request Apr 4, 2025

Use new diagnostic functionality #17203

Open

7 tasks

RazerM added 5 commits June 18, 2025 22:11

Add lint rule for calling chmod with non-octal integers

83cf848

Resolves astral-sh#18464

Rename to NonOctalPermissions

775f3f3

Expand to all standard library functions which take a numeric mode

fe33540

Explain more in fix safety section

464457c

Only suggest fixes for common cases

df60a38

RazerM force-pushed the feature/chmod-non-octal branch from 3bebfc1 to df60a38 Compare June 18, 2025 20:46

Update schema

259c01c

MichaReiser approved these changes Jun 19, 2025

View reviewed changes

MichaReiser merged commit f67ff33 into astral-sh:main Jun 19, 2025
33 of 34 checks passed

BrewTestBot mentioned this pull request Jun 26, 2025

ruff 0.12.1 Homebrew/homebrew-core#228300

Merged

11happy mentioned this pull request Jan 30, 2026

[ruff] Add sub-diagnostics with permissions (RUF064) #22972

Merged

	diagnostic.set_fix(Fix::applicable_edit(edit, Applicability::Unsafe));
	diagnostic.set_fix(Fix::unsafe_edit(edit));

Conversation

RazerM commented Jun 7, 2025

Summary

Test Plan

Uh oh!

github-actions bot commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

MichaReiser commented Jun 9, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented Jun 9, 2025

Uh oh!

RazerM commented Jun 9, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RazerM commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaReiser commented Jun 11, 2025

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Jun 8, 2025 •

edited

Loading

`ruff-ecosystem` results

RazerM commented Jun 10, 2025 •

edited

Loading