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

Fix incorrect tracking of "final" Instances #6763

Merged

Conversation

@Michael0x2a
Copy link
Collaborator

commented May 4, 2019

This diff makes three changes: it fixes a bug where we incorrectly track "final" Instances, does some related refactoring, and finally modifies tuple indexing to be aware of literal contexts.

Specifically, here is an example of the bug. Note that mypy ignores the mutable nature of bar:

def expect_3(x: Literal[3]) -> None: ...

foo: Final = 3
bar = foo
for i in range(10):
    bar = i

# Currently type-check; this PR makes mypy correctly report an error
expect_3(bar)

To fix this bug, I decided to adjust the variable assignment logic: if the variable is non-final, we now scan the inferred type we try assigning and recursively erase all set instance.final_value fields.

This change ended up making the in_final_declaration field redundant -- after all, we're going to be actively erasing types on non-final assignments anyways. So, I decided to just remove this field.

I suspect this change will also result in some nice dividends down the road: defaulting to preserving the underlying literal when inferring expression types will probably make it easier to add more sophisticated
literal-related inference down the road.

In the process of implementing the above two, I discovered that "nested" Instance types are effectively ignored. So, the following program does not type check, despite the Final and despite that tuples are
immutable:

bar: Final = (3, 2, 1)

# 'bar[0] == 3' is always true, but we currently report an error
expect_3(bar[0])

This is mildly annoying, and also made it slightly harder for me to verify my changes above, so I decided to modify visit_index_expr to also examine the literal context.

(Actually, I found I could move this check directly into the 'accept' method instead of special-casing things within visit_index_expr and analyze_var_ref. But I decided against this approach: the special-casing feels less intrusive, easier to audit, and slightly more efficient.)

Refine how Literal and Final interact
This diff makes three changes: it fixes a bug where we incorrectly
track "final" Instances, does some related refactoring, and finally
modifies tuple indexing to be aware of literal contexts.

Specifically, here is an example of the bug. Note that mypy ignores
the mutable nature of `bar`:

    def expect_3(x: Literal[3]) -> None: ...

    foo: Final = 3
    bar = foo
    for i in range(10):
        bar = i

    # Currently type-check; this PR makes mypy correctly report an error
    expect_3(bar)

To fix this bug, I decided to adjust the variable assignment logic: if
the variable is non-final, we now scan the inferred type we try assigning
and recursively erase all set `instance.final_value` fields.

This change ended up making the `in_final_declaration` field redundant
-- after all, we're going to be actively erasing types on non-final
assignments anyways. So, I decided to just remove this field.

I suspect this change will also result in some nice dividends down the
road: defaulting to preserving the underlying literal when inferring
expression types will probably make it easier to add more sophisticated
literal-related inference down the road.

In the process of implementing the above two, I discovered that "nested"
Instance types are effectively ignored. So, the following program does
not type check, despite the `Final` and despite that tuples are
immutable:

    bar: Final = (3, 2, 1)

    # 'bar[0] == 3' is always true, but we currently report an error
    expect_3(bar[0])

This is mildly annoying, and also made it slightly harder for me to
verify my changes above, so I decided to modify `visit_index_expr` to
also examine the literal context.

(Actually, I found I could move this check directly into the 'accept'
method instead of special-casing things within `visit_index_expr` and
`analyze_var_ref`. But I decided against this approach: the
special-casing feels less intrusive, easier to audit, and slightly
more efficient.)
@ilevkivskyi
Copy link
Collaborator

left a comment

I like this idea, here are some thoughts/questions:

  • Documentation for final_value should be updated to mention this is something like last known value. Maybe even rename it to last_known_value?
  • Could this cause troubles with invariance? Maybe add some tests just in case
  • IIRC final_value is used by mypyc, will it be still safe with the new semantics? We might need to add some tests to mypyc about this.

Michael0x2a added some commits May 7, 2019

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

commented May 7, 2019

@ilevkivskyi -- I renamed the field as suggested.

One complication of the rename is that it potentially invalidates any existing mypy caches -- we now look for the "last_known_value" entry in the cache, instead of "final_value".

Maybe this is too disruptive, especially during the pycon sprints? If you want, I can partially revert the changes I made to the Instance deserialization/serialization methods so they continue using "final_value" and submit a PR for those two methods closer to the next mypy release.

Regarding invariance -- I don't think it'll cause an issue because we always erase this field when binding Instances to a TypeVar. I think the existing testLiteralFinalPropagatesThroughGenerics test checks for this?

I also added testLiteralWithFinalPropagationIsNotLeaking which checks this for lists, dicts, and sets, and modified testLiteralFinalCollectionPropagation just now to do something similar.

Or maybe you were thinking of something else regarding invariance?

Regarding mypyc -- I'll start work on a PR to make any necessary changes now.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2019

One complication of the rename is that it potentially invalidates any existing mypy caches -- we now look for the "last_known_value" entry in the cache, instead of "final_value".

I don't think it is something worth worrying about, mypy normally ignores the cache from another mypy version anyway.

Or maybe you were thinking of something else regarding invariance

Nothing specific, just wanted to double-check that we have a good coverage for this.

Regarding mypyc -- I'll start work on a PR to make any necessary changes now.

OK, I think this can be merged now, because mypyc uses a pinned mypy commit hash. So your PR to mypyc should combine a pin move plus the related updates to mypyc code.

@ilevkivskyi ilevkivskyi merged commit 28815f1 into python:master May 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Michael0x2a added a commit to Michael0x2a/mypy that referenced this pull request May 7, 2019

Clean up last traces of 'final_value' field
This pull request is a follow-up to python#6763:
it renames some parameter names and variables that got missed up above.

One interesting side-note: it seems like 'Var' notes also contain a
`final_value` field that does almost the same thing as this
`last_known_value` field, but is ultimately unrelated: it was introduced
back in python#5522. I decided to leave
this field alone.

(I discovered this while updating mypyc and discovered (to my surprise)
that nothing broke.)

ilevkivskyi added a commit that referenced this pull request May 8, 2019

Clean up last traces of 'final_value' field (#6791)
This pull request is a follow-up to #6763:
it renames some parameter names and variables that got missed up above.

One interesting side-note: it seems like 'Var' notes also contain a
`final_value` field that does almost the same thing as this
`last_known_value` field, but is ultimately unrelated: it was introduced
back in #5522. I decided to leave
this field alone.

(I discovered this while updating mypyc and discovered (to my surprise)
that nothing broke.)
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.