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.

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

[ruff] Guard against use of default_factory as a keyword argument (RUF026) #9651

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
120 changes: 120 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF026.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
from collections import defaultdict

# Violation cases: RUF026


def func():
defaultdict(default_factory=None) # RUF026


def func():
defaultdict(default_factory=int) # RUF026


def func():
defaultdict(default_factory=float) # RUF026


def func():
defaultdict(default_factory=dict) # RUF026


def func():
defaultdict(default_factory=list) # RUF026


def func():
defaultdict(default_factory=tuple) # RUF026


def func():
def foo():
pass

defaultdict(default_factory=foo) # RUF026


def func():
defaultdict(default_factory=lambda: 1) # RUF026


def func():
from collections import deque

defaultdict(default_factory=deque) # RUF026


def func():
class MyCallable:
def __call__(self):
pass

defaultdict(default_factory=MyCallable()) # RUF026


def func():
defaultdict(default_factory=tuple, member=1) # RUF026


def func():
defaultdict(member=1, default_factory=tuple) # RUF026


def func():
defaultdict(member=1, default_factory=tuple,) # RUF026


def func():
defaultdict(
member=1,
default_factory=tuple,
) # RUF026


def func():
defaultdict(
default_factory=tuple,
member=1,
) # RUF026


# Non-violation cases: RUF026


def func():
defaultdict(default_factory=1) # OK


def func():
defaultdict(default_factory="wdefwef") # OK


def func():
defaultdict(default_factory=[1, 2, 3]) # OK


def func():
defaultdict() # OK


def func():
defaultdict(int) # OK


def func():
defaultdict(list) # OK


def func():
defaultdict(dict) # OK


def func():
defaultdict(dict, defaultdict=list) # OK


def func():
def constant_factory(value):
return lambda: value

defaultdict(constant_factory("<missing>"))
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,10 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::UnsortedDunderAll) {
ruff::rules::sort_dunder_all_extend_call(checker, call);
}

if checker.enabled(Rule::DefaultFactoryKwarg) {
ruff::rules::default_factory_kwarg(checker, call);
}
}
Expr::Dict(dict) => {
if checker.any_enabled(&[
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -931,6 +931,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "023") => (RuleGroup::Preview, rules::ruff::rules::UnsortedDunderSlots),
(Ruff, "024") => (RuleGroup::Preview, rules::ruff::rules::MutableFromkeysValue),
(Ruff, "025") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryDictComprehensionForIterable),
(Ruff, "026") => (RuleGroup::Preview, rules::ruff::rules::DefaultFactoryKwarg),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),

Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ mod tests {
#[test_case(Rule::UnsortedDunderSlots, Path::new("RUF023.py"))]
#[test_case(Rule::MutableFromkeysValue, Path::new("RUF024.py"))]
#[test_case(Rule::UnnecessaryDictComprehensionForIterable, Path::new("RUF025.py"))]
#[test_case(Rule::DefaultFactoryKwarg, Path::new("RUF026.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
163 changes: 163 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/default_factory_kwarg.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
use anyhow::Result;

use ast::Keyword;
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::is_constant;
use ruff_python_ast::{self as ast, Expr};
use ruff_source_file::Locator;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;
use crate::fix::edits::{remove_argument, Parentheses};
use crate::fix::snippet::SourceCodeSnippet;

/// ## What it does
/// Checks for incorrect usages of `default_factory` as a keyword argument when
/// initializing a `defaultdict`.
///
/// ## Why is this bad?
/// The `defaultdict` constructor accepts a callable as its first argument.
/// For example, it's common to initialize a `defaultdict` with `int` or `list`
/// via `defaultdict(int)` or `defaultdict(list)`, to create a dictionary that
/// returns `0` or `[]` respectively when a key is missing.
///
/// The default factory _must_ be provided as a positional argument, as all
/// keyword arguments to `defaultdict` are interpreted as initial entries in
/// the dictionary. For example, `defaultdict(foo=1, bar=2)` will create a
/// dictionary with `{"foo": 1, "bar": 2}` as its initial entries.
///
/// As such, `defaultdict(default_factory=list)` will create a dictionary with
/// `{"default_factory": list}` as its initial entry, instead of a dictionary
/// that returns `[]` when a key is missing. Specifying a `default_factory`
/// keyword argument is almost always a mistake, and one that type checkers
/// can't reliably detect.
///
/// ## Fix safety
/// This rule's fix is marked as unsafe, as converting `default_factory` from a
/// keyword to a positional argument will change the behavior of the code, even
/// if the keyword argument was used erroneously.
///
/// ## Examples
/// ```python
/// defaultdict(default_factory=int)
/// defaultdict(default_factory=list)
/// ```
///
/// Use instead:
/// ```python
/// defaultdict(int)
/// defaultdict(list)
/// ```
#[violation]
pub struct DefaultFactoryKwarg {
default_factory: SourceCodeSnippet,
}

impl Violation for DefaultFactoryKwarg {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!("`default_factory` is a positional-only argument to `defaultdict`")
}

fn fix_title(&self) -> Option<String> {
let DefaultFactoryKwarg { default_factory } = self;
if let Some(default_factory) = default_factory.full_display() {
Some(format!("Replace with `defaultdict({default_factory})`"))
} else {
Some("Use positional argument".to_string())
}
}
}

/// RUF026
pub(crate) fn default_factory_kwarg(checker: &mut Checker, call: &ast::ExprCall) {
// If the call isn't a `defaultdict` constructor, return.
if !checker
.semantic()
.resolve_call_path(call.func.as_ref())
.is_some_and(|call_path| matches!(call_path.as_slice(), ["collections", "defaultdict"]))
{
return;
}

// If the user provided a positional argument for `default_factory`, return.
if !call.arguments.args.is_empty() {
return;
}

// If the user didn't provide a `default_factory` keyword argument, return.
let Some(keyword) = call.arguments.find_keyword("default_factory") else {
return;
};

// If the value is definitively not callable, return.
if is_non_callable_value(&keyword.value) {
return;
}

let mut diagnostic = Diagnostic::new(
DefaultFactoryKwarg {
default_factory: SourceCodeSnippet::from_str(checker.locator().slice(keyword)),
},
call.range(),
);
diagnostic.try_set_fix(|| convert_to_positional(call, keyword, checker.locator()));
checker.diagnostics.push(diagnostic);
}

/// Returns `true` if a value is definitively not callable (e.g., `1` or `[]`).
fn is_non_callable_value(value: &Expr) -> bool {
is_constant(value)
|| matches!(value, |Expr::List(_)| Expr::Dict(_)
| Expr::Set(_)
| Expr::Tuple(_)
| Expr::Slice(_)
| Expr::ListComp(_)
| Expr::SetComp(_)
| Expr::DictComp(_)
| Expr::GeneratorExp(_)
| Expr::FString(_))
}

/// Generate an [`Expr`] to replace `defaultdict(default_factory=callable)` with
/// `defaultdict(callable)`.
///
/// For example, given `defaultdict(default_factory=list)`, generate `defaultdict(list)`.
fn convert_to_positional(
call: &ast::ExprCall,
default_factory: &Keyword,
locator: &Locator,
) -> Result<Fix> {
if call.arguments.len() == 1 {
// Ex) `defaultdict(default_factory=list)`
Ok(Fix::unsafe_edit(Edit::range_replacement(
locator.slice(&default_factory.value).to_string(),
default_factory.range(),
)))
} else {
// Ex) `defaultdict(member=1, default_factory=list)`

// First, remove the `default_factory` keyword argument.
let removal_edit = remove_argument(
default_factory,
&call.arguments,
Parentheses::Preserve,
locator.contents(),
)?;

// Second, insert the value as the first positional argument.
let insertion_edit = Edit::insertion(
format!("{}, ", locator.slice(&default_factory.value)),
call.arguments
.arguments_source_order()
.next()
.ok_or_else(|| anyhow::anyhow!("`default_factory` keyword argument not found"))?
.start(),
);

Ok(Fix::unsafe_edits(insertion_edit, [removal_edit]))
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ pub(crate) use ambiguous_unicode_character::*;
pub(crate) use assignment_in_assert::*;
pub(crate) use asyncio_dangling_task::*;
pub(crate) use collection_literal_concatenation::*;
pub(crate) use default_factory_kwarg::*;
pub(crate) use explicit_f_string_type_conversion::*;
pub(crate) use function_call_in_dataclass_default::*;
pub(crate) use implicit_optional::*;
Expand All @@ -27,6 +28,7 @@ mod assignment_in_assert;
mod asyncio_dangling_task;
mod collection_literal_concatenation;
mod confusables;
mod default_factory_kwarg;
mod explicit_f_string_type_conversion;
mod function_call_in_dataclass_default;
mod helpers;
Expand Down