gh-120782: Update internal type cache when reloading datetime by neonene · Pull Request #120829 · python/cpython · GitHub
Skip to content

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

Merged
kumaraditya303 merged 3 commits into
python:mainfrom
neonene:reload
Jun 21, 2024
Merged

gh-120782: Update internal type cache when reloading datetime#120829
kumaraditya303 merged 3 commits into
python:mainfrom
neonene:reload

Conversation

@neonene

@neonene neonene commented Jun 21, 2024

Copy link
Copy Markdown
Contributor

When reloading _datetime module, the single-phase version did not invoke the PyInit__datetime function, whereas the current multi-phase version updates the static types through the module init. The outdated static type cache in the interpreter state needs to be invalidated at the end of reloading the multi-phase module.

@neonene

neonene commented Jun 21, 2024

Copy link
Copy Markdown
Contributor Author

@ericsnowcurrently ericsnowcurrently 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.

LGTM

@Eclips4 Eclips4 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.

Thanks!

@kumaraditya303
kumaraditya303 merged commit a81d434 into python:main Jun 21, 2024
_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

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

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

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

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

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.

@neonene

neonene commented Jun 21, 2024

Copy link
Copy Markdown
Contributor Author

Thanks. I'll make a backport PR to 3.13 manually with an assertion to the test case.

@bedevere-app

bedevere-app Bot commented Jun 21, 2024

Copy link
Copy Markdown

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

@neonene
neonene deleted the reload branch June 21, 2024 20:13
kumaraditya303 pushed a commit that referenced this pull request Jul 3, 2024
…H-120829) (#120855)

* [3.13] gh-120782: Update internal type cache when reloading datetime

When reloading _datetime module, the single-phase version did not invoke the PyInit__datetime function, whereas the current multi-phase version updates the static types through the module init. The outdated static type cache in the interpreter state needs to be invalidated at the end of reloading the multi-phase module.
@bedevere-bot

Copy link
Copy Markdown

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.

6 participants