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.

Sign up for GitHub

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

Jump to bottom

Query's ExactSizeIterator does not account for partial iteration #5149

Closed
alice-i-cecile opened this issue Jun 30, 2022 · 1 comment
Closed

Query's ExactSizeIterator does not account for partial iteration #5149

alice-i-cecile opened this issue Jun 30, 2022 · 1 comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior

Comments

@alice-i-cecile
Copy link
Member

Bevy version

0.8-dev

Problem

The [ExactSizeIterator] docs imply that the value returned by len() should change as the iterator is partially consumed.

Our implementations do not do this.

We should carefully check to ensure that our size_hint methods are correct too.

Possible solutions

Solution 1: track directly

This solution solves the problem at hand, but creates unavoidable overhead in cases where we don't care.

Solution 2: create a ExactSizeQueryIter type

Only implement the ExactSizeIterator trait for this type.

Avoids overhead, gains the correct behavior, but ergonomics are a bit meh.
If we do this, this must be documented carefully.

Solution 3: scrap the ExactSizeIterator trait, and implement len() directly

This is a bit sad, because it reduces compatibility with other ecosystem crates that care about this trait.

Solution 4: YOLO

Keep the current behavior that does not comply with the spec. The docs do say you can't rely on this for soundness...
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Jun 30, 2022
This was referenced Jun 30, 2022
Closed
Closed
@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Jul 1, 2022 •
edited
Loading

Confirmed that our behavior is a bug: rust-lang/rust#98729

nicopap added a commit to nicopap/bevy that referenced this issue Jul 5, 2022
@nicopap
Fix size_hint for partially consumed QueryIter
888a1ce 
Instead of returning the total count of elements in the `QueryIter` in
`size_hint`, we return the count of remaining elements in it. This
Fixes bevyengine#5149 (although, if bevyengine#5148 is merged, bevyengine#5149 will re-open)

- bevyengine#5149
- bevyengine#5148
@nicopap nicopap mentioned this issue Jul 5, 2022
Closed
nicopap added a commit to nicopap/bevy that referenced this issue Jul 5, 2022
@nicopap
Fix size_hint for partially consumed QueryIter
ee4a84b 
Instead of returning the total count of elements in the `QueryIter` in
`size_hint`, we return the count of remaining elements in it. This
Fixes bevyengine#5149 (although, if bevyengine#5148 is merged, bevyengine#5149 will re-open)

- bevyengine#5149
- bevyengine#5148
nicopap added a commit to nicopap/bevy that referenced this issue Jul 5, 2022
@nicopap
Fix size_hint for partially consumed QueryIter
c94a1b3 
Instead of returning the total count of elements in the `QueryIter` in
`size_hint`, we return the count of remaining elements in it. This
Fixes bevyengine#5149. This is also true of `QueryCombinationIter`.

- bevyengine#5149
- bevyengine#5148
nicopap added a commit to nicopap/bevy that referenced this issue Jul 5, 2022
@nicopap
Fix size_hint for partially consumed QueryIter
14173c2 
Instead of returning the total count of elements in the `QueryIter` in
`size_hint`, we return the count of remaining elements in it. This
Fixes bevyengine#5149. This is also true of `QueryCombinationIter`.

- bevyengine#5149
- bevyengine#5148
nicopap added a commit to nicopap/bevy that referenced this issue Jul 13, 2022
@nicopap
Fix size_hint for partially consumed QueryIter
26adeab 
Instead of returning the total count of elements in the `QueryIter` in
`size_hint`, we return the count of remaining elements in it. This
Fixes bevyengine#5149. This is also true of `QueryCombinationIter`.

- bevyengine#5149
- bevyengine#5148
nicopap added a commit to nicopap/bevy that referenced this issue Jul 19, 2022
@nicopap
Fix size_hint for partially consumed QueryIter
83a1d5c 
Instead of returning the total count of elements in the `QueryIter` in
`size_hint`, we return the count of remaining elements in it. This
Fixes bevyengine#5149. This is also true of `QueryCombinationIter`.

- bevyengine#5149
- bevyengine#5148
nicopap added a commit to nicopap/bevy that referenced this issue Jul 22, 2022
@nicopap
Fix size_hint for partially consumed QueryIter
0ebf827 
Instead of returning the total count of elements in the `QueryIter` in
`size_hint`, we return the count of remaining elements in it. This
Fixes bevyengine#5149. This is also true of `QueryCombinationIter`.

- bevyengine#5149
- bevyengine#5148
nicopap added a commit to nicopap/bevy that referenced this issue Sep 1, 2022
@nicopap
Fix size_hint for partially consumed QueryIter
f8787f8 
Instead of returning the total count of elements in the `QueryIter` in
`size_hint`, we return the count of remaining elements in it. This
Fixes bevyengine#5149. This is also true of `QueryCombinationIter`.

- bevyengine#5149
- bevyengine#5148
nicopap added a commit to nicopap/bevy that referenced this issue Nov 1, 2022
@nicopap
Fix size_hint for partially consumed QueryIter
3880ef5 
Instead of returning the total count of elements in the `QueryIter` in
`size_hint`, we return the count of remaining elements in it. This
Fixes bevyengine#5149. This is also true of `QueryCombinationIter`.

- bevyengine#5149
- bevyengine#5148
@james7132 james7132 mentioned this issue Nov 12, 2022
Closed
@bors bors bot closed this as completed in 15ea93a Nov 21, 2022
cart pushed a commit that referenced this issue Nov 30, 2022
@nicopap @cart
Fix size_hint for partially consumed QueryIter and QueryCombinationIt…
a2c0f65 
…er (#5214)

# Objective

Fix #5149

## Solution

Instead of returning the **total count** of elements in the `QueryIter` in
`size_hint`, we return the **count of remaining elements**. This
Fixes #5149 even when #5148 gets merged.

- #5149
- #5148

---

## Changelog

- Fix partially consumed `QueryIter` and `QueryCombinationIter` having invalid `size_hint`


Co-authored-by: Nicola Papale <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@nicopap @ItsDoot
Fix size_hint for partially consumed QueryIter and QueryCombinationIt…
1321cba 
…er (bevyengine#5214)

# Objective

Fix bevyengine#5149

## Solution

Instead of returning the **total count** of elements in the `QueryIter` in
`size_hint`, we return the **count of remaining elements**. This
Fixes bevyengine#5149 even when bevyengine#5148 gets merged.

- bevyengine#5149
- bevyengine#5148

---

## Changelog

- Fix partially consumed `QueryIter` and `QueryCombinationIter` having invalid `size_hint`


Co-authored-by: Nicola Papale <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior
Projects
None yet
Milestone
No milestone
1 participant
@alice-i-cecile