Skip to content

Conversation

@alice-i-cecile
Copy link
Member

Objective

Solution

  1. Deprecate the methods.
  2. Fix the docs.
  3. Remove internal usages.
  4. Add a migration guide.

Testing

No more deprecation warnings!

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation C-Code-Quality A section of code that is hard to understand or change A-Reflection Runtime information about types X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 7, 2025
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

looks good. I'm fine with leaving deprecating the any methods to an issue.

@alice-i-cecile alice-i-cecile requested a review from kaoet November 9, 2025 17:46
@alice-i-cecile
Copy link
Member Author

I've gone ahead and removed the Any downcasting methods too while we're here. Good eye!

#[inline]
pub fn downcast_ref<T: Any>(&self) -> Option<&T> {
self.as_any().downcast_ref::<T>()
let any = self as &dyn Any;
Copy link
Member Author

Choose a reason for hiding this comment

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

Splitting this out explicitly is required to disambiguate and avoid infinite recursion.

@hymm
Copy link
Contributor

hymm commented Nov 10, 2025

Changes look good.

//! Since `T: Reflect` implies `T: PartialReflect`, conversion from a `dyn Reflect` to a `dyn PartialReflect`
//! trait object (upcasting) is infallible and can be performed with one of the following methods.
//! Note that these are temporary while [the language feature for dyn upcasting coercion] is experimental:
//! * [`PartialReflect::as_partial_reflect`] for `&dyn PartialReflect`
Copy link
Contributor

Choose a reason for hiding this comment

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

These removed docs are for PartialReflect::*, which are not deprecated in this PR.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 10, 2025
@alice-i-cecile alice-i-cecile added this to the 0.18 milestone Nov 10, 2025
@alice-i-cecile alice-i-cecile added S-Needs-Help The author needs help finishing this PR. and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Nov 17, 2025
@alice-i-cecile
Copy link
Member Author

I'm getting a number of stack overflows, but I can't quite figure out what's going wrong. I would appreciate a hand spotting where I've made a mistake.

Moving to a very explicit set of changes that explicitly coerces everywhere is probably best, but I would prefer to have a better understanding rather than just blindly making this change.

@kaoet
Copy link
Contributor

kaoet commented Nov 17, 2025

I'm getting a number of stack overflows, but I can't quite figure out what's going wrong. I would appreciate a hand spotting where I've made a mistake.

The stack overflow is caused by downcast() in impl dyn Reflect calling itself.

BUT. When I tried to fix that function, several tests under bevy_reflect start to fail. One example is tests::should_reflect_remote_value_type.

It seems that as_any, as_any_mut, and into_any are not always equivalent to coercion and should not be deprecated. As the doc of them says, they have special behavior for remote wrapper types.

#[inline]
fn into_any(self: #bevy_reflect_path::__macro_exports::alloc_utils::Box<Self>) -> #bevy_reflect_path::__macro_exports::alloc_utils::Box<dyn #FQAny> {
#bevy_reflect_path::__macro_exports::alloc_utils::Box::new(self.0)
}
#[inline]
fn as_any(&self) -> &dyn #FQAny {
&self.0
}
#[inline]
fn as_any_mut(&mut self) -> &mut dyn #FQAny {
&mut self.0
}

Sorry for suggesting deprecating them at first. 🤣

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

Labels

A-Reflection Runtime information about types C-Code-Quality A section of code that is hard to understand or change C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Help The author needs help finishing this PR. X-Uncontroversial This work is generally agreed upon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove dedicated methods for upcasting from bevy_reflect

3 participants