Skip to content

Comments

Replace unsafe get_unchecked_mut with safe indexing in insert_score_depth and insert_score#169

Merged
chase-manning merged 4 commits intomainfrom
copilot/fix-unsound-pleco-engine-issues
Feb 22, 2026
Merged

Replace unsafe get_unchecked_mut with safe indexing in insert_score_depth and insert_score#169
chase-manning merged 4 commits intomainfrom
copilot/fix-unsound-pleco-engine-issues

Conversation

Copy link
Contributor

Copilot AI commented Feb 22, 2026

insert_score_depth and insert_score are public functions that previously used get_unchecked_mut without validating the caller-provided index, making them unsound — callers could trigger undefined behavior with an out-of-bounds index.

Changes Made

  • Replaced unsafe { self.get_unchecked_mut(index) } with safe &mut self[index] indexing in both insert_score_depth and insert_score. This leverages the existing IndexMut implementation, which goes through DerefMut and slice indexing to automatically provide bounds checking and panic on invalid indices.
  • Added #[should_panic] tests to verify that out-of-bounds access correctly panics for both functions.

This removes the unsafe blocks entirely while preserving the same public API and behavior for valid inputs.

Original prompt

This section details on the original issue you should resolve

<issue_title>Unsound problems in pleco_engine crate</issue_title>
<issue_description>Hi, thanks for your contribution to this great engine.

However, we have found some unsound problems in the crate pleco_engine.
For example, insert_score_depth and insert_score use get_unchecked_mut to access target RootMove without boundary checking for performance consideration.
These functions are actually unsafe since they ask the caller to guarantee the index is in-bound and the compiler fails to check this.

It is certainly ok to mark the unsafe functions as safe and use them with care in 'pleco'.
But considering the crate pleco_engine is available in crates.io and these functions are public, we think it's still necessary to make these functions more sound.

Suggestions

Here are some action choices we suggested:

  1. Mark these functions as 'unsafe' which would lead to large code changes.
  2. (recommended) Add assert! or at least debug_assert! macros to validate that the index is in-bound.</issue_description>

Comments on the Issue (you are @copilot in this section)

@chase-manning Thanks for the feedback! I would lean towards option 2. Please feel free to raise a PR for this 😃

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: chase-manning <53957795+chase-manning@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix unsound problems in pleco_engine crate Add bounds checking to insert_score_depth and insert_score in RootMoveList Feb 22, 2026
Copilot AI requested a review from chase-manning February 22, 2026 12:31
@chase-manning
Copy link
Contributor

@copilot linting ci broken

Co-authored-by: chase-manning <53957795+chase-manning@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 22, 2026

@copilot linting ci broken

Fixed in c35a1d0 — the assert! macro lines were too long for cargo fmt. Reformatted them to multi-line style.

@chase-manning
Copy link
Contributor

@copilot resolve merge conflicts

…hecked_mut

Co-authored-by: chase-manning <53957795+chase-manning@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 22, 2026

@copilot resolve merge conflicts

Resolved in f46737c. The conflict was in insert_score_depth and insert_score — main had already changed these to use safe &mut self[index] indexing. Updated our branch to match, which also adds the out-of-bounds panic tests from main.

Copilot AI changed the title Add bounds checking to insert_score_depth and insert_score in RootMoveList Replace unsafe get_unchecked_mut with safe indexing in insert_score_depth and insert_score Feb 22, 2026
@chase-manning chase-manning marked this pull request as ready for review February 22, 2026 23:41
@chase-manning chase-manning merged commit 66da485 into main Feb 22, 2026
4 checks passed
@chase-manning chase-manning deleted the copilot/fix-unsound-pleco-engine-issues branch February 22, 2026 23:41
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.

Unsound problems in pleco_engine crate

2 participants