-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
The method for storing docstrings in code objects is awkward and prevents optimizations. #126072
Comments
Hi, I'm not sure whether the PR is simple. If so, I would like to take this. |
I don't know how simple it will be. It will involve the internals of the bytecode compiler. Feel free to ask questions if you get stuck. |
IIUC, this issue does not include these topics, these are the future works, right?
Thanks, I'll give it a try and open a PR. |
Yes |
Note that once this is done, |
Hi Irit, I'm also interested in this. But currently I'm not sure how Lines 703 to 717 in dcad8fe
Currently, I can enter this def has_docstring(x, y):
"""This is a fisrt-line doc string"""
"""This is a second-line doc string"""
a = x + y
b = x - y
return a, b IIUC, it converts the expression in the first statement from |
This function removes the docstrings in optimised mode (running It's a hack, will be good to get rid of it now if we can. But I see your point about this happening before symtable. Maybe the docstring removal optimization can move to the symtable where you currently mark the docstring as existing/not existing? This would need to be documented as a visible change to the "optimised ast", which will no longer have docstrings removed. But I don't think that a problem because all these apis are considered unstable, and the "optimised ast" was only exposed to users in 3.13 in #108154 so there shouldn't be much disruption in changing it now. |
It's feasible. But I'm not sure whether deferring the optimization from the AST to code object construction will affect the semantics of |
After preliminary investigation, I think they follow the same logic with function, and it's not the concern. Maybe we can open another issue to move the |
Just found some cases we missed, need to fix them together with the docstring removal PR or a new isolated PR: Lines 667 to 669 in d467d92
Lines 1595 to 1597 in d467d92
Lines 1893 to 1895 in d467d92
I think we can use |
Would be good to cover those cases with some tests. |
Currently, the zeroth constant in a code object's
co_consts
tuple is the docstring, iff it is a string.This means that any code object without a docstring must not have a string as its first constant. To guarantee this we generally insert
None
as the first constant.This prevents a few improvements we would like to make, such as moving
None
fromLOAD_CONST
toLOAD_COMMON_CONST
,and complicates handling of code objects in the compiler.
I propose adding a flag to
co_flags
,CO_HAS_DOCSTRING
. If this flag is set then the docstring is the zeroth string, otherwise there is no docstring.Linked PRs
None
toco_consts
if no docstring #126101The text was updated successfully, but these errors were encountered: