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

Respect duplicates when rewriting type aliases #9905

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Conversation

charliermarsh
Copy link
Member

Summary

If a generic appears multiple times on the right-hand side, we should only include it once on the left-hand side when rewriting.

Closes #9904.

@charliermarsh charliermarsh requested a review from zanieb February 9, 2024 03:12
@charliermarsh charliermarsh added the bug Something isn't working label Feb 9, 2024
@charliermarsh charliermarsh marked this pull request as ready for review February 9, 2024 03:12
Copy link
Contributor

github-actions bot commented Feb 9, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -46,3 +46,7 @@ class Foo:
# OK
x: TypeAlias
x: int = 1

# UP040 won't raise if a generic is repeated.
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we are actually emitting a violation here, right? (And I think that's correct!)

Suggested change
# UP040 won't raise if a generic is repeated.
# Make sure "T" appears only once
# in the type parameters for the modernized type alias

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thank you!

let vars = vars
.into_iter()
.unique_by(|TypeVar { name, .. }| name.id.as_str())
.collect::<Vec<_>>();
Copy link
Member

Choose a reason for hiding this comment

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

Since Itertools is already in scope, you could avoid the turbofish here:

Suggested change
.collect::<Vec<_>>();
.collect_vec();

Copy link
Member Author

Choose a reason for hiding this comment

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

As a habit, I try to avoid using Itertools where it's not providing much value since it (1) makes it a little easier if we remove the dep in the future and (2) in my opinion at least it's actually easier to read the explicit version.

@charliermarsh charliermarsh enabled auto-merge (squash) February 9, 2024 13:57
@charliermarsh charliermarsh merged commit 52ebfc9 into main Feb 9, 2024
15 of 16 checks passed
@charliermarsh charliermarsh deleted the charlie/type branch February 9, 2024 14:02
nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
## Summary

If a generic appears multiple times on the right-hand side, we should
only include it once on the left-hand side when rewriting.

Closes astral-sh#9904.
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.

Rule UP040 incorrect fix when TypeVar is used multiple times
2 participants