Making unittest type annotations for assertIsNone and assertIsNotNone stricter

[1kastner]

Dear typeshed team,

first of all thanks for all the effort in this project. I am rather new to using Python typhints and I have taken quite some functionality in my IDE for granted. Recently, I tried to check a project of mine project with mypy and got some interesting results. As unittests and working examples were in place, I realized that several of the found issues were caused by not-so-perfect type hinting in the code. In one case, however, I thought that maybe the type hints provided by typeshed could be stricter.

In python/mypy#5088 (comment), it is proposed that in the unittest stubs, NoReturn could be used whenever we know that an assert method call will fail. By elaborating the typehints of assertIsNotNone, unittests could be even statically checked before execution. Obviously, the same is true for its symmetric partner assertIsNone.

Would a corresponding pull request be accepted given that it satisfies the quality criteria of this repository?

[sobolevn]

I personally think that this is a good idea. For example, we can catch things like:

def init_database() -> None:
   print('db is ready')

class Test(unittest.TestCase):
   def test_init_database(self) -> None:
      self.assertIsNotNone(init_database())
      print('finished')  # will be reported as unreachable by mypy

[1kastner]

Exactly! In addition, the unittest testcase method self.assertIsNone() will get the equivalent functionality of assert is None (and its negation).

[1kastner]

Then I would give it a shot

[sobolevn]

Take a look at #8566 as well for an alternative opinion

[1kastner]

@sobolevn thank you for the link to that issue. As far as I understand, debonte suggests that the overload could be dropped (something not preferable in this case as I will argue shortly) or that it moved down in the overload ordering (for this planned PR, we could choose any ordering). The response of JelleZijlstra is "Returning NoReturn isn't a good fit for cases where the code throws an error the user probably isn't expecting, because it can easily lead to type checkers silently skipping the code." I cannot follow this line of argument. How is it possible that code is skipped? Could you provide an example? That would be awesome! For the first part, do I understand correctly that maybe we should not use type checking at all in unittests because we are just looking out for the unexpected to happen, such as that we failed to correctly annotate the types? In that case this raises the question of where to draw the line? Why should we use type annotations at all? Just as the devil's advocate - should we maybe drop the whole folder https://github.com/python/typeshed/tree/master/stdlib/unittest? I do not seriously suggest that, not at all, I just try to understand the part of code being falsely treated by static type checks.

The anti-pattern discussed in python/mypy#5088 is that we need a double check for one concept we want to test. First, the unittest checks whether it is not None (the intended unittest) and then an assertion statement checks the same (just for mypy to be happy). The assertion statement is supported by mypy for type narrowing and is required to pass the linting while it it dropped in optimized code. Thus, dropping the unittest changes the behavior of the code and dropping the assert affects the static type checks. Having both of them leads repeated code like this:

def my_func() -> Optional[str]:
    ...


class MyUnitTest(unittest.TestCase):
    def test_my_func(self):
        result = my_func()
        self.assertIsNotNone(result)
        assert result is not None
        ....
        # continue to work with the string
        ....

This is what I would like to avoid.

[JelleZijlstra]

I cannot follow this line of argument. How is it possible that code is skipped?

Mypy generally doesn't typecheck code it considers unreachable. Here you can see an example: https://mypy-play.net/?mypy=0.971&python=3.10&gist=e5d8fe2c9552e2556d3de0e639349c57 (the type error on the return line isn't caught).

[AlexWaygood]

Python type checkers generally do "reachability analysis" on code: they analyze code to detect if certain blocks are unreachable. They then often decline to report errors on code that they surmise is unreachable. As such, if you have code like this, then mypy will report no errors on the code:

def func() -> None:
    quit()
    1 + "foo"

Mypy has detected that the line with the obvious type error is unreachable, so it hasn't even bothered analysing it. (I think this is meant to be a "feature" of type checkers, though in my experience it often seems to cause quite surprising behaviour.) Mypy will report an error if you have the --warn-unreachable configuration setting applied, but that isn't applied by default.

The issue with having overloads that return NoReturn is that different type checkers have different algorithms for selecting which overload to apply. Pyright and Pyre have algorithms that, more so than mypy, are biased towards picking the first overload in situations where they aren't sure what the type is. That means that if we annotated assertIsNotNone like this:

class TestCase:
    @overload
    def assertIsNotNone(self, obj: None, msg: Any = ...) -> NoReturn: ...
    @overload
    def assertIsNotNone(self, obj: object, msg: Any = ...) -> None: ...

...it would have quite a detrimental effect on users of Pyright and Pyre in some situations.

If presented with code like this, where the arguments are unannotated and so Pyright isn't sure what the types are:

class MyTestCase(TestCase):
    def test_thing(self, obj):
        self.assertIsNotNone(obj)
        1 + "foo"

...Pyright might then pick the first overload, causing it to detect that the rest of the function is unreachable, and meaning that it fails to detect the obvious type error later on in the function. That behaviour might be highly surprising for users.

For this reason, I'd prefer it if we left the stub for assertIsNotNone as it is for now.

[AlexWaygood]

I'm not really sure we should change the signature of assertIsNone, either. It's true that we could do something like this, so that a type checker would error if anything except None was passed into the function:

class TestCase:
    def assertIsNone(self, obj: None, msg: Any = ...) -> None: ...

But I think that might end up being really annoying for users. If you have to do type narrowing to make sure an object is None before you pass it in to TestCase.assertIsNone(), that sort of defeats the point of TestCase.assertIsNone(), surely?

[1kastner]

Thank you for the elaboration! I was assuming that unreachable code was reported by default and thus I did not see that point before.

Regarding type narrowing, well, in some states the variable might be None and in others it should not. Definitely there are programming patterns, e.g. those that work with several state-dependent immutable representations of a concept, that could avoid this type narrowing. However, it might be a bit of over-engineering in my case.

[1kastner]

Then it seems like the behavior of assert statements cannot be imitated by any method, right? Something like "if we have passed this point, we can be sure that this variable is not None." Or in more general terms, some types of a union can be excluded after some methods or functions are invoked. I was also checking on the TypeGuard approach but it seemed to require an if block and was thus not a perfect match for the given situation. And thanks again for your valuable and insightful comments!

[AlexWaygood]

Then it seems like the behavior of assert statements cannot be imitated by any method, right? Something like "if we have passed this point, we can be sure that this variable is not None." Or in more general terms, some types of a union can be excluded after some methods or functions are invoked. I was also checking on the TypeGuard approach but it seemed to require an if block and was thus not a perfect match for the given situation. And thanks again for your valuable and insightful comments!

Yeah, we really need something like python/typing#930 (interestingly, assertIsNotNone has already been mentioned in that issue!).

[1kastner]

Thanks for the link! I understand the concerns for my initially proposed quick fix. I guess this issue can be closed for now. Let's hope the discussion you linked to continues to be fruitful.

[sobolevn]

Ok then! Let's wait for @typing_error helper (whatever the name is).

@JelleZijlstra do you think that we need a PEP for it? I can totally work on it and announce it as a draft in typing-sig as soon as I have something real to show.

In my opinion this can bring python's type system to a new level!

[JelleZijlstra]

Yes, this would need a PEP.