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

Pyright doesn't respect the Python numbers hierarchy #1575

Closed
gjoseph92 opened this issue Mar 5, 2021 · 10 comments
Closed

Pyright doesn't respect the Python numbers hierarchy #1575

gjoseph92 opened this issue Mar 5, 2021 · 10 comments
Labels
as designed Not a bug, working as intended

Comments

@gjoseph92
Copy link

Describe the bug
When using abstract types from numbers, Pyright doesn't recognize either literals (1.1) or primitive types (float) as compatible with the abstract bases like Number, Integral, etc.

To Reproduce

import numbers

a: numbers.Number = 0 + 1j
#                   ~~~~~~
# Expression of type "complex" cannot be assigned to declared type "Number"
#  "complex" is incompatible with "Number"
print(isinstance(a, numbers.Number))
# True

b: numbers.Real = 1.1
#                 ~~~
# Expression of type "float" cannot be assigned to declared type "Real"
#  "float" is incompatible with "Real"
print(isinstance(b, numbers.Real))
# True

c: numbers.Integral = 3
#                     ~
# Expression of type "Literal[3]" cannot be assigned to declared type "Integral"
#  "Literal[3]" is incompatible with "Integral"
print(isinstance(c, numbers.Integral))
# True

Expected behavior
None of these statements would be type errors.

VS Code extension or command-line
VS Code extension, v1.1.117

Additional context
I'm not totally sure that this is pyright's fault—I don't really understand how abc and typing interact, and whether an abstract base class implicitly supports structural subtype-checking, or whether only explicit subclasses of Protocol support that. Maybe the solution here is actually some change to numbers in the standard library (or some additional stubs in typeshed).

Though this might seem extremely pedantic, using types from numbers can be handy when you don't know (and don't care) if you'll be getting a Python type, or a scalar from NumPy. Union[int, float] (correctly) rejects NumPy scalars. That said, of course the checker can't statically know if a NumPy scalar is Real vs Integral, so Number is probably the only practical type to use in that case.

@erictraut
Copy link
Collaborator

Pyright is doing the right thing here. The expression 1.1 is of type float, which is not assignable to type Real. There is no class relationship between float and Real.

I confirmed that mypy reports the same errors.

@erictraut erictraut added the as designed Not a bug, working as intended label Mar 5, 2021
@rsokl
Copy link

rsokl commented Aug 9, 2021

I was going to open an issue about how the following code seems to produce a false-positive from pyright

from typing import Union
from numbers import Real

def f(x: Union[list, float]):
    if not isinstance(x, Real):
        y = len(x) # <- list | float cannot be assigned Sized   

But @erictraut you state that

There is no class relationship between float and Real

However float is a subclass of Real:

>>> from numbers import Real
>>> issubclass(float, Real)
True

>>> isinstance(float(2), Real)
True

is there something that I am missing here?

@rsokl
Copy link

rsokl commented Aug 9, 2021

I found a relevant discussion on mypy, spurred by Guido. It looks like they created this open-since-2016 issue on python.typing.

And it basically seems like "the numbers module was a mistake" is Guido's take:

There are very few reasons to implement your own numeric types from scratch, since the built-in ones are really pretty good; the only popular alternative numeric type that I know of, Decimal, explicitly doesn't follow the numeric ABCs (there's a note in numbers.py explaining why).
I suppose numpy also counts as popular alternate number implementation, and they do support PEP 3141. But I don't know how much people use that fact. Certainly numpy's own code has very few mentions of the numbers module, and I could not find any mention of it in the numpy docs.

which makes me sad, because here I am needing to accommodate users who pass my function numpy-floats.

Anyway, I figured I'd share some of my additional findings. Feel free to ignore all of this.

@leycec
Copy link

leycec commented Nov 1, 2021

Guido reversed his position on the numeric tower in 2020:

So I stick to my observation that the numeric tower is still the best way to check for a specific type of number, since all builtin types are registered as member of the appropriate level in the tower.

PEP 3141 is an accepted standard. NumPy, SymPy, and effectively all numeric frameworks comply with that standard. Ideally, pyright should also comply with that standard. Mypy not complying with that standard should not be seen as evidence contravening that standard. Mypy's failure is a mypy-specific open issue that everyone would like to see resolved.

It'd be kinda nice for the data science community if this could be reopened at some point. </shrug>

@erictraut
Copy link
Collaborator

Pyright follows the type rules defined in PEP 484 (and follow-on typing PEPs) and the type information provided in typeshed stdlib stubs. If you think that the type information in those type stubs is incorrect, please file bugs or PRs in typeshed.

If you think that changes are required to PEP 484 or other typing standards to accommodate your use case, then I recommend opening a discussion thread in the python/typing or posting to the typing-sig mailing list.

@leycec
Copy link

leycec commented Nov 1, 2021

Hmm. But PEP 3141 is an accepted standard. PEP 484 neither contravenes nor deprecates PEP 3141. Ideally, pyright and other static type checkers should comply with both – not merely PEP 484. After all, they're both active standards.

That said, I concede this might very well exceed pyright's mandate. In theory, it should be trivial to handle the numeric tower for at least the core builtin numeric types (e.g., int, float, complex). In practice, you know your job better than me.

As always, thanks for all static type-checking! pyright continues to rock the type-hinting scene. 🎸

@erictraut
Copy link
Collaborator

PEP 3141 describes a number of abstract classes, and the type definitions of these classes are reflected in numbers.pyi in typeshed. Pyright honors and enforces the type information in this stub.

A type checker should not ignore type information in stdlib stubs and substitute its own understanding of how types work. There are a few exceptions where PEP 484 specifically calls out behaviors that cannot be specified in stubs and must be hard-coded in a type checker. The implicit relationship between complex, float and int is one such example. No such carve-out is made for Number, Complex, etc., which means that type checkers must follow the type rules as specified in the stubs.

If you think that the type information in numbers.pyi is incorrect, then filing bugs and PRs in typeshed is the best approach.

If you think that the types described in PEP 3141 cannot be adequately described using the current typing standards, then we either need to augment the typing standards or carve out some special-case (hard-coded) behaviors that all standards-compliant type checkers must implement. Both of those would require discussions within the typing community.

@ktbarrett
Copy link

This functionality does not require hard-coding information, so it does not require a PEP. It requires pyright understand ABC.register calls and typeshed to add those calls to numbers.pyi. The reason this information is not included in typeshed is likely because no type checker supports ABC.register. I would have not closed this issue as it is a limitation of pyright (and typeshed), not of anything upstream.

@erictraut
Copy link
Collaborator

ABC.register is a dynamic registration call whose functionality depends on call ordering. It's not something that a static type checker can support. So if your proposed solution depends on type checkers supporting ABC.register, then that solution won't work.

@ktbarrett
Copy link

See python/mypy#2922 for discussion on this in mypy. I'm not sure there is a consensus that it shouldn't be supported, only that it would be difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended
Projects
None yet
Development

No branches or pull requests

5 participants