FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-126072: do not add `None` to `co_consts` if no docstring by xuantengh · Pull Request #126101 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-126072: do not add None to co_consts if no docstring - #126101

Merged
markshannon merged 26 commits into
python:mainfrom
xuantengh:main
Oct 30, 2024
ConversationCommits (26)ChecksFiles changed
Merged

gh-126072: do not add None to co_consts if no docstring#126101
markshannon merged 26 commits into
python:mainfrom
xuantengh:main

Conversation

xuantengh commented Oct 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 Bot commented Oct 29, 2024

Copy link
Copy Markdown

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.

bedevere-app Bot mentioned this pull request Oct 29, 2024
Closed
JelleZijlstra reviewed Oct 29, 2024
View reviewed changes

JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Python/codegen.c Outdated
iritkatriel reviewed Oct 29, 2024
View reviewed changes
Comment thread Objects/funcobject.c Outdated
Comment thread Python/codegen.c

xuantengh commented Oct 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Oct 29, 2024

Copy link
Copy Markdown
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 commented Oct 29, 2024

Copy link
Copy Markdown
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 commented Oct 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 commented Oct 29, 2024

Copy link
Copy Markdown
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).

markshannon reviewed Oct 29, 2024
View reviewed changes
Comment thread Lib/test/test_compiler_assemble.py Outdated
Comment thread Lib/test/test_inspect/test_inspect.py Outdated
willingc approved these changes Oct 29, 2024
View reviewed changes

willingc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good. One small edit for the language reference.

Comment thread Doc/reference/datamodel.rst Outdated
markshannon approved these changes Oct 29, 2024
View reviewed changes

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for doing this.

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

iritkatriel reviewed Oct 29, 2024
View reviewed changes
Comment thread Python/codegen.c Outdated

xuantengh commented Oct 30, 2024

Copy link
Copy Markdown
Contributor Author

Update doc inspect.rst as well.

willingc approved these changes Oct 30, 2024
View reviewed changes

willingc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One small comment on inspect.rst.

Comment thread Doc/library/inspect.rst Outdated

markshannon commented Oct 30, 2024

Copy link
Copy Markdown
Member

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

markshannon merged commit 35df4eb into python:main Oct 30, 2024

xuantengh commented Oct 30, 2024

Copy link
Copy Markdown
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 commented Oct 30, 2024

Copy link
Copy Markdown
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).

xuantengh mentioned this pull request Oct 31, 2024
Merged
picnixz pushed a commit to picnixz/cpython that referenced this pull request Dec 8, 2024
pythongh-126072: do not add None to co_consts if there is no docs…
6671047 
…tring (pythonGH-126101)
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
pythongh-126072: do not add None to co_consts if there is no docs…
dcf0b8a 
…tring (pythonGH-126101)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JelleZijlstra JelleZijlstra left review comments
iritkatriel iritkatriel left review comments
willingc willingc approved these changes
markshannon markshannon approved these changes
carljm Awaiting requested review from carljm carljm is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL