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

Set dunder operators too permissive? #3184

Open
kaste opened this issue Aug 13, 2019 · 4 comments
Open

Set dunder operators too permissive? #3184

kaste opened this issue Aug 13, 2019 · 4 comments
Labels
stubs: improvement Improve/refactor existing annotations, other stubs issues

Comments

@kaste
Copy link
Contributor

kaste commented Aug 13, 2019

Similar to #1840

With #3181 I made KeysView consistent with Set but I really think the operators are confusingly permissive.

Let's look at them

# difference
{'1'} - {2}
reveal_type({'1'} - {2})  # => Set[str]

This should fail because this is always a null operation and thus
a programmer error.

# intersection
{'1'} & {2}  # => Set[str]
reveal_type({'1'} & {2})  # => Set[str]

This really should fail because it is unlikely meaningful; the
return type is always an empty set thus it is also not a Set[str]

# sym difference
{'1'} ^ {2}
reveal_type({'1'} ^ {2})  # => Set[Union[str, int]]

Although the assumed union type is correct, Set[A] & Set[B] => Set[A|B] just doesn't make sense. The operator can only meaningful be applied to same type sets. What we do here is just a form of plain concat or union.

# union
{'1'} | {2} # MAYBE OK
reveal_type({'1'} | {2})  # => Set[Union[str, int]]

If we look at Sets as just containers without some
properties, we could accept it to construct mixed
type Sets using 'union'. I think it is okay for Python,
the name union makes it meaningful, but I maybe never did
that in practice.
Since it adds to the type it is also likely that
the following part of the program catches mistakes.

(But lists don't allow that [1] + ['2'] although they're really
arbitrary containers. Except my PR #3183.)

@srittau srittau added the stubs: improvement Improve/refactor existing annotations, other stubs issues label Sep 3, 2019
@srittau
Copy link
Collaborator

srittau commented Sep 3, 2019

Could you prepare an experimental PR (maybe after #3181 is merged), so interested parties can try this against real world code bases? This way we could better evaluate the impact this change would have (good or bad).

@kaste
Copy link
Contributor Author

kaste commented Sep 5, 2019

We have set, frozenset, AbstractSet, MutableSet, and ItemsView and KeysView. Should a PR make all of them consistent?

@srittau
Copy link
Collaborator

srittau commented Sep 5, 2019

That would make sense to me.

@kaste
Copy link
Contributor Author

kaste commented Sep 6, 2019

I think I need help here. Current situation in master is. Given:

ints: Set[int]; floats: Set[float]; strs: Set[str]

union is defined "restrictive" T -> T -> T and __or__ is "additive permissive" T -> S -> Union[T, S]

strs.union(ints)   # FAILS
ints.union(floats) # FAILS
floats.union(ints) # PASSES
strs | ints        # PASSES
ints | floats      # PASSES
floats | ints      # PASSES

In contrast intersection and __and__ are permissive T -> Any -> T.

# ALL PASS
strs.intersection(ints)
ints.intersection(floats)
floats.intersection(ints)
strs & ints
ints & floats
floats & ints

These are the two type variants we see for Set. Do we want consistency between the methods and the dunder constructors?

If I make intersection "restrictive" T -> T -> T mypy will reject strs & ints but we break the law ints & floats == floats & ints because mypy surprisingly accepts the latter. It should reject both. On the other side the union method already behaves like that.

We could also go pragmatic: union and symmetric_difference as "additive" T -> S -> Union[T, S], intersection as T -> S -> S t.i. returning the Right. Whenever we change the type we have at least a good chance the following code will catch an error. No change for difference comes to my mind though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stubs: improvement Improve/refactor existing annotations, other stubs issues
Projects
None yet
Development

No branches or pull requests

2 participants