Skip to content

[mypyc] Use correct rtype for variable with inferred optional type#15206

Merged
JukkaL merged 2 commits into
python:masterfrom
ichard26:fix-var-type
Jun 2, 2023
Merged

[mypyc] Use correct rtype for variable with inferred optional type#15206
JukkaL merged 2 commits into
python:masterfrom
ichard26:fix-var-type

Conversation

@ichard26

@ichard26 ichard26 commented May 7, 2023

Copy link
Copy Markdown
Collaborator
def f(b: bool) -> None:
    if b:
        y = 1
    else:
        y = None

f(False)

On encountering y = 1, mypyc uses the expression type to determine the
variable rtype. This usually works (as mypy infers the variable type
based on the first assignment and doesn't let it change afterwards),
except in this situation.

NameExpr(y)'s type is int, but the variable should really be int | None.
Fortunately, we can use the Var node's type information which is
correct... instead of blindly assuming the first assignment's type is
right.

Fixes mypyc/mypyc#964

```python
def f(b: bool) -> None:
    if b:
        y = 1
    else:
        y = None

f(False)
```

On encountering y = 1, mypyc uses the expression type to determine the
variable rtype. This usually works (as mypy infers the variable type
based on the first assignment and doesn't let it change afterwards),
except in this situation.

NameExpr(y)'s type is int, but the variable should really be int | None.
Fortunately, we can use the Var node's type information which is
correct... instead of blindly assuming the first assignment's type is
right.
Comment thread mypyc/irbuild/builder.py
@twoertwein

Copy link
Copy Markdown

Is there a reason to limit combining infered types across branches to None? For example, pyright consistently combines any types from different branches:

def test(flag: bool):
    if flag:
        x = 1
    else:
        x = ""
    reveal_type(x)
    # mypy: builtins.int
    # pyright: Literal[1, ""]

I assume this might be a big change for mypy but when adding a special case for None, it seems only consistent to use the same logic more broadly.

@ichard26

ichard26 commented May 12, 2023

Copy link
Copy Markdown
Collaborator Author

@twoertwein that is a question for the mypy maintainers. I'm part of the core team, but I only work on mypyc. This PR simply fixes mypyc (the compiler) to do the right thing when mypy infers a combined type across branches (and not generate code that immediately crashes). In what situations mypy infers a combined type is not within the domain of this PR.

Edit: found the relevant mypy issue -> #6233.

@JukkaL JukkaL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! Looks good. Left some suggestions about tests.

Comment thread mypyc/test-data/run-misc.test Outdated
Comment thread mypyc/irbuild/builder.py
@ichard26
ichard26 requested a review from JukkaL May 27, 2023 15:29

@JukkaL JukkaL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, thanks!

@JukkaL
JukkaL merged commit f8f9453 into python:master Jun 2, 2023
@ichard26
ichard26 deleted the fix-var-type branch June 2, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unexpected runtime type error with inferred optional type

3 participants