PIE804: Prevent keyword arguments duplication
#8450
Conversation
crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE804.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hm... I'm not sure just changing this fix to unsafe is the best approach. The unsafe fix will will still always result in a syntax error when applied.
This fix just turns a runtime error into a syntax error though:
def foo(*args, **kwargs):
pass
foo(**{'a': 1}, **{'a': 2})❯ python example.py
Traceback (most recent call last):
File "/Users/mz/eng/src/astral-sh/puffin/example.py", line 4, in <module>
foo(**{'a': 1}, **{'a': 2})
TypeError: __main__.foo() got multiple values for keyword argument 'a'
Since the keys are static, can't we just add a guard here to determine if a key already exists?
T-256
changed the title
PIE804: Convert to unsafe autofixesPIE804: Prevent keyword arguments duplication
crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_dict_kwargs.rs
Outdated
Show resolved
Hide resolved
|
While this doesn't introduce a syntax error, there's still an error that I don't think any lint rule addresses yet. Maybe we should also consider a rule that catches duplicate keyword arguments? |
crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE804.py
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_dict_kwargs.rs
Outdated
Show resolved
Hide resolved
|
Sorry for all violations, I'm not expert in Rust, they are fixing. |
Perhaps I could work on it, which namespace should put this rule? |
| # Duplicated key names wont be fixed to avoid syntax error. | ||
| abc(**{'a': b}, **{'a': c}) # PIE804 |
There was a problem hiding this comment.
How would this work with positional arguments? For example,
def abc(a):
pass
abc(1, **{'a': 2})Correct me if I'm wrong but I guess it would fix it to abc(1, a=2) which isn't a syntax error although it's a runtime error.
I don't think this is in scope for this rule so maybe we could just highlight it in the docs. I'll leave this up to @zanieb as the main reviewer :)
There was a problem hiding this comment.
Yes, needs info in docs, with considering bellow code as correct:
def abc(a, /, **kw): ...
abc(1, **{'a': 2})There was a problem hiding this comment.
Yeah since we're just transforming invalid code at runtime to more invalid code that seems fine. We could probably include this in a new rule too 😬
The first step is probably a new issue for discussion. We could probably encompass the positional case in the same issue. |
|
Fix safety added to docs. @zanieb Can you open issue for the new rule? Since I don't know what's correct behavior for new rule on: If function definition uses |
.../src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE804_PIE804.py.snap
Outdated
Show resolved
Hide resolved
charliermarsh
force-pushed
the
pie804-unsafe
branch
from
dcb9c10 to
3f2fd98
Compare
Summary
According to the #8402 (comment)
Test Plan
Added new unsafe test