Skip to content

Conversation

davidhewitt
Copy link
Member

As per #5468 (comment) this actions the planned deprecation of .downcast() and similar methods.

I reverted their behavioural change in #5387 so that these methods will not change when users upgrade, just be deprecated.

Once they migrate to the .cast() functions they will then get better ergonmics:

T: PyTypeCheck,
{
self.cast()
if T::type_check(self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think this implies T has to be unsafe, because otherwise it should be legal to have a PyTypeCheck::type_check that always returned true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ungh, you're right. PyTypeCheck should be an unsafe trait. 🤦

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that:

  • A user would have to write a PyTypeCheck implementation and then cast to that type, which feels low risk to be an exploit.
  • Changing the trait definition in a patch release is breaking.

I think we just fix this for 0.27? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems fine, I don't think there's a lot of people implementing themselves so it'd be low risk for 0.27

@davidhewitt
Copy link
Member Author

Note to self: I think this probably warrants a migration guide entry (maybe can be written in #5468 and describe both).

Copy link

codspeed-hq bot commented Sep 27, 2025

CodSpeed Performance Report

Merging #5472 will not alter performance

Comparing davidhewitt:deprecate-downcast (8bc8792) with main (bf22000)

Summary

✅ 98 untouched

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.

2 participants