Shoobx / mypy-zope

Plugin for mypy to support zope.interface
MIT License
38 stars 11 forks source link

Fix metaclass conflict #90

Closed kedder closed 1 year ago

kedder commented 1 year ago

This fixes the regression when #88 combined with #89: mypy-1.0 started producing the error:

Metaclass conflict: the metaclass of a derived class must be a
(non-strict) subclass of the metaclasses of all its bases
kedder commented 1 year ago

@euresti please take a look. I had to remove this line to fix errors that started to appear after upgrade to mypy-1.0.

How important do you think this faketi.metaclass_type = iface.metaclass_type is?

euresti commented 1 year ago

Hi. If you remove this line you bring back the caching issue in #86. Basically in the mypy_cache the typeinfo has the metaclass_type but in the fakeinfo it does not. So you'll only see the errors when you rerun the code.

kedder commented 1 year ago

Can you double-check if that is still the case with mypy-1.0? We don't seem to have a regression test for this.

euresti commented 1 year ago

Yeah. Let me double check. Writing a caching regression tests is pretty hard.

euresti commented 1 year ago

Yeah the caching bug is still there:

$ rm -rf .mypy_cache 
$ mypy foo.py iface.py 
Success: no issues found in 2 source files
$ rm -rf .mypy_cache 
$ mypy iface.py 
Success: no issues found in 1 source file
$ mypy foo.py 
foo.py:3: error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases  [misc]
foo.py:6: error: Metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases  [misc]
Found 2 errors in 1 file (checked 1 source file)
kedder commented 1 year ago

Hm, interesting. So on mypy-1.0, instead of hiding the error, #88 actually exposes it with the cold cache?

kedder commented 1 year ago

@euresti can you share those foo.py and face.py?

euresti commented 1 year ago

They are in https://github.com/Shoobx/mypy-zope/issues/86

euresti commented 1 year ago

So a horrible thing we can do is add a ignored_lines (basically a runtime added # type: ignore) for the error that we know is going to be emitted. The only other options are to figure out how to trick check_metaclass_compatibility to not fail the error.

kedder commented 1 year ago

ignored_lines doesn't sound great, because it could just hide other unrelated errors on that line...

euresti commented 1 year ago

True. This failure doesn't have an error code either, it just says misc.

There's another thing that can be done with is to add impl.metaclass_type = iface.metaclass_type That will prevent the @implementer from triggering a mypy failure.

But if you subclass the implementer you still have issues.

kedder commented 1 year ago

@euresti I came up with this much simpler and less hacky implementation that seems to fix the caching issue and this new error as well. The mypy-zope tests pass, but to be sure, can you test it on your real codebase?

Also, I removed test_mro_computation_in_forward_reference_to_implementer, because I think it doesn't make sense anymore without this mro tweaking that got removed as well. Correct me if I'm wrong here.

euresti commented 1 year ago

Wow. That worked! I did wonder why we were creating a faketi in the first place.

kedder commented 1 year ago

Released in mypy-zope-0.9.0

DMRobertson commented 1 year ago

Thank you both for tackling the fallout from #89!