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

Check for redundant object unions #283

Closed
Avasam opened this issue Sep 15, 2022 · 12 comments
Closed

Check for redundant object unions #283

Avasam opened this issue Sep 15, 2022 · 12 comments

Comments

@Avasam
Copy link
Contributor

Avasam commented Sep 15, 2022

I'd like a check similar to Y041, but for unions with object. Especially for object | None, coming from a Typescript background, it is not obvious to me that this type union is redundant, and I often make that mistake (or forget about it and leave it in a stub).

@AlexWaygood
Copy link
Collaborator

Hmm, I'm not sure.

Y041 and Y051 both essentially deal with special cases; since we're not a type checker, there's no easy, generalised way for us to answer the question "Is X a subtype of Y?". So the question is -- is this problem common enough to be worth adding another special case that we check for here? And I don't feel like I've seen this problem come up much in typeshed.

@twoertwein, have you seen this problem come up much in pandas-stubs at all?

@twoertwein
Copy link
Contributor

twoertwein commented Sep 15, 2022

@twoertwein, have you seen this problem come up much in pandas-stubs at all?

Not with object but often with Hashable. Could maybe have a rule with all built-in hashable objects (slice, tuple, ...) that occur in a Union with Hashable?

edit: and something like str | Sequence[str] string is a Sequence of strings :)

@AlexWaygood
Copy link
Collaborator

Could maybe have a rule with all built-in hashable objects (slice, tuple, ...) that occur in a Union with Hashable?

Hashable is kind of problematic as a type :\\\

See, e.g., the comment in typeshed. Personally, I'd be quite hesitant to ever use Hashable as an annotation, so I'm not sure I'd want to write a lint rule encouraging it. Mypy has a pretty poor understanding of hashability currently (e.g. it hilariously thinks all unhashable classes break LSP).

And to be fair to mypy, hashability does break all kinds of OOP norms in Python. Consider the following code. Is Y a subtype of Hashable or not?

from typing import Hashable

class X(Hashable):
    def __hash__(self) -> int:
        return 42

class Y(X):
    __hash__ = None

@AlexWaygood
Copy link
Collaborator

something like str | Sequence[str] string is a Sequence of strings :)

Strictly speaking that's redundant, but you might often want that for documentation purposes :)

@twoertwein
Copy link
Contributor

Hashable is kind of problematic as a type :\\

good to know - pandas(-stubs) use it quite extensively. I assume the same applies to object: it is also Hashable - a list "obviously" cannot possibly inherit from object because it is not Hashable :)

@AlexWaygood
Copy link
Collaborator

I assume the same applies to object: it is also Hashable - a list "obviously" cannot possibly inherit from object because it is not Hashable :)

Yup! It's type: ignored in typeshed to make mypy happy, just like all the other unhashable classes... https://github.com/python/typeshed/blob/c9346f32e10fc631e691745e2a8c24bc3216642f/stdlib/builtins.pyi#L991

@AlexWaygood
Copy link
Collaborator

AlexWaygood commented Sep 15, 2022

There was an attempt a while back to make the dict KeyType TypeVar bound to Hashable, but it would have broken half the typed-Python ecosystem: python/typeshed#6244

@AlexWaygood
Copy link
Collaborator

I'm closing this issue for now, but I'm happy to reopen if I see more evidence that this is a widespread issue :)

@AlexWaygood AlexWaygood closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2022
@AlexWaygood
Copy link
Collaborator

This recently came up again in python/typeshed#8801 (comment)

@AlexWaygood
Copy link
Collaborator

@JelleZijlstra
Copy link
Collaborator

Though that's because of a mypy bug (python/mypy#13760).

@Avasam
Copy link
Contributor Author

Avasam commented Oct 16, 2023

Relevant pyright discussion: microsoft/pyright#3949

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

No branches or pull requests

4 participants