Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pylint] Add add_argument utility and autofix for PLW1514 #8928

Merged
merged 7 commits into from
Dec 1, 2023
Merged

[pylint] Add add_argument utility and autofix for PLW1514 #8928

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
25849a4
Add autofix for PLW1514
qdegraaf Nov 30, 2023
d4d0b49
Add test and fixture for PY39 or lower case
qdegraaf Nov 30, 2023
fec762c
clippy
qdegraaf Nov 30, 2023
5e7be98
Add generic `add_argument` util and replace fix with call to it
qdegraaf Dec 1, 2023
afde711
Add no arg fixture
qdegraaf Dec 1, 2023
70bef5c
Handle parentheses
charliermarsh Dec 1, 2023
d4c7cca
Fix multi-parentheses
charliermarsh Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,4 +318,15 @@ mod tests {
assert_messages!(diagnostics);
Ok(())
}

#[test]
fn unspecified_encoding_python39_or_lower() -> Result<()> {
let diagnostics = test_path(
Path::new("pylint/unspecified_encoding.py"),
&LinterSettings::for_rule(Rule::UnspecifiedEncoding)
.with_target_version(PythonVersion::Py39),
)?;
assert_messages!(diagnostics);
Ok(())
}
}
35 changes: 30 additions & 5 deletions crates/ruff_linter/src/rules/pylint/rules/unspecified_encoding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_ast::call_path::{format_call_path, CallPath};
use ruff_text_size::Ranged;
use ruff_python_ast::imports::{AnyImport, Import};
use ruff_text_size::{Ranged, TextSize};

use crate::checkers::ast::Checker;
use crate::settings::types::PythonVersion;

/// ## What it does
/// Checks for uses of `open` and related calls without an explicit `encoding`
Expand Down Expand Up @@ -35,7 +37,7 @@ pub struct UnspecifiedEncoding {
mode: Mode,
}

impl Violation for UnspecifiedEncoding {
impl AlwaysFixableViolation for UnspecifiedEncoding {
#[derive_message_formats]
fn message(&self) -> String {
let UnspecifiedEncoding {
Expand All @@ -52,6 +54,10 @@ impl Violation for UnspecifiedEncoding {
}
}
}

fn fix_title(&self) -> String {
format!("Add explicit `encoding` argument")
}
}

/// PLW1514
Expand All @@ -70,13 +76,32 @@ pub(crate) fn unspecified_encoding(checker: &mut Checker, call: &ast::ExprCall)
return;
};

checker.diagnostics.push(Diagnostic::new(
let mut diagnostic = Diagnostic::new(
UnspecifiedEncoding {
function_name,
mode,
},
call.func.range(),
));
);

if checker.settings.target_version >= PythonVersion::Py310 {
diagnostic.set_fix(Fix::unsafe_edit(Edit::insertion(
", encoding=\"locale\"".to_string(),
call.range().end() - TextSize::from(1),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this introduce a syntax error if the call contains no arguments? Are you interested in writing a more robust add_argument utility, similar to the remove_argument that exists today?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it would. open() and similar calls all have a required first argument AFAIK but it's still a pretty dirty and unsafe fix.

I'd be interested in adding that utility for sure! Will set this to draft and use the autofix as a test case for the utility if there is no urgency behind it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks @qdegraaf!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented a coarse first version which only handles keyword arguments, but does check for trailing commas and work with empty calls. It explicitly forbids positional arguments but could still maybe break when adding a first argument to a class instantiation.

I could either use the lexer to check for parentheses and panic if they aren't there and leave the implementation of that to a future PR or implement it in this one. What's your preference? If the latter is preferred, if you happen to know of a rule where something like that happens on which I can test the util in absence of unit test infrastructure let me know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to leave this as function-only for now, so we can assume the parentheses always exist.

)));
} else {
let import_edit = checker.importer().add_import(
&AnyImport::Import(Import::module("locale")),
TextSize::default(),
);
let call_edit = Edit::insertion(
", encoding=locale.getpreferredencoding(False)".to_string(),
call.range().end() - TextSize::from(1),
);
diagnostic.set_fix(Fix::unsafe_edits(import_edit, [call_edit]));
}

checker.diagnostics.push(diagnostic);
}

/// Returns `true` if the given expression is a string literal containing a `b` character.
Expand Down
91 changes: 84 additions & 7 deletions .../pylint/snapshots/ruff_linter__rules__pylint__tests__PLW1514_unspecified_encoding.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,27 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
unspecified_encoding.py:8:1: PLW1514 `open` in text mode without explicit `encoding` argument
unspecified_encoding.py:8:1: PLW1514 [*] `open` in text mode without explicit `encoding` argument
|
7 | # Errors.
8 | open("test.txt")
| ^^^^ PLW1514
9 | io.TextIOWrapper(io.FileIO("test.txt"))
10 | hugo.TextIOWrapper(hugo.FileIO("test.txt"))
|
= help: Add explicit `encoding` argument

unspecified_encoding.py:9:1: PLW1514 `io.TextIOWrapper` without explicit `encoding` argument
ℹ Unsafe fix
5 5 | import codecs
6 6 |
7 7 | # Errors.
8 |-open("test.txt")
8 |+open("test.txt", encoding="locale")
9 9 | io.TextIOWrapper(io.FileIO("test.txt"))
10 10 | hugo.TextIOWrapper(hugo.FileIO("test.txt"))
11 11 | tempfile.NamedTemporaryFile("w")

unspecified_encoding.py:9:1: PLW1514 [*] `io.TextIOWrapper` without explicit `encoding` argument
|
7 | # Errors.
8 | open("test.txt")
Expand All @@ -19,8 +30,19 @@ unspecified_encoding.py:9:1: PLW1514 `io.TextIOWrapper` without explicit `encodi
10 | hugo.TextIOWrapper(hugo.FileIO("test.txt"))
11 | tempfile.NamedTemporaryFile("w")
|
= help: Add explicit `encoding` argument

ℹ Unsafe fix
6 6 |
7 7 | # Errors.
8 8 | open("test.txt")
9 |-io.TextIOWrapper(io.FileIO("test.txt"))
9 |+io.TextIOWrapper(io.FileIO("test.txt"), encoding="locale")
10 10 | hugo.TextIOWrapper(hugo.FileIO("test.txt"))
11 11 | tempfile.NamedTemporaryFile("w")
12 12 | tempfile.TemporaryFile("w")

unspecified_encoding.py:10:1: PLW1514 `io.TextIOWrapper` without explicit `encoding` argument
unspecified_encoding.py:10:1: PLW1514 [*] `io.TextIOWrapper` without explicit `encoding` argument
|
8 | open("test.txt")
9 | io.TextIOWrapper(io.FileIO("test.txt"))
Expand All @@ -29,8 +51,19 @@ unspecified_encoding.py:10:1: PLW1514 `io.TextIOWrapper` without explicit `encod
11 | tempfile.NamedTemporaryFile("w")
12 | tempfile.TemporaryFile("w")
|
= help: Add explicit `encoding` argument

unspecified_encoding.py:11:1: PLW1514 `tempfile.NamedTemporaryFile` in text mode without explicit `encoding` argument
ℹ Unsafe fix
7 7 | # Errors.
8 8 | open("test.txt")
9 9 | io.TextIOWrapper(io.FileIO("test.txt"))
10 |-hugo.TextIOWrapper(hugo.FileIO("test.txt"))
10 |+hugo.TextIOWrapper(hugo.FileIO("test.txt"), encoding="locale")
11 11 | tempfile.NamedTemporaryFile("w")
12 12 | tempfile.TemporaryFile("w")
13 13 | codecs.open("test.txt")

unspecified_encoding.py:11:1: PLW1514 [*] `tempfile.NamedTemporaryFile` in text mode without explicit `encoding` argument
|
9 | io.TextIOWrapper(io.FileIO("test.txt"))
10 | hugo.TextIOWrapper(hugo.FileIO("test.txt"))
Expand All @@ -39,8 +72,19 @@ unspecified_encoding.py:11:1: PLW1514 `tempfile.NamedTemporaryFile` in text mode
12 | tempfile.TemporaryFile("w")
13 | codecs.open("test.txt")
|
= help: Add explicit `encoding` argument

ℹ Unsafe fix
8 8 | open("test.txt")
9 9 | io.TextIOWrapper(io.FileIO("test.txt"))
10 10 | hugo.TextIOWrapper(hugo.FileIO("test.txt"))
11 |-tempfile.NamedTemporaryFile("w")
11 |+tempfile.NamedTemporaryFile("w", encoding="locale")
12 12 | tempfile.TemporaryFile("w")
13 13 | codecs.open("test.txt")
14 14 | tempfile.SpooledTemporaryFile(0, "w")

unspecified_encoding.py:12:1: PLW1514 `tempfile.TemporaryFile` in text mode without explicit `encoding` argument
unspecified_encoding.py:12:1: PLW1514 [*] `tempfile.TemporaryFile` in text mode without explicit `encoding` argument
|
10 | hugo.TextIOWrapper(hugo.FileIO("test.txt"))
11 | tempfile.NamedTemporaryFile("w")
Expand All @@ -49,17 +93,39 @@ unspecified_encoding.py:12:1: PLW1514 `tempfile.TemporaryFile` in text mode with
13 | codecs.open("test.txt")
14 | tempfile.SpooledTemporaryFile(0, "w")
|
= help: Add explicit `encoding` argument

unspecified_encoding.py:13:1: PLW1514 `codecs.open` in text mode without explicit `encoding` argument
ℹ Unsafe fix
9 9 | io.TextIOWrapper(io.FileIO("test.txt"))
10 10 | hugo.TextIOWrapper(hugo.FileIO("test.txt"))
11 11 | tempfile.NamedTemporaryFile("w")
12 |-tempfile.TemporaryFile("w")
12 |+tempfile.TemporaryFile("w", encoding="locale")
13 13 | codecs.open("test.txt")
14 14 | tempfile.SpooledTemporaryFile(0, "w")
15 15 |

unspecified_encoding.py:13:1: PLW1514 [*] `codecs.open` in text mode without explicit `encoding` argument
|
11 | tempfile.NamedTemporaryFile("w")
12 | tempfile.TemporaryFile("w")
13 | codecs.open("test.txt")
| ^^^^^^^^^^^ PLW1514
14 | tempfile.SpooledTemporaryFile(0, "w")
|
= help: Add explicit `encoding` argument

ℹ Unsafe fix
10 10 | hugo.TextIOWrapper(hugo.FileIO("test.txt"))
11 11 | tempfile.NamedTemporaryFile("w")
12 12 | tempfile.TemporaryFile("w")
13 |-codecs.open("test.txt")
13 |+codecs.open("test.txt", encoding="locale")
14 14 | tempfile.SpooledTemporaryFile(0, "w")
15 15 |
16 16 | # Non-errors.

unspecified_encoding.py:14:1: PLW1514 `tempfile.SpooledTemporaryFile` in text mode without explicit `encoding` argument
unspecified_encoding.py:14:1: PLW1514 [*] `tempfile.SpooledTemporaryFile` in text mode without explicit `encoding` argument
|
12 | tempfile.TemporaryFile("w")
13 | codecs.open("test.txt")
Expand All @@ -68,5 +134,16 @@ unspecified_encoding.py:14:1: PLW1514 `tempfile.SpooledTemporaryFile` in text mo
15 |
16 | # Non-errors.
|
= help: Add explicit `encoding` argument

ℹ Unsafe fix
11 11 | tempfile.NamedTemporaryFile("w")
12 12 | tempfile.TemporaryFile("w")
13 13 | codecs.open("test.txt")
14 |-tempfile.SpooledTemporaryFile(0, "w")
14 |+tempfile.SpooledTemporaryFile(0, "w", encoding="locale")
15 15 |
16 16 | # Non-errors.
17 17 | open("test.txt", encoding="utf-8")


Loading
Loading