core: bond_breakage: handle ActionType::NONE as a no-op (bug-sweep #33)#5364
core: bond_breakage: handle ActionType::NONE as a no-op (bug-sweep #33)#5364RudolfWeeber wants to merge 3 commits into
Conversation
For an ActionType::NONE pair-bond queue entry, actions_for_breakage fell
through to the REVERT/angle-bond else branch and unconditionally evaluated
*(e.bond_partners[1]), which is std::nullopt for a pair bond. This is a
read of an empty std::optional (undefined behaviour). It traps under
_GLIBCXX_ASSERTIONS or a sanitizer, and even in a plain build it
dereferences the value-initialized payload 0, resolving to a real
non-virtual particle and emitting a spurious REVERT_BIND_AT_POINT_OF_
COLLISION runtime error (or, in other configurations, deleting the wrong
bond). NONE entries always queue because check_and_handle_breakage never
inspects action_type and breakage_length defaults to 0.0.
ActionType::NONE is a documented, valid configuration ("none" in the
Python/script-interface layer) and is the value of a default-constructed
BreakageSpec, so the correct behaviour is to do nothing. Add an early
return of an empty action set for ActionType::NONE at the top of
actions_for_breakage. This also resolves finding #34 (NONE on a virtual-
site angle bond silently deleting bonds via the same else branch).
Add a regression test (none_action_pair_bond_is_noop) that queues a NONE
pair-bond entry and checks process_queue is a clean no-op: no exception,
no runtime error, and the particles' bonds are preserved.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eakage-empty-optional
|
I agree that this is a bug. The proposed patch is incorrect, though: the bug is actually in the if/else specification, and adding a new bond breakage protocol after the virtual sites breakage handler will necessarily make this bug re-surface. I have rewritten the if/else to properly fall through when a specific bond breakage protocol requires no action in the affected function. I also took the liberty of moving the integration test out of the unit test and instead into the python test. This way I can highlight another bug in the bond breakage protocol: with @RudolfWeeber Please try out |
For an ActionType::NONE pair-bond queue entry, actions_for_breakage fell
through to the REVERT/angle-bond else branch and unconditionally evaluated
*(e.bond_partners[1]), which is std::nullopt for a pair bond. This is a
read of an empty std::optional (undefined behaviour). It traps under
GLIBCXX_ASSERTIONS or a sanitizer, and even in a plain build it
dereferences the value-initialized payload 0, resolving to a real
non-virtual particle and emitting a spurious REVERT_BIND_AT_POINT_OF
COLLISION runtime error (or, in other configurations, deleting the wrong
bond). NONE entries always queue because check_and_handle_breakage never
inspects action_type and breakage_length defaults to 0.0.
ActionType::NONE is a documented, valid configuration ("none" in the
Python/script-interface layer) and is the value of a default-constructed
BreakageSpec, so the correct behaviour is to do nothing. Add an early
return of an empty action set for ActionType::NONE at the top of
actions_for_breakage. This also resolves finding #34 (NONE on a virtual-
site angle bond silently deleting bonds via the same else branch).
Add a regression test (none_action_pair_bond_is_noop) that queues a NONE
pair-bond entry and checks process_queue is a clean no-op: no exception,
no runtime error, and the particles' bonds are preserved.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
🤖 Generated with Claude Code