Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

collections: Add missing reflected BinOp methods #7207

Merged
merged 4 commits into from
Feb 14, 2022

Conversation

AlexWaygood
Copy link
Member

Testing confirms that mypy currently raises false positives for these operations, whereas they all succeed at runtime.

@github-actions

This comment has been minimized.

def __mod__(self: Self, args: Any) -> Self: ...
def __rmod__(self: Self, template: object) -> Self: ...
Copy link
Member

Choose a reason for hiding this comment

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

This one is actually broken:

In [12]: [] % collections.UserString("x")

Traceback (most recent call last):
  File "/main_instance_shell/jelle/venv/lib/python3.6/site-packages/IPython/core/interactiveshell.py", line 3343, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-12-0ca5ac41c5a2>", line 1, in <module>
    [] % collections.UserString("x")
  File "/main_instance_shell/jelle/venv/lib/python3.6/collections/__init__.py", line 1162, in __rmod__
    return self.__class__(format % args)
NameError: name 'args' is not defined

It should error but not with a NameError.

Copy link
Member Author

@AlexWaygood AlexWaygood Feb 14, 2022

Choose a reason for hiding this comment

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

Re the NameError, do you wanna file the BPO issue or shall I? :)

Copy link
Member

Choose a reason for hiding this comment

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

Go ahead! I think this method just shouldn't exist. It's presumably there to support %-formatting, but I don't think a reflected method makes sense for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, I can't reproduce the NameError. This is what I get on my machine:

>>> from collections import UserString
>>> [] % UserString('foo')
'[]'
>>> [] % UserString('x')
'[]'
>>>

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it got fixed in https://bugs.python.org/issue25652. That's what I get for trying things out on 3.6. I admit I don't understand the reasoning they ended up with for keeping the method.

Copy link
Member Author

@AlexWaygood AlexWaygood Feb 14, 2022

Choose a reason for hiding this comment

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

collections.UserString does feel like it was written during a phase when Raymond Hettinger was incredibly hyped about Javascript or something:

>>> 5 + UserString('foo')
'5foo'
>>> NotImplemented + UserString('foo')
'NotImplementedfoo'

Like... wut...

Copy link
Member

Choose a reason for hiding this comment

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

In [17]: collections.UserString("x") + collections.UserList(["y"])
Out[17]: "x['y']"

In [18]: collections.UserList(["y"]) + collections.UserString("x")
Out[18]: ['y', 'x']

Copy link
Member Author

Choose a reason for hiding this comment

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

Truly a nightmare of what Python could have been

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

The mypy-primer errors are here: https://github.com/sphinx-doc/sphinx/blob/4.x/sphinx/locale/__init__.py#L67. They're already type-ignoring similar issues with __add__, so it seems fine to produce the same error on __radd__.

@github-actions
Copy link
Contributor

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

sphinx (https://github.com/sphinx-doc/sphinx)
+ sphinx/locale/__init__.py: note: In member "__radd__" of class "_TranslationProxy":
+ sphinx/locale/__init__.py:70:5: error: Argument 1 of "__radd__" is incompatible with supertype "UserString"; supertype defines the argument type as "object"
+ sphinx/locale/__init__.py:70:5: note: This violates the Liskov substitution principle
+ sphinx/locale/__init__.py:70:5: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
+ sphinx/locale/__init__.py:70:5: error: Return type "str" of "__radd__" incompatible with return type "_TranslationProxy" in supertype "UserString"
+ sphinx/locale/__init__.py: note: In member "__rmod__" of class "_TranslationProxy":
+ sphinx/locale/__init__.py:76:5: error: Return type "str" of "__rmod__" incompatible with return type "_TranslationProxy" in supertype "UserString"
+ sphinx/locale/__init__.py: note: In member "__rmul__" of class "_TranslationProxy":
+ sphinx/locale/__init__.py:82:5: error: Return type "str" of "__rmul__" incompatible with return type "_TranslationProxy" in supertype "UserString"

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants