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

Breaking backward compatibility between ctypes and metaclasses in Python 3.13. #124520

Closed
junkmd opened this issue Sep 25, 2024 · 19 comments
Closed
Labels
3.13 bugs and security fixes docs Documentation in the Doc dir topic-ctypes

Comments

@junkmd
Copy link
Contributor

junkmd commented Sep 25, 2024

Documentation

Background

I am one of the maintainers of comtypes. comtypes is based on ctypes and uses metaclasses to implement IUnknown.

It was reported to the comtypes community that an error occurs when attempting to use conventional metaclasses with Python 3.13.

A similar error was encountered in pyglet, which also uses metaclasses to implement COM interfaces, when running on Python 3.13.

By referring to pyglet/pyglet#1196 and pyglet/pyglet#1199, I made several modifications to the code through trial and error, and now comtypes works in both Python 3.13 and earlier versions without problems:

  • https://github.com/enthought/comtypes/compare/a3a8733..04b766a
    • It is necessary to pass arguments directly to the metaclass instead of using __new__ in places where the metaclass is instantiated (i.e., where the class is dynamically defined). This also works in versions prior to Python 3.13.
    • After calling type.__new__, any remaining initialization would be handled in __init__ instead of in __new__. Since this results in an error in versions prior to Python 3.13, a bridge using sys.version_info is necessary.

I think these changes are likely related to #114314 and #117142 and were introduced by the PRs linked to those issues.

Since this change to ctypes breaks compatibility, I think it should be mentioned in the What’s New In Python 3.13 and/or in the ctypes documentation.
There are likely other projects besides comtypes and pyglet that rely on the combination of ctypes and metaclasses, and I want to prevent confusion for those maintainers when they try to support Python 3.13.

(Additionally, I would like to ask with the ctypes maintainers to confirm whether the changes for the metaclasses in comtypes (and pyglet) are appropriate.)

Linked PRs

@junkmd junkmd added the docs Documentation in the Doc dir label Sep 25, 2024
@AA-Turner
Copy link
Member

cc @vstinner @ericsnowcurrently

@encukou
Copy link
Member

encukou commented Sep 25, 2024

Thanks for reporting this! If I knew about this earlier I'd try to make your life easier, but at this point it looks like a docs change is the best way to go.

I'm afraid that by using type(c_void_p) to get to ctypes.PyCSimpleType internals, and by importing _pointer_type_cache, you're getting in the realm of things that can change.
Unfortunately, ctypes has been... stable? neglected? ... for a long time, and these workarounds are common in the wild.
If you can, please do plan to test with 3.14 as the alphas/betas come out, and ping me on any other incompatibilities you might find.

For the diff you linked:

Unfortunately I don't have a Windows box to test on. What error are you getting if you put all of the logic in __init__? (Is it __init__() should return None? Don't return self from __init__.)

If the if/else is needed, I recommend putting the logic in common methods like _new and _init to remove the duplication, so the branch becomes something like:

if sys.version_info >= (3, 13):
    def __new__(cls, ...):
       return cls._new(...)
    def __init__(self, ...):
       self._init(...)
else:
    def __new__(cls, ...):
       self = cls._new(cls, ...)
       self.__init__(...)
       return self

or even:

if sys.version_info >= (3, 13):
    __new__ = _new
    __init__ = _init
else:
    def __new__(cls, ...):
       self = cls._new(cls, ...)
       self._init(...)
       return self

@encukou
Copy link
Member

encukou commented Sep 25, 2024

It is necessary to pass arguments directly to the metaclass instead of using new in places where the metaclass is instantiated (i.e., where the class is dynamically defined).

Hm, I don't understand this point. Do you have an example?

@junkmd
Copy link
Contributor Author

junkmd commented Sep 25, 2024

It is necessary to pass arguments directly to the metaclass instead of using new in places where the metaclass is instantiated (i.e., where the class is dynamically defined).

Hm, I don't understand this point. Do you have an example?

In comtypes, the following part corresponds to this.

https://github.com/enthought/comtypes/compare/c631f97..6f036d4

    meta = type(_safearray.tagSAFEARRAY)
-   sa_type = meta.__new__(
-       meta, "SAFEARRAY_%s" % itemtype.__name__, (_safearray.tagSAFEARRAY,), {}
-   )
+   sa_type = meta(f"SAFEARRAY_{itemtype.__name__}", (_safearray.tagSAFEARRAY,), {})

enthought/comtypes#618 (comment)

@junkmd
Copy link
Contributor Author

junkmd commented Sep 25, 2024

Unfortunately I don't have a Windows box to test on. What error are you getting if you put all of the logic in __init__?

--- a/comtypes/_post_coinit/unknwn.py
+++ b/comtypes/_post_coinit/unknwn.py
@@ -75,7 +75,9 @@ class _cominterface_meta(type):
             new_cls._methods_ = methods
         if dispmethods is not None:
             new_cls._disp_methods_ = dispmethods
+        return new_cls

+    def __init__(self, name, bases, namespace):
         # If we sublass a COM interface, for example:
         #
         # class IDispatch(IUnknown):
@@ -85,23 +87,22 @@ class _cominterface_meta(type):
         # subclass of POINTER(IUnknown) because of the way ctypes
         # typechecks work.
         if bases == (object,):
-            _ptr_bases = (new_cls, _compointer_base)
+            _ptr_bases = (self, _compointer_base)
         else:
-            _ptr_bases = (new_cls, POINTER(bases[0]))
+            _ptr_bases = (self, POINTER(bases[0]))

         # The interface 'new_cls' is used as a mixin.
         p = type(_compointer_base)(
-            "POINTER(%s)" % new_cls.__name__,
+            "POINTER(%s)" % self.__name__,
             _ptr_bases,
-            {"__com_interface__": new_cls, "_needs_com_addref_": None},
+            {"__com_interface__": self, "_needs_com_addref_": None},
         )

         from ctypes import _pointer_type_cache

-        _pointer_type_cache[new_cls] = p
-
-        if new_cls._case_insensitive_:
+        _pointer_type_cache[self] = p

+        if self._case_insensitive_:
             @patcher.Patch(p)
             class CaseInsensitive(object):
                 # case insensitive attributes for COM methods and properties
@@ -155,8 +156,6 @@ class _cominterface_meta(type):

                 CopyComPointer(value, self)

-        return new_cls
-

The following error occurs in Python 3.11 when making the changes mentioned above.

Traceback (most recent call last):
  File "...\Python311\Lib\unittest\__main__.py", line 18, in <module>
    main(module=None)
  File "...\Python311\Lib\unittest\main.py", line 101, in __init__
    self.parseArgs(argv)
  File "...\Python311\Lib\unittest\main.py", line 150, in parseArgs
    self.createTests()
  File "...\Python311\Lib\unittest\main.py", line 161, in createTests
    self.test = self.testLoader.loadTestsFromNames(self.testNames,
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\Python311\Lib\unittest\loader.py", line 220, in loadTestsFromNames
    suites = [self.loadTestsFromName(name, module) for name in names]
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\Python311\Lib\unittest\loader.py", line 220, in <listcomp>
    suites = [self.loadTestsFromName(name, module) for name in names]
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\Python311\Lib\unittest\loader.py", line 154, in loadTestsFromName
    module = __import__(module_name)
             ^^^^^^^^^^^^^^^^^^^^^^^
    from comtypes._post_coinit import _shutdown
  File "...\comtypes\_post_coinit\__init__.py", line 15, in <module>
    from comtypes._post_coinit.unknwn import _shutdown  # noqa
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\comtypes\_post_coinit\unknwn.py", line 370, in <module>
    class _compointer_base(c_void_p, metaclass=_compointer_meta):
  File "...\comtypes\_post_coinit\unknwn.py", line 95, in __init__
    p = type(_compointer_base)(
             ^^^^^^^^^^^^^^^^
NameError: name '_compointer_base' is not defined. Did you mean: '_compointer_meta'?

@encukou
Copy link
Member

encukou commented Sep 25, 2024

Thanks!
I won't be able to debug that by reading the code. I'll follow up next week when I get back to a Windows PC.

@junkmd
Copy link
Contributor Author

junkmd commented Sep 25, 2024

Thank you.

If I knew about this earlier I'd try to make your life easier, but at this point it looks like a docs change is the best way to go.

Fortunately, libraries like comtypes and pyglet were able to adapt with changes to the codebase.
However, if there is a codebase that performs more complex pre-processing before calling super().__new__ within __new__, it might need more complex workarounds.

If it is still possible to modify things so that performing all initialization in __new__ does not result in an error, that would be great.
Alternatively, documenting a workaround for such cases would be helpful for maintainers of those kinds of packages.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 27, 2024
…__ change (pythonGH-124546)

(cherry picked from commit 3387f76)

Co-authored-by: Petr Viktorin <[email protected]>
Yhg1s pushed a commit that referenced this issue Sep 27, 2024
…t__ change (GH-124546) (#124708)

gh-124520: What's New entry for ctypes metaclass __new__/__init__ change (GH-124546)
(cherry picked from commit 3387f76)

Co-authored-by: Petr Viktorin <[email protected]>
junkmd added a commit to junkmd/comtypes that referenced this issue Sep 29, 2024
where it runs successfully on Python 3.13, but a `NameError` occurs
on Python 3.11.

It corresponds to an updated version of the reproducer reported at
python/cpython#124520 (comment).
@junkmd
Copy link
Contributor Author

junkmd commented Sep 29, 2024

@encukou

comtypes codebase has changed.

To reproduce the error that occurs in Python 3.11 by modifying the current comtypes codebase, I recommend using the changes committed to the PoC branch.
(diff is here)

junkmd added a commit to junkmd/comtypes that referenced this issue Sep 29, 2024
where it runs successfully on Python 3.13, but a `NameError` occurs
on Python 3.11.

It corresponds to an updated version of the reproducer reported at
python/cpython#124520 (comment).
junkmd added a commit to junkmd/comtypes that referenced this issue Sep 29, 2024
where it runs successfully on Python 3.13, but a `NameError` occurs
on Python 3.11.

It corresponds to an updated version of the reproducer reported at
python/cpython#124520 (comment).
junkmd added a commit to junkmd/comtypes that referenced this issue Sep 29, 2024
@encukou
Copy link
Member

encukou commented Oct 4, 2024

Hi,
I have no experience with COM, and only have a rudimentary dev/debug setup on Windows. Please check if what I say makes sense :)


It seems the main complexity it the pre-processing is that while an instance of a metaclass is being created, its __new__ (or __init__) function creates another instance of the metaclass. That would normally be recursive, but you are skipping part of normal initialization to avoid that recursion.
The recursive call
I recommend blocking the recursion explicitly, wrapping the metaclass call with something like:

_initializing_coclass_meta = False
class _coclass_meta(type):
    def __init__(self, name, bases, namespace):
        ...
        
        global _initializing_coclass_meta
        if _initializing_coclass_meta:
            return
        try:
            _initializing_coclass_meta = True
            PTR = _coclass_pointer_meta(
                "POINTER(%s)" % self.__name__,
                (self, c_void_p),
                {
                    "__ctypes_from_outparam__": _wrap_coclass,
                    "from_param": classmethod(_coclass_from_param),
                },
            )
        finally:
            _initializing_coclass_meta = False

        ...

…and similarly in _cominterface_meta.
This looks ugly, but IMO it better expresses that a part of the usual initialazation is skipped (and why).
(There might be a better way to detect the recursion?)


In _cominterface_meta, the handling of _methods_ and _disp_methods_ seems to only be there to run the __setattr__ on them. You can move this to __init__:

        # Run code from __setattr__ on _methods_ & _disp_methods_
        methods = namespace.get("_methods_", None)
        if methods is not None:
            self._make_methods(methods)
            self._make_specials()
        dispmethods = namespace.get("_disp_methods_", None)
        if dispmethods is not None:
            self._make_dispmethods(dispmethods)
            self._make_specials()

You can remove the __new__ that only delegates to the superclass:

-    def __new__(cls, name, bases, namespace):
-        return type.__new__(cls, name, bases, namespace)

I haven't yet been able to untangle why _cominterface_meta needs __new__ on 3.11.
Instead of duplicating the code, you can define __new__ for 3.11 but __init__ for 3.13 like this:

class _cominterface_meta(type):
    def __init(self, name, bases, namespace):
        # (put all the init code here)

    if sys.version_info < (3, 13):
        def __new__(cls, name, bases, namespace):
            self = type.__new__(cls, name, bases, namespace)
            self.__init(name, bases, namespace)
            return self
    else:
        __init__ = __init

junkmd added a commit to junkmd/comtypes that referenced this issue Oct 4, 2024
@junkmd
Copy link
Contributor Author

junkmd commented Oct 4, 2024

Hi,

Thank you for your investigation.

_initializing_coclass_meta = False
class _coclass_meta(type):
    def __init__(self, name, bases, namespace):
        ...
        
        global _initializing_coclass_meta
        if _initializing_coclass_meta:
            return
        try:
            _initializing_coclass_meta = True
            PTR = _coclass_pointer_meta(
                "POINTER(%s)" % self.__name__,
                (self, c_void_p),
                {
                    "__ctypes_from_outparam__": _wrap_coclass,
                    "from_param": classmethod(_coclass_from_param),
                },
            )
        finally:
            _initializing_coclass_meta = False

        ...

…and similarly in _cominterface_meta.

Instead of duplicating the code, you can define __new__ for 3.11 but __init__ for 3.13 like this:

class _cominterface_meta(type):
    def __init(self, name, bases, namespace):
        # (put all the init code here)

    if sys.version_info < (3, 13):
        def __new__(cls, name, bases, namespace):
            self = type.__new__(cls, name, bases, namespace)
            self.__init(name, bases, namespace)
            return self
    else:
        __init__ = __init

it definitely worked in Python 3.13 and earlier versions.
(I modified the version bridge to use if sys.version_info >= (3, 13):.)
(You can see also enthought/comtypes@00cf639^...3ef727c, https://github.com/enthought/comtypes/actions/runs/11183030620)

But,

This looks ugly, but IMO it better expresses that a part of the usual initialazation is skipped (and why).
(There might be a better way to detect the recursion?)

As you mentioned, using global and try...finally... to modify a module-level signal variable from within a method seems like stranger code compared to defining _new and _init and switching how they are called using a version bridge (like in enthought/comtypes@6c24dcf^...ea9d9fc).

Until we come up with a more elegant way to signal the stopping of recursion, I believe it might be better to add a comment like # This stops excessive recursion in addition to the implementation like enthought/comtypes@6c24dcf^...ea9d9fc.

I would be happy to hear any further thoughts you may have.

@junkmd
Copy link
Contributor Author

junkmd commented Oct 5, 2024

I investigated what values are assigned to self, name, bases, and namespace when using the global variable _initializing_..._meta as a signal for early returns.
Based on that, I modified the codebases to perform early returns depending on which subclass/instance self is and the components of bases, which worked well.
This approach eliminates the need to define __init__ and version bridges.

These changes have been committed and pushed to enthought/comtypes@4c779617^...312e268 (https://github.com/junkmd/comtypes/tree/py313_early_returns).

This change makes explicit what the metaclass would have done implicitly until now.
Since it is not intuitive and requires multiple code jumps to follow the flow of processing, I added some comments to aid understanding.

The changes using version bridges also has been committed to enthought/comtypes@4c779617^...6501fd1 (https://github.com/junkmd/comtypes/tree/py313_version_bridges).

Any opinions would be appreciated.

@encukou
Copy link
Member

encukou commented Oct 7, 2024

That sounds reasonable.
My main point was that you only want to skip only the recursion (i.e. registering the pointer class), but not any other step of the initialization. Doing that by skipping an entire __new__ or __init__ of a particular base is fragile, and confusing to readers.
An early return before your customizations is the way to go; as for the conditions for it, you're better suited to define them :)

(That said, I hope the details of the initialization won't need to change for another decade and so the fragility will be just theoretical from now on. Again, I'd appreciate if you test with 3.14 alphas/betas as they come out.)

One more possibility to consider would be catching the NameError raised when accessing _compointer_base early, e.g.:

        try:
            is_compointer = issubclass(self, _compointer_base)
        except NameError:
            # On some versions of Python, `_compointer_base` is not
            # yet available in the accessible namespace at this
            # point in its initialization.
            # In this case, `self` will become `_compointer_base`
            # later, so `issubclass(self, _compointer_base)`
            # will be True.
            is_compointer = True

            # Double-check that self is actually _compointer_base.
            assert bases == (c_void_p,)

        if is_compointer:
            # `self` is `POINTER(interface)` type.
            # Prevent registering a pointer to a pointer (to a pointer...),
            # which would lead to infinite recursion.
            # Depending on a version or revision of Python, this may be essential.
            return self

(Not tested; I didn't boot Windows today.)

@encukou
Copy link
Member

encukou commented Oct 7, 2024

I'll close the issue as there's nothing more to do in CPython code/docs.

@encukou encukou closed this as completed Oct 7, 2024
junkmd added a commit to junkmd/comtypes that referenced this issue Oct 7, 2024
@junkmd
Copy link
Contributor Author

junkmd commented Oct 7, 2024

Thank you for your feedback.

That sounds reasonable.
My main point was that you only want to skip only the recursion (i.e. registering the pointer class), but not any other step of the initialization. Doing that by skipping an entire __new__ or __init__ of a particular base is fragile, and confusing to readers.
An early return before your customizations is the way to go; as for the conditions for it, you're better suited to define them :)

It's very reassuring to hear something like this from the maintainer of the library.

(That said, I hope the details of the initialization won't need to change for another decade and so the fragility will be just theoretical from now on. Again, I'd appreciate if you test with 3.14 alphas/betas as they come out.)

When Python 3.14 alphas/betas is released, I plan to create a repository has a GitHub Actions workflow that will run tests of comtypes on those versions.
I’m considering sharing those URLs to make information dissemination easier.

One more possibility to consider would be catching the NameError raised when accessing _compointer_base early, e.g.:

I have confirmed that the code you proposed above also works correctly.
(enthought/comtypes@464f566, https://github.com/enthought/comtypes/actions/runs/11215471734)
However, since _com..._meta and _com..._base defined in comtypes._post_coinit.unknwn are intended to be used only for defining classes for IUnknown or POINTER(interface), I believe that as long as if bases is (c_void_p,), it should be safe to assume that self is _compointer_base. The try...except... for the NameError seemed a bit excessive to me.
That said, your comments on the code base would be very helpful for readers in understanding the process flow whether with or without catching the NameError. I will definitely take them into account. Thank you.

@junkmd
Copy link
Contributor Author

junkmd commented Oct 7, 2024

I'll close the issue as there's nothing more to do in CPython code/docs.

Thank you very much again.

I am also considering submitting another issue/PR to cpython to add tests to ensure that no regressions occur in metaclass implementations similar to those used in comtypes.
If you have any thoughts, I'd love to hear them.

@encukou
Copy link
Member

encukou commented Oct 7, 2024

I am also considering submitting another issue/PR to cpython to add tests to ensure that no regressions occur in metaclass implementations similar to those used in comtypes.

Thank you!
You can have them refer to this closed issue (gh-124520:). If you send a PR, please comment here so I'll notice it.

@junkmd
Copy link
Contributor Author

junkmd commented Oct 21, 2024

@encukou

I have created a PoC repository to verify whether comtypes works with development or pre-release versions of Python.
https://github.com/junkmd/comtypes-python-dev-compatibility/

In the above repository, I have confirmed that the combination of comtypes and 3.14.0-alpha.1 works.
https://github.com/junkmd/comtypes-python-dev-compatibility/actions/runs/11438904158/job/31821475344

@junkmd
Copy link
Contributor Author

junkmd commented Oct 21, 2024

@encukou

I am also considering submitting another issue/PR to cpython to add tests to ensure that no regressions occur in metaclass implementations similar to those used in comtypes.

Thank you! You can have them refer to this closed issue (gh-124520:). If you send a PR, please comment here so I'll notice it.

Please see this: #125783

@junkmd
Copy link
Contributor Author

junkmd commented Oct 24, 2024

... and see also #125881

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes docs Documentation in the Doc dir topic-ctypes
Projects
Development

No branches or pull requests

3 participants