Support making a variable Optional in an else branch #11002

msullivan · 2021-08-20T20:13:09Z

That is, support patterns such as:

if condition:
    foo = Foo()
else:
    foo = None

Currently this does not work, but the the reverse does (because foo
will be inferred as a PartialType).

I think it might be worth tackling this in a more general way, for
other types, though I think that is a little fiddlier and likely to be
more controversial, so I'm starting with something special-cased for
the "assigning literal None" case first.

The rule we implement is that we allow updating the type of a variable
when assigning None to it if the variable's type was inferred and it
was defined in an earlier branch of the same if/then/else statement.

Some infrastructure is added to make determinations about that.

Given that this is probably my single biggest frustration with mypy,
and that this PR took me less than three hours to prepare, I am pretty
angry at myself for not having done this three years ago.

That is, support patterns such as: ``` if condition: foo = Foo() else: foo = None ``` Currently this does not work, but the the *reverse* does (because foo will be inferred as a PartialType). I think it might be worth tackling this in a more general way, for other types, though I think that is a little fiddlier and likely to be more controversial, so I'm starting with something special-cased for the "assigning literal None" case first. The rule we implement is that we allow updating the type of a variable when assigning `None` to it if the variable's type was inferred and it was defined in an earlier branch of the same `if/then/else` statement. Some infrastructure is added to make determinations about that. Given that this is probably my single biggest frustration with mypy, and that this PR took me less than three hours to prepare, I am pretty angry at myself for not having done this three years ago.

emmatyping

This seems really great, its a common annoyance so fixing it would be a nice win :)

I had a couple of suggestions but otherwise looks good.

mypy/binder.py

emmatyping · 2021-08-20T22:50:38Z

test-data/unit/check-optional.test

@@ -832,3 +832,99 @@ main:4: error: Argument 1 to "asdf" has incompatible type "List[str]"; expected
 main:4: note: "List" is invariant -- see https://mypy.readthedocs.io/en/stable/common_issues.html#variance
 main:4: note: Consider using "Sequence" instead, which is covariant
 [builtins fixtures/list.pyi]
+


Other ideas for tests would be mixing different kinds of frames and control flow e.g. try/except and with statements

Hm, related to that: maybe it's worth supporting this pattern for try/except also?

I decided to try that, though I could be easily convinced to drop it!

emmatyping · 2021-08-21T09:37:42Z

test-data/unit/check-optional.test

+    return z  # E: Incompatible return value type (got "Optional[int]", expected "int")
+
+def f3(b: bool) -> int:
+    # XXX: This one is a little questionable! Maybe we *do* want to allow this?


I think this would be nice to have, but it probably should be discussed more (esp. since there are few cases where the type is different from the first assignment)

Yeah, and if we want to allow this, this can be implemented in a separate PR.

emmatyping

I think this looks good. Might be nice to get some more opinions on the TODO re: assigning None in an if clause, but otherwise it seems good to land.

msullivan · 2021-08-23T18:07:16Z

@JukkaL what do you think?

JukkaL

Thanks, this is a very nice feature that I've also wanted to have for a long time! I didn't do a full review yet, but I came up with various additional test cases.

JukkaL · 2021-08-24T16:19:52Z

test-data/unit/check-optional.test

+    return z  # E: Incompatible return value type (got "Optional[int]", expected "int")
+
+def f3(b: bool) -> int:
+    # XXX: This one is a little questionable! Maybe we *do* want to allow this?


Yeah, and if we want to allow this, this can be implemented in a separate PR.

JukkaL · 2021-08-24T16:19:57Z

test-data/unit/check-optional.test

+    else:
+        y = None
+    reveal_type(y)  # N: Revealed type is "Union[builtins.int, None]"
+


Ideas for additional test cases below. I'm not sure what the semantics should be in some of these cases, but at least it makes sense to have some well-defined semantics for these. Omitted most reveal_type(z) etc. for brevity.

Case 1:

if b: z = 2 elif c: z = None elif d: z = 3

Case 2:

if c: x = [] else: x = None

Case 3:

if b: z = 4 else: if c: z = 5 else: z = None

Case 4:

Use this at module top level, or within class body.

Case 5:

if b: z = 5 else: z = None if c: z = 6

Case 6:

if b: x = [] x.append(1) else: x = None

Case 7:

if b: x: Any = 5 else: x = None reveal_type(x)

Case 8:

[Tricky, optional] Use at module top level, and have a forward reference that causes two passes over the top level during type checking.

Case 9:

[Tricky, optional] Have a forward reference where the type of the reference target is not ready yet within a function between the first and second assignment. This causes the function to be deferred.

Case 10:

Define an attribute using self.x = 5 / self.x = None. I think that it's not safe to allow this, and we may need to reject this, since the attribute type is visible outside the method (see below for an example where this might cause trouble).

Case 11:

[Very tricky, optional] Define an attribute, and cause the method to be deferred after the initial self.x assignment. Now another method refers to the x attribute, and it should not see the non-optional type.

OK, I've addressed most of the tests. Some notes:
2. Demands a type annotation
4. Is disallowed at top level. I'm not sure it has to be, but there could be complications and it seems unlikely to come up? We ignore = None assignments at class level so that pre 3.6 code can write type comments, so nothing useful happens there.
6. Works
7. Infers Any
8, 10, 11: All are disallowed
9: Works fine

JukkaL

Thanks for the updates! Looks pretty good. It is hacky, but this is an important use case so it seems worth it, as I can't think of a cleaner way to do this (without a major redesign). Left a few remaining comments. Feel free to merge once you've addressed them.

JukkaL · 2021-08-26T16:13:32Z

mypy/checker.py

@@ -203,6 +203,12 @@ class TypeChecker(NodeVisitor[None], CheckerPluginInterface):
    # directly or indirectly.
    module_refs: Set[str]

+    # A map from variable nodes to a snapshot of the current frames


"Current frames" here means frames active when the variable is first initialized? Can you explain this in more detail here?

Added some more text. Also realized that we don't need to store the bools at all, since we can just read it out from the frame

JukkaL · 2021-08-26T16:15:18Z

mypy/checker.py

+    # A map from variable nodes to a snapshot of the current frames
+    # and whether they are associated with `if` statements. This can
+    # be used to determine if a variable is defined in a different
+    # branch of the same `if` statement.


Mention that the inner dictionary is keyed by frame id.

Maybe clarify that if we have the same frame id in both first initialization and follow-up assignment with a true value, they are in different branches of the same if statement (assuming that I got this right)?

mypy/checker.py

test-data/unit/check-optional.test

github-actions · 2021-08-27T00:06:37Z

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

jax (https://github.com/google/jax.git)
+ jax/interpreters/pxla.py:807: error: Argument 1 to "len" has incompatible type "Optional[List[Any]]"; expected "Sized"  [arg-type]
- jax/_src/numpy/lax_numpy.py:1846: error: Argument 1 to "_wraps" has incompatible type "Optional[Any]"; expected "Callable[..., Any]"  [arg-type]
+ jax/_src/numpy/lax_numpy.py:1846: note: (Skipping most remaining errors due to unresolved imports or missing stubs; fix these first)
- jax/experimental/host_callback.py:1705: note: (Skipping most remaining errors due to unresolved imports or missing stubs; fix these first)

aioredis (https://github.com/aio-libs/aioredis.git)
- aioredis/lock.py:218: error: Incompatible types in assignment (expression has type "None", variable has type "int")
- aioredis/client.py:3403: error: Incompatible types in assignment (expression has type "None", variable has type "ValuesView[Any]")

pandas (https://github.com/pandas-dev/pandas.git)
+ pandas/plotting/_matplotlib/core.py:1664: error: unused "type: ignore" comment

poetry (https://github.com/python-poetry/poetry.git)
+ poetry/mixology/failure.py:144: error: Item "NoVersionsCause" of "Union[RootCause, NoVersionsCause, DependencyCause, ConflictCause, PythonCause, PlatformCause, PackageNotFoundCause]" has no attribute "conflict"

ikonst · 2022-01-26T15:46:54Z

@msullivan Am I missing something? Could not get this to work: https://mypy-play.net/?mypy=latest&python=3.10&gist=c62637d27e9292d01b8473f68b9e45f5

msullivan force-pushed the rage-optional branch from f413067 to eecc3b3 Compare August 20, 2021 20:22

This comment has been minimized.

Sign in to view

Don't update the type if it is Any

bfd3727

This comment has been minimized.

Sign in to view

emmatyping reviewed Aug 20, 2021

View reviewed changes

msullivan added 3 commits August 20, 2021 16:27

thread outer_if_frame in and rename it

5859a79

sure, some more tests

08f09ea

try it with try/loops

2b24306

This comment has been minimized.

Sign in to view

emmatyping reviewed Aug 21, 2021

View reviewed changes

emmatyping approved these changes Aug 21, 2021

View reviewed changes

JukkaL reviewed Aug 24, 2021

View reviewed changes

Implement most of the tests that Jukka requested

4817560

This comment has been minimized.

Sign in to view

JukkaL approved these changes Aug 26, 2021

View reviewed changes

tweaks

804f52e

msullivan merged commit 56b6803 into master Aug 27, 2021

msullivan deleted the rage-optional branch August 27, 2021 03:59

samueljsb mentioned this pull request May 13, 2022

Making a variable optional in an else block doesn't work for try/else #12782

Open

Uh oh!

Support making a variable Optional in an else branch #11002

Support making a variable Optional in an else branch #11002

Uh oh!

Conversation

msullivan commented Aug 20, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emmatyping left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

emmatyping Aug 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmatyping left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msullivan commented Aug 23, 2021

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msullivan Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Aug 27, 2021

Uh oh!

ikonst commented Jan 26, 2022

Uh oh!

Uh oh!

emmatyping Aug 21, 2021 •

edited

Loading

emmatyping left a comment •

edited

Loading

msullivan Aug 25, 2021 •

edited

Loading