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

PIE804: Prevent keyword arguments duplication #8450

Merged
merged 9 commits into from
Dec 12, 2023

Conversation

T-256
Copy link
Contributor

@T-256 T-256 commented Nov 3, 2023

Summary

According to the #8402 (comment)

Test Plan

Added new unsafe test

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

github-actions bot commented Nov 3, 2023

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+24 -24 violations, +0 -0 fixes in 41 projects)

apache/airflow (+24 -24 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-preview --select ALL

- tests/providers/amazon/aws/operators/test_eks.py:728:30: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/operators/test_eks.py:728:45: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:102:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:103:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:132:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:133:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:181:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:182:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:219:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:220:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:268:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:269:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:61:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:62:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/cncf/kubernetes/utils/test_pod_manager.py:826:30: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/cncf/kubernetes/utils/test_pod_manager.py:826:46: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/cloud/hooks/test_life_sciences.py:125:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/cloud/hooks/test_life_sciences.py:126:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/cloud/hooks/test_life_sciences.py:154:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/cloud/hooks/test_life_sciences.py:154:34: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/cloud/hooks/test_life_sciences.py:231:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/cloud/hooks/test_life_sciences.py:231:34: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/cloud/hooks/test_life_sciences.py:259:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/cloud/hooks/test_life_sciences.py:259:34: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/cloud/operators/test_kubernetes_engine.py:406:30: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/cloud/operators/test_kubernetes_engine.py:406:45: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/firebase/hooks/test_firestore.py:100:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/firebase/hooks/test_firestore.py:101:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/firebase/hooks/test_firestore.py:122:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/firebase/hooks/test_firestore.py:122:34: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/firebase/hooks/test_firestore.py:189:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/firebase/hooks/test_firestore.py:190:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/firebase/hooks/test_firestore.py:216:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/firebase/hooks/test_firestore.py:216:34: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/telegram/hooks/test_telegram.py:109:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/telegram/hooks/test_telegram.py:110:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/telegram/hooks/test_telegram.py:128:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/telegram/hooks/test_telegram.py:129:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/telegram/hooks/test_telegram.py:147:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/telegram/hooks/test_telegram.py:148:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/telegram/hooks/test_telegram.py:174:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/telegram/hooks/test_telegram.py:175:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/telegram/hooks/test_telegram.py:194:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/telegram/hooks/test_telegram.py:195:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/weaviate/operators/test_weaviate.py:49:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/weaviate/operators/test_weaviate.py:50:71: PIE804 [*] Unnecessary `dict` kwargs
- tests/utils/test_compression.py:76:13: PIE804 [*] Unnecessary `dict` kwargs
+ tests/utils/test_compression.py:77:17: PIE804 [*] Unnecessary `dict` kwargs

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PIE804 48 24 24 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+24 -24 violations, +0 -0 fixes in 41 projects)

apache/airflow (+24 -24 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

- tests/providers/amazon/aws/operators/test_eks.py:728:30: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/operators/test_eks.py:728:45: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:102:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:103:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:132:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:133:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:181:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:182:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:219:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:220:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:268:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:269:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/amazon/aws/utils/test_waiter_with_logging.py:61:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/amazon/aws/utils/test_waiter_with_logging.py:62:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/cncf/kubernetes/utils/test_pod_manager.py:826:30: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/cncf/kubernetes/utils/test_pod_manager.py:826:46: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/cloud/hooks/test_life_sciences.py:125:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/cloud/hooks/test_life_sciences.py:126:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/cloud/hooks/test_life_sciences.py:154:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/cloud/hooks/test_life_sciences.py:154:34: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/cloud/hooks/test_life_sciences.py:231:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/cloud/hooks/test_life_sciences.py:231:34: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/cloud/hooks/test_life_sciences.py:259:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/cloud/hooks/test_life_sciences.py:259:34: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/cloud/operators/test_kubernetes_engine.py:406:30: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/cloud/operators/test_kubernetes_engine.py:406:45: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/firebase/hooks/test_firestore.py:100:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/firebase/hooks/test_firestore.py:101:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/firebase/hooks/test_firestore.py:122:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/firebase/hooks/test_firestore.py:122:34: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/firebase/hooks/test_firestore.py:189:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/firebase/hooks/test_firestore.py:190:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/google/firebase/hooks/test_firestore.py:216:24: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/google/firebase/hooks/test_firestore.py:216:34: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/telegram/hooks/test_telegram.py:109:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/telegram/hooks/test_telegram.py:110:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/telegram/hooks/test_telegram.py:128:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/telegram/hooks/test_telegram.py:129:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/telegram/hooks/test_telegram.py:147:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/telegram/hooks/test_telegram.py:148:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/telegram/hooks/test_telegram.py:174:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/telegram/hooks/test_telegram.py:175:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/telegram/hooks/test_telegram.py:194:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/telegram/hooks/test_telegram.py:195:13: PIE804 [*] Unnecessary `dict` kwargs
- tests/providers/weaviate/operators/test_weaviate.py:49:9: PIE804 [*] Unnecessary `dict` kwargs
+ tests/providers/weaviate/operators/test_weaviate.py:50:71: PIE804 [*] Unnecessary `dict` kwargs
- tests/utils/test_compression.py:76:13: PIE804 [*] Unnecessary `dict` kwargs
+ tests/utils/test_compression.py:77:17: PIE804 [*] Unnecessary `dict` kwargs

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PIE804 48 24 24 0 0

@T-256 T-256 marked this pull request as draft November 3, 2023 02:17
@T-256 T-256 changed the title PIE804: Convert to unsafe autofixes PIE804: Prevent keyword arguments duplication Nov 3, 2023
@T-256 T-256 marked this pull request as ready for review November 3, 2023 04:19
@zanieb
Copy link
Member

zanieb commented Nov 3, 2023

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?

@T-256
Copy link
Contributor Author

T-256 commented Nov 3, 2023

Sorry for all violations, I'm not expert in Rust, they are fixing.

@T-256
Copy link
Contributor Author

T-256 commented Nov 3, 2023

Maybe we should also consider a rule that catches duplicate keyword arguments?

Perhaps I could work on it, which namespace should put this rule?

Comment on lines 24 to 25
# Duplicated key names wont be fixed to avoid syntax error.
abc(**{'a': b}, **{'a': c}) # PIE804
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, needs info in docs, with considering bellow code as correct:

def abc(a, /, **kw): ...

abc(1, **{'a': 2})

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😬

@zanieb
Copy link
Member

zanieb commented Nov 6, 2023

Maybe we should also consider a rule that catches duplicate keyword arguments?

Perhaps I could work on it, which namespace should put this rule?

The first step is probably a new issue for discussion. We could probably encompass the positional case in the same issue.

@T-256
Copy link
Contributor Author

T-256 commented Nov 6, 2023

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:

abc(a=1, **{'a': 2})

If function definition uses / for kwargs then above function calling is correct. It's not easy get access to definition for checking /.

@zanieb zanieb self-assigned this Nov 6, 2023
@T-256 T-256 requested a review from zanieb November 10, 2023 12:34
@charliermarsh
Copy link
Member

@zanieb - Relatedly, there's now a PR for duplicate keyword arguments: #8706

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified to match Zanie's feedback, but otherwise LGTM.

@charliermarsh charliermarsh added the bug Something isn't working label Dec 12, 2023
@charliermarsh charliermarsh enabled auto-merge (squash) December 12, 2023 23:16
@charliermarsh charliermarsh merged commit cb201bc into astral-sh:main Dec 12, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants