-
Notifications
You must be signed in to change notification settings - Fork 13
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
Attempt to reproduce #91 #92
base: master
Are you sure you want to change the base?
Conversation
Try applying diff --git a/tests/test_samples.py b/tests/test_samples.py
index f853009..6111e7e 100644
--- a/tests/test_samples.py
+++ b/tests/test_samples.py
@@ -26,6 +26,7 @@ def test_samples(samplefile, mypy_cache_dir):
opts.show_traceback = True
opts.namespace_packages = True
opts.hide_error_codes = True
+ opts.warn_unreachable = True
opts.plugins = ['mypy_zope:plugin']
# Config file is needed to load plugins, it doesn't not exist and is not
# supposed to. To get the unreachable error |
Works for me locally:
|
I spent a little time trying to track this down by sticking print statements into mypy. I'm a bit suspicious of the fact that mypy decides to return in this
In this situation:
It seems a bit odd to conclude that |
Sorry for lack of attention to this, I was on my vacation for the last couple of weeks. Getting back to the usual rhythm now. And thanks for digging in. What looks alarming in that snippet is |
@DMRobertson I did a pretty crazy hack to work around this (see the diffs). I'm both proud of it and afraid of it at the same time :) I suspect it could cause some memory leaking in the long-running dmypy instances, but it is the best I could come up with so far. Would it be possible to check the fix on your codebase? |
Apologies for the delay in getting back to you. I'm trying this out on matrix-org/synapse#15313, as well as locally.
EDIT: I don't seem able to reproduce that outcome now... not sure what is happening! I haven't had the chance to dig into the remaining error yet; when I get time I'll see if I can reduce that to a minimal example for you. Before:
After:
Afraid I don't have any information to report here: I don't use dmypy myself. I'm not sure if anyone else on the team does either (@clokep maybe?) |
I do not use it. Sorry. 😢 |
Erm, I have just reran the commands
after which I saw the full suite of 11 errors. So I am not sure why I only saw one error in the sample above. Some kind of caching problem? (But I explicitly nuked the cache...) |
Hm, you mean changes in this branch doesn't make any difference for you? I wonder how can we ensure you are actually getting the right branch. Could you check out the branch in a separate directory and install it via |
It looks like it :( I had one mypy run where it seemed to make a difference, but I'm unable to reproduce that---even with an editable install of mypy-zope checked out on this branch. I reduced the test failures I saw in our project to a case. It fails for me on this branch of mypy-zope: do you see the same? diff --git a/tests/samples/dmr_unreachable.py b/tests/samples/dmr_unreachable.py
index e69de29..36f2b13 100644
--- a/tests/samples/dmr_unreachable.py
+++ b/tests/samples/dmr_unreachable.py
@@ -0,0 +1,31 @@
+from typing import Optional
+
+from zope.interface import Interface, implementer
+
+
+class IFoo(Interface):
+ pass
+
+
+@implementer(IFoo)
+class BaseFoo:
+ pass
+
+
+class ChildFoo(BaseFoo):
+ pass
+
+
+class IFooFactory(Interface):
+ def build() -> Optional[IFoo]:
+ pass
+
+
+def build_and_use_foo(client_factory: IFooFactory) -> None:
+ client_protocol = client_factory.build()
+ assert isinstance(client_protocol, ChildFoo)
+ print("Hello")
+
+
+"""
+"""
\ No newline at end of file |
@DMRobertson just for the sake of experiment, could you copy https://github.com/Shoobx/mypy-zope/blob/96518e2725401adb8ecbe177441971666037fd81/tests/samples/isinstance_impl.py into the working directory of your project and run mypy on that file? |
Certainly! This is matrix-org/synapse@ |
class ChildFoo(BaseFoo): | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the "important" difference between this test case and isinstance_impl
. On line 26 below we are asserting that something of type IFoo
could be an instance of a subclass of a class implementing IFoo
.
In isinstance_impl
line 20, we do the same but without the additional subclass.
No description provided.