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

Use union instead of join for unifying types #12056

Open
JelleZijlstra opened this issue Jan 24, 2022 · 9 comments
Open

Use union instead of join for unifying types #12056

JelleZijlstra opened this issue Jan 24, 2022 · 9 comments
Labels
bug mypy got something wrong needs discussion priority-0-high topic-join-v-union Using join vs. using unions

Comments

@JelleZijlstra
Copy link
Member

@erictraut has gathered a long list of problems caused by mypy's behavior of using the "join" operator to unify types in various contexts.

#12053
#12009
#11934
#11440
#11618
#10740
#10442
#7884
#7835
#7616
#6968
#6079
#5512
#5128
#4134
#3339

I believe that we should change this behavior.

If someone opens a PR to do this, it would be interesting to see what the results look like in mypy-primer and mypy's CI.

@A5rocks
Copy link
Contributor

A5rocks commented Jan 24, 2022

How will this interact with TypeForm vs Type?

Does this mean join(A,B) will still be accepted for Type[T]? Union[A, B] is not.

Nevermind: I was being confused. This doesn't matter I believe. (TypeForm is more about passing a type in, not about the type of an object)

@bluetech
Copy link
Contributor

I noticed that this was tried for the specific case of conditional expressions (type of x if y else z) in 532f3fb (issue #3487) but was partly reverted in 94309a1 because it "caused many problems in internal Dropbox repos that we aren't ready to fix yet.".

@KotlinIsland
Copy link
Contributor

KotlinIsland commented May 9, 2023

Basedmypy uses union. We have seen great success with it. (we also form intersections at meets)

@JukkaL
Copy link
Collaborator

JukkaL commented May 10, 2023

I agree that we should probably do this, but I also want to see the fallout first. Due to our backward compatibility policy, the change needs to be first introduced behind a feature flag, and we need a major mypy release when we switch the default (e.g. 2.0). This will let us iterate on the change and allow users to experiment with it easily before we make the switch.

@ilevkivskyi
Copy link
Member

Just to add my 50 cents here: I don't like the idea of completely switching to unions. The fact that this will fix 20 issues doesn't guarantee that we will not get 40 new issues caused by things like:

def basket() -> set[Fruit]:
    res = {Banana(), Apple(), Orange()}
    # do something with res...
    return res

I think a better (and probably safer) approach is: we should infer unions in "heterogeneous" contexts, while keeping joins in "homogeneous" contexts. By "heterogeneous" contexts I mean things like tuple types, ternary expressions, user declared unions, star arguments (which are also tuples under the hood), maybe there are some more. Note that a significant amount of issues mentioned above are about these cases. We can try switching these cases one by one, and see what is the fallout.

My prediction is that fallout from switching tuple fallback to union will be minimal (in particular since tuple instance is covariant), we can start with this.

@KotlinIsland
Copy link
Contributor

@ilevkivskyi In my experience, we haven't seen any issues with this functionality in basedmypy. You even get a very useful message in the example you provided:

class Fruit: ...
class Apple(Fruit): ...
class Orange(Fruit): ...
class Banana(Fruit): ...

def f() -> set[Fruit]:
    result = {Apple(), Orange(), Banana()}
    return result  # test.py:26: error: Incompatible return value type (got "set[Apple | Orange | Banana]", expected "set[Fruit]")  [return-value]
                   # test.py:26: note: Perhaps you need a type annotation for "a"? Suggestion: "set[Fruit]"

Just some opinions from the basedmypy team is that TypeScript always performs a unionization here as well.

karlch added a commit to karlch/vimiv-qt that referenced this issue May 10, 2024
ilevkivskyi added a commit that referenced this issue Jun 22, 2024
Ref #12056

If `mypy_primer` will look good, I will add some logic to shorted unions
in error messages.

cc @JukkaL

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
ilevkivskyi added a commit that referenced this issue Jul 2, 2024
Ref #12056

cc @JukkaL 

Again, let's check the primer...
@asottile-sentry
Copy link

because I'm curious -- is there any trick to adjust mypy's inference to resolve to the union that's shorter than enumerating all the types?

class Base: pass
class A(Base): pass
class B(Base): pass
class C(Base): pass

x = [A, B, C]  # implicitly `list[type[Base]]`
y: list[type[A | B | C]] = [A, B, C]  # works but is a little cumbersome as the number increases

@ilevkivskyi
Copy link
Member

I am actually going to add a flag so that people can opt in if they want more unions (the default however is unlikely to change in next few years).

@mishamsk
Copy link

mishamsk commented Jul 3, 2024

@ilevkivskyi what's the best way to express support for the union behavior? Also, judging by your phrasing, may I assume that mypy may get the "union way" under as an opt-in sooner rather than later?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong needs discussion priority-0-high topic-join-v-union Using join vs. using unions
Projects
None yet
Development

No branches or pull requests

8 participants