Skip to content

Commit

Permalink
[ruff] Guard against use of default_factory as a keyword argument…
Browse files Browse the repository at this point in the history
… (`RUF026`) (#9651)

## Summary

Add a rule for defaultdict(default_factory=callable). Instead suggest
using defaultdict(callable).

See: #9509

If a user tries to bind a "non-callable" to default_factory, the rule
ignores it. Another option would be to warn that it's probably not what
you want. Because Python allows the following:

```python 
from collections import defaultdict

defaultdict(default_factory=1)
```
this raises after you actually try to use it:

```python
dd = defaultdict(default_factory=1)
dd[1]
```
=> 
```bash
KeyError: 1
```

Instead using callable directly in the constructor it will raise (not
being a callable):

```python 
from collections import defaultdict

defaultdict(1)
```
=> 
```bash
TypeError: first argument must be callable or None
```




## Test Plan

```bash
cargo test
```
  • Loading branch information
mikeleppane committed Jan 26, 2024
1 parent b61b0ed commit d496c16
Show file tree
Hide file tree
Showing 8 changed files with 561 additions and 0 deletions.
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

0 comments on commit d496c16

Please sign in to comment.