Revert "Remove useless NoReturn in NodeNG.statement's typing (#1304)"

[cdce8p]

As mentioned in #1304 (comment), this is actually a false-negative in mypy and not an error.
Pyright / pylance correctly detects it (in strict mode).

[Pierre-Sassoulas]

Maybe we can also add pylance to the pre-commit conf ?

[cdce8p]

Maybe we can also add pylance to the pre-commit conf ?

Let's start with one (mypy) first. pyright will be much more time consuming to get right, especially in strict mode. Additionally it's Typescript, not Python which might complicate the setup. I haven't looked into it though.

[tusharsadhwani]

this is actually a false-negative in mypy

are you sure?

mypy docs and pep 484 both seem to point that NoReturn should only be in the function signature if the function always never returns. if it's inside a union, the NoReturn is automatically unnecessary.

[cdce8p]

this is actually a false-negative in mypy

are you sure?

Yes!

mypy docs and pep 484 both seem to point that NoReturn should only be in the function signature if the function always never returns. if it's inside a union, the NoReturn is automatically unnecessary.

You are missing that we defined overloads for this method. The Union type you are referring to can never actually be returned. It would be just as valid to use Any or remove the types from the actual definition all together. Both would result in the same mypy results. The more concrete types just limit which overloads are actually possible.

There is one overload that will always and only ever return NoReturn: Calling statement() on a Module node (without future=true).
https://github.com/PyCQA/astroid/blob/e41a896e47d8b5dcd4c460c72bde8063e7c9b447/astroid/nodes/scoped_nodes/scoped_nodes.py#L664-L666

[tushar-deepsource]

I still don't buy it.

... that will always and only ever return NoReturn

NoReturn doesn't actually refer to a return type. It's a special sentinel of sorts, it only implies that there's
*never* a return from this function.

The case of Union[int, NoReturn] would be just as nonsensical as this case as it is in the case of statement:

def f() -> Union[int, NoReturn]:
    if random() > 0.5:
        sys.exit()

    return 1

Even though the function does never return in some cases, that doesn't mean this type makes any sense. Even though mypy doesn't complain about it.

It is a mypy false negative

It could just as well be a pyright false positive, right?

[cdce8p]

I still don't buy it.

... that will always and only ever return NoReturn

NoReturn doesn't actually refer to a return type. It's a special sentinel of sorts, it only implies that there's never a return from this function.

True. My wording here wasn't entirely correct.

The case of Union[int, NoReturn] would be just as nonsensical as this case as it is in the case of statement:

def f() -> Union[int, NoReturn]:
    if random() > 0.5:
        sys.exit()

    return 1

Even though the function does never return in some cases, that doesn't mean this type makes any sense. Even though mypy doesn't complain about it.

You are missing the overload definitions.

    @overload
    def statement(self) -> "Module":
        ...

    @overload
    def statement(self, *, future: Literal[True]) -> NoReturn:
        ...

    def statement(
        self, *, future: Literal[None, True] = None
    ) -> Union["NoReturn", "Module"]:
        # function body        
        ...

Union["NoReturn", "Module"] is only used to validate the return types of the overloads itself, not for actual type checking. That's what the overloads are for.

A small example.

from astroid import nodes

node = nodes.Module("name", "")
reveal_type(node.statement(future=True))

This is what Pylance / pyright infers. No Union return type, just NoReturn.

Type of "node.statement(future=True)" is "NoReturn"

And for mypy.

Revealed type is "<nothing>"

It is a mypy false negative

It could just as well be a pyright false positive, right?

I'm quite confident it isn't but by all means feel free to open a new issue in the Pylance repo.

[tushar-deepsource]

Union["NoReturn", "Module"] is only used to validate the return types of the overloads itself

Which is unnecessary, and mypy clearly understands that. Which is my point.

[cdce8p]

Union["NoReturn", "Module"] is only used to validate the return types of the overloads itself

Which is unnecessary, and mypy clearly understands that. Which is my point.

Mypy just doesn't seem to check overwritten function signature against the parent one, as I already mentioned a false-negative. (This should probably be a feature request for mypy.)

class NodeNG:
    def statement(
        self, *, future: Literal[None, True] = None
    ) -> Union["nodes.Statement", "nodes.Module", "NoReturn"]:
        ...

class Module(NodeNG):
    def statement(
        self, *, future: Literal[None, True] = None
    ) -> Union["NoReturn", "Module"]:  # `NoReturn` is needed here
        ...

All return types of Module.statement should also be present in NodeNG.statement. Otherwise you would argue that an overwritten method can suddenly return a different type.

[tushar-deepsource]

$ cat mytest.py
class C:
    def f(self, x: int) -> int:
        return x


class D(C):
    def f(self, x: int) -> str:
        return 'some string'


$ mypy mytest.py
mytest.py:7: error: Return type "str" of "f" incompatible with return type "int" in supertype "C"
Found 1 error in 1 file (checked 1 source file)

Mypy does check signatures against parent. It's only NoReturn which is a special case, because Union[X, NoReturn] is exactly equal to X itself.

[tushar-deepsource]

I don't know if it's of any help, but I found this comment which says the same thing

[Pierre-Sassoulas]

I don't have a particular opinion about the issue. I think if Guido think analyzing NoReturn is too complicated -as seen in the linked comment- this is going to move slowly in mypy and probably not have to be changed anytime soon. Also it's probably not worth arguing for a long time over, what do you think @cdce8p ?

I'm going to move this MR to 2.10.0 if the discussion is not resolved when it's the last issue to handle for astroid 2.9.1. I don't think it's worth to block the release for.

[tusharsadhwani]

It's a really tiny thing to be honest, I'd be fine with it staying here until mypy complains about it one way or another. It doesn't really affect the types in code either way.

[cdce8p]

The comment doesn't really change much. Adding NoReturn isn't false, it's just not fully implemented in mypy as the use cases are limited. I think there are two ways forward.

1. Remove the NoReturn overload from statement() completely. It isn't used anyway.
2. Leave it and fix the base type (this PR). Just because mypy doesn't complain about it doesn't mean it's wrong. It does solve the pyright issue.

I would prefer (2). Especially considering that the NoReturn case will become the default in astroid 3.0 (without the Union type).

https://github.com/PyCQA/astroid/blob/39c37c1fa944a577124f3482e3714f82f0c1b6dd/astroid/nodes/scoped_nodes/scoped_nodes.py#L660-L680

[tusharsadhwani]

Meanwhile, I've gotten in touch with mypy's team, it's getting interesting. We'll figure this case out, such that mypy will (hopefully) only allow one of these two forms in the future. No more ambiguity!

[cdce8p]

Meanwhile, I've gotten in touch with mypy's team, it's getting interesting. We'll figure this case out, such that mypy will (hopefully) only allow one of these two forms in the future. No more ambiguity!

Did you create an issue? If so, you could post the link?

[tusharsadhwani]

I DM'd someone :p

I'll be emailing typing-sig soon, I'll drop the link then.

[tusharsadhwani]

https://mail.python.org/archives/list/[email protected]/thread/WI4G3VAHMX73VJC7JERBRXDG24WZZPQL/

[tushar-deepsource]

Mailing list isn't as active it seems, so I posted it in typing discussions

[cdce8p]

It seems like Eric mentioned what I already said: It can make sense when used with overload.
That's basically the exact example we have here: python/typing#994 (comment)

I would like to merge this PR now. Although I don't expect it to but should the discussion reach another conclusion someday, we can always change it later.

[tushar-deepsource]

Yeah, no problems.

[Pierre-Sassoulas]

Thank you for taking the time of asking questions on mailing lists and typing discussions @tushar-deepsource !

[tushar-deepsource]

Eric, author of pyright, agrees that it is indeed a pyright bug:

microsoft/pyright#2765

Though this PR isn't wrong, mypy wasn't giving a false negative here. Just wanted to let you know.

[cdce8p]

I just saw in an unrelated topic that NoReturn basically means an empty Union internally. From that perspective it would also make sense not to require it when used with overload.

Thanks @tushar-deepsource for helping to get to the bottom of it! To finally put this to rest, would you like to do the honors and open the revert-revert PR?

[tusharsadhwani]

will do