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

Revert: PyJType for java.util.Map now extends collections.abc.Mapping #503

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

bsteffensmeier
Copy link
Member

The changes in #469 are incompatible with the cpython changes in python/cpython#28748. Testing the current dev_4.2 branch on python 3.12 results in TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases from test_maps and test_builtins.

The problem is that in python java.util.Map is extending collections.abc.Mapping. The metaclass for collections.abc.Mapping is abc.ABCMeta. In Python 3.12 java.util.Map inherits the abc.ABCMeta metaclass. When jep tries to define a type for java.util.AbstractMap, or any other mapping implementation, it extends from java.lang.Object with a metaclass of PyJType but it also extends from the java.util.Map with a metaclass of abc.ABCMeta and as the error mentions you cannot mix these two metaclasses.

In earlier versions of Python the metaclass was not inherited by java.util.Map so the metaclass was simply type, which is compatible with PyJType so jep works. The type for java.util.Map is defined using PySpec_FromType and the documentation for this function helpfully mentions the new incompatibility "Changed in version 3.12: The function now finds and uses a metaclass corresponding to the base classes provided in Py_tp_base[s] slots. Previously, only type instances were returned."

I believe the new behavior of Python is correct, so Jep must be updated to work with newer Python. Similar problems have always existed when using metaclasses in pure python and I have been experimenting with the solutions mentioned on this stackoverflow post which is similar to advice I have found elsewhere. I tried to make a new Metaclass for java.util.Map which combines both PyJType and abc.ABCMeta. However since java.util.Map is defined with a PyType_Spec it is necessary to use the new PyType_FromMetaclass function and my attempts to use that function with a combine metaclass result in TypeError: Metaclasses with custom tp_new are not supported. I think we may be able to get around that error by defining a custom class to act as the parent class to java.util.Map which uses the combined metaclass and is defined by calling type(), without a spec, however even if that compiles and runs the same problem theoretically exists, the tp_new for abc.ABCMeta would not run for the java.util.Map type. I have become convinced that we should not introduce abc types into the type hierarchy for Java types, the mixing of metaclasses is just too complicated.

I think the only reasonable solution is to revert #469. We have not released this feature yet so there is no compatibility issues if we remove it. I'd be happy to try other solutions if anyone else has ideas. I really like the idea of using the collections.abc base classes to increase compatibility between Java and Python with minimal code but I don't see away to make the Metaclasses align. In the future I think we can achieve better interoperability between java.util.Map and dict, by defining the necessary functions(items(), keys(), and values()) in c but I am not confident I will have time to implement that before the release of 3.12 later this year(Contributions are always welcome if someone else is interested in working on this) so for now I am just proposing a reversion. Unfortunately the new PyBuiltins interface introduces in dev_4.2 was relying on this interoperability to create dicts so that functionality has also been removed.

This change reverts #469 and the new code in PyBuiltins that depends on it. With this change the unit tests will pass on Python 3.12

@ndjensen
Copy link
Member

I trust your judgment on this. I enjoyed the increased interoperability of Map with dict, but it's ok to take it out since we hadn't released it yet and it's not compatible with Python. I forgot what the original use case was where we got an error that said something like, "items() is not implemented". Do we just need to implement a few of those methods in C on PyJMap? Maybe I can do that, but unsure on when I can get to it.

@bsteffensmeier bsteffensmeier merged commit 924824e into ninia:dev_4.2 Oct 17, 2023
1 check passed
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.

2 participants