[ruff] Add sub-diagnostics with permissions (RUF064)#22972
[ruff] Add sub-diagnostics with permissions (RUF064)#22972ntBre merged 9 commits intoastral-sh:mainfrom
ruff] Add sub-diagnostics with permissions (RUF064)#22972Conversation
Signed-off-by: Bhuminjay <bhuminjaysoni@gmail.com>
Signed-off-by: Bhuminjay <bhuminjaysoni@gmail.com>
|
ntBre
left a comment
There was a problem hiding this comment.
Thank you! This looks great overall. I just had two very small nits and a couple of suggestions about the info messages.
| } | ||
| } | ||
|
|
||
| fn get_permissions(mode: u16) -> String { |
There was a problem hiding this comment.
nit: let's move this to the end of the file to keep the main rule implementation at the top.
crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/non_octal_permissions.rs
Outdated
Show resolved
Hide resolved
| info: current value 444 will be interpreted as 0o674, permissions: u=rw-, g=rwx, o=r-- | ||
| info: suggested value: 0o444, permissions: u=r--, g=r--, o=r-- |
There was a problem hiding this comment.
I think something like this might sound slightly better:
| info: current value 444 will be interpreted as 0o674, permissions: u=rw-, g=rwx, o=r-- | |
| info: suggested value: 0o444, permissions: u=r--, g=r--, o=r-- | |
| info: Current value of 444 (0o674) sets permissions: u=rw-, g=rwx, o=r-- | |
| info: Suggested value of 0o444 sets permissions: u=r--, g=r--, o=r-- |
I actually don't mind Micha's very short example from his earlier comment, but he said it needed better wording 😆
There was a problem hiding this comment.
done, I also here particularly like those suggested ones . Should we move with that ?
info: 0oxxx (256) is u=rw, g=w, o=
info: 0o256 is u=....
these look particularly very concise & clean.
|
|
||
| diagnostic.info(format!( | ||
| "current value {mode_literal} will be interpreted as 0o{:o}, permissions: {}", | ||
| mode & 0o777, |
There was a problem hiding this comment.
| mode & 0o777, | |
| mode & 0o7777, |
os.chmod() supports setting sticky/setuid/setgid bits as well as permissions, so it could be helpful to correctly display those bits as well. Not sure it's worth displaying them in the permissions part though.
There was a problem hiding this comment.
done changes to 0o7777 , should I add an extra info like special bits: <>, I am not very sure if we should add this ?
Signed-off-by: Bhuminjay <bhuminjaysoni@gmail.com>
ntBre
left a comment
There was a problem hiding this comment.
Thanks! I pushed a few commits addressing the remaining nits I found, and I left one comment flagging something we should follow up on in the future, but this looks good to me.
| help: Replace with octal literal | ||
| info: Current value of mode 644 (0o1204) sets permissions: u=-w-, g=---, o=r--) | ||
| info: Suggested value of 420 sets permissions: u=rw-, g=r--, o=r-- | ||
| 21 | os.mkdir("foo", 600) # Error | ||
| 22 | os.mkdir("foo", 0o600) # OK | ||
| 23 | | ||
| - os.makedirs("foo", 644) # Error | ||
| 24 + os.makedirs("foo", 0o644) # Error | ||
| 25 | os.makedirs("foo", 0o644) # OK | ||
| 26 | | ||
| 27 | os.mkfifo("foo", 640) # Error | ||
| note: This is an unsafe fix and may change runtime behavior |
There was a problem hiding this comment.
I don't think this needs to block this PR, I'm just flagging this for a follow-up, but I think this is our first usage of a sub-diagnostic in Ruff, and you can see that the info messages are emitted between the help message and the diff that was intended to come right after it.
Summary -- As noted in #22972 (comment), attaching sub-diagnostics to Ruff's diagnostics can interact a bit awkwardly with how we attach the `Violation::fix_title` and then render a fix diff. In particular, the fix title is currently attached as a `help` sub-diagnostic when the diagnostic is originally created, meaning that any additional sub-diagnostics appear between the fix title and the rendered fix: ``` RUF064 [*] Non-octal mode --> RUF064.py:6:17 | 4 | from pathlib import Path 5 | 6 | os.chmod("foo", 444) # Error | ^^^ 7 | os.chmod("foo", 0o444) # OK 8 | os.chmod("foo", 7777) # Error | help: Replace with octal literal info: Current value of mode 444 (0o674) sets permissions: u=rw-, g=rwx, o=r--) info: Suggested value of 292 sets permissions: u=r--, g=r--, o=r-- 3 | import os 4 | from pathlib import Path 5 | - os.chmod("foo", 444) # Error 6 + os.chmod("foo", 0o444) # Error 7 | os.chmod("foo", 0o444) # OK 8 | os.chmod("foo", 7777) # Error 9 | os.chmod("foo", 10000) # Error note: This is an unsafe fix and may change runtime behavior ``` Instead of adding the fix title immediately, this PR stores it on our `DiagnosticGuard` type to be added just before the guard is dropped and the diagnostic is stored. I think a better long-term fix would be to attach the diff to the sub-diagnostic with the `help` message/fix title somehow and render these `info` sub-diagnostics _after_ the diff (or let the diagnostic author choose the order), but this seemed like an easy improvement over the current approach, at least. Test Plan -- Existing tests for ISC004 and from #22972 showing the `help` message at the end.
ruff] Add sub-diagnostics with permissions (RUF064)
* main: (48 commits) add info for non_octal permissions (#22972) Fix empty body rule rendering (#23039) [ty] Infer `ParamSpec` from class constructors for callable protocols (#22853) Update NPM Development dependencies (#23030) Update CodSpeedHQ/action action to v4.8.2 (#23029) [ty] remove special handling for `Any()` in match class patterns (#23011) Update Rust crate get-size2 to v0.7.4 (#23022) Update Rust crate insta to v1.46.1 (#23023) Update taiki-e/install-action action to v2.67.11 (#23033) Update Rust crate colored to v3.1.1 (#23031) Update cargo-bins/cargo-binstall action to v1.17.3 (#23028) Update Rust crate uuid to v1.20.0 (#23032) [ty] Avoid using `.node()` for detecting `Self` (#23000) Update Rust crate proc-macro2 to v1.0.106 (#23024) Update actions/setup-python action to v6.2.0 (#23027) [ty] fix query cycles in decorated function with parameter defaults (#23014) Update Rust crate quote to v1.0.44 (#23025) Update Rust crate thiserror to v2.0.18 (#23026) Update Rust crate filetime to v0.2.27 (#23021) Update Rust crate clearscreen to v4.0.3 (#23020) ...
Summary
Part of #17203, comment: #18541 (comment)
Test Plan
updated snapshot