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-112205: Support docstring for @getter #113160

Merged
merged 12 commits into from
Dec 20, 2023
Merged

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Dec 15, 2023

@corona10
Copy link
Member Author

corona10 commented Dec 15, 2023

@erlend-aasland @AlexWaygood
This PR will be the last PR for #112205.

I'm sharing my consideration for the design of this PR.

  • The @setter only case is very rare than the @getter only, and there is no such case in the CPython codebase, and AFAIK, in Python @property decorator does not allow such cases. (without using hacky way)
  • Reading the docstring of @setter can be categorized as the read operation, so if the user wants to declare the docstring of the property, then the user should declare @getter.
  • Even if @getter is not declared, @setter can be declared independently without docstring.

I will update the devguide about the restriction of docstring for property.

@corona10
Copy link
Member Author

@erlend-aasland @AlexWaygood Updated :) PTAL

@erlend-aasland
Copy link
Contributor

Ideally, I would prefer this:

  • If no docstring is supplied, clinic simply puts a NULL in the generated struct (no added preprocessor complexity generated)
  • If a docstring is supplied in either of setter/getter, a PyDoc_STRVAR declaration is generated (with {getset_basename}_doc as the template variable name), and the struct is updated to use the declared variable.
  • If a docstring is supplied both in setter and getter, fail(...)

What do you think, @AlexWaygood?

@corona10
Copy link
Member Author

corona10 commented Dec 18, 2023

Ideally, I would prefer this:
..

Ideally, yes, but I think that this very difficult requirement in current status if you want to support all of the situations in a AC; since the AC does not care about another method state (aka stateless), it's very difficult to interact with another method or propety state.

@corona10
Copy link
Member Author

corona10 commented Dec 18, 2023

If we want to make AC care about other method states, we should reconstruct AC from scratch.
I would prefer to have those requirements as future plan if we really need those things. I am not sure that we really want to support @setter only case with docstring (If I've searched such cases even for the pure python case from SO, they recommend not to do it) is real and worth reconstructing AC architecture.

@corona10
Copy link
Member Author

Note: The reason I used the macro declaration of generated c codes was that it was very easy to care about other property declaration statuses without touching overall AC architecture. And the generated C code obviously not affected to performance issue at all :)

@corona10
Copy link
Member Author

corona10 commented Dec 18, 2023

Workaround for @setteronly with docstring:
Declare both @getter and @setter and raise AttributeError from @getter.. not pretty but possible.
Or not using AC...

@vstinner
Copy link
Member

This generated code is verbose, but I read AC code, and I know that it's tricky to add new features in a safe way.

PyDoc_STRVAR(_io__TextIOBase_encoding__doc__,
"Encoding of the text stream.\n"
"\n"
"Subclasses should override.");
#define _io__TextIOBase_encoding_HAS_DOCSTR

#if defined(_io__TextIOBase_encoding_HAS_DOCSTR)
#  define _io__TextIOBase_encoding_DOCSTR _io__TextIOBase_encoding__doc__
#else
#  define _io__TextIOBase_encoding_DOCSTR NULL
#endif

I would prefer to not hold this nice enhancement until AC is refactored in a way which avoids such verbose code.

Since the code is is generated, I don't care too much about the details, as soon as the C code, after the preprocessor is passed, remains efficient :-)

@AlexWaygood
Copy link
Member

I'll defer to you three on this one; I agree that @erlend-aasland's idea sounds nicer in principle, but I also don't mind there being slightly ugly generated C code for now, as long as it results in the same behaviour for Python users :)

@erlend-aasland
Copy link
Contributor

If there is a need to get this in very quick, I'm ok with rushing it. A lot of the setter/getter code is special cased, mostly because clinic was designed for PyMethodDef, not PyGetSetDef :) If we don't need to rush this, I would prefer if we could try to fit it more nicely into clinic.py. Is this PR blocking other changes?

I would prefer to not hold this nice enhancement until AC is refactored in a way which avoids such verbose code.

Well, clinic is already very flexible, so we might not need to refactor it at all, in order to make it work more smoothly.

@corona10
Copy link
Member Author

corona10 commented Dec 18, 2023

@erlend-aasland @AlexWaygood @vstinner

If there is a need to get this in very quick, I'm ok with rushing it.

Okay, negotiation time :)
Some cases need to use docstring declaration but we still have time to consider your design until we make them thread-safe status. Until then, we can think about a better way if you want. But we may not be able to delay this feature forever because, at some moments, we need to make them thread-safe anyway.

Well, clinic is already very flexible, so we might not need to refactor it at all, in order to make it work more smoothly.

Here is a note that I considered cases for this issue that you should consider if you want to suggest other designs.

  1. The declaration order of @getter and @setter can always be changed, I didn't want to restrict those things to users.
    (The current PR does not restrict the declaration order whether the @getter have a docstring or not)
  2. From your suggestion, if the user declares a docstring with @setter or @getter at first, the following @getter or @setter should use the declared docstring from the former declarations and it means that the last declaration should care about whether docstring was declared before or not because the user may want to use the docstring that is declared by latter @getter or @setter. It's a very stateful issue.
  • @getter only or @setter only with optional docstring.
  • @getter/setter + @getter/setter
  • @getter/setter with docstring + @getter/setter
  • @getter/setter + @getter/setter with docstring
  • @getter/setter with docstring + @getter/setterwith docstring: You need to care about this status from the AC side because you want to mark this status as failed.

@erlend-aasland
Copy link
Contributor

As I already wrote, I'm fine with proceeding with this implementation as a temporary workaround. IMO, I think we (myself included) should have put more time into the design of the PyGetSetDef feature before starting to land the recent clinic PRs. If Alex and Victor are ok with this PR, I won't block it. But please create a follow-up issue for refactoring (and possibly redesigning) the PyGetSetDef feature of Argument Clinic, so we don't forget about that :)

Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Lib/test/test_clinic.py Outdated Show resolved Hide resolved
corona10 and others added 2 commits December 19, 2023 22:30
@corona10 corona10 closed this Dec 19, 2023
@corona10 corona10 reopened this Dec 19, 2023
@vstinner
Copy link
Member

So far, I avoided "large refactoring" of clinic.py just because of the number of lines. For me, the file is so big that it's a blocker issue for me. I created gh-113299 to split this file into sub-files.

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

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; please wait for Alex's thumbs up before landing.

@corona10
Copy link
Member Author

@AlexWaygood ^^^^ :)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @corona10 <3

@corona10 corona10 merged commit 57b7e52 into python:main Dec 20, 2023
38 checks passed
@corona10 corona10 deleted the gh-112205-doc branch December 20, 2023 12:52
ryan-duve pushed a commit to ryan-duve/cpython that referenced this pull request Dec 26, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants