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-126072: do not add None to co_consts if no docstring #126101

Merged
merged 26 commits into from
Oct 30, 2024

Conversation

xuantengh
Copy link
Contributor

@xuantengh xuantengh commented Oct 29, 2024

Hi @markshannon this is my attempt to implement #126072. Initially the value of CO_HAS_DOCSTRING is set as 0x0040, but I found it has been used by the CO_NOFREE in inspect.

cpython/Lib/inspect.py

Lines 409 to 411 in 85799f1

co_flags bitmap: 1=optimized | 2=newlocals | 4=*arg | 8=**arg
| 16=nested | 32=generator | 64=nofree | 128=coroutine
| 256=iterable_coroutine | 512=async_generator

And I've tested it with the following code: See the test cases in changed test files

I've also updated the related documentation, but maybe there is something I missed. Please help me improve this PR, thank you!


📚 Documentation preview 📚: https://cpython-previews--126101.org.readthedocs.build/

@bedevere-app
Copy link

bedevere-app bot commented Oct 29, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Please add some tests, probably to test_code.py.

You'll also probably need to change the __doc__ descriptor on function objects so it continues to work correctly.

Python/codegen.c Outdated Show resolved Hide resolved
Objects/funcobject.c Outdated Show resolved Hide resolved
Python/codegen.c Show resolved Hide resolved
@xuantengh
Copy link
Contributor Author

xuantengh commented Oct 29, 2024

Maybe the optimization pass should be updated as the hypothesis may not hold:

cpython/Python/flowgraph.c

Lines 2117 to 2119 in 9b14083

// The first constant may be docstring; keep it always.
index_map[0] = 0;

The generated bytecodes for the same function are different before and after this PR.

@markshannon
Copy link
Member

FYI, we are in the process of removing small ints from the co_consts tuple: #125972

Can you change the ints in your tests to strings. Doing so will also help test that non-docstring strings do not get interpreted as docstrings.

@markshannon
Copy link
Member

Maybe the optimization pass should be updated as the hypothesis may not hold:

cpython/Python/flowgraph.c

Lines 2117 to 2119 in 9b14083

// The first constant may be docstring; keep it always.
index_map[0] = 0;

The generated bytecodes for the same function are different before and after this PR.

There are a few improvements we can make to the bytecode compiler once this PR is done.
It is probably best to do those in other PRs, once this is working.

@xuantengh
Copy link
Contributor Author

xuantengh commented Oct 29, 2024

It is probably best to do those in other PRs, once this is working.

Yeah, what I concern is whether the constant removal optimization will lead to a unexpected results when the first element in co_consts is no longer None if there is no docstring.

@xuantengh
Copy link
Contributor Author

Currently, as the const removal optimization unconditionally keeps the first element in consts, None will present in co_consts even if co has no docstring if there is only one const (i.e., None itself).

Lib/test/test_compiler_assemble.py Outdated Show resolved Hide resolved
Lib/test/test_inspect/test_inspect.py Outdated Show resolved Hide resolved
Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Looks good. One small edit for the language reference.

Doc/reference/datamodel.rst Outdated Show resolved Hide resolved
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

Thanks for doing this.

This was one of those little things that's been bothering me for a while.
Glad to see it fixed

Python/codegen.c Outdated Show resolved Hide resolved
@xuantengh
Copy link
Contributor Author

Update doc inspect.rst as well.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

One small comment on inspect.rst.

Doc/library/inspect.rst Outdated Show resolved Hide resolved
@markshannon
Copy link
Member

The UI shows 2 jobs still in progress. But they have completed successfully 🤷‍♂️

@markshannon markshannon merged commit 35df4eb into python:main Oct 30, 2024
39 checks passed
@xuantengh
Copy link
Contributor Author

Thanks for your review, it's my first PR merged into CPython! And I'm also interested in the following related optimizations.

@iritkatriel
Copy link
Member

Thanks for your review, it's my first PR merged into CPython! And I'm also interested in the following related optimizations.

Congratulations! in your next PR, add your name to Misc/ACKS (we should have done it here, but too late now).

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.

5 participants