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

Add Implementation for Pylint E1132: Repeated Keyword #8706

Merged
merged 7 commits into from
Nov 19, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
def func(a=10, b=20, c=30):
pass

# Valid
func(a=11, b=21, c=31)
func(b=11, a=21, c=31)
func(c=11, a=21, b=31)
func(a=11, b=31, **{"c": 41})
func(a=11, **{"b": 21}, **{"c": 41})
func(a=11, **{"b": 21, "c": 41})
func(**{"b": 21, "c": 41})
func(**{"a": 11}, **{"b": 21}, **{"c": 41})
func(**{"a": 11, "b": 21, "c": 41})

# Invalid
func(a=11, c=31, **{"c": 41})
func(a=11, c=31, **{"c": 41, "a": 51})
func(a=11, b=21, c=31, **{"b": 22, "c": 41, "a": 51})
func(a=11, b=21, **{"c": 31}, **{"c": 32})
func(a=11, b=21, **{"c": 31, "c": 32})
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::NestedMinMax) {
pylint::rules::nested_min_max(checker, expr, func, args, keywords);
}
if checker.enabled(Rule::RepeatedKeywords) {
pylint::rules::repeated_keywords(checker, keywords);
AlexBieg marked this conversation as resolved.
Show resolved Hide resolved
}
if checker.enabled(Rule::PytestPatchWithLambda) {
if let Some(diagnostic) = flake8_pytest_style::rules::patch_with_lambda(call) {
checker.diagnostics.push(diagnostic);
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 @@ -228,6 +228,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "E0604") => (RuleGroup::Stable, rules::pylint::rules::InvalidAllObject),
(Pylint, "E0605") => (RuleGroup::Stable, rules::pylint::rules::InvalidAllFormat),
(Pylint, "E0704") => (RuleGroup::Preview, rules::pylint::rules::MisplacedBareRaise),
(Pylint, "E1132") => (RuleGroup::Preview, rules::pylint::rules::RepeatedKeywords),
(Pylint, "E1142") => (RuleGroup::Stable, rules::pylint::rules::AwaitOutsideAsync),
(Pylint, "E1205") => (RuleGroup::Stable, rules::pylint::rules::LoggingTooManyArgs),
(Pylint, "E1206") => (RuleGroup::Stable, rules::pylint::rules::LoggingTooFewArgs),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ mod tests {
#[test_case(Rule::UnnecessaryLambda, Path::new("unnecessary_lambda.py"))]
#[test_case(Rule::NonAsciiImportName, Path::new("non_ascii_module_import.py"))]
#[test_case(Rule::NonAsciiName, Path::new("non_ascii_name.py"))]
#[test_case(Rule::RepeatedKeywords, Path::new("repeated_keywords.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
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ pub(crate) use redefined_argument_from_local::*;
pub(crate) use redefined_loop_name::*;
pub(crate) use repeated_equality_comparison::*;
pub(crate) use repeated_isinstance_calls::*;
pub(crate) use repeated_keywords::*;
pub(crate) use return_in_init::*;
pub(crate) use self_assigning_variable::*;
pub(crate) use single_string_slots::*;
Expand Down Expand Up @@ -116,6 +117,7 @@ mod redefined_argument_from_local;
mod redefined_loop_name;
mod repeated_equality_comparison;
mod repeated_isinstance_calls;
mod repeated_keywords;
mod return_in_init;
mod self_assigning_variable;
mod single_string_slots;
Expand Down
93 changes: 93 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/repeated_keywords.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
use std::collections::HashSet;

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{Expr, ExprDict, ExprStringLiteral, Keyword};
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for repeated keyword arguments passed to a function call
///
/// ## Why is this bad?
/// Python does not allow for multiple values to be assigned to the same
/// keyword argument in a single function call.
///
/// ## Example
/// ```python
/// func(1, 2, c=3, **{"c": 4})
/// ```
///
/// Use instead:
/// ```python
/// func(1, 2, **{"c": 4})
/// ```
///
/// ## References
/// - [Python documentation: Argument](https://docs.python.org/3/glossary.html#term-argument)
#[violation]
pub struct RepeatedKeywords {
duplicate_keyword: String,
}

impl Violation for RepeatedKeywords {
#[derive_message_formats]
fn message(&self) -> String {
let dupe = &self.duplicate_keyword;
AlexBieg marked this conversation as resolved.
Show resolved Hide resolved
format!("Repeated keyword argument: `{dupe}`")
}
}

type KeywordRecordFn<'a> = Box<dyn FnMut(&str, TextRange) + 'a>;

fn generate_record_func<'a>(checker: &'a mut Checker) -> KeywordRecordFn<'a> {
// init some hash sets to be captured by the closure
let mut seen = HashSet::<String>::new();
let mut dupes = HashSet::<String>::new();

let inner = move |keyword: &str, range| {
// Add an error the first time we see the duplicate
if seen.contains(keyword) && !dupes.contains(keyword) {
dupes.insert(String::from(keyword));
checker.diagnostics.push(Diagnostic::new(
RepeatedKeywords {
duplicate_keyword: keyword.into(),
},
range,
));
} else {
seen.insert(String::from(keyword));
}
};

Box::new(inner)
}

pub(crate) fn repeated_keywords(checker: &mut Checker, keywords: &Vec<Keyword>) {
AlexBieg marked this conversation as resolved.
Show resolved Hide resolved
let mut record_keyword = generate_record_func(checker);

for keyword in keywords {
if let Some(id) = &keyword.arg {
record_keyword(id.as_str(), keyword.range());
} else if let Expr::Dict(ExprDict {
// We only want to check dict keys if there is NO arg associated with them
keys,
range: _,
values: _,
}) = &keyword.value
{
for key in keys.iter().flatten() {
if let Expr::StringLiteral(ExprStringLiteral {
value,
range: _,
unicode: _,
implicit_concatenated: _,
}) = key
{
record_keyword(value, key.range());
}
}
}
}
}
AlexBieg marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
repeated_keywords.py:16:21: PLE1132 Repeated keyword argument: `c`
|
15 | # Invalid
16 | func(a=11, c=31, **{"c": 41})
| ^^^ PLE1132
17 | func(a=11, c=31, **{"c": 41, "a": 51})
18 | func(a=11, b=21, c=31, **{"b": 22, "c": 41, "a": 51})
|

repeated_keywords.py:17:21: PLE1132 Repeated keyword argument: `c`
|
15 | # Invalid
16 | func(a=11, c=31, **{"c": 41})
17 | func(a=11, c=31, **{"c": 41, "a": 51})
| ^^^ PLE1132
18 | func(a=11, b=21, c=31, **{"b": 22, "c": 41, "a": 51})
19 | func(a=11, b=21, **{"c": 31}, **{"c": 32})
|

repeated_keywords.py:17:30: PLE1132 Repeated keyword argument: `a`
|
15 | # Invalid
16 | func(a=11, c=31, **{"c": 41})
17 | func(a=11, c=31, **{"c": 41, "a": 51})
| ^^^ PLE1132
18 | func(a=11, b=21, c=31, **{"b": 22, "c": 41, "a": 51})
19 | func(a=11, b=21, **{"c": 31}, **{"c": 32})
|

repeated_keywords.py:18:27: PLE1132 Repeated keyword argument: `b`
|
16 | func(a=11, c=31, **{"c": 41})
17 | func(a=11, c=31, **{"c": 41, "a": 51})
18 | func(a=11, b=21, c=31, **{"b": 22, "c": 41, "a": 51})
| ^^^ PLE1132
19 | func(a=11, b=21, **{"c": 31}, **{"c": 32})
20 | func(a=11, b=21, **{"c": 31, "c": 32})
|

repeated_keywords.py:18:36: PLE1132 Repeated keyword argument: `c`
|
16 | func(a=11, c=31, **{"c": 41})
17 | func(a=11, c=31, **{"c": 41, "a": 51})
18 | func(a=11, b=21, c=31, **{"b": 22, "c": 41, "a": 51})
| ^^^ PLE1132
19 | func(a=11, b=21, **{"c": 31}, **{"c": 32})
20 | func(a=11, b=21, **{"c": 31, "c": 32})
|

repeated_keywords.py:18:45: PLE1132 Repeated keyword argument: `a`
|
16 | func(a=11, c=31, **{"c": 41})
17 | func(a=11, c=31, **{"c": 41, "a": 51})
18 | func(a=11, b=21, c=31, **{"b": 22, "c": 41, "a": 51})
| ^^^ PLE1132
19 | func(a=11, b=21, **{"c": 31}, **{"c": 32})
20 | func(a=11, b=21, **{"c": 31, "c": 32})
|

repeated_keywords.py:19:34: PLE1132 Repeated keyword argument: `c`
|
17 | func(a=11, c=31, **{"c": 41, "a": 51})
18 | func(a=11, b=21, c=31, **{"b": 22, "c": 41, "a": 51})
19 | func(a=11, b=21, **{"c": 31}, **{"c": 32})
| ^^^ PLE1132
20 | func(a=11, b=21, **{"c": 31, "c": 32})
|

repeated_keywords.py:20:30: PLE1132 Repeated keyword argument: `c`
|
18 | func(a=11, b=21, c=31, **{"b": 22, "c": 41, "a": 51})
19 | func(a=11, b=21, **{"c": 31}, **{"c": 32})
20 | func(a=11, b=21, **{"c": 31, "c": 32})
| ^^^ PLE1132
|


2 changes: 2 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading