Bug: CollectionState.sweep_for_events() semantics may have been accidentally changed in #361

[Mysteryem]

What happened?

I was looking at the history of CollectionState.sweep_for_events() to try to understand it better and noticed that the semantics changed in #361 (cebd7fb54597e21310d41686ee0e8d21d0f85596) and the change does not look intentional to me.

The semantics for reachable_events were previously equivalent to: A and ((not B) or C) and D

{location for location in locations
 if location.event
 and (not key_only or getattr(location.item, "locked_dungeon_item", False)
 and location.can_reach(self)}

But in #361 (cebd7fb54597e21310d41686ee0e8d21d0f85596), the semantics changed to be equivalent to: ((A and (not B)) or C) and D

{location for location in locations
 if ((location.event and not key_only)
     or getattr(location.item, "locked_dungeon_item", False))
 and location.can_reach(self)}

---

This change in semantics is still present in the sweep_for_events() implementation today (https://github.com/ArchipelagoMW/Archipelago/blob/9c2933f8033c8e8b9d9acdd341b043d7eca89d76/BaseClasses.py#L688C9-L689C91):

locations = {location for location in locations if location.advancement and location not in self.events and
             not key_only or getattr(location.item, "locked_dungeon_item", False)}

Going by the code prior to #361, this should likely instead be

locations = {location for location in locations if location.advancement and location not in self.events and
             (not key_only or getattr(location.item, "locked_dungeon_item", False))}

---

I'm not sure what the implications of this being an unintended change are. There appear to be only two places which use key_only=True, so these and any items for which locked_dungeon_item exists and returns True, are likely the only places affected by the change in semantics. https://github.com/ArchipelagoMW/Archipelago/blob/9c2933f8033c8e8b9d9acdd341b043d7eca89d76/BaseClasses.py#L1294 and https://github.com/ArchipelagoMW/Archipelago/blob/9c2933f8033c8e8b9d9acdd341b043d7eca89d76/Fill.py#L649

What were the expected results?

361 seems to have been intended as a performance change, not one that should have changed semantics.

Software

Local generation

[Exempt-Medic]

@Berserker66 I suppose

[Berserker66]

related fix: https://github.com/ArchipelagoMW/Archipelago/pull/2239

[Exempt-Medic]

So is this fixed now?

[Mysteryem]

2239 was merged which removes the key_only parameter, so the code that may have had unintentional changes to its semantics has been removed, so I'm closing this issue.