Do not add the same interface multiple times to the MRO.

[clokep]

This fixes #34 for me. When taking a look at the zope logs I noticed that IProtocol was getting added multiple times to the MRO, which seems wrong.

Implementation-wise, this attempts to only add a single instance of an interface into the MRO. In order for the comparison check to work (to see if the interface is already in the MRO) this adds a cache to only generate a single TypeInfo for each interface.

I don't know the internals of mypy so maybe this is incorrect and we should be checking a different way?

I suspect that this will work for most cases, but some odd diamond-inheritance patterns might break as the MRO might end up in the wrong order. I.e. this ensure that only a single instance of IFoo exists in the MRO, but I'm not confident it ends up in the proper place. 😄

I attempted to make a reduced test-case of this again but was unable to. 😢 I think it has something to do with:

• Having an interface (or two).
• Having a base class that implements part of that interface, but isn't marked as an implementer.
• Having a class that is an implementer.
• Having a sub-class of the implementer.

Additionally the result needs to be cached so I think this needs to be imported from another package / module and isn't part of what is being "inspected", but I'm not 100% sure about this.

[clokep]

Arg...after a bit more testing this doesn't seem to fix the issue in Synapse, only the reduced test-case. 😢

It is also worth mentioning that if the "bad" values are cached from before this change they'll still be there. Hopefully a new version of mypy-zope would cause the caches to be invalidated?

[kedder]

Wow, this actually fixes the problem our project has! (assuming you start from cold caches, which is understandable).

I very much suspect that the reason it is only reproducible with warm mypy cache is the distinction between object instances. Since mypy-zope generates a new TypeInfo instance each time it adds a type to MRO (talking about a version in master branch now), those instances are perceived as "different" (and not causing MRO problem). Once they are stored to cache and loaded back, they might end up as identical instances (I hope this makes sense). I guess It might be difficult to make a unit test for that.

Anyways, the fix looks great. The only thing that bothers me is that we keep references to all interfaces in the plugin itself and relying on that dict to contain everything when we handle @implementer. When mypy caches are warm, _interfaces dict might not be fully populated and we might still be adding duplicates to the MRO. I wonder if we could look up the interface TypeInfo that is known to mypy through self._lookup_type().

[kedder]

Ok, disregard what I have said about self._lookup_type() - it seems that the actual type we store in faketi doesn't really matter. What matters (and determines if assignment to interface will be valid) is _promote attribute.

So this stateless version of a check works too in my case:

            promote = Instance(iface_type, [])
            if not any(ti._promote == promote for ti in class_info.mro):
                faketi = TypeInfo(SymbolTable(), iface_type.defn, iface_type.module_name)
                faketi._promote = promote
                class_info.mro.append(faketi)

It looks a bit less performant due to iteration over MRO, but I did not notice any regression on our quite large project.

Can you check if it solves the issue in Synapse?

[clokep]

So this stateless version of a check works too in my case:

That does seem like a better fix! 👍 It seems to fix the issues we're having in Synapse too. 🎉 I've pushed a commit to this branch with your changes (hope that's OK!)

[kedder]

Awesome, thanks for figuring out the root of the problem!

[kedder]

I'll do a release later today.

[clokep]

Thanks for making this package! 👍 We would certainly appreciate a release.

[kedder]

mypy-zope-0.2.13 is out: https://pypi.org/project/mypy-zope/

[clokep]

Thank you so much!

[graingert]

@kedder @clokep I'm still getting this issue in twisted on mypy-zope==0.2.13

mypy run-test: commands[0] | mypy --cache-dir=/home/graingert/projects/twisted/.tox/mypy_cache --pretty src
src/twisted/trial/_dist/test/test_worker.py:35:1: error: Cannot determine consistent method resolution order (MRO) for "FakeAMP"  [misc]
    class FakeAMP(AMP):
    ^
src/twisted/trial/_dist/test/test_worker.py:275:1: error: Cannot determine consistent method resolution order (MRO) for "FakeAMProtocol"  [misc]
    class FakeAMProtocol(AMP):
    ^
src/twisted/trial/_dist/test/test_workertrial.py:27:1: error: Cannot determine consistent method resolution order (MRO) for "FakeAMP"  [misc]

[kedder]

@graingert did you clean the mypy cache after upgrade to 0.2.13?

[graingert]

Hmm, I didn't as it worked the first time then started happening after a couple of runs, I'll investigate further

[kedder]

The case that we've fixed here was reproducible this way:

• Clean the mypy cache, run mypy - it passes without errros
• Run mypy again without changing anything - MRO error appears

The bad MRO was stored in the cache, so simply upgrading the mypy_zope isn't enough - you have to clean the cache.

After cleaning (and rebuilding) the cache the error should not reappear. At least that what happened to the case I was experiencing. Maybe yours is different, but we have to be able to repro it somehow in order to fix.

[graingert]

I'm trying to repro now after wiping everything. Previously I was having a problem with all @implementer classes but in my last few runs it was only AMP classes

[graingert]

here's my mypy cache:

mypy_cache.zip

[graingert]

here's a recording: https://asciinema.org/a/406776 failure at 2:48

[graingert]

@clokep @kedder a smaller repro: https://github.com/graingert/amp-mypy-zope-demo/tree/default

[graingert]

I think it's the IResponderLocator twice:

                                'mro': ['amp_mypy_zope_demo.bar.HakeAMP',
                                        'amp_mypy_zope_demo.foo.AMP',
                                        'amp_mypy_zope_demo.foo.CommandLocator',
                                        'amp_mypy_zope_demo.foo.SimpleStringLocator',
                                        'builtins.object',
                                        'amp_mypy_zope_demo.foo.IResponderLocator',
                                        'amp_mypy_zope_demo.foo.IResponderLocator'],

[kedder]

Interesting. I'll try it out.

[clokep]

Might be best to file a new issue (or re-open the old one)?

[graingert]

@clokep @kedder new issue here: #49

[DMRobertson]

Things might have changed a bit since this PR, but I'm a bit confused by how this works.

_promote seems to be List[Instance] (on +258); mypy describes it as List[mypy.types.Types]. But on +256 we compare it to a standalone Instance, not enclosed in a list. So isn't the equality always going to be False?

[DMRobertson]

_promote seems to be List[Instance] (on +258); mypy describes it as List[mypy.types.Types].

Ahh, sorry, I'm commenting on too-old a change. It looks like this changed in #75, see https://github.com/Shoobx/mypy-zope/blame/d3896c754a961fd36797df2bacb710e3b4ec69f8/src/mypy_zope/plugin.py#L685-L701

However I still think that with that change the comparison always returns False?