Skip to content

Conversation

@ever0de
Copy link
Contributor

@ever0de ever0de commented Jul 11, 2025

The Py_TPFLAGS_MANAGED_DICT constant in Lib/test/test_class.py was incorrectly set to (1 << 2) instead of the correct (1 << 4) from object.h.

issue: #136535

@python-cla-bot
Copy link

python-cla-bot bot commented Jul 11, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app bedevere-app bot added awaiting review tests Tests in the Lib/test dir labels Jul 11, 2025
@bedevere-app
Copy link

bedevere-app bot commented Jul 11, 2025

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.

…ass.py`

The `Py_TPFLAGS_MANAGED_DICT` constant in `Lib/test/test_class.py`
was incorrectly set to (1 << 2) instead of the correct (1 << 4) from
object.h.
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

LGTM, I also double checked with the original reference:

#define Py_TPFLAGS_MANAGED_DICT (1 << 4)

I am waiting @Fidget-Spinner or @markshannon 's review also.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Hi would you like to improve the test if the is Py_TPFLAGS_MANAGED_DICT wrongly set?

With my local change it is passed if I modify the value into 1 << 2, I prefer to prevent regression in the future.

@bedevere-app
Copy link

bedevere-app bot commented Jul 13, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@corona10 corona10 added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Jul 13, 2025
@ever0de
Copy link
Contributor Author

ever0de commented Jul 13, 2025

Hi would you like to improve the test if the is Py_TPFLAGS_MANAGED_DICT wrongly set?

With my local change it is passed if I modify the value into 1 << 2, I prefer to prevent regression in the future.

Hello. I thought the test I added would prevent regression issues, but it didn't.
I think I can prevent this problem by checking Py_TPFLAGS_INLINE_VALUES (1 << 2 flag) in NoManagedDict. Would it be better to add it this way?
If you have any other better improvement suggestions, could you please propose them? Thank you!


class TestInlineValues(unittest.TestCase):

def test_flags(self):
Copy link
Member

Choose a reason for hiding this comment

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

Please revive test_flags also.

Copy link
Member

Choose a reason for hiding this comment

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

@ever0de Oh it was my miss, you don't have to revive :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see, Should I drop this commit then?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, please revert b39ca1a

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm!

self.c = 3
self.d = 4

class VarSizedSubclass(tuple):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class VarSizedSubclass(tuple):
class VarSizedSubclass(tuple):

Py_TPFLAGS_MANAGED_DICT = (1 << 4)

class NoManagedDict:
__slots__ = ('a',)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
__slots__ = ('a',)
__slots__ = ('a',)

pep 8

Copy link
Member

Choose a reason for hiding this comment

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

@ever0de
Copy link
Contributor Author

ever0de commented Aug 11, 2025

Is there any feedback or are there any requested changes I should be aware of?

@Fidget-Spinner
Copy link
Member

I completely forgot how the managed dict works as it's been a few years, so I don't think I can review this. Sorry!

@corona10 corona10 merged commit aa4b5a7 into python:main Oct 24, 2025
40 checks passed
@miss-islington-app
Copy link

Thanks @ever0de for the PR, and @corona10 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 24, 2025
…ass.py` (pythongh-136538)

(cherry picked from commit aa4b5a7)

Co-authored-by: Jiseok CHOI <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 24, 2025
…ass.py` (pythongh-136538)

(cherry picked from commit aa4b5a7)

Co-authored-by: Jiseok CHOI <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 24, 2025

GH-140532 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Oct 24, 2025
@bedevere-app
Copy link

bedevere-app bot commented Oct 24, 2025

GH-140533 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 24, 2025
corona10 pushed a commit that referenced this pull request Oct 24, 2025
corona10 pushed a commit that referenced this pull request Oct 24, 2025
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Windows11 Bigmem 3.14 (tier-1) has failed when building commit 08f6b3f.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1799/builds/435) and take a look at the build logs.
  4. Check if the failure is related to this commit (08f6b3f) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1799/builds/435

Failed tests:

  • test_os

Failed subtests:

  • test_listmounts - test.test_os.Win32ListdriveTests.test_listmounts

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "R:\buildarea\3.14.ambv-bb-win11.bigmem\build\Lib\test\test_os.py", line 3014, in test_listmounts
    self.assertSetEqual(
    ~~~~~~~~~~~~~~~~~~~^
        set(mounts),
        ^^^^^^^^^^^^
        self.known_mounts & set(mounts),
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
AssertionError: Items in the first set but not the second:
'R:\\'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants