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

[flake8-pyi] Implement PYI062 (duplicate-literal-member) #11269

Merged
merged 12 commits into from May 7, 2024

Conversation

tusharsadhwani
Copy link
Contributor

@tusharsadhwani tusharsadhwani commented May 3, 2024

Summary

Implements Y062 from flake8-pyi, mostly adopted from Rule::DuplicateUnionMember (without the | BinOp parts).

Currently DuplicateUnionMember also doesn't have an autofix for Union[] cases, so implementing both together seems like a good idea.

Test Plan

cargo test / cargo insta review


x: Literal[True, False, True, False] # PYI062 twice here

y: Literal[1, print("hello"), 3, Literal[4, 1]] # PYI062 on the last 1
Copy link
Member

Choose a reason for hiding this comment

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

Can you add test cases where the inner literal is entirely redundant? e.g.

Literal[1, Literal[1]]
Literal[1, 2, Literal[1, 2]]
Literal[1, Literal[1], Literal[1]]
Literal[1, Literal[2], Literal[2]]
Literal[1, Literal[2, Literal[1]]]

Copy link
Member

Choose a reason for hiding this comment

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

These will be more important for when we add a fix, I think.

Copy link
Member

Choose a reason for hiding this comment

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

It's also good to have coverage for cases like Literal[1, 1, 1] where we should raise the diagnostic twice.

Copy link
Contributor

github-actions bot commented May 3, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+15 -0 violations, +0 -0 fixes in 2 projects; 42 projects unchanged)

bokeh/bokeh (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ src/bokeh/events.py:569:36: PYI062 Duplicate literal member `-1`

python/typeshed (+14 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select E,F,FA,I,PYI,RUF,UP,W

+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:10239: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:10244: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:10249: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:10254: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:3424: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:5833: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:5838: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:7915: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:7920: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:8172: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/compiler/xla/xla_pb2.pyi:1051:8177: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:496:918: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:496:923: PYI062 Duplicate literal member `...`
+ stubs/tensorflow/tensorflow/core/protobuf/rewriter_config_pb2.pyi:496:928: PYI062 Duplicate literal member `...`

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PYI062 15 15 0 0 0

@zanieb
Copy link
Member

zanieb commented May 3, 2024

Hm the typeshed ecosystem checks look concerning, do you know what's going on there?

@tusharsadhwani
Copy link
Contributor Author

@zanieb Doesn't seem to reproduce locally at least with copypasting the concerned files :/

@dhruvmanila
Copy link
Member

@tusharsadhwani can you try rebasing your PR on the latest main? Maybe it's out of sync.

@AlexWaygood
Copy link
Member

AlexWaygood commented May 6, 2024

@tusharsadhwani can you try rebasing your PR on the latest main? Maybe it's out of sync.

I think something might be up with the ecosystem checks generally when it comes to typeshed. I've had several odd results on some of my other PRs that have touched PYI checks, e.g. #11128. I want to try to look into this today.

@tusharsadhwani
Copy link
Contributor Author

Can confirm, merging main doesn't fix the issue.

@dhruvmanila
Copy link
Member

I think something might be up with the ecosystem checks generally when it comes to typeshed. I've had several odd results on some of my other PRs that have touched PYI checks, e.g. #11128. I want to try to look into this today.

Oh, I see. @AlexWaygood Can you open a new issue to keep track of it? No need to go into detail there but if you're unable to look into it we're still aware of it.

@AlexWaygood
Copy link
Member

I think something might be up with the ecosystem checks generally when it comes to typeshed. I've had several odd results on some of my other PRs that have touched PYI checks, e.g. #11128. I want to try to look into this today.

Oh, I see. @AlexWaygood Can you open a new issue to keep track of it? No need to go into detail there but if you're unable to look into it we're still aware of it.

Done:

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.

Nice!

tusharsadhwani and others added 2 commits May 7, 2024 21:17
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
ruff.schema.json Outdated Show resolved Hide resolved
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, thanks!

@AlexWaygood
Copy link
Member

I double-checked again, and I cannot reproduce the ecosystem results locally on my clone of typeshed. I checked out this PR branch locally in my ruff clone, and then ran this command from the directory of my typeshed clone, with the following results

% cargo run --manifest-path ../ruff/Cargo.toml -- check stubs stdlib --isolated --select=PYI062 --preview --no-cache
    Finished dev [unoptimized + debuginfo] target(s) in 0.10s
     Running `/Users/alexw/dev/ruff/target/debug/ruff check stubs stdlib --isolated --select=PYI062 --preview --no-cache`
All checks passed!

I also confirmed that the same command did highlight duplicate literal members if I purposefully introduced some to a random .pyi file in typeshed.

@AlexWaygood AlexWaygood changed the title [flake8-pyi] Implement PYI062 [flake8-pyi] Implement PYI062 (duplicate-literal-member) May 7, 2024
@AlexWaygood AlexWaygood merged commit 56b4c47 into astral-sh:main May 7, 2024
19 checks passed
@tusharsadhwani tusharsadhwani deleted the pyi-062 branch May 7, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants