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 revealed type of Final vars distinct from non-Final vars #7955

Merged
merged 3 commits into from Nov 15, 2019

Conversation

@Michael0x2a
Copy link
Collaborator

Michael0x2a commented Nov 15, 2019

This diff changes how we format Instances with a last known value when displaying them with reveal_type.

Previously, we would always ignore the last_known_value field:

x: Final = 3
reveal_type(x)  # N: Revealed type is 'builtins.int'

Now, we format it like Literal[3]?. Note that we use the question mark suffix as a way of distinguishing the type from true Literal types.

x: Final = 3
y: Literal[3] = 3
reveal_type(x)  # N: Revealed type is 'Literal[3]?'
reveal_type(y)  # N: Revealed type is 'Literal[3]'

While making this change and auditing our tests, I also discovered we were accidentally copying over the last_known_value in a few places by accident. For example:

from typing_extensions import Final

a = []
a.append(1)
a.append(2)     # Got no error here?
reveal_type(a)  # Incorrect revealed type: got builtins.list[Literal[1]?]

b = [0, None]
b.append(1)     # Got no error here?
reveal_type(b)  # Incorrect revealed type: got builtins.list[Union[Literal[0]?, None]]

The other code changes I made were largely cosmetic.

Similarly, most of the remaining test changes were just due to places where we were doing something like reveal_type(0) or reveal_type(SomeEnum.BLAH).

The main motivation behind this diff is that once this lands, it should become much simpler for me to write some tests I'll need while revamping #7169. It also helps make a somewhat confusing and implicit part of mypy internals more visible.

This diff changes how we format Instances with a last known value
when displaying them with `reveal_type`.

Previously, we would always ignore the `last_known_value` field:

```python
x: Final = 3
reveal_type(x)  # N: Revealed type is 'builtins.int'
```

Now, we format it like `Literal[3]?`. Note that we use the question
mark suffix as a way of distinguishing the type from true Literal types.

```python
x: Final = 3
y: Literal[3] = 3
reveal_type(x)  # N: Revealed type is 'Literal[3]?'
reveal_type(y)  # N: Revealed type is 'Literal[3]'
```

While making this change and auditing our tests, I also discovered we
were accidentally copying over the `last_known_value` in a few places
by accident. For example:

```python
from typing_extensions import Final

a = []
a.append(1)
a.append(2)     # Got no error here?
reveal_type(a)  # Incorrect revealed type: got builtins.list[Literal[1]?]

b = [0, None]
b.append(1)     # Got no error here?
reveal_type(b)  # Incorrect revealed type: got builtins.list[Union[Literal[0]?, None]]
```

The other code changes I made were largely cosmetic.

Similarly, most of the remaining test changes were just due to places
where we were doing something like `reveal_type(0)` or
`reveal_type(SomeEnum.BLAH)`.

This changes should help greatly simplify some of the tests I want
to write for #7169 and also help
make a somewhat confusing and implicit part of mypy more visible.
@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Nov 15, 2019

Note: I wasn't 100% sure what the new reveal_type format ought to look like. I was initially thinking of doing things like builtins.int(fallback=Literal[1]) (more explicit) or builtins.int(1) (concise, mimics a runtime call), but eventually tentatively settled on Literal[1]? mostly because it seemed to best balance explicitness with concision.

I'm open to other ideas though.

Copy link
Collaborator

ilevkivskyi left a comment

Thanks! We could bikeshed this for a long time, but I think it is not critical, as all the proposed forms are fine and definitely better than the current one, which hides important details.

Go ahead after updating the Python 3.8 tests!

@@ -138,8 +138,27 @@ the above program almost as if it were written like so:
reveal_type(19)
expects_literal(19)
This is why ``expects_literal(19)`` type-checks despite the fact that ``reveal_type(c)``
reports ``int``.
In other words, the type of ``c`` is *context-dependent*: It could be either ``int``

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 15, 2019

Collaborator

This sentence is a bit repetitive, there is almost the same one paragraph above, but it uses term "context-sensitive" instead of "context-dependent". It is better to be more consistent with terms here.

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Nov 15, 2019

Author Collaborator

I tried editing this section of the docs more heavily. If you have time, could you give the new version a second sanity check?

mypy/checkmember.py Outdated Show resolved Hide resolved
return t.copy_modified(
args=[a.accept(self) for a in t.args],
last_known_value=None,
)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 15, 2019

Collaborator

Good catch! Did you add a test for this?

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Nov 15, 2019

Author Collaborator

Good point -- I initially didn't since we already had several tests unrelated to literal types that were breaking because of this.

But I think it'd be good to exercise this more directly, so I added a test to check-literals.test.

Copy link
Collaborator

ilevkivskyi left a comment

Thank for updates! I double-checked the docs, just couple small suggestions there.

when you try using those types in places that are not explicitly expecting a ``Literal[...]``.
For example, compare and contrast what happens when you try appending these types to a list:

..code-block:: python

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 15, 2019

Collaborator
Suggested change
..code-block:: python
.. code-block:: python
b: Literal[19] = 19

# Mypy will chose to infer List[int] instead of List[Literal[19]?] or
# List[Literal[19]] since the former is most likely more useful.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 15, 2019

Collaborator

Shorten this comment to say just # Mypy will chose to infer List[int] here

@Michael0x2a Michael0x2a merged commit 384f32c into python:master Nov 15, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Michael0x2a Michael0x2a deleted the Michael0x2a:adjust-final-var-reporting branch Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.