Skip to content

Fix KeyError in get_components() when the smallest set is found last - #122

Open
uttam12331 wants to merge 1 commit into
benmoran56:masterfrom
uttam12331:fix-get-components-smallest-set-keyerror
Open

Fix KeyError in get_components() when the smallest set is found last#122
uttam12331 wants to merge 1 commit into
benmoran56:masterfrom
uttam12331:fix-get-components-smallest-set-keyerror

Conversation

@uttam12331

Copy link
Copy Markdown

Summary

get_components() can raise a KeyError for a perfectly valid query when three or more component types are requested.

_get_components() iterates the smallest component set first and sets the other requested types aside for a membership check. When it finds a new smallest set, it moves the previously-smallest type into other_types. It computed that type with an index into the query tuple:

if set_size < min_size:
    if min_set is not None:
        other_types.append(component_types[len(other_types)])  # wrong once a type was already set aside

component_types[len(other_types)] only points at the previous minimum while nothing has gone through the else branch yet. Once a larger set has already been appended to other_types, the index is off — it re-appends an already-present type and the real previous-smallest type is silently dropped.

The dropped type is then never used to filter, but it is still read when the result tuple is built (tuple(entity_comps[ct] for ct in component_types)), so an entity that matches on the remaining types but lacks the dropped one raises a KeyError instead of being skipped.

Reproduce

import esper

class Pos: pass
class Vel: pass
class Tag: pass

esper.create_entity(Vel(), Tag())      # has Vel + Tag, but NOT Pos
esper.create_entity(Pos()); esper.create_entity(Pos())
esper.create_entity(Vel()); esper.create_entity(Vel())

esper.get_components(Pos, Vel, Tag)    # expected: []  -> actual: KeyError: <class 'Pos'>

No entity has all three components, so the correct result is an empty list. Instead it crashes: Pos is dropped from the internal filter, so the Vel+Tag entity passes the check and then blows up building entity_comps[Pos].

Fix

Track the previous-smallest type directly instead of deriving it from an index:

min_type = None
...
if set_size < min_size:
    if min_type is not None:
        other_types.append(min_type)
    min_size = set_size
    min_set = comp_set
    min_type = ct

The smallest-set-first scan was added as a performance optimization (#116); this keeps that optimization but makes the "demote the old minimum" step correct.

Verification

  • Added test_get_components_smallest_set_missing_component, which fails on master (KeyError) and passes with the fix.
  • Full test suite: 54 passed.
  • mypy esper: clean.
  • A randomized fuzz over ~3000 queries (2–5 types) against a brute-force reference: 0 crashes and 0 mismatches with the fix (72 crashes without it).

_get_components() scans the requested component types to iterate the
smallest component set first, setting the other types aside for a
membership check. When it found a new smallest set, it tried to move the
previous smallest type into other_types using an index into
component_types (component_types[len(other_types)]). That index is only
correct while no type has already been set aside; once one has, it points
at the wrong type and the real previous-smallest type is dropped.

The dropped type is then never checked, so an entity that matches on the
remaining types but lacks it slips through and raises a KeyError while
building the result tuple. For example, querying three types where the
final (smallest) set contains an entity missing the first type crashes
instead of returning the correct matches.

Track the previous smallest type directly instead of computing it from an
index. Adds a regression test covering the size-ordering path.
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.

1 participant