Skip to content

Conversation

@Mysvac
Copy link
Contributor

@Mysvac Mysvac commented Jan 23, 2026

Objective

Fix incorrect TypeId retrieval in ComponentCloneCtx::write_target_component_reflect().

When cloning reflected components, the code incorrectly called .type_id() on a Box<dyn Reflect>, which returns the container's TypeId rather than the underlying concrete type's TypeId.

For example:

let boxed: Box<dyn Reflect> = Box::new(String::from("test"));

// Correct: dereferencing gets the concrete type's TypeId
assert_eq!((*boxed).type_id(), TypeId::of::<String>());

// Wrong: calling on Box returns something else
assert_ne!(boxed.type_id(), TypeId::of::<String>());

See Rust documentation: https://doc.rust-lang.org/std/any/index.html#smart-pointers-and-dyn-any.

Solution

Dereference the Box<dyn Reflect> before calling .type_id() to obtain the concrete type's identifier.

Changed from:

let component_type_id = component.type_id();  // Wrong: returns Box's TypeId

To:

let component_type_id = (*component).type_id();  // Correct: returns concrete type's TypeId

Testing

  • cargo fmt --check
  • cargo test -p bevy_reflect
  • cargo test -p bevy_ecs

Additional Notes

This pattern is error-prone. A safer long-term solution would be for Reflect trait to provide an unambiguous method like typeid() or reflect_type_id() that clearly indicates it returns the underlying type's identifier, not the trait object's.


Code Change

View the specific change

File: crates/bevy_ecs/src/entity/clone_entities.rs

 pub fn write_target_component_reflect(&mut self, component: Box<dyn bevy_reflect::Reflect>) {
     /* ... */
     let source_type_id = self
         .component_info
         .type_id()
         .expect("Source component must have TypeId");
     
-    let component_type_id = component.type_id();
+    let component_type_id = (*component).type_id();
     
     if source_type_id != component_type_id {
         panic!("Passed component TypeId does not match source component TypeId")
     }
     /* ... */
 }

@eugineerd
Copy link
Contributor

eugineerd commented Jan 23, 2026

I'm a bit confused about this.

  1. The provided example doesn't run (assert_ne! fails)
  2. Tests already cover this path, and if type ids were incorrect they would've panicked

@Mysvac
Copy link
Contributor Author

Mysvac commented Jan 23, 2026

Okay, that's true, but I've discovered a more troublesome issue.

If the scope does not import Any, then boxed.type_id() will call type_id() of dyn Reflect. However, if core::any::Any is imported, then boxed.type_id() will call type_id() of Box<dyn Reflect>.

#[test]
fn unused_any() {
    use bevy_reflect::Reflect;

    let boxed: Box<dyn Reflect> = Box::new(String::from("test"));
    assert_eq!((*boxed).type_id(), TypeId::of::<String>());
    assert_eq!(boxed.type_id(), TypeId::of::<String>());
}

#[test]
fn used_any() {
    use bevy_reflect::Reflect;
    use core::any::Any;

    let boxed: Box<dyn Reflect> = Box::new(String::from("test"));
    assert_eq!((*boxed).type_id(), TypeId::of::<String>());
    assert_ne!(boxed.type_id(), TypeId::of::<String>()); // !!!
}

The source code does not import Any, so it can run normally, but this is quite "subtle".

@eugineerd
Copy link
Contributor

eugineerd commented Jan 23, 2026

If the scope does not import Any, then boxed.type_id() will call type_id() of dyn Reflect. However, if core::any::Any is imported, then boxed.type_id() will call type_id() of Box<dyn Reflect>.

Oh, that's true. Looks like clippy catches this by default at least:

warning: calling `.type_id()` on `Box<dyn Reflect>`
   --> crates/bevy_ecs/src/entity/clone_entities.rs:247:33
    |
247 |         let component_type_id = component.type_id();
    |                                 ---------^^^^^^^^^^
    |                                 |
    |                                 help: consider dereferencing first: `(*component)`
    |
    = note: this returns the type id of the literal type `Box<_>` instead of the type id of the boxed value, which is most likely not what you want
    = note: if this is intentional, use `TypeId::of::<Box<dyn Reflect>>()` instead, which makes it more clear
    = help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.92.0/index.html#type_id_on_box
    = note: `#[warn(clippy::type_id_on_box)]` on by default

@Mysvac Mysvac closed this Jan 23, 2026
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