Skip to content

Fix array search equality and reentrant callbacks - #8870

Merged
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:array_search_reentry
Sep 27, 2026
Merged

Fix array search equality and reentrant callbacks#8870
youknowone merged 1 commit into
RustPython:mainfrom
1ndahous3:array_search_reentry

Conversation

@1ndahous3

@1ndahous3 1ndahous3 commented Sep 27, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Compare array search values using Python equality. Equal integers and floats now agree across count, index, remove, and membership; single-precision arrays no longer round the search value into a false match.
  • Propagate comparison errors and keep searches valid when equality callbacks change the array. Preserve native scans for exact builtin numbers and strings, including invalid Unicode error ordering.
  • Run append/insert value conversion and generic extend callbacks outside array locks. Recheck exported buffers after conversion and preserve completed extension progress if iteration fails.

Related to #6549; follows up on the closed #6576.

Performance

Release build, Windows x64; medians of seven samples against the same main revision.

Operation Before After Speedup
Missing integer membership in a 4,096-element array 99.1 µs 1.29 µs 76.8×
Finding the last integer in a 4,096-element array 2.40 µs 1.37 µs 1.75×

AI assistance

Written with Codex (GPT-6), reviewed by a human before submission.

Summary by CodeRabbit

  • Bug Fixes
    • Array searches now better match Python equality behavior, including for numbers and custom comparisons.
    • Search and containment operations now report comparison and validation errors instead of silently ignoring them.
    • index supports optional start and stop bounds, including negative indices.
    • Array updates handle conversions and mutations during comparisons or iteration more consistently.

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml

Review profile: CHILL

Plan: Advanced

Run ID: 07c187f0-8026-4c2a-a124-9697d7d355a9

📥 Commits

Reviewing files that changed from the base of the PR and between 456a8dc and f4e74ad.

📒 Files selected for processing (2)
  • crates/stdlib/src/array.rs
  • extra_tests/snippets/stdlib_array.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📝 Walkthrough

Walkthrough

Array search methods now support typed exact searches and Python equality, with comparison errors propagated. The changes also update index bounds and array mutation paths, and add tests for search callbacks, conversion callbacks, and iterator mutations.

Changes

Array search and mutation updates

Layer / File(s) Summary
Search semantics and APIs
crates/stdlib/src/array.rs, extra_tests/snippets/stdlib_array.py		 Searches use exact typed paths when supported and Python equality otherwise. count, find, index, remove, and containment propagate comparison errors. Unicode searches validate visited values. index accepts optional start and stop bounds. Tests cover equality behavior, Unicode errors, and mutations during comparisons.
Conversion and iterable mutation paths
crates/stdlib/src/array.rs, extra_tests/snippets/stdlib_array.py		 append and insert convert values before checking resizability. Iterable extend appends items individually. Tests cover conversion callbacks, buffer exports, and iterator mutations followed by an exception.

Priority: ⚪ Not assessed

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: youknowone

Merge Risk: ⚪ Minimal · up to f4e74

No identified array behavior issue prevents merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f4e74

Array searches can now invoke Python comparison callbacks, but the reviewed mutation paths still check exported buffers before resizing. No privilege escalation or wider-system exposure was established. Some coverage remains incomplete.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated callback and mutation effects are within an array and its Python VM execution context. The available dependency evidence does not establish a newly reachable privileged service or data-store sink.

Trust Boundaries and Controls

  • observed — Non-exact search values can reach Python equality callbacks. The reviewed resize paths invoke those callbacks without holding the array lock and still reject resizing while buffers are exported.

Resilience and Maintainability Implications

  • inferred — Reentrant equality can shift an element between a successful comparison and remove's deletion; the length checks prevent out-of-bounds deletion, but deletion uses the current position rather than preserving element identity. The available compatibility counterexample supports this positional behavior, so it is not retained as a PR security concern.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.53% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main changes: correcting array search equality behavior and handling reentrant callbacks.
Linked Issues check ✅ Passed No active, directly linked issue targets remain. Issue #6576 is closed and supplies historical context only. Its acceptance criteria do not create coding requirements for this pull request.
Out of Scope Changes check ✅ Passed The changes stay within the stated array-search and reentrant-callback scope. The implementation updates search equality and error handling, moves append and insert conversion before resize locking, a…
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@1ndahous3

1ndahous3 commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

@youknowone FYI this and some neighbor PRs failed CI on i686-linux* with assert introduced in #8857 (align_of::<AtomicU64>() == align_of::<u64>())

youknowone
youknowone approved these changes Sep 27, 2026
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@youknowone

youknowone commented Sep 27, 2026

Copy link
Copy Markdown
Member

@1ndahous3 Thanks!

@youknowone
youknowone merged commit afae370 into RustPython:main Sep 27, 2026
28 of 30 checks passed
@1ndahous3
1ndahous3 deleted the array_search_reentry branch September 28, 2026 16:09
@1ndahous3 1ndahous3 mentioned this pull request Oct 9, 2026
Merged
1 task done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@1ndahous3 @youknowone