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

Preferred idiom for exhaustiveness checking of unions #5818

Closed
bluetech opened this issue Oct 22, 2018 · 21 comments
Closed

Preferred idiom for exhaustiveness checking of unions #5818

bluetech opened this issue Oct 22, 2018 · 21 comments

Comments

@bluetech
Copy link
Contributor

bluetech commented Oct 22, 2018

  • Are you reporting a bug, or opening a feature request?

A question...

  • Please insert below the code you are checking with mypy.

I have some union, e.g.

from typing import Union

MyUnion = Union[int, str]

and I have a function which takes the union and matches on it:

def handle_my_union(x: MyUnion) -> None:
    if isinstance(x, int): ...
    elif isinstance(x, str): ...

Now suppose that the union gets added a new member:

- MyUnion = Union[int, str]
+ MyUnion = Union[int, str, bytes]

I have to remember to add it to all of the places which match on the union, for example handle_my_union. As written above, mypy doesn't warn (and has no reason to). So what I use is this function (stolen mostly verbatim from TypeScript):

from typing import NoReturn

def assert_never(x: NoReturn) -> NoReturn:
    assert False, "Unhandled type: {}".format(type(x).__name__)

and use it as follows:

 def handle_my_union(x: MyUnion) -> None:
     if isinstance(x, int): ...
     elif isinstance(x, str): ...
+    else:
+        assert_never(x)
  • What is the actual behavior/output?

Now mypy warns:

error: Argument 1 to "assert_never" has incompatible type "bytes"; expected "NoReturn"

When I add a branch to handle bytes:

 def handle_my_union(x: MyUnion) -> None:
     if isinstance(x, int): ...
     elif isinstance(x, str): ...
+    elif isinstance(x, bytes): ...
     else:
         assert_never(x)

the error is gone.

  • What is the behavior/output you expect?

I am happy with this solution, I just wonder if this is something that is considered appropriate to rely on, or if there's another preferable way to achieve exhaustiveness checking?

  • What are the versions of mypy and Python you are using?

Python 3.5.5, mypy 0.630.

  • What are the mypy flags you are using? (For example --strict-optional)

no_implicit_optional, check_untyped_defs, warn_unused_ignores.

@srittau
Copy link
Contributor

srittau commented Oct 22, 2018

Typescript also has a never type that is not compatible to anything, not even itself. I wonder whether it makes sense to have something like this in typing, which could be used in place of the argument type here.

@bluetech
Copy link
Contributor Author

Isn't NoReturn equivalent to never? I'm just concluding by analogy here, specifically

  • In TypeScript, as you say, the never is used for the "no return" case, e.g. node process.exit.

  • In Rust, the "no return" case actually came first (written as e.g. pub fn exit(code: i32) -> !) and was then generalized to a never type.

@gvanrossum
Copy link
Member

This is a clever hack, and I would never have come up with that (probably because the use case is not one I commonly encounter). It makes sense to legitimize some form of this and document it.

@Michael0x2a
Copy link
Collaborator

Typescript also has a never type that is not compatible to anything, not even itself.

Hmm, at least according to the docs, it seems like Typescript's never is compatible with itself. Nothing else is though, not even Any -- which is where Typescript's type system appears to diverge from ours. (We do seem to allow assigning values of type Any to variables of type NoReturn.)

This difference might actually make this pattern unsafe in a few cases. For example:

from typing import Union, Any

# where 'blah' is some 3rd party module with no type hints -- so 'Foo' has type 'Any'
from blah import Foo   

MyUnion = Union[int, str, Foo]

def assert_never(x: NoReturn) -> NoReturn:
    assert False

def handle_my_union(x: MyUnion) -> None:
    if isinstance(x, int):
        pass
    elif isinstance(x, str):
        pass
    else:
        assert_never(x)   # Uh-oh, no error here!

In this case, since blah has no type hints, Foo is inferred to be of type Any. And due to how Any interacts with NoReturn in our type system, the assert_never(...) doesn't end up warning us that our branches are non-exhaustive in this case.

Not sure how big of an issue this is though in practice -- I figured I ought to just bring it up since it seems like a useful caveat to mention if we do document this trick.

(Or I guess we could make NoReturn and Any interact in the same way they do in Typescript -- not sure how easy or hard that would be though.)

@emmatyping
Copy link
Collaborator

I would also point out this would be inconsistent with PEP 484 which states that NoReturn can only be used as a return type:

The NoReturn type is only valid as a return annotation of functions, and considered an error if it appears in other positions

Also, it might be nice to warn about using NoReturn in other places.

@bluetech
Copy link
Contributor Author

(Or I guess we could make NoReturn and Any interact in the same way they do in Typescript -- not sure how easy or hard that would be though.)

FWIW, after applying the following change, to which I reached with some printf debugging, mypy's test suite still passes, and the NoReturn ← Any assignment fails with "error: Incompatible types in assignment (expression has type "Any", variable has type "NoReturn")", like TypeScript does.

diff --git a/mypy/subtypes.py b/mypy/subtypes.py
index d06ad40f..c4a62a08 100644
--- a/mypy/subtypes.py
+++ b/mypy/subtypes.py
@@ -162,7 +162,7 @@ class SubtypeVisitor(TypeVisitor[bool]):
         return True
 
     def visit_any(self, left: AnyType) -> bool:
-        return True
+        return not isinstance(self.right, UninhabitedType)
 
     def visit_none_type(self, left: NoneTyp) -> bool:
         if experiments.STRICT_OPTIONAL:

@bluetech
Copy link
Contributor Author

I would also point out this would be inconsistent with PEP 484 which states that NoReturn can only be used as a return type:

The value of the argument of assert_never doesn't really matter as long as it doesn't match, I think. So assert_never can be defined like this instead:

from typing import NoReturn
import enum

class Impossible(enum.Enum):
    pass

def assert_never(x: Impossible) -> NoReturn:
    assert False, "Unhandled type: {}".format(type(x).__name__)

@gvanrossum
Copy link
Member

gvanrossum commented Oct 22, 2018 via email

@ilevkivskyi
Copy link
Member

The NoReturn type is only valid as a return annotation of functions, and considered an error if it appears in other positions

I would actually propose to remove this restriction from the PEP (if it is still OK), and make NoReturn a normal type. Proposed pattern is sufficient motivation for this, I could imagine it is useful to statically assert that a block is unreachable, especially that mypy already can do this.

Regarding the subtyping between Any and NoReturn we have a separate issue #3194.

@srittau
Copy link
Contributor

srittau commented Oct 24, 2018

Wouldn't it make sense to introduce an alias for NoReturn in that case? Annotating arguments with NoReturn is not an obvious pattern.

@ilevkivskyi
Copy link
Member

The name is a bit unusual indeed, but typical alternatives Uninhabited, BottomType, etc. are not better IMO. Also after using it a bit, it actually kind of starts to make sense (at least for me).

@srittau
Copy link
Contributor

srittau commented Oct 24, 2018

Personally, I find Typescript's never type to be quite intuitive.

@bluetech
Copy link
Contributor Author

I also like the name Never. I would go as far as deprecating NoReturn in favor of using Never for everything, like TypeScript does.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 24, 2018

I my opinion NoReturn really only makes sense as a function return type. I think that Never could work better in other contexts. Nothing is another plausible name, but it can be confused with None.

I think that subtyping between Any and Never (or whatever it would be called) should be discussed separately from #3194, since that issue doesn't seem to be moving forward. Intuitively, Any is some value, whereas NoReturn is no value, so perhaps Any shouldn't be compatible with NoReturn.

@ilevkivskyi
Copy link
Member

I would go as far as deprecating NoReturn in favor of using Never for everything.

This is not going to happen, it's too late. There are around 30K files in GitHub that use NoReturn.

@ilevkivskyi
Copy link
Member

since that issue doesn't seem to be moving forward

Is this really an argument? I think it is always better to consider such edge cases in the context of the whole picture, otherwise it may be inconsistent. For example joins and meets should behave consistently w.r.t. to subtyping.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 24, 2018

I think it is always better to consider such edge cases in the context of the whole picture, otherwise it may be inconsistent.

Sure, but I'm not seeing how #3194 is directly relevant here. #3194 proposes that we change the type system in certain ways, which may affect how Any and NoReturn interact. Of course any such proposed change needs to be internally consistent (and with other goals we have). However, it may be possible to change only how Any/NoReturn interact while otherwise keeping the current type system semantics, while becoming no less consistent. We don't know this yet, so I think that it's worth discussing.

Or do you believe that the current type system is inconsistent, and any additional changes to the type system could make it harder to make it consistent in the future? If so, I'd like to see more examples of what is currently wrong.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 24, 2018

This is not going to happen, it's too late.

We could perhaps rename NoReturn to Never and change NoReturn into an alias for Never? NoReturn could then be deprecated at some point. Not sure if this would be worth the possible confusion.

@ilevkivskyi
Copy link
Member

Not sure if this would be worth the possible confusion.

This is the main question. We can of course do this ("two step" deprecation), but the question is whether Never is really so much better. I agree this is a better name, bot not hugely better. So taking into account other priorities, I would not do this.

Another question to consider is how to repr this type. Currently it is <nothing>, which is probably OK, but if we are going forward should we change it to <never>? List[<never>] probably looks a bit weird, although we sometimes infer such types.

rexledesma added a commit to dagster-io/dagster that referenced this issue May 17, 2021
Summary:
Ran into this when refactoring `PartitionSetDefinition`.

This idiom appears in https://hakibenita.com/python-mypy-exhaustive-checking and
python/mypy#5818 - this is a nice to ensure exhaustion
when doing conditionals on enums.

Test Plan: N/A

Reviewers: dgibson, cdecarolis, alangenfeld

Reviewed By: dgibson

Differential Revision: https://dagster.phacility.com/D7642
@bluetech
Copy link
Contributor Author

assert_never and Never (an alias to NoReturn) will be included in Python 3.11 and are already included in typing-extensions 4.1.0, thanks to @JelleZijlstra, so I believe this issue is settled!

@JelleZijlstra
Copy link
Member

Correct! It's also documented in https://typing.readthedocs.io/en/latest/source/unreachable.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants