From ea0246c51a3c130076d5a2a2da0407f5807e5cc9 Mon Sep 17 00:00:00 2001 From: Tom Kuson Date: Mon, 2 Sep 2024 13:10:55 +0100 Subject: [PATCH] [`ruff`] Implement post-init-default (`RUF033`) (#13192) Co-authored-by: Alex Waygood --- .../resources/test/fixtures/ruff/RUF033.py | 67 +++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/ruff/mod.rs | 1 + .../ruff_linter/src/rules/ruff/rules/mod.rs | 3 + .../src/rules/ruff/rules/post_init_default.rs | 187 ++++++++++++++++++ ..._rules__ruff__tests__RUF033_RUF033.py.snap | 158 +++++++++++++++ ruff.schema.json | 1 + 8 files changed, 421 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF033.py create mode 100644 crates/ruff_linter/src/rules/ruff/rules/post_init_default.rs create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF033_RUF033.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF033.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF033.py new file mode 100644 index 0000000000..95ab12165b --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF033.py @@ -0,0 +1,67 @@ +from dataclasses import InitVar, dataclass + + +# OK +@dataclass +class Foo: + bar: InitVar[int] = 0 + baz: InitVar[int] = 1 + + def __post_init__(self, bar, baz) -> None: ... + + +# RUF033 +@dataclass +class Foo: + bar: InitVar[int] = 0 + baz: InitVar[int] = 1 + + def __post_init__(self, bar = 11, baz = 11) -> None: ... + + +# RUF033 +@dataclass +class Foo: + def __post_init__(self, bar = 11, baz = 11) -> None: ... + + +# OK +@dataclass +class Foo: + def __something_else__(self, bar = 11, baz = 11) -> None: ... + + +# OK +def __post_init__(foo: bool = True) -> None: ... + + +# OK +class Foo: + def __post_init__(self, x="hmm") -> None: ... + + +# RUF033 +@dataclass +class Foo: + def __post_init__(self, bar: int = 11, baz: Something[Whatever | None] = 11) -> None: ... + + +# RUF033 +@dataclass +class Foo: + """A very helpful docstring. + + Docstrings are very important and totally not a waste of time. + """ + + ping = "pong" + + def __post_init__(self, bar: int = 11, baz: int = 12) -> None: ... + + +# RUF033 +@dataclass +class Foo: + bar = "should've used attrs" + + def __post_init__(self, bar: str = "ahhh", baz: str = "hmm") -> None: ... diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index cc3b772d88..4f73f7fe37 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -380,6 +380,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { if checker.enabled(Rule::WhitespaceAfterDecorator) { pycodestyle::rules::whitespace_after_decorator(checker, decorator_list); } + if checker.enabled(Rule::PostInitDefault) { + ruff::rules::post_init_default(checker, function_def); + } } Stmt::Return(_) => { if checker.enabled(Rule::ReturnOutsideFunction) { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 0171944422..b458629c3d 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -960,6 +960,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "030") => (RuleGroup::Preview, rules::ruff::rules::AssertWithPrintMessage), (Ruff, "031") => (RuleGroup::Preview, rules::ruff::rules::IncorrectlyParenthesizedTupleInSubscript), (Ruff, "032") => (RuleGroup::Preview, rules::ruff::rules::DecimalFromFloatLiteral), + (Ruff, "033") => (RuleGroup::Preview, rules::ruff::rules::PostInitDefault), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index d02c9f7f0e..b6966c2574 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -59,6 +59,7 @@ mod tests { #[test_case(Rule::IncorrectlyParenthesizedTupleInSubscript, Path::new("RUF031.py"))] #[test_case(Rule::DecimalFromFloatLiteral, Path::new("RUF032.py"))] #[test_case(Rule::RedirectedNOQA, Path::new("RUF101.py"))] + #[test_case(Rule::PostInitDefault, Path::new("RUF033.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 4d34f7cff8..b95b436221 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -74,3 +74,6 @@ pub(crate) enum Context { Docstring, Comment, } +pub(crate) use post_init_default::*; + +mod post_init_default; diff --git a/crates/ruff_linter/src/rules/ruff/rules/post_init_default.rs b/crates/ruff_linter/src/rules/ruff/rules/post_init_default.rs new file mode 100644 index 0000000000..a13949f167 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/post_init_default.rs @@ -0,0 +1,187 @@ +use anyhow::Context; + +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast as ast; +use ruff_python_semantic::{Scope, ScopeKind}; +use ruff_python_trivia::{indentation_at_offset, textwrap}; +use ruff_text_size::Ranged; + +use crate::{checkers::ast::Checker, importer::ImportRequest}; + +use super::helpers::is_dataclass; + +/// ## What it does +/// Checks for `__post_init__` dataclass methods with parameter defaults. +/// +/// ## Why is this bad? +/// Adding a default value to a parameter in a `__post_init__` method has no +/// impact on whether the parameter will have a default value in the dataclass's +/// generated `__init__` method. To create an init-only dataclass parameter with +/// a default value, you should use an `InitVar` field in the dataclass's class +/// body and give that `InitVar` field a default value. +/// +/// As the [documentation] states: +/// +/// > Init-only fields are added as parameters to the generated `__init__()` +/// > method, and are passed to the optional `__post_init__()` method. They are +/// > not otherwise used by dataclasses. +/// +/// ## Example +/// ```python +/// from dataclasses import InitVar, dataclass +/// +/// +/// @dataclass +/// class Foo: +/// bar: InitVar[int] = 0 +/// +/// def __post_init__(self, bar: int = 1, baz: int = 2) -> None: +/// print(bar, baz) +/// +/// +/// foo = Foo() # Prints '0 2'. +/// ``` +/// +/// Use instead: +/// ```python +/// from dataclasses import InitVar, dataclass +/// +/// +/// @dataclass +/// class Foo: +/// bar: InitVar[int] = 1 +/// baz: InitVar[int] = 2 +/// +/// def __post_init__(self, bar: int, baz: int) -> None: +/// print(bar, baz) +/// +/// +/// foo = Foo() # Prints '1 2'. +/// ``` +/// +/// ## References +/// - [Python documentation: Post-init processing](https://docs.python.org/3/library/dataclasses.html#post-init-processing) +/// - [Python documentation: Init-only variables](https://docs.python.org/3/library/dataclasses.html#init-only-variables) +/// +/// [documentation]: https://docs.python.org/3/library/dataclasses.html#init-only-variables +#[violation] +pub struct PostInitDefault; + +impl Violation for PostInitDefault { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + + #[derive_message_formats] + fn message(&self) -> String { + format!("`__post_init__` method with argument defaults") + } + + fn fix_title(&self) -> Option { + Some(format!("Use `dataclasses.InitVar` instead")) + } +} + +/// RUF033 +pub(crate) fn post_init_default(checker: &mut Checker, function_def: &ast::StmtFunctionDef) { + if &function_def.name != "__post_init__" { + return; + } + + let current_scope = checker.semantic().current_scope(); + match current_scope.kind { + ScopeKind::Class(class_def) => { + if !is_dataclass(class_def, checker.semantic()) { + return; + } + } + _ => return, + } + + let mut stopped_fixes = false; + let mut diagnostics = vec![]; + + for ast::ParameterWithDefault { + parameter, + default, + range: _, + } in function_def.parameters.iter_non_variadic_params() + { + let Some(default) = default else { + continue; + }; + let mut diagnostic = Diagnostic::new(PostInitDefault, default.range()); + + if !stopped_fixes { + diagnostic.try_set_fix(|| { + use_initvar(current_scope, function_def, parameter, default, checker) + }); + // Need to stop fixes as soon as there is a parameter we cannot fix. + // Otherwise, we risk a syntax error (a parameter without a default + // following parameter with a default). + stopped_fixes |= diagnostic.fix.is_none(); + } + + diagnostics.push(diagnostic); + } + + checker.diagnostics.extend(diagnostics); +} + +/// Generate a [`Fix`] to transform a `__post_init__` default argument into a +/// `dataclasses.InitVar` pseudo-field. +fn use_initvar( + current_scope: &Scope, + post_init_def: &ast::StmtFunctionDef, + parameter: &ast::Parameter, + default: &ast::Expr, + checker: &Checker, +) -> anyhow::Result { + if current_scope.has(¶meter.name) { + return Err(anyhow::anyhow!( + "Cannot add a `{}: InitVar` field to the class body, as a field by that name already exists", + parameter.name + )); + } + + // Ensure that `dataclasses.InitVar` is accessible. For example, + // + `from dataclasses import InitVar` + let (import_edit, initvar_binding) = checker.importer().get_or_import_symbol( + &ImportRequest::import("dataclasses", "InitVar"), + default.start(), + checker.semantic(), + )?; + + // Delete the default value. For example, + // - def __post_init__(self, foo: int = 0) -> None: ... + // + def __post_init__(self, foo: int) -> None: ... + let default_edit = Edit::deletion(parameter.end(), default.end()); + + // Add `dataclasses.InitVar` field to class body. + let locator = checker.locator(); + + let content = { + let parameter_name = locator.slice(¶meter.name); + let default = locator.slice(default); + let line_ending = checker.stylist().line_ending().as_str(); + + if let Some(annotation) = ¶meter + .annotation + .as_deref() + .map(|annotation| locator.slice(annotation)) + { + format!("{parameter_name}: {initvar_binding}[{annotation}] = {default}{line_ending}") + } else { + format!("{parameter_name}: {initvar_binding} = {default}{line_ending}") + } + }; + + let indentation = indentation_at_offset(post_init_def.start(), checker.locator()) + .context("Failed to calculate leading indentation of `__post_init__` method")?; + let content = textwrap::indent(&content, indentation); + + let initvar_edit = Edit::insertion( + content.into_owned(), + locator.line_start(post_init_def.start()), + ); + Ok(Fix::unsafe_edits(import_edit, [default_edit, initvar_edit])) +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF033_RUF033.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF033_RUF033.py.snap new file mode 100644 index 0000000000..a5b42e3f4a --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF033_RUF033.py.snap @@ -0,0 +1,158 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF033.py:19:35: RUF033 `__post_init__` method with argument defaults + | +17 | baz: InitVar[int] = 1 +18 | +19 | def __post_init__(self, bar = 11, baz = 11) -> None: ... + | ^^ RUF033 + | + = help: Use `dataclasses.InitVar` instead + +RUF033.py:19:45: RUF033 `__post_init__` method with argument defaults + | +17 | baz: InitVar[int] = 1 +18 | +19 | def __post_init__(self, bar = 11, baz = 11) -> None: ... + | ^^ RUF033 + | + = help: Use `dataclasses.InitVar` instead + +RUF033.py:25:35: RUF033 [*] `__post_init__` method with argument defaults + | +23 | @dataclass +24 | class Foo: +25 | def __post_init__(self, bar = 11, baz = 11) -> None: ... + | ^^ RUF033 + | + = help: Use `dataclasses.InitVar` instead + +ℹ Unsafe fix +22 22 | # RUF033 +23 23 | @dataclass +24 24 | class Foo: +25 |- def __post_init__(self, bar = 11, baz = 11) -> None: ... + 25 |+ bar: InitVar = 11 + 26 |+ def __post_init__(self, bar, baz = 11) -> None: ... +26 27 | +27 28 | +28 29 | # OK + +RUF033.py:25:45: RUF033 [*] `__post_init__` method with argument defaults + | +23 | @dataclass +24 | class Foo: +25 | def __post_init__(self, bar = 11, baz = 11) -> None: ... + | ^^ RUF033 + | + = help: Use `dataclasses.InitVar` instead + +ℹ Unsafe fix +22 22 | # RUF033 +23 23 | @dataclass +24 24 | class Foo: +25 |- def __post_init__(self, bar = 11, baz = 11) -> None: ... + 25 |+ baz: InitVar = 11 + 26 |+ def __post_init__(self, bar = 11, baz) -> None: ... +26 27 | +27 28 | +28 29 | # OK + +RUF033.py:46:40: RUF033 [*] `__post_init__` method with argument defaults + | +44 | @dataclass +45 | class Foo: +46 | def __post_init__(self, bar: int = 11, baz: Something[Whatever | None] = 11) -> None: ... + | ^^ RUF033 + | + = help: Use `dataclasses.InitVar` instead + +ℹ Unsafe fix +43 43 | # RUF033 +44 44 | @dataclass +45 45 | class Foo: +46 |- def __post_init__(self, bar: int = 11, baz: Something[Whatever | None] = 11) -> None: ... + 46 |+ bar: InitVar[int] = 11 + 47 |+ def __post_init__(self, bar: int, baz: Something[Whatever | None] = 11) -> None: ... +47 48 | +48 49 | +49 50 | # RUF033 + +RUF033.py:46:78: RUF033 [*] `__post_init__` method with argument defaults + | +44 | @dataclass +45 | class Foo: +46 | def __post_init__(self, bar: int = 11, baz: Something[Whatever | None] = 11) -> None: ... + | ^^ RUF033 + | + = help: Use `dataclasses.InitVar` instead + +ℹ Unsafe fix +43 43 | # RUF033 +44 44 | @dataclass +45 45 | class Foo: +46 |- def __post_init__(self, bar: int = 11, baz: Something[Whatever | None] = 11) -> None: ... + 46 |+ baz: InitVar[Something[Whatever | None]] = 11 + 47 |+ def __post_init__(self, bar: int = 11, baz: Something[Whatever | None]) -> None: ... +47 48 | +48 49 | +49 50 | # RUF033 + +RUF033.py:59:40: RUF033 [*] `__post_init__` method with argument defaults + | +57 | ping = "pong" +58 | +59 | def __post_init__(self, bar: int = 11, baz: int = 12) -> None: ... + | ^^ RUF033 + | + = help: Use `dataclasses.InitVar` instead + +ℹ Unsafe fix +56 56 | +57 57 | ping = "pong" +58 58 | +59 |- def __post_init__(self, bar: int = 11, baz: int = 12) -> None: ... + 59 |+ bar: InitVar[int] = 11 + 60 |+ def __post_init__(self, bar: int, baz: int = 12) -> None: ... +60 61 | +61 62 | +62 63 | # RUF033 + +RUF033.py:59:55: RUF033 [*] `__post_init__` method with argument defaults + | +57 | ping = "pong" +58 | +59 | def __post_init__(self, bar: int = 11, baz: int = 12) -> None: ... + | ^^ RUF033 + | + = help: Use `dataclasses.InitVar` instead + +ℹ Unsafe fix +56 56 | +57 57 | ping = "pong" +58 58 | +59 |- def __post_init__(self, bar: int = 11, baz: int = 12) -> None: ... + 59 |+ baz: InitVar[int] = 12 + 60 |+ def __post_init__(self, bar: int = 11, baz: int) -> None: ... +60 61 | +61 62 | +62 63 | # RUF033 + +RUF033.py:67:40: RUF033 `__post_init__` method with argument defaults + | +65 | bar = "should've used attrs" +66 | +67 | def __post_init__(self, bar: str = "ahhh", baz: str = "hmm") -> None: ... + | ^^^^^^ RUF033 + | + = help: Use `dataclasses.InitVar` instead + +RUF033.py:67:59: RUF033 `__post_init__` method with argument defaults + | +65 | bar = "should've used attrs" +66 | +67 | def __post_init__(self, bar: str = "ahhh", baz: str = "hmm") -> None: ... + | ^^^^^ RUF033 + | + = help: Use `dataclasses.InitVar` instead diff --git a/ruff.schema.json b/ruff.schema.json index 100be3d06c..eba40f1aa4 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3739,6 +3739,7 @@ "RUF030", "RUF031", "RUF032", + "RUF033", "RUF1", "RUF10", "RUF100",