Skip to content

Fix out-of-bounds molidx searches silently returning the last molecule#432

Open
lohedges wants to merge 2 commits intodevelfrom
fix_286
Open

Fix out-of-bounds molidx searches silently returning the last molecule#432
lohedges wants to merge 2 commits intodevelfrom
fix_286

Conversation

@lohedges
Copy link
Copy Markdown
Contributor

@lohedges lohedges commented Apr 17, 2026

This PR closes #286, fixing a bug where out-of-bounds molidx searches silently returned the last molecule instead of raising a KeyError. The root cause was in IDIndexEngine::searchMolIdx(): unlike searchIdx<T> (used for atoms, residues, etc.), it had no single-value fast path and instead fell through to a match() call that used slice.begin(count, true) with auto_fix=true, silently clamping any out-of-bounds index to the last valid position. The fix adds a _is_single_value fast path to searchMolIdx that reads the raw index directly from RangeValue::start (bypassing the _to_single_value helper, which corrupts negative indices by mapping them against INT_MAX) and applies strict Python-style bounds checking. Negative indices within range (e.g. -1) continue to work correctly, while out-of-bounds values in either direction now return no match, consistent with residx and atomidx behaviour. A secondary bug in MolIdx::map(const Molecules &) where the loop counter i was never decremented is also fixed. A regression test is included.

Debugged with assistance from Claude Code Opus 4.6.

@chryswoods: Just want to address you concerns about different container types and use case. In the C++ layer we have MolIdx::map(const Molecules&), where Molecules is a QHash<MolNum, ViewsOfMol>, so has no stable iteration order. I think this isn't an issue for user-facing code, since it goes through another pathway, but good to check. The comment referencing static map() in the issue thread, this pathway isn't touched by this fix, so was a red-herring, i.e. that's used for ranges and comparisons only. For the additional bug fix: I don't think this was ever an issue since this low-level overload probably wasn't used by any callers.

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
  • I confirm that I have added a changelog entry to the changelog (we will add a link to this PR as part of the review): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

@lohedges lohedges added the bug Something isn't working label Apr 17, 2026
@lohedges lohedges requested a review from chryswoods April 17, 2026 14:00
@lohedges lohedges deployed to sire-build April 17, 2026 14:00 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Sire selection allows out-of-bounds indexing of molecules

1 participant