gh#346: WTF-8 dict-key and type-lookup residual coverage - #1177
Conversation
The two frontend identity gates for a generic-clause `msg.into()` (`trait_clause_into_string_identity`, `trait_into_string_alias`) accepted only `alloc::string::String` as the destination. The `PyError` constructor family takes `impl Into<Wtf8Buf>`, whose `.into()` targets `Wtf8Buf`, so both gates rejected it and the call lowered to an unregistered `["Into", "into"]`, failing the lift of `PyError::new` / `type_error` and every graph raising through them. Widen both gates to `tyref_is_string_value` (`String` / `Wtf8` / `Wtf8Buf` / `str`, all projecting to the single immutable `s_unicode0`). Remove the now-unused `tyref_adt_name_path`. Prepass census (b9ac697): phaseA 1709 -> 1381, phaseB 14 -> 4; 328 graphs lift, 0 new failures. cargo test --features dynasm clean; check.py bit-exact dynasm 420/420, cranelift 419/419, wasm 414/414. Assisted-by: Claude
Three WTF-8 dict adapters (w_dict_getitem_wtf8, w_dict_getitem_wtf8_checked, w_dict_setitem_wtf8) branched on `key.as_str()`, whose `Result<&str, Utf8Error>` cannot cross the residual-call boundary. Decompose each into a `#[dont_look_inside]` validity probe (wtf8_key_is_utf8), an identity-folded reinterpret (wtf8_key_as_str_unchecked, folded to its receiver by wtf8_string_identity_alias), and a `#[dont_look_inside]` lone-surrogate key object (wtf8_surrogate_key_str_object). Mark lookup_in_type_wtf8_uncached `#[dont_look_inside]`, matching its `&str` twin lookup_in_type_where_uncached: its `.map` over the opaque lookup_where_pair_wtf8_uncached's `Option<(PyObjectRef, PyObjectRef)>` kept a tuple-payload projection on the trace surface. It now residualises to the single-word `Option<PyObjectRef>`. Register the three new residual helpers in jit_fnaddr. Assisted-by: Claude
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe change adds shared WTF-8 dictionary key helpers, extends MIR string identity handling to WTF-8 values, and registers runtime aliases for residualized helpers. ChangesWTF-8 support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DictionaryAccess
participant Wtf8Helpers
participant MIRTranslator
participant JITRuntime
DictionaryAccess->>Wtf8Helpers: validate and convert WTF-8 keys
MIRTranslator->>MIRTranslator: fold string-family identity conversions
MIRTranslator->>JITRuntime: resolve residual helper addresses
JITRuntime-->>MIRTranslator: return registered function addresses
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit bd4acf9). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔇 Additional comments (4)
pyre/pyre-object/src/dictmultiobject.rs (1)
3436-3477: LGTM!Also applies to: 3511-3514, 3531-3534
majit/majit-translate/src/front/mir.rs (1)
10598-10623: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that every generic
Tbound byInto<Wtf8Buf>/Into<String>reaching this fold is string-representable.
trait_clause_into_string_identityandtrait_into_string_aliasboth alias the destination directly to the call argument wheneverdest_tyis string-family (tyref_is_string_value). Neither checks the source operand's type. The source is the unresolved genericTin the trait clause, so the check cannot see what concrete type flows in at any given call site sharing this generic body.Widening the target check from "exact
StringADT" to "any string-family destination" (includingWtf8/Wtf8Buf) enlarges the call-site population this identity fold applies to. If any type satisfyingInto<Wtf8Buf>is not string-representable in the lifted value model (for example a byte-vector conversion), this fold would silently alias a real conversion to identity, producing a JIT-traced value that diverges from the interpreter.As per coding guidelines, "The generated JIT must preserve the interpreter's semantics; never justify a mismatch by claiming Rust cannot be meta-traced. Fix deviations as generation defects." Please confirm no such source type exists for the call sites this fold now reaches.
Also applies to: 12678-12704
pyre/pyre-interpreter/src/baseobjspace.rs (1)
9034-9042: LGTM!pyre/pyre-interpreter/src/jit_fnaddr.rs (1)
898-913: LGTM!Also applies to: 1070-1084
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da4a2b77-d73f-413d-93a3-d3fe91539c3f
📒 Files selected for processing (4)
majit/majit-translate/src/front/mir.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-object/src/dictmultiobject.rs
Mark `object_getattr_miss` `#[dont_look_inside]` and register it in `jit_fnaddr`. It is the miss / special-attribute path of `getattribute`, reached only after the fast descriptor and instance-dict lookups return nothing. Its `[Option<PyObjectRef>; 2].iter().flatten()` metaclass walks and array indexing are opaque to the annotator; it returns the blessed `PyResult` carrier, so the whole cold path residualises behind one boundary. The prepass census `Flatten::next` fallback clears; the getattr graphs relocate to their next wall rather than lifting (0 lift). Assisted-by: Claude
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/bd4acf94387ca4a20a1ace5bdb738a1a22e9f60b/pyre-interpreter/src/jit_fnaddr.rs#L906
On wasm32, when a compiled trace reaches a WTF-8 dict adapter, this registers the raw Rust fn(&Wtf8) -> bool, whose WebAssembly type is (i32) -> i32, while residual call_indirect descriptors use the uniform (i64) -> i64 word ABI; jit_fnaddr.rs:1106-1109 and :2103-2110 document that this mismatch traps at runtime. The sibling surrogate helper registered at line 912 has the same reference-argument problem. Expose/register word-ABI trampolines rather than these raw pointers.
AGENTS.md reference: AGENTS.md:L14-L19
https://github.com/youknowone/pyre/blob/bd4acf94387ca4a20a1ace5bdb738a1a22e9f60b/pyre-interpreter/src/baseobjspace.rs#L7344
__getattr__ path traceable
When a loop repeatedly reads a missing instance or metaclass attribute, this annotation residualizes the entire dispatcher, including the get_and_call_function invocation of user __getattr__, so each iteration exits the parent trace instead of inlining the lookup and callback. This path is demonstrably hot in pyre/bench/synth/metaclass_getattr.py:24-32, while PyPy performs the corresponding hook call in the undecorated _handle_getattribute path (pypy/objspace/descroperation.py:234-245); isolate the unsupported projection/array operation rather than black-boxing the dispatcher.
AGENTS.md reference: AGENTS.md:L231-L233
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Two
gh#346slices growing two-phase (annotate→rtype) rtyper coverage so more JIT-traced graphs lift through the CodeWriter path instead of the legacy walker.3ea880f2875— genericInto<Wtf8Buf>message aliasWidens the String-only
Into<Wtf8Buf>message-conversion aliases totyref_is_string_value, soPyError::new'simpl Into<Wtf8Buf>clause folds to its operand. phaseA prepass fallback 1709 → 1381 (−328).5735933d692— WTF-8 dict-key split + type-lookup projectionw_dict_getitem_wtf8,w_dict_getitem_wtf8_checked,w_dict_setitem_wtf8) branched onkey.as_str(), whoseResult<&str, Utf8Error>cannot cross the residual-call boundary. Each is decomposed into a#[dont_look_inside]validity probe (wtf8_key_is_utf8), an identity-folded reinterpret (wtf8_key_as_str_unchecked, folded to its receiver bywtf8_string_identity_alias), and a#[dont_look_inside]lone-surrogate key object (wtf8_surrogate_key_str_object).lookup_in_type_wtf8_uncachedis marked#[dont_look_inside], restoring symmetry with its already-residual&strtwinlookup_in_type_where_uncached: its.mapover the opaquelookup_where_pair_wtf8_uncached'sOption<(PyObjectRef, PyObjectRef)>kept a tuple-payload projection on the trace surface; it now residualises to the single-wordOption<PyObjectRef>.#1168ullbc A/B): +17 lifted graphs — the attribute/descriptor lookup spine (lookup,lookup_in_type_where_wtf8,is_data_descr,is_object_getattribute_descr,is_type_getattribute_descr,descr_has_delete, …) — 0 regressions. The dict-adapter decompose is a correct, separable prerequisite (0 standalone lift; its_iotarget cluster relocates to the next wall,Flatten::next).bd4acf94387—getattributeattribute-miss residualobject_getattr_miss— the miss / special-attribute path ofgetattribute, reached only after the fast descriptor and instance-dict lookups return nothing — is marked#[dont_look_inside]and registered injit_fnaddr. Its[Option<PyObjectRef>; 2].iter().flatten()metaclass walks and array indexing are opaque to the annotator; it returns the blessedPyResultcarrier, so the whole cold path residualises behind one boundary. This clears theFlatten::nextprepass fallback (the next wall of the dict-routed_io/load_*cluster surfaced by the slice above); the getattr graphs relocate to their next wall rather than lifting (0 standalone lift — a prerequisite that clears the wall class, not a lift).Gates
cargo test --all --no-default-features --features dynasm— 7669 passed / 0 failed.check.py3-backend bit-exact — dynasm 420/420, cranelift 419/419, wasm 414/414, cpython-suite 209 modules.— commented by Claude
Summary by CodeRabbit
Bug Fixes
Performance