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

New check: duplicate types in type hint, or duplicate literals in set #371

Open
jakkdl opened this issue Mar 21, 2023 · 5 comments
Open

Comments

@jakkdl
Copy link
Contributor

jakkdl commented Mar 21, 2023

a: int | int | float = ...
b: Union[List[int, int]] = ...
c: Optional[int] | int | None = ... # can probably rely on other linters to not have to deal with this one
d: set[int] = {1, 2, 3, 3, 5}

e: Tuple[int, int] = ... # not an error

these should pretty much always be typos and/or bad copy-pastes, with very few to no false alarms. Don't think it's super common, but definitely not rare or super esoteric.

The check shouldn't have to be too complicated to implement, for the type hint probably need a whitelist of what types you look in (List/list, Set/set, Dict/dict (with special logic), OrderedDict, Optional, Union, Literal...).

For sets you only need to look inside set() (ast.Call) and inside {} (ast.Set), and probably only include literal int/float/strings.

@cooperlees
Copy link
Collaborator

This seems super uncontroversial if we can AST parse this out nicely covering the edge cases you've outlined and we can improve on reports ... I'd be happy to add this.

We can run this over some large type checked repos before releasing too to be extra safe.

@AlexWaygood
Copy link
Contributor

AlexWaygood commented Mar 22, 2023

FYI we already have a check for duplicate items in a union over at flake8-pyi (error code Y016). But we only support .pyi files currently. Feel free to take inspiration from our implementation :)

Our tests for Y016 are here, along with our tests for our various other union-related lints: https://github.com/PyCQA/flake8-pyi/blob/main/tests/union_duplicates.pyi

@FozzieHi
Copy link
Contributor

I think these should be two different checks, but I'm happy to work on a duplicate items in a set check.

For sets you only need to look inside set() (ast.Call) and inside {} (ast.Set), and probably only include literal int/float/strings.

What do you mean by checking ast.Call? Do we also want to catch these?
set((1, 2, 3, 3, 5))
set([1, 2, 3, 3, 5])

I'm not entirely sure how common these would be, and it would increase the complexity of the check, but I could be missing another syntax here!

@jakkdl
Copy link
Contributor Author

jakkdl commented Mar 22, 2023

I think these should be two different checks, but I'm happy to work on a duplicate items in a set check.

For sets you only need to look inside set() (ast.Call) and inside {} (ast.Set), and probably only include literal int/float/strings.

What do you mean by checking ast.Call? Do we also want to catch these?
set((1, 2, 3, 3, 5))
set([1, 2, 3, 3, 5])

I'm not entirely sure how common these would be, and it would increase the complexity of the check, but I could be missing another syntax here!

oops, I misremembered and thought one could write set(1, 2, 3) when writing the OP - but that's very much not valid code - so it should only be for ast.Set.

@JelleZijlstra
Copy link
Collaborator

I'm supportive of doing this for sets and also for dict keys, where it's likely to mean the code silently does something different from what the user wants.

Less sure about the other examples brought up here. List[int, int] is invalid to a type checker, and if you're not running a type checker annotations are unlikely to be correct anyway, so we wouldn't add much value. Duplicate values in unions (int | int | float, Optional[int] | int | None) aren't that bad; I think that's better left to type checkers too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants