Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jun 2, 2023

Conversation

ichard26
Copy link
Collaborator

@ichard26 ichard26 commented May 7, 2023

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.
@twoertwein
Copy link

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
Copy link
Collaborator Author

ichard26 commented May 12, 2023

@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.

Copy link
Collaborator

@JukkaL JukkaL left a comment

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.

mypyc/test-data/run-misc.test Outdated Show resolved Hide resolved
mypyc/irbuild/builder.py Show resolved Hide resolved
@ichard26 ichard26 requested a review from JukkaL May 27, 2023 15:29
Copy link
Collaborator

@JukkaL JukkaL left a comment

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