Skip to content

Improve in-place BinOp methods for sets #7161

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

Merged
merged 1 commit into from
Feb 8, 2022
Merged

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Feb 8, 2022

This PR proposes altering the annotations of four BinOp dunder methods for set classes, so that they return Self. The four methods in question are:

  • __ior__
  • __ixor__
  • __iand__
  • __isub__

I have several reasons for proposing this change:

  1. The existing annotations for __ior__ and __ixor__ are unsafe. If I have a set s = {1, 2} it has type set[int], but if I then do s |= {'foo', 'bar'}, the type of the variable has mutated from set[int] to set[str | int].
  2. Mypy will actually complain about point (1) if you try to do it, but it gives an obscure error: Result type of | incompatible in assignment. If type checkers are going to balk at this kind of thing anyway, then we may as well improve the error message.
  3. The proposed new annotations will be more accurate in subclasses of set: these methods do, in fact, return self at runtime.
  4. This brings the signatures of these dunder methods in line with their semantically equivalent methods (see table below).
  5. Consistency with the rest of typeshed: these methods are currently the only (or, if there are others, they're very rare) in-place BinOp methods that do not return Self.
  6. It just doesn't really make sense for any subclass of typing.AbstractSet or typing.MutableSet to return anything except Self from any of these methods, given that they update the object in-place.

For those who (like me) haven't got their set operations memorized, here's a rundown I made for myself...

Dunder method Symbol Equivalent method What it does
__ior__ |= .update() Add elements to the first set if they are in the second set but not in the first set
__ixor__ ^= .symmetric_difference_update() Add any elements to the first set if they appear in the second set but not in the first set; remove any elements from the first set if they also appear in the second set
__iand__ &= .intersection_update() Remove any elements from the first set if they do not appear in the second set
__isub__ -= .difference_update() Remove any elements from the first set if they appear in the second set.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

def __sub__(self: Self, other: Iterable[_T]) -> Self: ...
def difference_update(self, other: Iterable[_T]) -> None: ...
def __isub__(self: Self, other: Iterable[_T]) -> Self: ...
def __sub__(self: Self, other: Iterable[Any]) -> Self: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you change this (as well as __and__)? If nothing else, it seems a bit off-topic for this PR, but also inconsistent with the super class annotations.

Copy link
Member Author

@AlexWaygood AlexWaygood Feb 8, 2022

Choose a reason for hiding this comment

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

__sub__ and __and__ were changed in weakrefset because:

  • mypy complained about inconsistencies between __sub__ and __isub__ if I changed only __sub__ (same thing with __and__), so, although I agree that it's off-topic for this PR, I had to either change them in this PR or add an unnecessary # type: ignore comment.
  • These methods are inconsistent with __sub__ and __and__ in typing.AbstractSet, builtins.set and builtins.frozenset.

The signature of __sub__ in typing.AbstractSet is

def __sub__(self, s: AbstractSet[Any]) -> AbstractSet[_T_co]: ...

weakref.WeakSet accepts any iterable as the second argument, unlike AbstractSet, which only accepts AbstractSets. But the element-type (the only thing I'm changing here) should still be of type Any, as:

  • The return type will always be of type Self no matter what the element type in other is; but,
  • It will raise an exception if the element type in other is not hashable (currently inexpressable in the type system).

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.

2 participants