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

Fix metaclass conflict #90

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Fix metaclass conflict #90

merged 1 commit into from
Feb 10, 2023

Conversation

kedder
Copy link
Member

@kedder kedder commented Feb 9, 2023

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
Copy link
Member Author

kedder commented Feb 9, 2023

@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
Copy link
Contributor

euresti commented Feb 9, 2023

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
Copy link
Member Author

kedder commented Feb 9, 2023

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
Copy link
Contributor

euresti commented Feb 9, 2023

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

@euresti
Copy link
Contributor

euresti commented Feb 9, 2023

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
Copy link
Member Author

kedder commented Feb 9, 2023

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

@kedder
Copy link
Member Author

kedder commented Feb 9, 2023

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

@euresti
Copy link
Contributor

euresti commented Feb 9, 2023

They are in #86

@euresti
Copy link
Contributor

euresti commented Feb 9, 2023

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
Copy link
Member Author

kedder commented Feb 9, 2023

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

@euresti
Copy link
Contributor

euresti commented Feb 9, 2023

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 kedder force-pushed the fix-metaclass-conflict branch from 858bf45 to d06e2ac Compare February 9, 2023 22:10
@kedder
Copy link
Member Author

kedder commented Feb 9, 2023

@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
Copy link
Contributor

euresti commented Feb 9, 2023

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

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 kedder force-pushed the fix-metaclass-conflict branch from d06e2ac to a9954b7 Compare February 10, 2023 06:02
@kedder kedder merged commit 9fc2bd7 into master Feb 10, 2023
@kedder kedder deleted the fix-metaclass-conflict branch February 10, 2023 06:11
@kedder
Copy link
Member Author

kedder commented Feb 10, 2023

Released in mypy-zope-0.9.0

@DMRobertson
Copy link
Contributor

Thank you both for tackling the fallout from #89!

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.

3 participants