Stricter None handling with --no-strict-optional, refs #11705

[sobolevn]

First of all, this is really hard to wrap your head around! Because --no-strict-optional is messy.

But, looks like I found several hacks to make this work as expected.
However, I am pretty sure that we do have a lot of other places where things can blow up with --no-strict-optional.

Closes #11705

[sobolevn]

CC @ethan-leba

I would appreciate your review! 👀

[ethan-leba]

Left one comment -- I haven't looked at mypy in a while so I might be missing some context! There's also some extraneous changes you might want to move into a separate commit (removing type comments, fixing the docstrings)

[ethan-leba]

Do we need the keep_none flag if we're checking for strict_optional and NoneType? The logic seems similar to mypy/checkexpr.py@L2826.

[sobolevn]

Yes, we do. Because we only need this logic to be executed at some use-cases. If I remove this param, we will have 5-6 failing tests.

[sobolevn]

There's also some extraneous changes you might want to move into a separate commit (removing type comments, fixing the docstrings)

Yeah, I sometimes can't resist fixing things I find while working on other feature 🙂

[JukkaL]

Can you look at the new error reported by mypy_primer, in steam/utils.py? Is it a false positive?

[sobolevn]

@JukkaL looks valid to me. They have: https://github.com/Gobot1234/steam.py/blob/fb0bed85bbdb5c953b4a0380a78fdb2418f3c55e/steam/utils.py#L140-L143

        try:
            id, type, universe, instance = id2_to_tuple(id) or id3_to_tuple(id)
        except TypeError:
            raise InvalidSteamID(id, "it cannot be parsed") from None

Where both id[2,3]_to_tuple have Tuple[..., ..., ..., ...] | None return type. It still can be None if both id[2,3]_to_tuple return None. So, it is a new issue that we now can discover.

And since this error is handled by except TypeError - I assume it happens sometimes. Runtime also agrees with me.

[JukkaL]

Hmm they seem to have strict optional checking disabled (https://github.com/Gobot1234/steam.py/blob/fb0bed85bbdb5c953b4a0380a78fdb2418f3c55e/pyproject.toml#L79), so errors about None should normally not be reported by mypy. So maybe it's a regression after all?

If they actually don't use strict optional checking, then I'm wondering why the error wasn't produced previously, as this PR should only impact --no-strict-optional mode, AFAICT.

[JukkaL]

@sobolevn Have you had a chance to look at the the mypy_primer false positive?

[sobolevn]

Nope, I've missed it. Will do today!

[sobolevn]

Looks like that I have to revisit --no-strict-optional docs. I've never used it myself and I've missed that even explicit Optional[int] is still just int.

[sobolevn]

Ok, I have problems with this one, because --no-strict-optional does not fit my brain at all. Let's start with the very basic things.

Docs: https://mypy.readthedocs.io/en/latest/kinds_of_types.html#disabling-strict-optional-checking

• x: Optional[int]: to my surprise there's no explicit test for it with --no-strict-optional. What should it be?

  a) int
  b) int | None
  c) They are the same

  Because it is not clear to me what

  Mypy also has an option to treat None as a valid value for every type

  means. Looks like int and int | None should be the same thing semantically. Mypy also has this line in the docs

  Mypy treats this as semantically equivalent to the previous example if strict optional checking is disabled, since None is implicitly valid for any type, but it’s much more useful for a programmer who is reading the code

  to support my opinion.

• So, if it is the same thing, Optional annotation should do literally nothing. So, this original example:

from typing import Dict, Optional, NamedTuple
class C(NamedTuple):
    x: int

t: Optional[C]

d: Dict[C, bytes]
x = t and d[t]
reveal_type(x)  # N: Revealed type is "builtins.bytes*"
if x:
    reveal_type(x)  # N: Revealed type is "builtins.bytes*"

Should be the same as:

from typing import Dict, NamedTuple
class C(NamedTuple):
    x: int

t: C

d: Dict[C, bytes]
x = t and d[t]
reveal_type(x)  # N: Revealed type is "builtins.bytes*"
if x:
    reveal_type(x)  # N: Revealed type is "builtins.bytes*"

Am I right?

CC @JukkaL

[sobolevn]

If my previous comment is correct, then we just need to make this change:

[JukkaL]

Am I right?

Yeah. Basically with strict optional disabled, Optional[X] behaves the same as X. However, other modules with strict optional enabled that import something with an optional type from a module with strict optional disabled actually see the Optional[X] and treat it as a regular optional type. So using Optional[X] inside a module that has strict optional disabled can make a difference, but only outside this module -- i.e. we should try to preserve optional types but not generate errors about them, but this is not very important to get 100% right.

Additionally, I don't care much about actively improving the behavior of mypy when strict optional is disabled, but it's still important to maintain backward compatibility. Some users are relying on it, and it can be difficult to migrate some code to strict optional checking so even new users might prefer it in some cases.

And sorry for the slow reply! Hopefully this made things clearer and didn't just add more confusion :-) The implementation is a bit messy and to some extent a historical artifact from the time when mypy didn't support optional types.

[sobolevn]

@JukkaL thank you! TIL!
New version is waiting for your review 🙂

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

steam.py (https://github.com/Gobot1234/steam.py)
- steam/utils.py:141: error: Incompatible types in assignment (expression has type "int", variable has type "Optional[Union[Literal[0], Literal[1], Literal[2], Literal[4]]]")  [assignment]

[JukkaL]

Looks good -- thanks for the fix!