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

Attempt to reproduce #91 #92

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Attempt to reproduce #91 #92

wants to merge 2 commits into from

Conversation

kedder
Copy link
Member

@kedder kedder commented Feb 10, 2023

No description provided.

@DMRobertson
Copy link
Contributor

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

@DMRobertson
Copy link
Contributor

Works for me locally:

=================================================== FAILURES ===================================================
________________________________________ test_samples[isinstance_impl] _________________________________________

samplefile = '/home/dmr/workspace/mypy-zope/tests/samples/isinstance_impl.py'
mypy_cache_dir = '/tmp/pytest-of-dmr/pytest-0/.mypy_cahe0'

    def test_samples(samplefile, mypy_cache_dir):
        opts = options.Options()
        opts.cache_dir = 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.
        opts.config_file = '    not_existing_config.ini'
    
        try:
            base_dir = os.path.dirname(samplefile)
            source = BuildSource(samplefile,
                                 module=None,
                                 text=None,
                                 base_dir=base_dir)
            res = build.build(
                sources=[source],
                options=opts)
        except CompileError as e:
            assert False, e
    
        normalized = normalize_errors(res.errors, samplefile)
        actual = '\n'.join(normalized)
        expected = find_expected_output(samplefile)
>       assert actual == expected
E       assert 'isinstance_i...s unreachable' == 'isinstance_i....IFoo, None]"'
E         - isinstance_impl.py:19: note: Revealed type is "Union[__main__.IFoo, None]"
E         + isinstance_impl.py:19: note: Revealed type is "Union[__main__.IFoo, None]"
E         ?                                                                           +
E         + isinstance_impl.py:23: error: Statement is unreachable

tests/test_samples.py:50: AssertionError

@DMRobertson
Copy link
Contributor

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 elif branch: https://github.com/python/mypy/blob/db440ab063c3f01819a29d45e3e2288562f39891/mypy/checker.py#L6708-L6712

        elif not is_overlapping_types(
            current_type, proposed_type, prohibit_none_typevar_overlap=True, ignore_promotions=True
        ):
            # Expression is never of any type in proposed_type_ranges
            return UninhabitedType(), default

In this situation:

  • current_type=Union[isinstance_impl.IFoo, None]
  • proposed_type=isinstance_impl.MyFoo

It seems a bit odd to conclude that # Expression is never of any type in proposed_type_ranges in this situation... but I don't fully understand mypy's logic and reasoning here.

@kedder
Copy link
Member Author

kedder commented Feb 25, 2023

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 ignore_promotions=True parameter. "Promotion" mechanism is how we pretend the implementation is a "kinda subclass" of an interface. We used to inject the interface into class hierarchy previously, so maybe that is why it worked before. Hmm...

@kedder
Copy link
Member Author

kedder commented Mar 13, 2023

@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?

@DMRobertson
Copy link
Contributor

DMRobertson commented Mar 23, 2023

Apologies for the delay in getting back to you. I'm trying this out on matrix-org/synapse#15313, as well as locally.

It looks like this PR fixes all-but-one of the unreachable errors we had when I raised #91. Many thanks for your efforts!

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:

dmr on titan in synapse on  dmr/test-mypy-zope-92 is 📦 v1.80.0rc2 via 🐍 v3.11.2 (matrix-synapse-py3.11) via 🦀 v1.68.0 took 1m8s 
2023-03-23 13:58:55 ✗ 1 ERROR  $ pip install mypy-zope==0.9.0 && rm -r .mypy_cache/ && mypy
Collecting mypy-zope==0.9.0
  Using cached mypy_zope-0.9.0-py3-none-any.whl (32 kB)
Requirement already satisfied: mypy==1.0.0 in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib64/python3.11/site-packages (from mypy-zope==0.9.0) (1.0.0)
Requirement already satisfied: zope.interface in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib64/python3.11/site-packages (from mypy-zope==0.9.0) (5.4.0)
Requirement already satisfied: zope.schema in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib/python3.11/site-packages (from mypy-zope==0.9.0) (6.2.0)
Requirement already satisfied: typing-extensions>=3.10 in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib/python3.11/site-packages (from mypy==1.0.0->mypy-zope==0.9.0) (4.5.0)
Requirement already satisfied: mypy-extensions>=0.4.3 in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib/python3.11/site-packages (from mypy==1.0.0->mypy-zope==0.9.0) (0.4.3)
Requirement already satisfied: setuptools in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib/python3.11/site-packages (from zope.interface->mypy-zope==0.9.0) (65.5.1)
Requirement already satisfied: zope.event in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib/python3.11/site-packages (from zope.schema->mypy-zope==0.9.0) (4.5.0)
Installing collected packages: mypy-zope
  Attempting uninstall: mypy-zope
    Found existing installation: mypy-zope 0.9.1.dev0
    Uninstalling mypy-zope-0.9.1.dev0:
      Successfully uninstalled mypy-zope-0.9.1.dev0
Successfully installed mypy-zope-0.9.0

[notice] A new release of pip is available: 23.0 -> 23.0.1
[notice] To update, run: pip install --upgrade pip
tests/http/test_proxyagent.py:649: error: Statement is unreachable  [unreachable]
tests/http/test_proxyagent.py:768: error: Statement is unreachable  [unreachable]
tests/http/test_proxyagent.py:833: error: Statement is unreachable  [unreachable]
tests/http/test_proxyagent.py:846: error: Statement is unreachable  [unreachable]
tests/http/test_proxyagent.py:854: error: Statement is unreachable  [unreachable]
tests/http/federation/test_matrix_federation_agent.py:152: error: Statement is unreachable  [unreachable]
tests/http/federation/test_matrix_federation_agent.py:471: error: Statement is unreachable  [unreachable]
tests/logging/test_remote_handler.py:61: error: Statement is unreachable  [unreachable]
tests/logging/test_remote_handler.py:95: error: Statement is unreachable  [unreachable]
tests/logging/test_remote_handler.py:130: error: Statement is unreachable  [unreachable]
tests/logging/test_remote_handler.py:156: error: Statement is unreachable  [unreachable]
Found 11 errors in 3 files (checked 793 source files)

After:

dmr on titan in synapse on  dmr/test-mypy-zope-92 is 📦 v1.80.0rc2 via 🐍 v3.11.2 (matrix-synapse-py3.11) via 🦀 v1.68.0 took 6s 
2023-03-23 13:57:09 ✗ 1 ERROR  $ pip install git+https://github.com/Shoobx/mypy-zope.git#fix-unreachable && rm -r .mypy_cache/ && mypy
Collecting git+https://github.com/Shoobx/mypy-zope.git#fix-unreachable
  Cloning https://github.com/Shoobx/mypy-zope.git to /tmp/pip-req-build-8pceb57w
  Running command git clone --filter=blob:none --quiet https://github.com/Shoobx/mypy-zope.git /tmp/pip-req-build-8pceb57w
  Resolved https://github.com/Shoobx/mypy-zope.git to commit 6945ee0ea814724712b75338c1b1967950a78f06
  Preparing metadata (setup.py) ... done
Requirement already satisfied: mypy==1.0.0 in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib64/python3.11/site-packages (from mypy-zope==0.9.1.dev0) (1.0.0)
Requirement already satisfied: zope.interface in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib64/python3.11/site-packages (from mypy-zope==0.9.1.dev0) (5.4.0)
Requirement already satisfied: zope.schema in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib/python3.11/site-packages (from mypy-zope==0.9.1.dev0) (6.2.0)
Requirement already satisfied: typing-extensions>=3.10 in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib/python3.11/site-packages (from mypy==1.0.0->mypy-zope==0.9.1.dev0) (4.5.0)
Requirement already satisfied: mypy-extensions>=0.4.3 in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib/python3.11/site-packages (from mypy==1.0.0->mypy-zope==0.9.1.dev0) (0.4.3)
Requirement already satisfied: setuptools in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib/python3.11/site-packages (from zope.interface->mypy-zope==0.9.1.dev0) (65.5.1)
Requirement already satisfied: zope.event in /home/dmr/.cache/pypoetry/virtualenvs/matrix-synapse-YNqvS2_4-py3.11/lib/python3.11/site-packages (from zope.schema->mypy-zope==0.9.1.dev0) (4.5.0)

[notice] A new release of pip is available: 23.0 -> 23.0.1
[notice] To update, run: pip install --upgrade pip
tests/http/federation/test_matrix_federation_agent.py:152: error: Statement is unreachable  [unreachable]
Found 1 error in 1 file (checked 793 source files)

I suspect it could cause some memory leaking in the long-running dmypy instances

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?)

@clokep
Copy link
Contributor

clokep commented Mar 23, 2023

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. 😢

@DMRobertson
Copy link
Contributor

Erm, I have just reran the commands

pip install git+https://github.com/Shoobx/mypy-zope.git#fix-unreachable && rm -r .mypy_cache/ && mypy

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...)

@kedder
Copy link
Member Author

kedder commented Mar 23, 2023

Erm, I have just reran the commands

pip install git+https://github.com/Shoobx/mypy-zope.git#fix-unreachable && rm -r .mypy_cache/ && mypy

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 pip install -e ../mypy-zope-unreachable (just to be 100% sure it is being picked up).

@DMRobertson
Copy link
Contributor

Hm, you mean changes in this branch doesn't make any difference for you?

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

@kedder
Copy link
Member Author

kedder commented Mar 24, 2023

@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?

@DMRobertson
Copy link
Contributor

@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@f031e40 (#15313). Mypy did not produce any errors for isintance_impl.py, see https://github.com/matrix-org/synapse/actions/runs/4531921476/jobs/7982662739#step:5:12

Comment on lines +15 to +16
class ChildFoo(BaseFoo):
pass
Copy link
Contributor

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.

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.

None yet

3 participants