Skip to content
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.

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

bevy_reflect::List::iter wraps silently on release #13230

Closed
TheNeikos opened this issue May 4, 2024 · 0 comments · Fixed by #13271
Closed

bevy_reflect::List::iter wraps silently on release #13230

TheNeikos opened this issue May 4, 2024 · 0 comments · Fixed by #13271
Labels
A-Reflection Runtime information about types C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon

Comments

@TheNeikos
Copy link
Contributor

TheNeikos commented May 4, 2024

Bevy version

0.13

What you did

Iterate over a ListIter until overflow of its usize.

What went wrong

You are back at the beginning.

Proposed Fix

ListIter should only increment its counter when the current index is Some. This would fix this behaviour. It does add a branch in a potential hot spot, but I don't think its much worse when you are already reflect-ing all over the place.

Repro

https://learnbevy.com/playground?share=679a8e4ac4eb4c72a2e1e16114eb8717da4257eb6068becaecd7ce45c51ce2be

let b = Box::new(vec![1u32]).into_reflect();

let ReflectRef::List(list) = b.reflect_ref() else {
    panic!("Not a list...");
};

let mut iter = list.iter();

let one = iter.next().unwrap();
assert_eq!(one.downcast_ref::<u32>().unwrap(), &1);

for _ in 0..(usize::MAX) {
    assert!(iter.next().is_none());
}

// We wrap around since thats what it does in release

let one = iter.next().unwrap();
assert_eq!(one.downcast_ref::<u32>().unwrap(), &1);

// Oops!

info!("We got {one:?}")
@TheNeikos TheNeikos added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels May 4, 2024
@alice-i-cecile alice-i-cecile added 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 and removed S-Needs-Triage This issue needs to be labelled labels May 4, 2024
github-merge-queue bot pushed a commit that referenced this issue May 12, 2024
# Objective
Fixes  #13230

## Solution
Uses solution described in  #13230
They mention a worry about adding a branch, but I'm not sure there is
one.

This code
```Rust
#[no_mangle]
pub fn next_if_some(num: i32, b: Option<bool>) -> i32 {
    num + b.is_some() as i32
}
```
produces this assembly with opt-level 3
```asm
next_if_some:
        xor     eax, eax
        cmp     sil, 2
        setne   al
        add     eax, edi
        ret
```

## Testing
Added test from #13230, tagged it as ignore as it is only useful in
release mode and very slow if you accidentally invoke it in debug mode.

---

## Changelog
Iterationg of ListIter will no longer overflow and wrap around

## Migration Guide
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-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants