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

Experimenting with inheritance from numbers ABCs #12894

Closed
wants to merge 6 commits into from

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Oct 24, 2024

Speaking of ABC registrations within the standard library, here's one that does happen. Unless I missed something in my search, it's one of only two in cpython to register to an ABC from within C (the other is array.array getting registered to MutableSequence).

This comment has been minimized.

@tungol tungol marked this pull request as draft October 24, 2024 07:44
@tungol
Copy link
Contributor Author

tungol commented Oct 24, 2024

I knew that the numbers module was problematic, but I thought maybe this one would be okay. A single hit isn't the worst, but it's probably bad enough in this case? The potential upside is pretty small and the error is a false positive since Number isn't a real base of Decimal at runtime, just kinda-sorta a base maybe.

While I'm here and for the record, the only other ABC registrations that occur in cpython but aren't currently represented in typeshed are all for classes from numbers:

Complex.register(complex)
Real.register(float)
Integral.register(int)

I believe that trying to get those into typeshed breaks approximately the entire world, if I understand the situation correctly.

@AlexWaygood
Copy link
Member

I believe that trying to get those into typeshed breaks approximately the entire world

The last time I tried it locally I couldn't even get to the "let's file a PR and see what mypy_primer says" stage, since mypy just crashed on me :-)

@tungol
Copy link
Contributor Author

tungol commented Oct 24, 2024

Look like it's only int that causes the crash. We should be able to see float and complex...

This comment has been minimized.

@tungol tungol changed the title decimal.Decimal is registered to numbers.Number Experimenting with inheritance from numbers ABCs Oct 24, 2024
@srittau
Copy link
Collaborator

srittau commented Oct 24, 2024

I'm not a fan of adding "registered" base classes to the base class list anyway. This is one of those cases, where we hack around limitations of the type system, fixing one problem (isinstance checks), but potentially causing other problems.

@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 24, 2024

The primer output (and CI failures) also indicate another reasons why this change would be problematic: float and complex don't actually satisfy (at least, not on all Python versions) the abstract ABCs that are meant to describe float-like and complex-like classes (Real and Complex respectively).

The fundamental reason why these ABCs are problematic is that they just don't make sense when it comes to static typing. It's impossible to fill in the missing annotations in these methods, because there's no way of doing so that would make them both suitable for flexible subclassing and would also accurately describe the stdlib runtime classes that are registered to them. Ultimately their use should just be discouraged for static typing purposes, so I think this PR goes in the wrong direction. (Really I think they need some kind of soft-deprecation notice in the docs, but that's a conversation we'd have to have over at CPython.)

@tungol
Copy link
Contributor Author

tungol commented Oct 24, 2024

this PR goes in the wrong direction

Yeah. To be clear there's nothing here that I'm advocating for at this point, but I'm still curious to see what it looks like if we tried. If it could be added to typeshed without causing problems I'd argue that we should add them even if actually using them in typing is a bad idea, but they can't be added without causing problems so it's a moot question.

This comment has been minimized.

@tungol
Copy link
Contributor Author

tungol commented Oct 24, 2024

My notes on what gets surfaced:

+ tornado/ioloop.py:837: error: "IOLoop" has no attribute "_timeout_counter"  [attr-defined]
+ xarray/core/variable.py: note: In member "_pad_options_dim_to_index" of class "Variable":
+ xarray/core/variable.py:1145: error: Unsupported target for indexed assignment ("Mapping[Any, int | float | tuple[int, int] | tuple[float, float]]")  [index]

These are both new errors only because mypy previously considered their branch unreachable.

Similarly, all the Unused "type: ignore" comment messages from core are because they're using both comparisons
to Number and also --warn-unreachable.


+ tanjun/schedules.py:570: error: Subclass of "Sequence[int]" and "float" cannot exist: would have incompatible method signatures  [unreachable]

This is coming from code like:

values: int | list[int] | None
if isinstance(values, int):
    return

if isinstance(values, float):
    raise TypeError

My guess is that there's some special casing of int and float that normally allows this but is broken by the change.
If that's the case, then this would be fixable from mypy's side, but I have no idea the effort that would
be required.


error: Definition of "__neg__" in base class "Complex" is incompatible with definition in base class "_RealLike"  [misc]
error: Definition of "__pos__" in base class "Complex" is incompatible with definition in base class "_RealLike"  [misc]

These show up 6 times, making it the most common issue. In the one I checked, it showed for something like

class foo: pass
class bar(float, foo): pass

Doesn't trigger for just class bar(float): pass.

It's bad, certainly, and fixing it in typeshed would probably mean making the stubs worse (as Alex's comment describes), but it doesn't seem entirely insurmountable.
It's not immediately obvious to me why Self couldn't be used though...


error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

This one is reasonably unpleasant. It's a false positive that's
purely a result of the way that class float(numbers.Real): is a lie and
would require an obnoxious work-around on the side of typeshed's consumers that might not be possible
in all cases.

The only part I don't quite understand yet is that for Decimal in this MR it shows up for code like

class FooMeta(type): ...
class Foo(Decimal, metaclass=FooMeta): ...

but for float that doesn't trigger it and it only shows up for code like

class FooMeta(type): ...
class Bar(metaclass=FooMeta): ...
class Foo(float, Bar): ...

All that said however, the same false positive already exists for classes registered to collections.abc classes:

class FooMeta(type): ...
class Foo(list, metaclass=FooMeta): ...  # error in mypy but not at runtime...

This comment has been minimized.

@tungol
Copy link
Contributor Author

tungol commented Oct 24, 2024

Looks like Self doesn't turn up any problems?

Is there a reason overrides were used instead of Self in #11375 ? Wouldn't work for __abs__ between _ComplexLike and _IntegralLike but it seems fine for __neg__ and __pos__

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

tornado (https://github.com/tornadoweb/tornado)
+ tornado/ioloop.py:837: error: "IOLoop" has no attribute "_timeout_counter"  [attr-defined]

ibis (https://github.com/ibis-project/ibis)
+ ibis/common/tests/test_patterns.py:1141: error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases  [misc]
+ ibis/common/tests/test_grounds.py:191: error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases  [misc]
+ ibis/common/tests/test_grounds.py:209: error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases  [misc]

xarray (https://github.com/pydata/xarray)
+ xarray/core/variable.py: note: In member "_pad_options_dim_to_index" of class "Variable":
+ xarray/core/variable.py:1145: error: Unsupported target for indexed assignment ("Mapping[Any, int | float | tuple[int, int] | tuple[float, float]]")  [index]

yarl (https://github.com/aio-libs/yarl)
+ tests/test_update_query.py:266:5: error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases  [misc]

pydantic (https://github.com/pydantic/pydantic)
+ pydantic/v1/types.py:667: error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases  [misc]

core (https://github.com/home-assistant/core)
+ homeassistant/util/unit_system.py:138: error: Unused "type: ignore" comment  [unused-ignore]
+ homeassistant/util/unit_system.py:148: error: Unused "type: ignore" comment  [unused-ignore]
+ homeassistant/util/unit_system.py:158: error: Unused "type: ignore" comment  [unused-ignore]
+ homeassistant/util/unit_system.py:168: error: Unused "type: ignore" comment  [unused-ignore]
+ homeassistant/util/unit_system.py:178: error: Unused "type: ignore" comment  [unused-ignore]
+ homeassistant/helpers/config_validation.py:259: error: Unused "type: ignore" comment  [unused-ignore]

@tungol
Copy link
Contributor Author

tungol commented Oct 25, 2024

I spent a little time with the mypy crash. It happens on top-level annotations of FOO: Literal[2] because builtins.int isn't fully resolved yet and there's no accommodation for that possibility. I tried letting that section of the type analyzer defer, and after that I could get mypy to run successfully on a fully numbers-ized version of typeshed.

  • The __abs__ inconsistency was still a problem
  • __round__ also broke things because int.__round__ doesn't accept None as an argument. Edit: Hah, this one got fixed already int.__round__(None) is not supported cpython#120080
  • I needed to make enum.EnumMeta a subclass of ABCMeta otherwise IntEnum complained about a metaclass conflict, and that's... pretty bad.

Fixing all those got me to a successful stubtest run though! Probably time to leave well enough alone after that, but it would be fun to see the primer output at that point.

@tungol
Copy link
Contributor Author

tungol commented Oct 25, 2024

python/mypy#18046 to fix the mypy crash, if that's a thing mypy wants to do

@tungol
Copy link
Contributor Author

tungol commented Nov 6, 2024

Nothing else to do here until/unless mypy accepts python/mypy#18046 and we can see what happens for full numberization.

@tungol tungol closed this Nov 6, 2024
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.

3 participants