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

Add tests to prevent regressions with the combination of ctypes and metaclasses. #125783

Closed
junkmd opened this issue Oct 21, 2024 · 7 comments
Closed
Labels
tests Tests in the Lib/test dir topic-ctypes type-feature A feature request or enhancement

Comments

@junkmd
Copy link
Contributor

junkmd commented Oct 21, 2024

Feature or enhancement

Proposal:

There were breaking changes to ctypes in Python 3.13.
Projects like comtypes and pyglet, which implement functionality by combining ctypes and metaclasses, no longer work unless their codebases are updated.
We discussed what changes such projects need to make to their codebases in gh-124520.

I believe the easiest and most effective way to prevent regressions from happening again is to add tests on the cpython side.

I wrote a simple test like the one below.

# Lib/test/test_ctypes/test_c_simple_type_meta.py?
import unittest
import ctypes
from ctypes import POINTER, c_void_p

from ._support import PyCSimpleType, _CData, _SimpleCData


class PyCSimpleTypeAsMetaclassTest(unittest.TestCase):
    def tearDown(self):
        # to not leak references, we must clean _pointer_type_cache
        ctypes._reset_cache()

    def test_early_return_in_dunder_new_1(self):
        # this implementation is used in `IUnknown` of comtypes.

        class _ct_meta(type):
            def __new__(cls, name, bases, namespace):
                self = super().__new__(cls, name, bases, namespace)
                if bases == (c_void_p,):
                    return self
                if issubclass(self, _PtrBase):
                    return self
                if bases == (object,):
                    _ptr_bases = (self, _PtrBase)
                else:
                    _ptr_bases = (self, POINTER(bases[0]))
                p = _p_meta(f"POINTER({self.__name__})", _ptr_bases, {})
                ctypes._pointer_type_cache[self] = p
                return self

        class _p_meta(PyCSimpleType, _ct_meta):
            pass

        class _PtrBase(c_void_p, metaclass=_p_meta):
            pass

        class _CtBase(object, metaclass=_ct_meta):
            pass

        class _Sub(_CtBase):
            pass

        class _Sub2(_Sub):
            pass


    def test_early_return_in_dunder_new_2(self):
        # this implementation is used in `CoClass` of comtypes.

        class _ct_meta(type):
            def __new__(cls, name, bases, namespace):
                self = super().__new__(cls, name, bases, namespace)
                if isinstance(self, _p_meta):
                    return self
                p = _p_meta(
                    f"POINTER({self.__name__})", (self, c_void_p), {}
                )
                ctypes._pointer_type_cache[self] = p
                return self

        class _p_meta(PyCSimpleType, _ct_meta):
            pass

        class _Base(object):
            pass

        class _Sub(_Base, metaclass=_ct_meta):
            pass

        class _Sub2(_Sub):
            pass

If no errors occur when this test is executed, we can assume that no regressions have occurred with the combination of ctypes and metaclasses.
However, I feel that this may not be enough. What should we specify as the target for the assert?

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

#124520

Linked PRs

@junkmd junkmd added the type-feature A feature request or enhancement label Oct 21, 2024
@ZeroIntensity ZeroIntensity added the tests Tests in the Lib/test dir label Oct 21, 2024
@encukou
Copy link
Member

encukou commented Oct 22, 2024

Before calling something a “breaking change”, please check that you're not using internal API. Things like this are fragile:

  • using an underscore-prefixed name
  • calling __new__ but not the corresponding __init__
  • instantiating an internal type (one with an underscore-prefixed name/module, even if it's also available as type(something_public))
  • Omitting super().__new__ in an overridden __new__, and the same for __init__

I appreciate that Python's backwards compatibility policy is unclear on some of these techniques; should we clarify it?

I also appreciate that what you're doing isn't possible with public API. I will try to not break your use case (and time permitting, to add supported API). But let's be clear that your warranty is void, so to say. Expect that with some CPython version will break comtypes again, and we'll need to work together to fix things. Ideally, test with alpha & beta releases so we can react as soon as possible -- IMO, that is the most effective way to prevent regressions here :)


All that said, it would definitely be good to add tests like this, to make sure the internals don't change unexpectedly -- especially if people are relying on the internals.

Even better: we could expose some public API for adding things to _pointer_type_cache. Could we solve your use case with a public register_pointer_type function -- or even better, a hook like register_pointer_type_factory, which could create a pointer type on demand (including pointers to pointers, for any level of recursion)?

@junkmd
Copy link
Contributor Author

junkmd commented Oct 22, 2024

As you pointed out, it is true that the implementation of comtypes is much more fragile than packages that rely on other standard Python libraries.
Thomas Heller, the originator of ctypes, was also the originator of comtypes, so I speculate that he used private ctypes APIs with a certain level of caution in the implementation of comtypes (although this is just speculation, as he has retired from CPython).

I don't think we can clearly define a backward compatibility policy for Python here.
I believe we focus on improving ctypes while ensuring that projects depending on it do not break critically.

I agree with transitioning from directly manipulating private elements to using the public API.
However, I would like to first backport this test to the bugfix status versions (3.12 and 3.13), ensuring that this implementation works compatibly with Python 3.14 and beyond.

For now, I think it’s reasonable to continue using _pointer_type_cache, and once the public API is implemented, we can refactor the test to use that instead.

@junkmd
Copy link
Contributor Author

junkmd commented Oct 22, 2024

Would defining a class with a metaclass be sufficient for the test?
For example, would adding assertions for mro, isinstance, and issubclass increase robustness without hindering improvements to the production code?

@junkmd
Copy link
Contributor Author

junkmd commented Oct 23, 2024

I came up with the idea of adding assertions to confirm superclass relationships to prevent projects like comtypes from breaking critically.
For example, ensuring that POINTER(IDispatch) is a subclass of POINTER(IUnknown) and POINTER(Scripting.Dictionary) is a subclass of CoClass.

Even if the registration method for pointer types changes in the future, as long as these assertions don't fail, those projects should continue to work.

@junkmd
Copy link
Contributor Author

junkmd commented Oct 26, 2024

Once the backport PRs (#125987 and #125988) are merged, I plan to add tests for cases where generating pointer types in __init__.

Since this approach cannot be used in 3.12, the backport would be limited to 3.13.

encukou pushed a commit that referenced this issue Oct 28, 2024
…on of `ctypes` and metaclasses. (GH-125881) (#125987)

gh-125783: Add tests to prevent regressions with the combination of `ctypes` and metaclasses. (GH-125881)
(cherry picked from commit 1384409)

Co-authored-by: Jun Komoda <[email protected]>
encukou pushed a commit that referenced this issue Oct 29, 2024
…on of `ctypes` and metaclasses. (GH-125881) (GH-125988)

cherry picked from commit 1384409
by Jun Komoda

Also: Add test_ctypes/_support.py from 3.13+
This partially backports be89ee5
(#113727)
by AN Long


Co-authored-by: Jun Komoda <[email protected]>
Co-authored-by: AN Long <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
@encukou
Copy link
Member

encukou commented Oct 29, 2024

Thank you! The backports are merged now.

encukou pushed a commit that referenced this issue Nov 1, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 1, 2024
…nation of ctypes and metaclasses. (pythonGH-126126)

(cherry picked from commit 6c67446)

Co-authored-by: Jun Komoda <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 1, 2024
…nation of ctypes and metaclasses. (pythonGH-126126)

(cherry picked from commit 6c67446)

Co-authored-by: Jun Komoda <[email protected]>
encukou pushed a commit that referenced this issue Nov 4, 2024
…ination of ctypes and metaclasses. (GH-126126) (GH-126275)

gh-125783: Add more tests to prevent regressions with the combination of ctypes and metaclasses. (GH-126126)
(cherry picked from commit 6c67446)

Co-authored-by: Jun Komoda <[email protected]>
encukou pushed a commit that referenced this issue Nov 4, 2024
…ination of ctypes and metaclasses. (GH-126126) (GH-126276)

gh-125783: Add more tests to prevent regressions with the combination of ctypes and metaclasses. (GH-126126)
(cherry picked from commit 6c67446)

Co-authored-by: Jun Komoda <[email protected]>
@junkmd
Copy link
Contributor Author

junkmd commented Nov 6, 2024

The PRs to add tests that creates and registers pointer types within the __new__ and __init__ methods of the metaclass has been merged.

I believe there is nothing more to do in this issue, so I’ll close it.

There may be some areas that haven’t been tested in terms of using COM with ctypes, and these will be addressed in separate issues, such as gh-126384.

I’m also interested in adding a public API like register_pointer_type, but I plan to first add tests for any untested parts of the public ctypes API currently used in comtypes before tackling that.

@junkmd junkmd closed this as completed Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir topic-ctypes type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants