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-109543: Remove unnecessary hasattr check #109544

Merged
merged 2 commits into from Sep 20, 2023

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Sep 18, 2023

Also added a new test case covering the scenario I thought this
might be about.

Also added a new test case covering the scenario I thought this
might be about.
class TD2(TD1):
b: str

self.assertIs(TD2.__total__, True)
Copy link
Member

Choose a reason for hiding this comment

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

I know you're not changing behaviour here, but are the precise semantics here specified anywhere? Mypy has some interesting behaviour here -- it treats all the keys present in TD2 as required, but all the keys present in TD1 as not required. That implies to me that TD2 should not be seen as a "total" TypedDict.

https://mypy-play.net/?mypy=latest&python=3.11&gist=e7ef88dd8c2b5697297c4739d471ac45

The __total__ attribute seems to be pretty meaningless these days, though, in a post-PEP-655 world:

>>> from typing import *
>>> class Foo(TypedDict):
...     x: Required[int]
...     y: NotRequired[str]
...
>>> Foo.__total__
True

Maybe we should treat that as a bug and make it meaningful. Or maybe we should just be clearer in the documentation that __total__ doesn't indicate whether or not all keys in the TypedDict are required or not, it just literally indicates whether that specific TypedDict was constructed using total=True (but I'm not sure why that would be useful to anybody).

Copy link
Member Author

Choose a reason for hiding this comment

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

The mypy behavior is right; that was the way to mix Required and NotRequired keys before PEP 655.

Agree that __total__ isn't that useful and you should generally look at __required_keys__ and __optional_keys__. However, I think the current behavior is right. If you do want to use __total__ for introspection, you should look not just at the current class's attribute, but also those of base classes.

Copy link
Member

Choose a reason for hiding this comment

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

It's strange to me that we iterate through the base classes as part of the class construction in order to make sure we have accurate __required_keys__, __optional_keys__ and __annotations__ attributes on the produced class, but we don't do the same for the __total__ attribute. (We probably shouldn't be doing that for the __annotations__ attribute, since that's not the way __annotations__ works on any other class... but we do it anyway.) It seems very inconsistent to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Submitted #109547 with some enhancements to the docs.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

The semantics of this attribute seem pretty confusing -- I'm honestly wondering whether it might not be better to deprecate it. But I'm pretty confident you're correct that there's no way for __total__ to already be set at this point.

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Sep 18, 2023
As discussed in comments to python#109544, the semantics of this attribute
are somewhat confusing. Add a note explaining its limitations and
steering users towards __required_keys__ and __optional_keys__ instead.
@JelleZijlstra JelleZijlstra merged commit 1293fcc into python:main Sep 20, 2023
24 checks passed
@JelleZijlstra JelleZijlstra deleted the tdtotal branch September 20, 2023 03:15
@bedevere-bot
Copy link

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

Hi! The buildbot s390x SLES 3.x has failed when building commit 1293fcc.

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/all/#builders/540/builds/6580) and take a look at the build logs.
  4. Check if the failure is related to this commit (1293fcc) 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/all/#builders/540/builds/6580

Failed tests:

  • test.test_asyncio.test_subprocess

Failed subtests:

  • test_subprocess_consistent_callbacks - test.test_asyncio.test_subprocess.SubprocessThreadedWatcherTests.test_subprocess_consistent_callbacks

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

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-sles-z/build/Lib/test/test_asyncio/test_subprocess.py", line 788, in test_subprocess_consistent_callbacks
    self.loop.run_until_complete(main())
  File "/home/dje/cpython-buildarea/3.x.edelsohn-sles-z/build/Lib/asyncio/base_events.py", line 664, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/dje/cpython-buildarea/3.x.edelsohn-sles-z/build/Lib/test/test_asyncio/test_subprocess.py", line 780, in main
    self.assertEqual(events, [
AssertionError: Lists differ: [('pi[29 chars]t'), 'pipe_connection_lost', ('pipe_data_recei[57 chars]ted'] != [('pi[29 chars]t'), ('pipe_data_received', 2, b'stderr'), 'pi[57 chars]ted']

@bedevere-bot
Copy link

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

Hi! The buildbot s390x RHEL7 LTO 3.x has failed when building commit 1293fcc.

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/all/#builders/402/builds/5427) and take a look at the build logs.
  4. Check if the failure is related to this commit (1293fcc) 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/all/#builders/402/builds/5427

Failed tests:

  • test.test_asyncio.test_subprocess

Failed subtests:

  • test_subprocess_consistent_callbacks - test.test_asyncio.test_subprocess.SubprocessThreadedWatcherTests.test_subprocess_consistent_callbacks

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

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/test/test_asyncio/test_subprocess.py", line 788, in test_subprocess_consistent_callbacks
    self.loop.run_until_complete(main())
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/asyncio/base_events.py", line 664, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/test/test_asyncio/test_subprocess.py", line 780, in main
    self.assertEqual(events, [
AssertionError: Lists differ: [('pi[29 chars]t'), 'pipe_connection_lost', ('pipe_data_recei[57 chars]ted'] != [('pi[29 chars]t'), ('pipe_data_received', 2, b'stderr'), 'pi[57 chars]ted']

@bedevere-bot
Copy link

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

Hi! The buildbot ARM Raspbian 3.x has failed when building commit 1293fcc.

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/all/#builders/424/builds/4967) and take a look at the build logs.
  4. Check if the failure is related to this commit (1293fcc) 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/all/#builders/424/builds/4967

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

==

Click to see traceback logs
remote: Enumerating objects: 20, done.        
remote: Counting objects:   5% (1/20)        
remote: Counting objects:  10% (2/20)        
remote: Counting objects:  15% (3/20)        
remote: Counting objects:  20% (4/20)        
remote: Counting objects:  25% (5/20)        
remote: Counting objects:  30% (6/20)        
remote: Counting objects:  35% (7/20)        
remote: Counting objects:  40% (8/20)        
remote: Counting objects:  45% (9/20)        
remote: Counting objects:  50% (10/20)        
remote: Counting objects:  55% (11/20)        
remote: Counting objects:  60% (12/20)        
remote: Counting objects:  65% (13/20)        
remote: Counting objects:  70% (14/20)        
remote: Counting objects:  75% (15/20)        
remote: Counting objects:  80% (16/20)        
remote: Counting objects:  85% (17/20)        
remote: Counting objects:  90% (18/20)        
remote: Counting objects:  95% (19/20)        
remote: Counting objects: 100% (20/20)        
remote: Counting objects: 100% (20/20), done.        
remote: Compressing objects:   9% (1/11)        
remote: Compressing objects:  18% (2/11)        
remote: Compressing objects:  27% (3/11)        
remote: Compressing objects:  36% (4/11)        
remote: Compressing objects:  45% (5/11)        
remote: Compressing objects:  54% (6/11)        
remote: Compressing objects:  63% (7/11)        
remote: Compressing objects:  72% (8/11)        
remote: Compressing objects:  81% (9/11)        
remote: Compressing objects:  90% (10/11)        
remote: Compressing objects: 100% (11/11)        
remote: Compressing objects: 100% (11/11), done.        
remote: Total 11 (delta 9), reused 1 (delta 0), pack-reused 0        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '1293fcc3c6b67b7e8d0081863ec6387e162341eb'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 1293fcc3c6 gh-109543: Remove unnecessary hasattr check (#109544)
Switched to and reset branch 'main'

./Modules/_testcapi/heaptype_relative.c: In function ‘make_sized_heaptypes’:
./Modules/_testcapi/heaptype_relative.c:60:28: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
   60 |                            (unsigned long long)data_ptr,
      |                            ^

make: *** [Makefile:2036: buildbottest] Error 3

@bedevere-bot
Copy link

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

Hi! The buildbot PPC64LE RHEL7 3.x has failed when building commit 1293fcc.

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/all/#builders/446/builds/3915) and take a look at the build logs.
  4. Check if the failure is related to this commit (1293fcc) 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/all/#builders/446/builds/3915

Failed tests:

  • test_socket

Failed subtests:

  • testSendmsgTimeout - test.test_socket.SendmsgTCPTest.testSendmsgTimeout

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

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le/build/Lib/test/test_socket.py", line 3145, in testSendmsgTimeout
    self.assertTrue(self.misc_event.wait(timeout=self.fail_timeout))
AssertionError: False is not true


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le/build/Lib/test/test_socket.py", line 365, in raise_queued_exception
    raise self.queue.get()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le/build/Lib/test/test_socket.py", line 402, in clientRun
    test_func()
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le/build/Lib/test/test_socket.py", line 3152, in _testSendmsgTimeout
    self.sendmsgToServer([b"a"*512])
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL7-ppc64le/build/Lib/test/test_socket.py", line 2840, in sendmsgToServer
    return self.cli_sock.sendmsg(
           ^^^^^^^^^^^^^^^^^^^^^^
ConnectionResetError: [Errno 104] Connection reset by peer

@bedevere-bot
Copy link

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

Hi! The buildbot PPC64LE Fedora Stable LTO 3.x has failed when building commit 1293fcc.

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/all/#builders/541/builds/3401) and take a look at the build logs.
  4. Check if the failure is related to this commit (1293fcc) 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/all/#builders/541/builds/3401

Failed tests:

  • test_venv

Failed subtests:

  • test_zippath_from_non_installed_posix - test.test_venv.BasicTest.test_zippath_from_non_installed_posix

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

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le.lto/build/Lib/test/test_venv.py", line 578, in test_zippath_from_non_installed_posix
    shutil.copytree(fn, os.path.join(libdir, name))
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le.lto/build/Lib/shutil.py", line 588, in copytree
    return _copytree(entries=entries, src=src, dst=dst, symlinks=symlinks,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le.lto/build/Lib/shutil.py", line 542, in _copytree
    raise Error(errors)
shutil.Error: [('/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le.lto/build/Lib/test/test_future_stmt/__pycache__/import_nested_scope_twice.cpython-313.pyc.140735998680240', '/tmp/test_python_vy4zhu8t/tmpp6uaij6t/lib/python3.13/test/test_future_stmt/__pycache__/import_nested_scope_twice.cpython-313.pyc.140735998680240', "[Errno 2] No such file or directory: '/home/buildbot/buildarea/3.x.cstratak-fedora-stable-ppc64le.lto/build/Lib/test/test_future_stmt/__pycache__/import_nested_scope_twice.cpython-313.pyc.140735998680240'")]

@bedevere-bot
Copy link

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

Hi! The buildbot PPC64LE RHEL8 LTO 3.x has failed when building commit 1293fcc.

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/all/#builders/361/builds/4077) and take a look at the build logs.
  4. Check if the failure is related to this commit (1293fcc) 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/all/#builders/361/builds/4077

Failed tests:

  • test.test_asyncio.test_unix_events

Failed subtests:

  • test_fork_signal_handling - test.test_asyncio.test_unix_events.TestFork.test_fork_signal_handling

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

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/unittest/async_case.py", line 90, in _callTestMethod
    if self._callMaybeAsync(method) is not None:
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/unittest/async_case.py", line 117, in _callMaybeAsync
    return self._asyncioTestContext.run(func, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/test/support/hashlib_helper.py", line 49, in wrapper
    return func_or_class(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le.lto/build/Lib/test/test_asyncio/test_unix_events.py", line 1937, in test_fork_signal_handling
    self.assertTrue(child_handled.is_set())
AssertionError: False is not true

JelleZijlstra added a commit that referenced this pull request Sep 27, 2023
As discussed in comments to #109544, the semantics of this attribute
are somewhat confusing. Add a note explaining its limitations and
steering users towards __required_keys__ and __optional_keys__ instead.
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 27, 2023
As discussed in comments to pythonGH-109544, the semantics of this attribute
are somewhat confusing. Add a note explaining its limitations and
steering users towards __required_keys__ and __optional_keys__ instead.
(cherry picked from commit f49958c)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 27, 2023
As discussed in comments to pythonGH-109544, the semantics of this attribute
are somewhat confusing. Add a note explaining its limitations and
steering users towards __required_keys__ and __optional_keys__ instead.
(cherry picked from commit f49958c)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
JelleZijlstra added a commit that referenced this pull request Sep 27, 2023
… (#109983)

As discussed in comments to GH-109544, the semantics of this attribute
are somewhat confusing. Add a note explaining its limitations and
steering users towards __required_keys__ and __optional_keys__ instead.
(cherry picked from commit f49958c)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
csm10495 pushed a commit to csm10495/cpython that referenced this pull request Sep 28, 2023
Also added a new test case covering the scenario I thought this
might be about.
csm10495 pushed a commit to csm10495/cpython that referenced this pull request Sep 28, 2023
As discussed in comments to python#109544, the semantics of this attribute
are somewhat confusing. Add a note explaining its limitations and
steering users towards __required_keys__ and __optional_keys__ instead.
vstinner pushed a commit that referenced this pull request Oct 4, 2023
… (#109982)

Enhance TypedDict docs around required/optional keys (GH-109547)

As discussed in comments to GH-109544, the semantics of this attribute
are somewhat confusing. Add a note explaining its limitations and
steering users towards __required_keys__ and __optional_keys__ instead.
(cherry picked from commit f49958c)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
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.

None yet

3 participants