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

Support getters and setters in Argument Clinic #112205

Closed
Tracked by #108219
colesbury opened this issue Nov 17, 2023 · 9 comments
Closed
Tracked by #108219

Support getters and setters in Argument Clinic #112205

colesbury opened this issue Nov 17, 2023 · 9 comments
Assignees
Labels
3.13 bugs and security fixes topic-argument-clinic topic-free-threading type-feature A feature request or enhancement

Comments

@colesbury
Copy link
Contributor

colesbury commented Nov 17, 2023

Feature or enhancement

In the C API, getters and setters are implemented using PyGetSetDef. Argument Clinic doesn't currently support writing getters and setters, probably because they are pretty straightforward to write manually -- there's not much argument parsing to be done.

Argument Clinic now supports the @critical_section directive, which avoids a bunch of boilerplate code when making things thread-safe with the --disable-gil builds. It would be helpful if Argument Clinic supported getters/setters so that we could avoid the critical section boilerplate in getters and setters as well.

Linked PRs

@colesbury colesbury added type-feature A feature request or enhancement topic-argument-clinic 3.13 bugs and security fixes topic-free-threading labels Nov 17, 2023
@colesbury
Copy link
Contributor Author

cc @aisk (who brought up this issue in #112116 (comment)) and @erlend-aasland for feedback

@corona10
Copy link
Member

Another example that needs this feature: #112298

@corona10
Copy link
Member

corona10 commented Nov 22, 2023

@colesbury @erlend-aasland @AlexWaygood
(Just for example)
What about following DSL for name setter / getter case?
It looks similar to Java lombok annotation :(

/*[clinic input] 
@setter
@getter
@critical_section
_io._Buffered.name
[clinic start generated code]*/

@corona10
Copy link
Member

corona10 commented Dec 2, 2023

/*[clinic input] 
@getter
@critical_section
_io._Buffered.name
[clinic start generated code]*/

/*[clinic input] 
@setter
@critical_section
_io._Buffered.name
[clinic start generated code]*/

I am working on supporting @setterwith the co-existence case of @getter.
Separating clinic input is more straightforward and cleaner than declaring with a single input.

@AlexWaygood
Copy link
Member

Separating clinic input is more straightforward and cleaner than declaring with a single input.

I think it looks nicer as well — having the three stacked decorators kinda looked ugly to me. Moreover, the slightly more verbose syntax of having the two inputs separated is closer to how you'd implement a Python property with a setter. In short: good choice, IMO :-)

@erlend-aasland
Copy link
Contributor

Following up discussions on #113160.

See my comments for context:

Here's a quick braindump of how I'd like the PyGetSetDef feature to be handled internally in Argument Clinic:

  • PyGetSetDef belongs to a type object, implying it needs to be paired with a class directive in the current clinic scope (file scope). We can put this guard (a fail(...)) at the end of the "state-modulename-name" stage of the parsing state machine.
  • The PyGetSetDef metadata (getter, setter, docstr, ...) could (should) be stored on the Class where the PyGetSetDef belongs; Class already has a functions member; perhaps we can use this. My first thought is to add a render() method to Class, and call this somewhere in the generation phase. This render() method could introspect its getters and setters (for example via self.functions), and produce the #define that will end up being used in the actual PyGetSetDef definition. We should end up with cleaner templates, cleaner generated code, and more maintainable clinic.py code.
  • The Function objects [for getters and setters] should not hack around with the PyMemberDef template stuff; it should only deal with the templates for the C function declarations.

I think the clinic input side of things work well! My complaints are strictly about clinic internals :)

@corona10
Copy link
Member

corona10 commented Dec 19, 2023

I think the clinic input side of things work well! My complaints are strictly about clinic internals :)

Yeah, I fully agree with your suggestion. The current hacky implementation is a kind of tech debt.
With refactoring, it will generate better C code. (and as I commented, I fully agree that it will need to be improved)
If you don't mind, can we create a new issue as the improving issue? I think that we agree that the design of input is acceptable.
I prefer to close this issue when the #113160 is merged as marking the completion of first phase of this feature :)

@erlend-aasland
Copy link
Contributor

If you don't mind, can we create a new issue as the improving issue? I think that we agree that the design of input is acceptable. I prefer to close this issue when the #113160 is merged as marking the completion of first phase of this feature :)

Yes, see what I wrote in #113160 (comment) ;)

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Dec 19, 2023
erlend-aasland added a commit that referenced this issue Dec 19, 2023
erlend-aasland added a commit to corona10/cpython that referenced this issue Dec 19, 2023
corona10 added a commit that referenced this issue Dec 20, 2023
---------

Co-authored-by: Erlend E. Aasland <[email protected]>
@corona10
Copy link
Member

I created a follow-up issue and am now closing the current issue.: #113318

Thank you for all your support @colesbury @erlend-aasland @AlexWaygood and @vstinner

ryan-duve pushed a commit to ryan-duve/cpython that referenced this issue Dec 26, 2023
ryan-duve pushed a commit to ryan-duve/cpython that referenced this issue Dec 26, 2023
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
---------

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
aisk pushed a commit to aisk/cpython that referenced this issue Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
---------

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
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 topic-argument-clinic topic-free-threading type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants