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

Inconsistent behavior depending on cache -- dict.get returning Union of key and value types #6509

Closed
pdeblanc opened this issue Mar 5, 2019 · 13 comments
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal topic-incremental

Comments

@pdeblanc
Copy link

pdeblanc commented Mar 5, 2019

Python 3.7.2, mypy 0.670 (I also tried 70565cd, which was the most recent commit on master at the time of my testing).

File foo.py:

from typing import Dict, Generic, TypeVar

Key = TypeVar('Key')

class A(Generic[Key]):
    @staticmethod
    def lookup(d: Dict[Key, int], key: Key) -> None:
        x = d.get(key, 0) + 1

x = 1 + "a"

The first time running mypy foo.py produces the expected output:

foo.py:10: error: Unsupported operand types for + ("int" and "str")

Now running mypy foo.py a second time produces a spurious error:

foo.py:8: error: Unsupported operand types for + ("Key" and "int")
foo.py:8: note: Left operand is of type "Union[int, Key]"
foo.py:10: error: Unsupported operand types for + ("int" and "str")
@gvanrossum gvanrossum added the bug mypy got something wrong label Mar 5, 2019
@gvanrossum
Copy link
Member

Confirmed on latest master. @ilevkivskyi probably knows what this is a duplicate of, though. :-)

@ilevkivskyi
Copy link
Member

I think this is actually something new. (And it looks like a pure mystery.)

@gvanrossum
Copy link
Member

@pdeblanc Is there an earlier mypy version where this was not broken?

@pdeblanc
Copy link
Author

pdeblanc commented Mar 6, 2019

I'm not sure.

@joshuawu
Copy link

I performed a git bisect. In the context of the @pdeblanc's repro in the OP, the first "bad" commit was the one enabling the module cache by default:

6b13652a466ccb102987a2ab1fa93d4b52fd8e3f is the first bad commit
commit 6b13652a466ccb102987a2ab1fa93d4b52fd8e3f
Author: Michael J. Sullivan <sully@msully.net>
Date:   Thu Apr 5 10:12:12 2018 -0700

    Make incremental mode the default (#4800)

:040000 040000 3ee965e31b88bd104fdd214e3c8dcba5aeb741ec 0809c2d4429a2998544b21816fa96de58673b4f4 M	docs
:040000 040000 8f5d2fbe8e9311b0e02413f5ef5e6e5fe67c9a8e 58347b30d38225744f5c4d85b0cbdabf4b530ae4 M	mypy
bisect run success

So I turned on --incremental for the older commits. Now I found this:

a2d654bbbbc94e6e3199001f9a0a95453f56fe85 is the first bad commit
commit a2d654bbbbc94e6e3199001f9a0a95453f56fe85
Author: Ivan Levkivskyi <levkivskyi@gmail.com>
Date:   Tue Feb 7 13:31:02 2017 +0100

    Fix meta level on member access (#2809)

    Fixes #2804

    The problem is that analyze_member_access calls freshen_function_type_vars
    if the member is a function (i.e. method). This is necessary to not mix the type
    variables during type inference of generic methods inside generic functions.
    However, if the method does not participate in type inference, type variables
    are left in meta_level = 1 state. This causes the error, since the types could not
    be serialized in the middle of type inference.

    In this PR I propose to restore meta_level = 0 on member access, this is safe,
    since anyway, check_call always calls freshen_function_type_vars on callee (for
    generic functions, generic methods, etc).

:040000 040000 8cfdb718a8acdd9e18a5df24c856bf3588851068 3e6ee42769347875a0863d3eb9fa14edf512bdf4 M	mypy
:040000 040000 3c3d4a7330dace291e7dc037550a3297a8d9263b dc3a0a109c82ed6304929c2d81e1b7fd630fb026 M	test-data
bisect run success

I double-checked that these are indeed the offending lines:

diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py
index 23ce56f..a5679f0 100644
--- a/mypy/checkexpr.py
+++ b/mypy/checkexpr.py
@@ -993,6 +993,13 @@ class ExpressionChecker(ExpressionVisitor[Type]):
                 e.name, original_type, e, is_lvalue, False, False,
                 self.named_type, self.not_ready_callback, self.msg,
                 original_type=original_type, chk=self.chk)
+            if isinstance(member_type, CallableType):
+                for v in member_type.variables:
+                    v.id.meta_level = 0
+            if isinstance(member_type, Overloaded):
+                for it in member_type.items():
+                    for v in it.variables:
+                        v.id.meta_level = 0    // This line causes the bug in the OP.
             if is_lvalue:
                 return member_type
             else:

On master (a64278411d9c9cc6edc3e8a183714e726b0d2e05 as of this writing), the corresponding code has been moved to mypy/checkmember.py:539#freeze_type_vars, called once on :189 in the same file. If you delete mypy/checkmember.py:189, OP's bug disappears.

However, the purpose of the original commit was to fix this issue from Feb 4, 2017. Deleting mypy/checkmember.py:189 resurrects the crash @gvanrossum described in the issue. (I was able to repro.)

In the PR thread of this commit, @JukkaL expressed concerns that sound relevant to me, though I have no idea what's going on (I've never used mypy).

@JukkaL: If we just switch the meta level, I think that we can get an id conflict between type variables in the 0 level if we happen to be in a generic function which has some additional type variables in scope.

He changed his mind about this concern and approved the PR, but it seems like he was right after all. I suppose that a fix will need to deal with the id conflicts.

@gvanrossum
Copy link
Member

gvanrossum commented Mar 22, 2019 via email

@joshuawu
Copy link

No problem!

@joshuawu
Copy link

This is probably not directly related, but I noticed that these type definitions seem incorrect:

test-data/unit/fixtures/dict.pyi:29:    def get(self, k: KT, default: Union[KT, T]) -> Union[VT, T]: pass
test-data/unit/fixtures/floatdict.pyi:51:    def get(self, k: KT, default: Union[KT, T]) -> Union[VT, T]: pass
test-data/unit/fixtures/floatdict_python2.pyi:51:    def get(self, k: KT, default: Union[KT, T]) -> Union[VT, T]: pass

I think the default parameter should be T instead of Union[KT, T].

@gvanrossum
Copy link
Member

No, we tried that, and it broke stuff. See python/typeshed#2817

@joshuawu
Copy link

What about Union[VT, T] instead of Union[KT, T]? I’m confused as to why KT is in the type of default.

@gvanrossum
Copy link
Member

You must be misreading something. The code restored in python/typeshed#2817 uses (essentially) Union[VT, T].

@joshuawu
Copy link

Got it, perhaps I was looking at some older commit.

@erictraut
Copy link

This bug appears to have been fixed at some point. The latest version of mypy produces consistent results when running multiple times on the code sample.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal topic-incremental
Projects
None yet
Development

No branches or pull requests

6 participants