Skip to content

gh-120782: Update internal type cache when reloading datetime #120829

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.

Sign up for GitHub

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

Merged
kumaraditya303 merged 3 commits into from
Jun 21, 2024
Merged

gh-120782: Update internal type cache when reloading datetime #120829

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9bdc48
add a test
neonene Jun 21, 2024
7c10284
invalidate type caches on reload
neonene Jun 21, 2024
285c3b6
📜🤖 Added by blurb_it.
blurb-it[bot] Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
add a test
  • Loading branch information
@neonene
neonene authored Jun 21, 2024
commit d9bdc48788d45d97042b984b22d42dac076147de
17 changes: 17 additions & 0 deletions Lib/test/datetimetester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6870,6 +6870,23 @@ def pickle_fake_date(datetime_) -> Type[FakeDate]:
""")
script_helper.assert_python_ok('-c', script)

def test_update_type_cache(self):
# gh-120782
script = textwrap.dedent("""
import sys
for i in range(5):
import _datetime
_datetime.date.max > _datetime.date.min
_datetime.time.max > _datetime.time.min
_datetime.datetime.max > _datetime.datetime.min
_datetime.timedelta.max > _datetime.timedelta.min
isinstance(_datetime.timezone.min, _datetime.tzinfo)
Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra Jun 21, 2024

Choose a reason for hiding this comment

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

Should this have assert?

Copy link
Copy Markdown
Contributor Author

@neonene neonene Jun 21, 2024

Choose a reason for hiding this comment

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

    assert isinstance(_datetime.timezone.min, _datetime.tzinfo)
           ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

    assert not isinstance(_datetime.timezone.min, _datetime.tzinfo)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError

Both fail, so this checks if it crashes or not. I agree this is not a good example.

Copy link
Copy Markdown
Member

@ericsnowcurrently ericsnowcurrently Jun 21, 2024

Choose a reason for hiding this comment

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

Given that the code looks a bit unusual, it would be worth adding a brief comment explaining that.

(Sorry I didn't notice this during my earlier review. Good catch, @JelleZijlstra.)

Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra Jun 21, 2024

Choose a reason for hiding this comment

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

Wouldn't it make sense to have these asserts actually check that the values are of the type they should be? The bug manifested itself as these values appearing to be objects of various random types.

Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra Jun 21, 2024

Choose a reason for hiding this comment

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

You could also assert that timezone.__dict__["min"] is timezone.min.

isinstance(_datetime.timezone.utc, _datetime.tzinfo)
isinstance(_datetime.timezone.max, _datetime.tzinfo)
del sys.modules['_datetime']
""")
script_helper.assert_python_ok('-c', script)


def load_tests(loader, standard_tests, pattern):
standard_tests.addTest(ZoneInfoCompleteTest())
Expand Down