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

gh-148374: Fix a bug in `_Py_uop_sym_get_type` by Sacul0457 · Pull Request #148375 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-148374: Fix a bug in _Py_uop_sym_get_type - #148375

Merged
Fidget-Spinner merged 5 commits into
python:mainfrom
Sacul0457:Fix-_Py_uop_sym_get_type
Apr 11, 2026
ConversationCommits (5)ChecksFiles changed
Merged

gh-148374: Fix a bug in _Py_uop_sym_get_type#148375
Fidget-Spinner merged 5 commits into
python:mainfrom
Sacul0457:Fix-_Py_uop_sym_get_type

Conversation

Sacul0457 commented Apr 11, 2026 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

bedevere-app Bot mentioned this pull request Apr 11, 2026
Closed

Sacul0457 commented Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

Also, would this need a news entry?

Fidget-Spinner reviewed Apr 11, 2026
View reviewed changes
Comment thread Python/optimizer_symbols.c Outdated
_Py_uop_sym_set_recorded_gen_func(ctx, rg1, func);
TEST_PREDICATE(_Py_uop_sym_matches_type(rg1, &PyGen_Type), "recorded gen func not treated as generator");
TEST_PREDICATE(!_Py_uop_sym_matches_type(rg1, &PyGen_Type), "recorded gen func treated as generator");
TEST_PREDICATE(_Py_uop_sym_get_probable_type(rg1) == NULL, "recorded gen func treated as generator");

Fidget-Spinner Apr 11, 2026

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

This should be PyGen_Type, please edit the probable type function

Sacul0457 Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

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

Oh sorry, I just realised what you meant in #148372, one sec

Sacul0457 Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

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

fixed!

Fidget-Spinner commented Apr 11, 2026

Copy link
Copy Markdown
Member

I think this doesnt need one because the bug wont manifest on its own, as its always tied for FOR_ITER_GEN_FRAME which prior to your PR, was properly guarded

Fidget-Spinner reviewed Apr 11, 2026
View reviewed changes
Comment thread Python/optimizer_symbols.c Outdated

Sacul0457 commented Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

I just realised there's more tests I need to update

Fidget-Spinner reviewed Apr 11, 2026
View reviewed changes
Comment thread Python/optimizer_symbols.c
_Py_uop_sym_set_recorded_gen_func(ctx, rg1, func);
TEST_PREDICATE(_Py_uop_sym_matches_type(rg1, &PyGen_Type), "recorded gen func not treated as generator");
TEST_PREDICATE(!_Py_uop_sym_matches_type(rg1, &PyGen_Type), "recorded gen func treated as generator");
TEST_PREDICATE(_Py_uop_sym_get_probable_type(rg1) == &PyGen_Type, "recorded gen func not treated as generator");

Fidget-Spinner Apr 11, 2026

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

Seems like the test is failing. Not sure why. can you do some debugging please and check what type returns? You can do something like printf("TYPE: %s\n", rg1->tp_name); (assuming rg1 is not NULL).

Sacul0457 Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

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

I thought it was due to this test?

I have to go for a bit now, I'll upload the debug prints when i'm back

Sacul0457 Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

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

Okay, I think I understand what's going on.
Also, it seems like all the tests use rg1. Is that intended?

JitOptRef doesn't seem to have a tp_name member I'm guessing you meant something else?
In the meantime, I did this instead:

    JitOptRef rg1 = _Py_uop_sym_new_unknown(ctx);
    _Py_uop_sym_set_recorded_gen_func(ctx, rg1, func);
    TEST_PREDICATE(!_Py_uop_sym_matches_type(rg1, &PyGen_Type), "recorded gen func treated as generator");
    TEST_PREDICATE(_Py_uop_sym_get_probable_type(rg1) == &PyGen_Type, "rg1: recorded gen func not treated as generator");
    TEST_PREDICATE(_Py_uop_sym_get_const(ctx, rg1) == NULL, "recorded gen func is treated as known value");

    /* Test that setting type narrows correctly */

    JitOptRef rg2 = _Py_uop_sym_new_unknown(ctx);
    _Py_uop_sym_set_recorded_gen_func(ctx, rg2, func);
    _Py_uop_sym_set_type(ctx, rg2, &PyGen_Type);
    TEST_PREDICATE(!_Py_uop_sym_matches_type(rg2, &PyGen_Type), "rg2: recorded gen func not treated as generator");
    TEST_PREDICATE(_Py_uop_sym_get_const(ctx, rg2) == NULL, "known type is treated as known value");

    JitOptRef rg3 = _Py_uop_sym_new_unknown(ctx);
    _Py_uop_sym_set_recorded_gen_func(ctx, rg3, func);
    _Py_uop_sym_set_type_version(ctx, rg3, PyGen_Type.tp_version_tag);
    TEST_PREDICATE(!_Py_uop_sym_matches_type(rg3, &PyGen_Type), "rg3: recorded gen func not treated as generator");
    TEST_PREDICATE(_Py_uop_sym_get_const(ctx, rg3) == NULL, "recorded value with type is treated as known");

Output:

Running Debug|x64 interpreter...
F.....
======================================================================
FAIL: test_optimizer_symbols (test.test_optimizer.TestOptimizerSymbols.test_optimizer_symbols)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Sacul Dev Stuff\Cpython - Contributing\cpython\Lib\test\test_optimizer.py", line 86, in test_optimizer_symbols
    _testinternalcapi.uop_symbols_test()
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
AssertionError: rg2: recorded gen func not treated as generator


Running Debug|x64 interpreter...
F.....
======================================================================
FAIL: test_optimizer_symbols (test.test_optimizer.TestOptimizerSymbols.test_optimizer_symbols)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Sacul Dev Stuff\Cpython - Contributing\cpython\Lib\test\test_optimizer.py", line 86, in test_optimizer_symbols
    _testinternalcapi.uop_symbols_test()
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
AssertionError: rg3: recorded gen func not treated as generator

Fidget-Spinner Apr 11, 2026

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

Oh yeah you're right. Please amend the other tests.

Fidget-Spinner reviewed Apr 11, 2026
View reviewed changes
Comment thread Python/optimizer_symbols.c
_Py_uop_sym_set_recorded_gen_func(ctx, rg2, func);
_Py_uop_sym_set_type(ctx, rg2, &PyGen_Type);
TEST_PREDICATE(_Py_uop_sym_matches_type(rg1, &PyGen_Type), "recorded gen func not treated as generator");
TEST_PREDICATE(!_Py_uop_sym_matches_type(rg2, &PyGen_Type), "recorded gen func treated as generator");

Fidget-Spinner Apr 11, 2026

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 for this test and below the converse as well

TEST_PREDICATE(_Py_uop_sym_get_probable_type(rg2) == &PyGen_Type, "recorded gen func not treated as generator");

Sacul0457 Apr 11, 2026

Copy link
Copy Markdown
Contributor Author

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

added 👍

Fidget-Spinner approved these changes Apr 11, 2026
View reviewed changes

Fidget-Spinner 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

Fidget-Spinner merged commit 83f33dc into python:main Apr 11, 2026
73 checks passed
Sacul0457 deleted the Fix-_Py_uop_sym_get_type branch April 11, 2026 17:41
ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
pythongh-148374: Fix a bug in _Py_uop_sym_get_type (pythonGH-148375)
e4aac3e
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

Fidget-Spinner Fidget-Spinner approved these changes
markshannon Awaiting requested review from markshannon markshannon is a code owner
tomasr8 Awaiting requested review from tomasr8 tomasr8 is a code owner
savannahostrowski Awaiting requested review from savannahostrowski

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL