GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-103092: Port some `_ctypes` data types to heap types by neonene · Pull Request #113630 · python/cpython

Conversation

@neonene

@neonene neonene commented Jan 1, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Currently, PyType_From* functions refuse or ignore creating classes whose metaclass overrides tp_new (#103968):

>>> import ctypes
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
    import ctypes
  File "C:\cp\Lib\ctypes\__init__.py", line 8, in <module>
    from _ctypes import Union, Structure, Array
TypeError: Metaclasses with custom tp_new are not supported.

The ctypes extension needs some workaround for that.

UPDATE: Added TypeError message above.

  • Issue: Isolate Stdlib Extension Modules #103092

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
neonene and others added 2 commits January 2, 2024 03:47
@neonene
PyCSimpleType_new() -> init()
8bb0d97
@neonene
Make Simple_Type a heap type
45795e4
@neonene
neonene requested a review from ericsnowcurrently as a code owner January 1, 2024 20:08
@bedevere-app bedevere-app Bot added the awaiting review label Jan 1, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jan 1, 2024
Isolate Stdlib Extension Modules #103092
Closed
24 of 25 tasks
@neonene

neonene commented Jan 2, 2024

Copy link
Copy Markdown
Contributor Author

@vstinner Could you review this behavior change?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

neonene and others added 8 commits January 2, 2024 15:57
@neonene
reconsider Simple_traverse()
550e467
@neonene
warning
6f005f4
@neonene
non-fix edit
ae9e2f5
@neonene
fixes a critical typo
68d308b
@neonene
base check (for now)
282d5d4
@neonene
cleanup
525de82
@neonene
nit
d92e896
@neonene
convert PyCData_Type family to heap types
b5c2b40
@neonene neonene changed the title gh-103092: Make Simple_Type in ctypes into heap types gh-103092: Port some _ctypes data types to heap types Jan 3, 2024
neonene added 2 commits January 4, 2024 04:48
@neonene
CIFuzz friendly
4a9dfe4
@neonene
minor adjustments
a1752d4
@neonene

neonene commented Jan 5, 2024

Copy link
Copy Markdown
Contributor Author

This is an attempt without hacking type slots.

Problem: It is possible that accessing metaclasses cause a crash with bad arguments (e.g. type(c_int).__init__(...)), which this PR does not tackle.

cc PEP 687 experts: @erlend-aasland @kumaraditya303

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@neonene
drop heap type check
442194a
@erlend-aasland
erlend-aasland requested review from erlend-aasland and kumaraditya303 January 6, 2024 13:43
@neonene neonene mentioned this pull request Jan 6, 2024
gh-103092: Port some _ctypes data types to heap types (alt) #113774
Closed
@neonene

neonene commented Jan 6, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Leaving an alternative PR (#113774) which does not have the problem above. However, tp_new slot is hacked. Is there any better way to isolate _ctypes?

cc @encukou

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Jan 8, 2024

Copy link
Copy Markdown
Member

To make this easier to review -- and to make the issue easier to see for as many reviewers as possible, would you mind sending a PR that adds the *_Type pointers to ctypes_state, but initializes them pointers to the existing static types for now? Also change all the *_Check calls -- those should look the same regardless of how the metaclass issue is solved. The next PR can then focus only on the problematic case.

IMO, the correct way with the current heap type creation API is to move the initialization to tp_init as you do, but ensure that tp_init was called exactly once: that is, all operations should fail cleanly if it was not run on a given type, and it should fail (or do nothing) if called a second time.

Another option is to improve the type creation API -- e.g. design a slot that works like __subclass_init__ but CPython guarantees to call it exactly once.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Jan 8, 2024

Copy link
Copy Markdown
Member

@vstinner Could you review this behavior change?

I would prefer a smaller PR. You moved 7 types. Can you start with a PR which change less types and less files at once?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@neonene neonene mentioned this pull request Jan 9, 2024
gh-103092: Ensure _ctypes.c static types are accessed via global state #113857
Merged
@neonene

neonene commented Jan 10, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@encukou Is it worth opening a new issue for that? Maybe a custom meta tp_init can be checked without switching to a heap type?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented Jan 10, 2024

Copy link
Copy Markdown
Member

Oh, I think it's worth writing a PEP for that :)
If you want to help there, a good first step would be a proof-of-concept PR (without an issue).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@neonene

neonene commented Jan 10, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Sorry, I'm not qualified, as I'm volunteering anonymously.
A fix for issues around #113055 without porting _ctypes is also fine with me.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@neonene
neonene marked this pull request as draft January 19, 2024 01:46
@bedevere-app bedevere-app Bot removed the awaiting review label Jan 19, 2024
@neonene
Merge branch 'main' into ctypes_simple
c80223f
@vstinner

vstinner commented Jan 19, 2024

Copy link
Copy Markdown
Member

Oh, I think it's worth writing a PEP for that :)

Why do you think that a PEP is needed for this specific change? Many static types were converted to heap types in the stdlib without a specific PEP.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jan 19, 2024

Copy link
Copy Markdown
Contributor

Oh, I think it's worth writing a PEP for that :)

Why do you think that a PEP is needed for this specific change? Many static types were converted to heap types in the stdlib without a specific PEP.

I think @encukou meant a PEP for this proposed idea in #113630 (comment) :)

Another option is to improve the type creation API -- e.g. design a slot that works like subclass_init but CPython guarantees to call it exactly once.

vstinner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Jan 19, 2024

Copy link
Copy Markdown
Member

How are ctypes types than other stdlib extensions types? Is there a metaclass? Why do they need special code for __init__()? I don't understand and the PR doesn't say much about it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jan 19, 2024

Copy link
Copy Markdown
Contributor

How are ctypes types than other stdlib extensions types? Is there a metaclass? Why do they need special code for __init__()? I don't understand and the PR doesn't say much about it.

_ctypes is not straight-forward to adapt, contrary to pretty much the rest of the extension modules in the stdlib. There is a lot of metaclass hacks.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Jan 19, 2024

Copy link
Copy Markdown
Member

Ah, PyCStructType_Type is special: it's a "meta type/class" says a comment. If it's special, I suggest to create a dedicated issue to collect more details about this specific type and how it's used. Currently, the PR is linked to a generic "Isolate Stdlib Extension Modules" issue.

pycstruct_type_slots defines the Py_tp_new slot as PyCStructType_new(). This function calls StructUnionType_new() which does the black magic:

/*
  PyCStructType_Type - a meta type/class.  Creating a new class using this one as
  __metaclass__ will call the constructor StructUnionType_new/init.  It replaces
  the tp_dict member with a new instance of StgDict, and initializes the C
  accessible fields somehow.
*/

Extracts:

    /* create the new instance (which is a class,
       since we are a metatype!) */

and:

    /* replace the class dict by our updated stgdict, which holds info
       about storage requirements of the instances */
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Jan 19, 2024

Copy link
Copy Markdown
Contributor

+1 for a dedicated issue; there is a lot of issues connected to the isolation of _ctypes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner mentioned this pull request Jan 19, 2024
Convert _ctypes static types (and the "meta class") to heap types #114314
Closed
@vstinner

vstinner commented Jan 19, 2024

Copy link
Copy Markdown
Member

I created the issue gh-114314 "Convert _ctypes PyCStgDict meta static type to a meta heap type".

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@neonene
sync callbacks.c with main
e817dc2
@vstinner

vstinner commented Jan 19, 2024

Copy link
Copy Markdown
Member

To make this easier to review -- and to make the issue easier to see for as many reviewers as possible, would you mind sending a PR that adds the *_Type pointers to ctypes_state, but initializes them pointers to the existing static types for now? Also change all the *_Check calls

I wrote PR #114316 for that. It's partially based on this PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@neonene

neonene commented Jan 20, 2024

Copy link
Copy Markdown
Contributor Author

Unlike this PR, the issue #114314 seems to consider how to refactor the hacks around PyCStgDict, without customizing tp_new/init. If so, that also sounds reasonable to me.

Any way is fine with me to avoid the error related to #103968:

>>> import ctypes
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
    import ctypes
  File "C:\cp\Lib\ctypes\__init__.py", line 8, in <module>
    from _ctypes import Union, Structure, Array
TypeError: Metaclasses with custom tp_new are not supported.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

neonene added 2 commits January 21, 2024 23:37
@neonene
reduce diff
06f81a0
@neonene
revert globals-to-fix.tsv
5354ffb
@neonene

This comment was marked as outdated.

Sign in to view
neonene and others added 3 commits January 22, 2024 01:56
@neonene
nit
724f893
@neonene
Merge branch 'main' into ctypes_simple
b9d2452
@neonene
Merge branch 'python:main' into ctypes_simple
ea8bc12
@neonene neonene mentioned this pull request Jan 30, 2024
gh-114314: Avoid possibly invalid args when initializing a ctypes meta type #114740
Closed
@neonene

neonene commented Mar 13, 2024

Copy link
Copy Markdown
Contributor Author

Closing the draft in favor of #116458.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@neonene neonene closed this Mar 13, 2024
@neonene
neonene deleted the ctypes_simple branch March 13, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently
@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland
@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@neonene @encukou @vstinner @erlend-aasland
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.