Sync upstream/main: merge PR 13 (the bounds test reads memory it owns, the options test covers all three decorators)

[nelson2005]

Merges upstream/main at 2af2c6d, the merge of Goykhman#13, into fork main. Git merged it clean, and the tree is identical to a683dd3, the head of #36, which ran green on the full fork matrix.

[github-actions]

MiniMax Code Review

Let me analyze this pull request to understand what it's trying to do.

The changes are in two files:

1. .github/scripts/mutation_guard_check.py - This appears to be a test file that checks for mutations in the codebase. It's adding new test cases.
2. test/test_cache.py - This is adding new tests for JIT options reaching decorators.

Let me understand what the PR is trying to fix:

Looking at the mutation_guard_check.py additions:

1. Three new test cases for is_null.py - testing that decorators (is_null, unpack_booleans, is_null_struct) stop reading options properly
2. Two new test cases for configurations.py - testing that cache option reaches unpack_booleans and that decorators forward only cache and boundscheck options

Looking at the test changes in test_cache.py:

1. Changed the bitmap size from 1 byte to 100000//8+1 bytes but then slices to [:1], which is still 1 byte. This seems odd - let me re-read this.

   • np.zeros(100000 // 8 + 1, dtype=np.uint8)[:1] creates an array of 12501 bytes, then slices to get first byte
   • The comment explains: "The bitmap is the first byte of an array that holds every byte the calls reach, so without bounds checking they read past the bitmap and stay inside memory the array owns. Behind a one-byte array of its own they read whatever followed it on the heap"
   • So this is creating an array that's larger than needed, so if boundscheck=False, reading past the first byte still stays within valid memory.

2. Added SHOW_NOGIL to check if nogil option is properly forwarded

3. Added tests to verify:

   • With cache: False, no cache files are created
   • With cache: True, cache files ARE created for all three functions
   • With nogil: True, the nogil option reaches all three functions

The mutation guard is essentially enforcing that certain JIT options must be properly forwarded through decorators, specifically requiring cache and boundscheck to be handled correctly while preventing other options like nogil from being passed through.

The modifications ensure that jit_with_options calls get converted to @numba.njit(...) with explicit parameters, and that configurations.py applies the same filtering logic to forwarded options. This maintains consistency in how JIT options are processed across different parts of the codebase.

Code Review

This PR adds test coverage to verify that JIT options (cache, boundscheck, nogil) properly reach the decorators in is_null.py and are correctly forwarded by the dispatchers in configurations.py.

Summary of Changes

1. .github/scripts/mutation_guard_check.py

Adds 5 new mutation test cases that will fail if:

• The jit_with_options decorators in is_null.py are replaced with plain @njit without preserving options
• The dispatchers in configurations.py stop forwarding cache and boundscheck options

2. test/test_cache.py

Adds tests to verify runtime behavior:

• Bounds checking: With boundscheck=True, out-of-bounds reads raise IndexError; with boundscheck=False (default), they don't
• Cache files: With cache=True, three cache index files are created; with cache=False, none are
• Option forwarding: The nogil option reaches all three functions (proving options flow through)

Issues Found

Issue 1: Suspicious Bitmap Size in Test

"bitmap = np.zeros(100000 // 8 + 1, dtype=np.uint8)[:1]\n"

This creates a 12,501-byte array and then slices to 1 byte. The comment says this is to ensure out-of-bounds reads stay within valid memory when boundscheck=False. However:

• The slice [:1] returns a new 1-byte array, so the underlying memory is not shared
• When boundscheck=False, Numba won't bounds-check, but it will still read from the bitmap's actual data (which is only 1 byte)
• This test may pass accidentally or behave unexpectedly

Recommendation: If the intent is to test bounds checking, either:

1. Use a 1-byte array and verify it raises IndexError with boundscheck, and potentially segfaults/crashes without (which the test seems to want to avoid)
2. Keep the full array without slicing to ensure valid memory access

Issue 2: Missing Cache Index File Assertion

In the cached test:

indexes = _index_files(tmp_path / "cached")
assert len(indexes) == 3, indexes

The test expects exactly 3 index files, but it doesn't verify they're the correct cache files. While the loop below checks for function names in the files, this is fine.

Issue 3: The nogil Test Logic

forwarded = _run(SHOW_NOGIL, _env(tmp_path / "nogil", {"cache": False, "nogil": True}), tmp_path)
assert forwarded.returncode == 0 and forwarded.stdout.split() == ["True", "True", "True"], forwarded.stderr

This test verifies that nogil=True reaches all three functions. However, if the PR's intent is to limit which options are forwarded (only cache and boundscheck), this test should actually verify that nogil does NOT reach the functions after the fix is applied.

Wait - re-reading the mutation guard cases, they test that decorators "stop forwarding any option but cache and boundscheck". So the test seems backwards. Let me re-check...

Looking at the mutation guard case:

'the decorators stop forwarding any option but cache and boundscheck'

And the SHOW_NOGIL test expects True, True, True - meaning nogil DOES reach the functions. This seems inconsistent with the mutation guard's intent.

Clarification needed: What is the expected behavior? Should nogil be forwarded or not?

Minor Suggestions

1. Add error output on failure: The test assertions could include stderr on failure for better debugging
2. Consider parametrizing: The three function tests could be parameterized to reduce duplication

Overall Assessment

The PR adds valuable test coverage for JIT option handling. The main concern is the bitmap size implementation - it may not be testing what was intended. The nogil test logic also needs clarification to ensure it matches the intended behavior of the mutation guards.

[Copilot]

Copilot review overview

🟢 Approval recommended

The focused test and mutation-guard changes are internally consistent and address the reported flaky access violation.

Review effort: Balanced
Findings: None

What changed in this PR

Hardens JIT-option tests by preventing out-of-allocation reads and expanding decorator coverage.

Changes:

• Keeps unchecked bitmap reads within owned memory.
• Verifies cache, bounds-checking, and nogil propagation across all three decorators.
• Adds matching mutation-guard cases.

File	Description
test/​test_cache.py	Makes bounds tests safe and expands JIT-option coverage.
.github/​scripts/​mutation_guard_check.py	Adds mutations validating decorator option forwarding.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[nelson2005]

From the fake Slim Shady:

On the review of 9157a2b, the sync of upstream's merge of Goykhman#13, its two questions.

1. A basic slice is a view, not a copy. The bitmap has OWNDATA False, its base is the 12501-byte array, np.shares_memory says so and the data pointer is the same, and the view keeps its base alive. So with bounds checking off the reads past byte 0 land inside memory the array owns, which is what the change is for.
2. That catalogue entry is a mutation, not the intended behaviour: the guard rewrites configurations.py to forward only cache and boundscheck and expects the suite to fail on it, and the nogil leg is what fails. nogil is forwarded, and the test reads it back from the three dispatchers so that a decorator dropping it is caught.

Nothing to change.