pyre: let a raised dict probe fail to match by identity too

[youknowone]

A failing dict store could delete an unrelated entry from a module dict, and
apply the store it had just reported as failed.

What was wrong

dict_keys_equal answered std::ptr::eq before it consulted
eq_error_pending():

if std::ptr::eq(a, b) { return true; }     // identity shortcut
if a.is_null() || b.is_null() { return false; }
if crate::dict_eq_hook::eq_error_pending() { return false; }   // ← too late

The pending-error test is how pyre stands in for RPython aborting the lookup:
ll_dict_lookup propagates at the first raising comparison
(rordereddict.py:1055), so nothing later in the bucket can be found. The Rust
Eq callback cannot abort an IndexMap scan, so it answers false for the
rest of it instead — but the identity shortcut sat above that test and kept
answering true.

That is reachable. A bucket can hold both a key whose __eq__ raises and the
incoming key's own object. Probed in that order the raise poisons the probe,
the identity hit still reports a match, and IndexMap::insert therefore
replaces instead of appending. w_module_dict_store_inner_checked's undo is
dict_entries_pop_last, which assumes the failed probe appended — so it drops
the dict's unrelated last entry, and leaves the replaced value overwritten.

Measured against CPython 3.14, storing under such a key in a module dict:

	CPython	pyre (before)
value under the key	'quiet-old' — store must not land	'quiet-new'
keys lost	none	['run'] — a module-level function, gone from globals()
net entries	0	−1

The fix

Move the pending-error test above the identity shortcut. A poisoned probe then
matches nothing, insert appends, and the existing pop_last undo is correct
again — no signature or ABI changes.

Verification

module_dict_invariants.py gains the case. It fails on both backends before
this change and passes on CPython, dynasm and cranelift after.

• pyre/extra_tests/parity_tests/run.py — no pyre-side failures.
• cargo test -p pyre-object 284, -p pyre-interpreter --features dynasm 430,
  0 failed.
• check.py — 339/340 on each of dynasm, cranelift and wasm.

Note

That one check.py failure is synth/str_search_index_bounds, a JIT-PANIC
(compile.rs, assert i == len(inputargs) failed (16 != 26)) identical on
all three backends. It is pre-existing on this base, not from this change:
reverting the two-line reorder and rebuilding reproduces the same panic. It
arrived with #860 and is unrelated to dict key comparison.

Provenance

Reported by the Codex parity review on #873 as a pre-existing mismatch. Its
stated mechanism — a later user comparison returning true — is not the live
one; eq_error_pending() already blocks that path. The identity shortcut
sitting above the guard is what was left, and it reproduces.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Fixed dictionary updates when key comparisons raise an exception during hash-collision handling.
  • Prevented partially applied mutations, including dropped entries, incorrect lengths, or unintended value changes.
  • Improved consistency of dictionary behavior in these error scenarios.

[coderabbitai]

Walkthrough

The dictionary equality probe now propagates pending comparison errors before identity checks, preventing partial mutation during raising __eq__ scenarios. A parity test validates preservation of module-dict contents and length after the failed write.

Changes

Dictionary mutation consistency

Layer / File(s)	Summary
Equality probe error handling pyre/pyre-object/src/dictmultiobject.rs	dict_keys_equal returns false immediately when an equality error is pending, before pointer-identity and null checks.
Module-dict invariant validation pyre/extra_tests/parity_tests/module_dict_invariants.py	Adds colliding key types and assertions that a failed mutation does not drop keys, change length, or overwrite the quiet key’s value.

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

Possibly related PRs

• youknowone/pyre#560: Addresses preservation of equality errors during hashed dictionary and set mutation paths.
• youknowone/pyre#710: Updates IndexMap probing and raising __eq__ handling for consistent dictionary and set state.

Poem

A rabbit found a probing flaw,
Where equal keys could unset the law.
Now errors halt the sneaky chase,
And quiet keys keep their place.
The module’s map stays whole—
Hop, hop, correctness goal! 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the fix: a raised dict probe should not match by identity.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch rewrite-tracer

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pyre/extra_tests/parity_tests/module_dict_invariants.py`:
- Line 104: Replace the `assert False` statement in the exception-propagation
test with an explicit `raise AssertionError`, preserving the existing failure
message so the check remains active under optimized Python execution.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 62866e02-e62b-44c1-8eb1-89bc448930a7

📥 Commits

Reviewing files that changed from the base of the PR and between 622bdce and 173b990.

📒 Files selected for processing (2)

• pyre/extra_tests/parity_tests/module_dict_invariants.py
• pyre/pyre-object/src/dictmultiobject.rs

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Replace assert False with raise AssertionError.

Under python -O, all assert statements (including this one) are stripped, silently defeating the "exception must propagate" check this test exists to enforce.

🐛 Proposed fix

-        assert False, "raising __eq__ must propagate out of the store"
+        raise AssertionError("raising __eq__ must propagate out of the store")

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	assert False, "raising __eq__ must propagate out of the store"
	raise AssertionError("raising __eq__ must propagate out of the store")

🧰 Tools

🪛 Ruff (0.16.0)

[warning] 104-104: Assertion always fails, replace with pytest.fail()

(PT015)

---

[warning] 104-104: Do not assert False (python -O removes these calls), raise AssertionError()

Replace assert False

(B011)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pyre/extra_tests/parity_tests/module_dict_invariants.py` at line 104, Replace
the `assert False` statement in the exception-propagation test with an explicit
`raise AssertionError`, preserving the existing failure message so the check
remains active under optimized Python execution.

Source: Linters/SAST tools

[github-actions]

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 173b990).
Updated: 2026-07-29T11:25:43.501Z

Files in the reviewed diff

pyre/extra_tests/parity_tests/module_dict_invariants.py
pyre/pyre-object/src/dictmultiobject.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

• pyre/extra_tests/parity_tests/module_dict_invariants.py:95 ↔ pypy/objspace/std/celldict.py:67-75: the new test says it verifies an unchanged “module dict,” but its first non-str key immediately switches PyPy’s ModuleDictStrategy to ObjectDictStrategy. The exceptional store therefore tests the post-switch object-backed dictionary, not module-cell storage.

3. Pre-existing mismatches (already present before this patch)

• pyre/pyre-object/src/dictmultiobject.rs:115-126 ↔ rpython/rtyper/lltypesystem/rdict.py:459-462: the infallible object_key_for explicitly consumes a hash_w error and continues, whereas upstream r_dict lets keyhash(key) propagate before lookup/store. This predates the patch.

4. Structural adaptations

• pyre/pyre-object/src/dictmultiobject.rs:1923-1943 ↔ rpython/rtyper/lltypesystem/rdict.py:578-634: Rust IndexMap cannot abort its equality callback mid-probe as r_dict can. The new pending-error check suppresses all later matches, including pointer identity, after the first raising comparison; this correctly preserves PyPy’s observable failed-store semantics.
• pyre/pyre-object/src/dictmultiobject.rs:2839-2845 ↔ rpython/rtyper/lltypesystem/rdict.py:459-490: the checked Rust store may have appended an entry before it can observe the callback error, so it removes that last entry. Upstream unwinds before _ll_dict_setitem_lookup_done; the append-and-undo sequence is a Rust-container adaptation.