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

Make narrow_declared_type correctly handle Literal types #6961

Conversation

Michael0x2a
Copy link
Collaborator

@Michael0x2a Michael0x2a commented Jun 8, 2019

This diff fixes the narrowed_declared_type function to correctly handle cases where the declared type is actually a Literal[...].

This fixes #6952.

In short, it seems like using the new semantic analyzer somehow made mypy call the narrow_type_from_binder(...) function where it previously did not.

Specifically, mypy passed in the var expression and a known_type of Literal[42], we got a restriction of builtins.int (with a last_known_value of 42), and attempted to narrow the Literal[42] on line 3512.

But due to the missing case in narrow_declared_type, we ended up returning just the int.

This diff fixes the `narrowed_declared_type` function to correctly
handle cases where the declared type is actually a `Literal[...]`.

This fixes python#6952. In short, it
seems like using the new semantic analyzer somehow made mypy call
[the `narrow_type_from_binder(...)` function][0] where it previously
did not.

Specifically, mypy passed in the `var` expression and a `known_type` of
`Literal[42]`, we got a restriction of `builtins.int` (with a
`last_known_value` of 42), and attempted to narrow the `Literal[42]` on
line 3512.

But due to the missing case in `narrow_declared_type`, we ended up
returning just the int.

  [0]: https://github.com/python/mypy/blob/master/mypy/checkexpr.py#L3504
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

The difference in behavior is likely related to #6458, and the fact that Final gets "stripped" in the process. Although this fix works for the issue, I would like to actually understand why it happens before deciding whether we should use this as a fix.

Note there is a little merge conflict.

@Michael0x2a
Copy link
Collaborator Author

The difference in behavior is likely related to #6458, and the fact that Final gets "stripped" in the process.

I'm not too familiar with the behavior of the new semantic analyzer, but fwiw it didn't seem as if this were the case. More precisely, it seemed we did correctly remember that the type of the variable was Literal[int] throughout the whole process -- we just ended up inferring builtins.int instead of Literal[int] when we tried narrowing Literal[int] against a builtin.int.

Although this fix works for the issue, I would like to actually understand why it happens before deciding whether we should use this as a fix.

I do think this fix would be useful independently from the linked issue. IIUC doing narrow_declared_type(ChildType, ParentType) yields just ChildType, so I think it makes sense to make narrow_declared_type(Literal[42], int) behave in the same way.

The fact that change fixes #6458 is almost a side-effect IMO.

@ilevkivskyi
Copy link
Member

I'm not too familiar with the behavior of the new semantic analyzer, but fwiw it didn't seem as if this were the case.

I mean this #6988.

I still think this PR has value, but it might also mask an actual flaw in the logic in new semantic analyzer.

@Michael0x2a Michael0x2a merged commit 6df281e into python:master Jun 14, 2019
@Michael0x2a Michael0x2a deleted the fix-literal-final-inference-with-deferral branch June 14, 2019 14:02
PattenR pushed a commit to PattenR/mypy that referenced this pull request Jun 23, 2019
This diff fixes the `narrowed_declared_type` function to correctly
handle cases where the declared type is actually a `Literal[...]`.

Specifically, doing something like the below will return `int` instead
of `Literal[42]`:

    narrowed_declared_type(Literal[42], int)

This is inconsistent with the behavior of doing something like the
below, which will return `ChildType`:

    narrowed_declared_type(ChildType, ParentType)

This diff modifies `narrowed_declared_type` to instead return `Literal[42]`
in the first example.
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.

Literal + Final don't work well on new semantic analyzer
2 participants