GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-126835: Remove unused docstring const by WolframAlph · Pull Request #130016 · python/cpython

Conversation

@WolframAlph

@WolframAlph WolframAlph commented Feb 11, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor
  • Issue: Move const folding to the peephole optimizer #126835

If no docstring present, extra unused constant from CFG optimization pass will reuse always preserved slot for docstring which leads to unused constant in co_consts. This breaks some unrelated tests when migrating optimizations from AST to CFG. Bug example:

cpython/Lib/test/test_compile.py

Lines 821 to 844 in 12bd15f

@support.cpython_only
def test_remove_unused_consts(self):
def f():
"docstring"
if True:
return "used"
else:
return "unused"
self.assertEqual(f.__code__.co_consts,
(f.__doc__, "used"))
@support.cpython_only
def test_remove_unused_consts_no_docstring(self):
# the first item (None for no docstring in this case) is
# always retained.
def f():
if True:
return "used"
else:
return "unused"
self.assertEqual(f.__code__.co_consts,
(True, "used"))

First test case does not have True in co_consts but second one does even though it is not used (not used in first either) which is funny. Another example:

def f():
    return x

print(f.__code__.co_consts)  #  (None,)

None is in co_consts even though None is not used in f. It sneaks in by implicitly adding return None when compiling code unit, and it stays there because of always preserved slot for docstring.

Related comment: #126835 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@WolframAlph
WolframAlph requested review from iritkatriel and markshannon as code owners February 11, 2025 20:46
@WolframAlph
WolframAlph marked this pull request as draft February 11, 2025 20:46
@bedevere-app bedevere-app Bot added awaiting review and removed awaiting review labels Feb 11, 2025
@WolframAlph
remove unused docstring const
3d34447
@WolframAlph
WolframAlph force-pushed the unused-const branch from bec0789 to 3d34447 Compare February 11, 2025 21:17
@WolframAlph WolframAlph changed the title remove unused docstring const Remove unused docstring const Feb 11, 2025
@WolframAlph WolframAlph changed the title Remove unused docstring const gh-130016: Remove unused docstring const Feb 11, 2025
@WolframAlph WolframAlph changed the title gh-130016: Remove unused docstring const gh-126835: Remove unused docstring const Feb 11, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Feb 11, 2025
Move const folding to the peephole optimizer #126835
Open
@WolframAlph
WolframAlph marked this pull request as ready for review February 11, 2025 21:25
@bedevere-app bedevere-app Bot added the awaiting review label Feb 11, 2025
@WolframAlph

WolframAlph commented Feb 11, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@iritkatriel let me know if you prefer having this linked to a separate issue. And also if we need to add some tests for this & news entry.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Eclips4
Merge branch 'main' into unused-const
5e0dbc4
tomasr8
tomasr8 reviewed Feb 11, 2025
View reviewed changes
Comment thread Python/flowgraph.c
index_map[i] = -1;
}
// The first constant may be docstring; keep it always.
index_map[0] = 0;

@tomasr8 tomasr8 Feb 11, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maybe update the comment above? It's only kept when we have the docstring now

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Eclips4 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@WolframAlph WolframAlph mentioned this pull request Feb 12, 2025
gh-126835: Move const list & set folding in for_iter & contains from ast_opt.c to flowgraph.c #130032
Merged
This was referenced Feb 22, 2025
gh-126835: Disable tuple folding in the AST optimizer #128802
Closed
gh-130480: Move duplicate LOAD_SMALL_INT optimization from codegen to CFG #130481
Merged
@github-actions

github-actions Bot commented Apr 21, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tomasr8 tomasr8 tomasr8 left review comments
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@WolframAlph @tomasr8 @Eclips4
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.