Closed DMRobertson closed 1 year ago
cc @clokep for interest!
@DMRobertson thanks for digging into it. That's a research well done! Given the complexity of the issue, I'd really like to have a regression test for it. I realize it is not easy though. Obviously we don't want to add twisted as a dependency for this.
Something I don't fully understand is why do we need to run mypy on two files to repro this. Is it somehow related to mypy cache?
Something I don't fully understand is why do we need to run mypy on two files to repro this. Is it somehow related to mypy cache?
Good question. I'm not completely sure what's going on: my current hypothesis is that we:
good.py
bad.py
That is: I think the cache exposes the bad MRO but doesn't cause it.
I'll see if I can confirm or deny that hypothesis.
(If it's possible to reproduce this with just one file then that'd make it much easier to test!)
Of course it wouldn't be that difficult to write a test that runs mypy two times on two files (just outside of that "check all files in that directory" mini-framework. The tricky thing here (I think) is to find that minimal test case.
Great work! 👍 I'm not sure why running it on two files is the issue, when I was previously testing #41 I could run it on the same file twice in a row to reproduce...
I'm not sure why running it on two files is the issue, when I was previously testing #41 I could run it on the same file twice in a row to reproduce...
I was building off the report in #76 here. But it seems like just bad.py
is enough to illustrate the problem from a clean cache:
Edit: whoops, I didn't correctly clear my cache or was somehow otherwise confused. I'll investigate further.
Good news: I have a minimal example:
from zope.interface import implementer, Interface
class IProtocol(Interface):
pass
class Factory:
# It seems important for "Protocol" show show up as an attribute annotation to
# trigger the bug(!?)
protocol: "Protocol"
@implementer(IProtocol)
class Protocol:
pass
To detect the bug, one can call result = mypy.build.build(...)
with a single BuildSource representing this file. By inspecting result
we can get our hands on a mypy TypeInfo object, which will let us inspect the MRO that mypy has created. This means we don't have to worry about caches, re-running mypy or investigating multiple files.
I hope to get a test with this together soon.
I hope to get a test with this together soon.
See f341c60. It's a bit rough-and-ready, but hopefully it's enough to serve as a proof of concept.
I'm not sure if this ought to be part of test_samples.py
rather than in its own test file? There's a fair bit of duplication here. I'm happy to clean things up as you see fit. Let me know what you think!
@DMRobertson looks great, thanks! A little duplication in a test is not a big deal, much more important that we have test and won't lose the fix. Will release a new version soon.
Fantastic---many thanks!
For your consideration: a patch which I believe fixes #76. This tackles the same kind of problem that #41 addresses. Seems to be a regression introduced in #75 as part of supporting mypy 0.970.
This turned out to be a bit of a rabbit hole. I've tried to thoroughly reproduce my working here because I really am not confident in my understanding of mypy and mypy-zope. Apologies for the verbosity! (I was very keen to nail this one down.)
Reproduction
I reproduced the problem as reported in #76:
Investigation
To attempt to understand what was going on, I added a bunch of debug
print
statements tomypy
andmypy-zope
until I reached enlightenment. I don't fully grok mypy's machinery, but from what I can tell:linearize_hierarchy
.linearize_heirarchy
of each of our base classes.When I run
mypy bad.py
I see that linear_hierarchy is called forHTTPPageGetter
with the followingTypeInfo
:In particular the MRO contains two copies of
IProtocol
andILoggingContext
, which doesn't seem right (c.f. #41). As I understand it,mypy bad.py
is loading theTypeInfo
forHttpPageGetter
that we persisted at the end ofmypy good.py
. So what was going on when we ran mypy the first time?I will try to summarise a shedload of debug information. At some point mypy computes the MRO for
twisted.internet.protocol.Protocol
. The argument tolinearize_hierarchy
isLater, mypy computes the MRO for a subclass
twisted.protocols.policies.ProtocolWrapper
. Linearize hierarchy receivesand recursively calls itself with the
twisted.internet.protocol.Protocol
TypeInfo
. This time the MRO and Names have already been computed!The MRO looks sensible there.
By the time mypy comes to compute the MRO for
twisted.spread.banana.Banana
(yeah, no idea there) we again lookup the MRO fortwisted.internet.protocol.Protocol
. But the MRO now has duplicate interfaces.Explanation
The following dirty patch adds useful logging.
Dirty patch
```diff diff --git a/src/mypy_zope/plugin.py b/src/mypy_zope/plugin.py index 312c0b5..17fa127 100644 --- a/src/mypy_zope/plugin.py +++ b/src/mypy_zope/plugin.py @@ -364,7 +364,7 @@ def analyze_subinterface(classdef_ctx: ClassDefContext) -> None: def get_customize_class_mro_hook( self, fullname: str ) -> Optional[Callable[[ClassDefContext], None]]: - # print(f"get_customize_class_mro_hook: {fullname}") + print(f"get_customize_class_mro_hook: {fullname}") def analyze_interface_base(classdef_ctx: ClassDefContext) -> None: # Create fake constructor to mimic adaptation signature @@ -406,6 +406,9 @@ def analyze(classdef_ctx: ClassDefContext) -> None: if directiface or subinterface: self._analyze_zope_interface(classdef_ctx.api, classdef_ctx.cls) + self.log(f"get_customize_class_mro_hook ending for {fullname}") + self.log(info.mro) + if fullname == "zope.interface.interface.Interface": return analyze_interface_base @@ -695,10 +698,19 @@ def _apply_interface(self, impl: TypeInfo, iface: TypeInfo) -> None: # there is a decorator for the class that will create a "type promotion", # but ensure this only gets applied a single time per interface. promote = Instance(iface, []) + if impl.fullname == "twisted.internet.protocol.Protocol": + for ti in impl.mro: + self.log(f"DMR HMM: {ti._promote}, {promote}, {ti.fullname}") + self.log(f"DMR HMM: {type(ti._promote)}, {type(promote)}, {ti.fullname}") if not any(ti._promote == promote for ti in impl.mro): faketi = TypeInfo(SymbolTable(), iface.defn, iface.module_name) faketi._promote = [promote] + if impl.fullname=="twisted.internet.protocol.Protocol": + self.log(f"============== APPENDING {iface.defn.fullname} =====") impl.mro.append(faketi) + self.log(f"mro of {impl.fullname}@{hex(id(impl))} ({hex(id(impl.mro))}) is now:") + self.log(impl.mro) + def plugin(version: str) -> PyType[Plugin]: ```We can then see where mypy-zope is adding the duplicate interfaces.
In particular, notice the line:
(which comes from
self.log(f"DMR HMM: {ti._promote}, {promote}, {ti.fullname}")
in the patch). The log lines shows we're comparingti._promote = [Instance(IProtocol)]
withpromote Instance(IProtocol)
. These two things are not equal[^1] therefore theany(...)
expression herehttps://github.com/Shoobx/mypy-zope/blob/5139181a07febfde688af3035d00319942df4dcf/src/mypy_zope/plugin.py#L698
is always False.[^2] So we add a duplicate IProtocol to the end of the MRO.
A similar pattern proceeds for ILoggingContext.
Fix
Check if
promote in ti._promote
for anyti in impl.mro
, now thatti._promote
is a list.I didn't notice a huge performance cost (
rm -r .mypy_cache/3.10/twisted/; mypy good.py; mypy bad.py
takes 2 seconds on my machine with the patch). I haven' tried this on a bigger project like matrix-org/synapse yet. (I'm happy to, but I doubt that anyone is implementing that many interfaces for this to be a problem.)Regression
I think this was introduced in #75. Presumably mypy made
_promote
a list of types rather than just a single type?[^1]: I even checked that
Instance
doesn't define a weird__req__
. [^2]: I'm slightly surprised thatmypy --strict-equality
doesn't catch this!Tests
Works For Me :tm: after applying the change:
make tests
also passes on my machine.I wasn't sure how to add a test for this (apologies). I was uncertain about the following:
reveal_mro
like there isreveal_type
?)