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

gh-121654: Add Py_tp_create_callback slot #124789

Closed
wants to merge 2 commits into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 30, 2024

@vstinner
Copy link
Member Author

cc @zooba

@zooba
Copy link
Member

zooba commented Sep 30, 2024

Yep, this is the idea I had in mind. It looks weird until you remember that we intend for the spec to be static, which means each type will have its own callback that knows how to add additional members. You could use the type object to look up dynamic state, but that's not the intent of the entire API to begin with.

@AraHaan
Copy link
Contributor

AraHaan commented Sep 30, 2024

I honestly like this PR, and the PyType_Freeze function PR as well.

@encukou
Copy link
Member

encukou commented Oct 2, 2024

It's not clear what kind of restrictions are in place for the callback. For example, are you allowed to instantiate the type in the callback? (I imagine a singleton class could want to instantiate itself and set tp_new to NULL; what's keeping users from doing that?)
Whatever restrictions would be added can also be added to the code between PyType_From* and PyType_Freeze. The only advantage here is that PyType_From* knows the future value of Py_TPFLAGS_IMMUTABLETYPE; with PyType_Freeze we'll need another flag (or a different mechanism) if/when we need that.

Arbitrary code between PyType_From* and PyType_Freeze has the advantage of access to any local variables the user might have around as state. In general, a callback without a void* arg raises a red flag.

we intend for the spec to be static

That's most common, but they could also be filled dynamically and live on the stack. (And so you can't get a spec back from a type.)

@zooba
Copy link
Member

zooba commented Oct 2, 2024

It's not clear what kind of restrictions are in place for the callback. For example, are you allowed to instantiate the type in the callback?

Yeah, it's a good point, and it probably would be good to allow that if it's doable (at great risk of getting attribute errors in any code that may run). It's why my alternate proposals focused around working with the type dict, rather than the type object itself.

Arbitrary code between PyType_From* and PyType_Freeze has the advantage of access to any local variables the user might have around as state. In general, a callback without a void* arg raises a red flag.

Yeah, agreed, which is why I preferred providing an array of struct {const char *name; PyObject *value} in the slot that would be inserted into the type's dict.

The callback pointer could become a struct with a pointer and a userdata void*, but now we're likely getting to three layers of indirection for anyone to use it. The chances of messing that up are probably about equal with remembering to Py_DECREF everything in a list of PyObject, but I still think either makes it easier to avoid obscure semantic issues compared to being able to PyType_Freeze an arbitrary object at any arbitrary time.

@vstinner
Copy link
Member Author

vstinner commented Oct 2, 2024

Arbitrary code between PyType_From* and PyType_Freeze has the advantage of access to any local variables the user might have around as state. In general, a callback without a void* arg raises a red flag.

Honestly, I prefer PyType_Freeze() API. It's more natural and convenient, and I'm not convinced that Py_tp_create_callback brings much forward compatibility or is more "future proof". Well, that's my opinion :-)

@zooba
Copy link
Member

zooba commented Oct 2, 2024

I'm not convinced that Py_tp_create_callback brings much forward compatibility or is more "future proof".

It means we can change the way the type is constructed or allocated based on the fact that it's going to be immutable.

It also avoids adding a new API, which proves the forwards-compatibility of _FromSpec. Having to add a totally new API just means that one we thought was forwards-compatible actually wasn't, and so we need to make backwards-incompatible changes (modules that use PyType_Freeze can't even compile or load on older versions, whereas with only a type slot it can do a runtime version check to avoid actually passing it).

@encukou
Copy link
Member

encukou commented Oct 7, 2024

with only a type slot it can do a runtime version check to avoid actually passing it

That's a good point, thank you!

I don't think this point is worth replacing imperative code with a callback, though. We don't have a good track records with those: as years roll by, callbacks always seem to be needing a bit more context than they're getting.

my alternate proposals focused around working with the type dict, rather than the type object itself.

Modifying tp_dict using dict API is one of the more dangerous things you can do. Let's do less of that.
Alternately, the callback could return entries to add to the dict -- but that makes the callback a lot less powerful. OTOH,

providing an array of struct {const char *name; PyObject *value} in the slot that would be inserted into the type's dict.

I think this is a good idea regardless. Especially if we add a PyType_ApplySlots function.
If this array is enough for some type, it will be able to set Py_TPFLAGS_IMMUTABLETYPE in the initial FromSpec call, and get the future optimization you're thinking of. With runtime version checks!


The callback pointer could become a struct with a pointer and a userdata void*

... and now the spec isn't static any more :(

@vstinner
Copy link
Member Author

vstinner commented Oct 9, 2024

The C API Working Group approved PyType_Freeze() API: capi-workgroup/decisions#40 So I suggest to close this PR.

@AraHaan
Copy link
Contributor

AraHaan commented Oct 9, 2024

The C API Working Group approved PyType_Freeze() API: capi-workgroup/decisions#40 So I suggest to close this PR.

I think having both is good though as there can be use cases to having both options.

Especially if the Freeze call is not in itself part of the limited API but the spec slot is and having it internally call Freeze for the user in a forwards compatible way.

@vstinner
Copy link
Member Author

I close this PR since the C API Working Group approved PyType_Freeze() API: capi-workgroup/decisions#40

@vstinner vstinner closed this Oct 17, 2024
@vstinner vstinner deleted the slot_create_callback branch October 17, 2024 14:02
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.

4 participants