macros: give #[pyre_methods] shims a Signature and drop the marker-dict peel - #1108
Merged
Conversation
WalkthroughThe PR canonicalizes enum variant registration, adjusts enum type-leaf bucketing, adds signature-aware builtin and descriptor construction, updates generated method wrappers, adds binding and JIT tests, and propagates checked dictionary-read errors before migration. ChangesEnum translation updates
Signature-aware Python wrappers
Checked dictionary reads
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MethodMacro
participant BuiltinGateway
participant DescriptorBuilder
participant BoundMethod
participant SmokeAndJITTests
MethodMacro->>BuiltinGateway: provide generated Signature
BuiltinGateway->>DescriptorBuilder: select signature-aware constructor
DescriptorBuilder->>BoundMethod: create configured builtin or __new__ descriptor
BoundMethod->>SmokeAndJITTests: bind positional, keyword-only, and default arguments
Possibly related issues
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 30b16fa). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
youknowone
force-pushed
the
rtyper-legacy
branch
from
August 7, 2026 17:22
af21891 to
16310a7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@majit/majit-translate/src/annotator/bookkeeper.rs`:
- Around line 759-786: Update the enum-base check in the surrounding
pre-registration logic to call reg.is_enum_base with the qualified root symbol
rather than the bare leaf alias, ensuring retained roots still pre-register
their variants after duplicate-leaf metadata hardening. Add a regression
covering two qualified enums sharing a leaf name but having different
discriminant maps, and verify both enums’ variant ClassDefs exist before
inheritance-ID assignment.
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 4234-4248: Stop using leading_non_null_count in the bound-argument
validation path, since a non-null prefix does not represent positional
provenance after bind_kwargs_to_signature. In pyre/pyre-macros/src/lib.rs lines
1989-1997, preserve raw positional provenance before binding or validate
required bound slots individually while keeping raw-input arity checks separate
from full bound scopes; update pyre/pyre-interpreter/src/builtins.rs lines
4234-4248 so leading_non_null_count is not documented or used as the true
positional count. Add a regression in
pyre/pyre-interpreter/src/module/_random/macro_smoke.rs lines 210-246 covering
an omitted optional slot before a required parameter supplied by keyword.
🪄 Autofix
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: d822105e-62ae-4ddf-87fc-e1436f534088
📒 Files selected for processing (10)
majit/majit-translate/src/annotator/bookkeeper.rsmajit/majit-translate/src/front/mir.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/gateway.rspyre/pyre-interpreter/src/module/_random/macro_smoke.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-macros/src/lib.rs
Comment on lines
+4234
to
+4248
| /// Length of the leading non-null run of `args`. | ||
| /// | ||
| /// `bind_kwargs_to_signature` pads the flat argument slice out to the full | ||
| /// parameter count with PY_NULL for keyword-only slots and absent optionals, | ||
| /// so the true positional count is the prefix before the first PY_NULL. | ||
| /// A single named function keeps the count off the annotator's shared | ||
| /// iterator-adapter graph: a `take_while` closure inlined per wrapper gives | ||
| /// every `__pyre_wrap_*` shim its own closure type, and merging those | ||
| /// distinct types on one `TakeWhile` graph's input has no common base class. | ||
| pub(crate) fn leading_non_null_count(args: &[PyObjectRef]) -> usize { | ||
| let mut count = 0; | ||
| while count < args.len() && !args[count].is_null() { | ||
| count += 1; | ||
| } | ||
| count |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not infer positional count from a signature-bound scope.
bind_kwargs_to_signature retains parameter slots, but it does not retain argument origin. For fn f(&self, #[default(0)] a: i64, b: i64), f(b=1) reaches the wrapper as [self, PY_NULL, 1]. leading_non_null_count returns 1, and the generated arity check rejects the call even though b is present.
pyre/pyre-interpreter/src/builtins.rs#L4234-L4248: Do not describe the non-null prefix as the true positional count after binding.pyre/pyre-macros/src/lib.rs#L1989-L1997: Preserve positional provenance before binding, or validate required bound slots individually. Keep raw-input arity checks separate from full bound scopes.pyre/pyre-interpreter/src/module/_random/macro_smoke.rs#L210-L246: Add a regression test with an omitted optional slot before a required parameter passed by keyword.
📍 Affects 3 files
pyre/pyre-interpreter/src/builtins.rs#L4234-L4248(this comment)pyre/pyre-macros/src/lib.rs#L1989-L1997pyre/pyre-interpreter/src/module/_random/macro_smoke.rs#L210-L246
🤖 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/pyre-interpreter/src/builtins.rs` around lines 4234 - 4248, Stop using
leading_non_null_count in the bound-argument validation path, since a non-null
prefix does not represent positional provenance after bind_kwargs_to_signature.
In pyre/pyre-macros/src/lib.rs lines 1989-1997, preserve raw positional
provenance before binding or validate required bound slots individually while
keeping raw-input arity checks separate from full bound scopes; update
pyre/pyre-interpreter/src/builtins.rs lines 4234-4248 so leading_non_null_count
is not documented or used as the true positional count. Add a regression in
pyre/pyre-interpreter/src/module/_random/macro_smoke.rs lines 210-246 covering
an omitted optional slot before a required parameter supplied by keyword.
pre_register_enum_variant_classes resolved each variant with a bare intern_enum_variant_host + getuniqueclassdef, which mints the variant classdef identity but does not project its payload rows. A variant field typed *mut PyObject stayed an untyped FORCE shell until subject-flow narrowing first reached it (enum_variant_narrowing_knowntypedata -> getuniqueclassdef_for_enum_variant), projecting the pointee struct mid-fixpoint. Route each variant through getuniqueclassdef_for_enum_variant, which materializes the discriminant-only base, interns the variant subclass under the same ::-qualified key, projects the variant payload rows, and drains the pending struct-row queue before returning — all at prologue time, before assign_inheritance_ids and any subject flow. The variant subtree still numbers as one contiguous bracket. Assisted-by: Claude
harden_duplicate_leaf_metadata's first pass groups struct-field keys by
their last :: segment and withdraws a bare leaf alias when two qualified
keys sharing that leaf carry divergent rows. A {Enum}::{Variant} key
whose variant leaf also names an enum base
(typedef::BytesSubArg::Buffer, rows [__pos_0], leaf Buffer) collided
with the same-named enum type buffer::Buffer (rows [__discriminant]) and
withdrew the bare "Buffer" alias on that spurious divergence.
The variant-leaf pass below already withdraws variant aliases keyed on
the Enum::Variant tail, so such a variant key does not belong in the
type-leaf bucket. Skip a variant key only when its leaf is itself an
enum base — the genuine type-vs-variant name collision — leaving every
other variant-leaf bucket intact.
Restores reg.fields.contains_key("Buffer"), so derive_subject_inputcells
seeds a &Buffer receiver as SomeInstance(Buffer) through
getuniqueclassdef_for_struct_root.
Assisted-by: Claude
instance_node_getdictvalue_checked read the mapdict node into a scoped Result, ran the unconditional maybe_migrate_to_boxed tail, then returned that Result — a shell forwarding through a side-effecting intervening block. lower_result_exc_returns declines that shape (the Err rewrite's set_raise_values would drop the migrate call), so the whole dont_look_inside callee was dropped from the SemanticProgram and its callers failed "not registered in PyreCallRegistry". Propagate the read with `?` and return Ok(w) after the migrate: the read raises only on the find_map_attr miss path (mapdict.py:846-847 -> 58 -> 312-313 returns None, never reaching _direct_read's migration tail), and maybe_migrate_to_boxed re-derives that None and no-ops, so the migrate is unobservable when the read raised. The callee now lowers cleanly and registers as its opaque residual. stamp_builtin_owner reads the per-type `static OWNER` from the method_owner table — a field-bearing frozen instance carrying a fn pointer, which the annotator cannot model as a prebuilt constant (the address-fold path is gated to zero-field structs). Mark it dont_look_inside so the owner-stamp setup is a residual-call boundary and getattr_str_impl's bound-method assembly is not dragged into the unmodellable static read. phaseA 1312 -> 1308, no phaseA regressions; check.py bit-exact 3/3 (dynasm 391, cranelift 391, wasm 387). Assisted-by: Claude
…ct peel `#[pyre_methods]` now builds a per-method `Signature` from its parameter tables and registers every arm through a signature-aware constructor: `make_builtin_function_maybe_sig` (default), the new `make_builtin_function_with_text_signature_and_sig` (all-required instance arm), and the new `make_new_descr_maybe_sig` (`__new__`). Instance methods prepend `self` as a positional-only slot; a raw `&[PyObjectRef]` whole-args method carries `None`. Removes the dead `is_new` raw_fn branch and generalizes the former kwonly-only signature arm to every method arm. With a Signature attached, the call path resolves keywords into positional PY_NULL-padded slots before the wrapper runs, so the method wrapper preamble drops `split_builtin_kwargs`/`has_builtin_kwargs`/`bind_builtin_kwargs` and computes `__pyre_positional_count` as the leading non-null run of `args`. The leading non-null count is a single named function `builtins::leading_non_null_count` (an explicit loop) rather than an inlined `take_while(|s| !s.is_null())` closure. A closure inlined per wrapper gives every `__pyre_wrap_*` shim its own closure type; merging those distinct types on one shared `TakeWhile` graph input has no common base class and fails RPython annotation. A named function keeps the count off that shared iterator-adapter graph. Both the function-side and method-side preambles call it. New gateway.rs/typedef.rs composers thread the optional Signature. macro_smoke.rs gains `Demo::combine` (positional-or-keyword + keyword-only default) and a dual-path test asserting positional and keyword-bound calls agree through `bind_kwargs_to_signature`. Rewrites the base-owned jitcode_dispatch test `keyword_builtin_wrapper_finds_colored_argument_slice_item_descr` (now `signature_bound_wrapper_reads_argument_slice_with_distinct_item_descr`): the `__pyre_wrap_getrandbits` wrapper no longer opens with the `split_builtin_kwargs` call and reads its argument array off the wrapper input register, so the test asserts the entry is not an `inline_call_*` and keeps the item-descr/length-descr distinctness invariant. De-stales the `wrapper_args_item_descr_index` doc comment accordingly. Functions registered at their call sites are unchanged; the marker producer and `split_builtin_kwargs` remain for null-signature functions and the hand-written direct callers. Assisted-by: Claude
youknowone
force-pushed
the
rtyper-legacy
branch
from
August 8, 2026 02:42
16310a7 to
30b16fa
Compare
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: 30b16fa161
ℹ️ 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".
Comment on lines
+2211
to
+2215
| crate::gateway::make_builtin_function_with_text_signature_and_sig( | ||
| #py_name, | ||
| #wrapper_name, | ||
| #text_signature, | ||
| #method_sig, |
There was a problem hiding this comment.
For every all-required instance method emitted by this branch, this helper ultimately calls builtin_code_new_with_signature, which sets fast_natural_arity = HOPELESS; consequently even an ordinary exact positional call now enters finish_builtin_code_positional, runs bind_kwargs_to_signature, and allocates a Vec. Before this change the same calls bypassed binding, and upstream BuiltinCode.__init__ still selects its BuiltinCodeN fast class after constructing sig. Preserve the known receiver-plus-parameter arity alongside the signature (analogous to the existing make_builtin_function_with_arity_and_maybe_sig) so adding keyword support does not impose allocation and rebinding on every positional method call.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pyre/pyre-interpreter/src/builtins.rs (1)
5413-5449: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward
kwargswhen delegating to the winning metaclass.At line 5449,
call_function_impl_result(w_metaclass_new, &new_args)only passes positionals. Whenkwargsis present andw_winner != default_meta, real class keywords inkwargsare left behind and the delegation cannot delivermetaclass(name, bases, dict, **kwds). Build the delegate args from the originalargs/kwargssplit or use a kwargs-aware call path before invokingw_metaclass_new.🤖 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/pyre-interpreter/src/builtins.rs` around lines 5413 - 5449, Update the delegation in the winning-metaclass branch of the class-construction flow to preserve and forward the original kwargs alongside positional arguments. Replace the positional-only call to call_function_impl_result for w_metaclass_new with the established kwargs-aware invocation path, using the original args/kwargs split so metaclass keywords reach the delegated __new__.majit/majit-translate/src/front/mir.rs (1)
1823-1873: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the type-vs-variant leaf collision this comment describes.
The comment at lines 1876-1887 documents a concrete, previously-mishandled scenario: a variant leaf (
typedef::BytesSubArg::Buffer) colliding with an unrelated enum's bare name (buffer::Buffer). The file already has targeted unit tests for sibling scenarios in this function (harden_withdraws_shape_divergent_bare_alias_and_tombstones_origin,harden_withdraws_discriminant_divergent_bare_enum_alias,harden_withdraws_shape_divergent_variant_leaf_alias), but none constructs this specific "leaf is itself an enum base" case that the new&& struct_fields.is_enum_base(leaf)condition targets.Add a test that registers two enum bases under the same bare leaf name shape (one being a variant tail of a different enum, one being a real enum's own name) and asserts the bare alias for the colliding leaf survives, while confirming an unrelated equal-leaf, non-enum-base collision still gets withdrawn as before. This pins the fix and prevents a silent regression back to the old over-broad skip.
🤖 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 `@majit/majit-translate/src/front/mir.rs` around lines 1823 - 1873, Add a regression test alongside the existing harden_duplicate_leaf_metadata tests, covering a variant tail such as typedef::BytesSubArg::Buffer colliding with an unrelated enum base such as buffer::Buffer. Register both enum bases and assert the shared bare Buffer alias is preserved, while also verifying an equal-leaf collision that is not an enum base is still withdrawn. Use the existing test helpers and naming conventions.pyre/pyre-interpreter/src/objspace/std/mapdict.rs (1)
4612-4627: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRoot
w_dictbefore callingself.delitem.
box_str_constantandmaybe_migrate_to_boxedcan move GC objects. The root set pinsw_obj, the key, and the value, but notw_dict. Line 4627 can therefore pass a stalew_dictpointer toself.delitem.Proposed fix
let _roots = pyre_object::gc_roots::push_roots(); -let obj_slot = pyre_object::gc_roots::pin_roots(&[w_obj]); +let slots = pyre_object::gc_roots::pin_roots(&[w_dict, w_obj]); +let dict_slot = slots; +let obj_slot = slots + 1; ... -self.delitem(w_dict, pyre_object::gc_roots::shadow_stack_get(key_slot)); +self.delitem( + pyre_object::gc_roots::shadow_stack_get(dict_slot), + pyre_object::gc_roots::shadow_stack_get(key_slot), +);🤖 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/pyre-interpreter/src/objspace/std/mapdict.rs` around lines 4612 - 4627, Root w_dict before the self.delitem call because box_str_constant and maybe_migrate_to_boxed may move it during the surrounding mapdict operation. Extend the existing GC root/pinning setup to include w_dict, then retrieve the current rooted w_dict immediately before invoking self.delitem, while preserving the existing rooted key handling.majit/majit-translate/src/annotator/bookkeeper.rs (2)
6454-6454: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
eprintln!debug probes.Both statements are labelled "probe". They print on every test run, including passing runs. The adjacent
assert!calls already carry failure messages that include the value.♻️ Proposed change
"{field} pre-seed must be untyped force shell, got {value:?}" ); - eprintln!("probe {field} after force seeding, before projection: {value:?}"); }let value = &attrs.attrs.get(field).expect("tuple field").s_value; - eprintln!("probe {field} after constructor setattr: {value:?}"); assert!(Also applies to: 6492-6492
🤖 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 `@majit/majit-translate/src/annotator/bookkeeper.rs` at line 6454, Remove both `eprintln!` debug probes labelled “probe” near the adjacent `assert!` calls, including the probes after force seeding and before projection and at the additional referenced location. Leave the assertions and their failure messages unchanged.
3775-3782: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the prefix and wrapper stripping with
project_pyre_field_type.
is_nullable_sum_spellingdecides which shaped-tuple rows the projector must skip. It must therefore recognize exactly the spellings thatproject_pyre_field_typemodels as payload-plus-None. It currently does not.Two divergences exist:
- Raw pointers.
project_pyre_field_typestrips*const/*mutat Line 2516 and projects the pointee.is_nullable_sum_spellingstrips only&andmut. A row spelled*mut Option<T>projects to the nullable union but is not filtered.- Transparent wrappers.
project_pyre_field_typeunwrapsBox<,Rc<,Arc<,RefCell<and the rest at Line 2664 and then reaches theOption</Result<arms.is_nullable_sum_spellingdoes not unwrap them. A row spelledBox<Option<Vec<*mut PyObject>>>projects to list-or-none but is not filtered.In both cases the row survives the
retainat Line 2186,filtered_nullable_fieldsomits it, and the force shell is not reset. The constructorsetattrthen unionsInstance(Option<…>::None)against the projected payload. That is thegeneralize_attrfailure the comment at Line 2176 describes.🐛 Proposed fix
fn is_nullable_sum_spelling(field_ty: &str) -> bool { - let t = field_ty + let mut t = field_ty .trim() .trim_start_matches('&') .trim_start_matches("mut ") + .trim_start_matches("*const ") + .trim_start_matches("*mut ") .trim(); + // Mirror `project_pyre_field_type`'s transparent-wrapper unwrap so both + // sides agree on which spellings model as payload-plus-`None`. + loop { + let mut peeled = None; + for wrapper in [ + "Rc<", "Arc<", "Box<", "RefCell<", "Cell<", "Pin<", "NonNull<", + "NonZero<", "MaybeUninit<", "ManuallyDrop<", "UnsafeCell<", + "Wrapping<", "Reverse<", + ] { + if let Some(inner) = strip_generic_one(t, wrapper) { + peeled = Some(inner.trim()); + break; + } + } + match peeled { + Some(inner) => t = inner, + None => break, + } + } strip_generic_one(t, "Option<").is_some() || strip_generic_one(t, "Result<").is_some() }A shared helper that both
project_pyre_field_typeand this predicate call for the strip-and-unwrap step would prevent the two lists from diverging again.🤖 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 `@majit/majit-translate/src/annotator/bookkeeper.rs` around lines 3775 - 3782, Update is_nullable_sum_spelling to share the same prefix-stripping and transparent-wrapper-unwrapping helper used by project_pyre_field_type, including raw pointers (*const/*mut) and wrappers such as Box, Rc, Arc, and RefCell. Ensure both paths reach the existing Option/Result detection consistently, without duplicating divergent stripping logic.
🤖 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 `@majit/majit-translate/src/annotator/bookkeeper.rs`:
- Line 786: Handle the Result returned by getuniqueclassdef_for_enum_variant in
the prologue pre-mint instead of discarding it with let _. Propagate the error,
or record it through the annotator backlink using try_annotator when no
annotator may be attached, while preserving the requirement that all variant
subtree members are materialized before assign_inheritance_ids.
---
Outside diff comments:
In `@majit/majit-translate/src/annotator/bookkeeper.rs`:
- Line 6454: Remove both `eprintln!` debug probes labelled “probe” near the
adjacent `assert!` calls, including the probes after force seeding and before
projection and at the additional referenced location. Leave the assertions and
their failure messages unchanged.
- Around line 3775-3782: Update is_nullable_sum_spelling to share the same
prefix-stripping and transparent-wrapper-unwrapping helper used by
project_pyre_field_type, including raw pointers (*const/*mut) and wrappers such
as Box, Rc, Arc, and RefCell. Ensure both paths reach the existing Option/Result
detection consistently, without duplicating divergent stripping logic.
In `@majit/majit-translate/src/front/mir.rs`:
- Around line 1823-1873: Add a regression test alongside the existing
harden_duplicate_leaf_metadata tests, covering a variant tail such as
typedef::BytesSubArg::Buffer colliding with an unrelated enum base such as
buffer::Buffer. Register both enum bases and assert the shared bare Buffer alias
is preserved, while also verifying an equal-leaf collision that is not an enum
base is still withdrawn. Use the existing test helpers and naming conventions.
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 5413-5449: Update the delegation in the winning-metaclass branch
of the class-construction flow to preserve and forward the original kwargs
alongside positional arguments. Replace the positional-only call to
call_function_impl_result for w_metaclass_new with the established kwargs-aware
invocation path, using the original args/kwargs split so metaclass keywords
reach the delegated __new__.
In `@pyre/pyre-interpreter/src/objspace/std/mapdict.rs`:
- Around line 4612-4627: Root w_dict before the self.delitem call because
box_str_constant and maybe_migrate_to_boxed may move it during the surrounding
mapdict operation. Extend the existing GC root/pinning setup to include w_dict,
then retrieve the current rooted w_dict immediately before invoking
self.delitem, while preserving the existing rooted key handling.
🪄 Autofix
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: 8f1d6818-4252-48c9-8d4f-bf62e9d32d4d
📒 Files selected for processing (7)
majit/majit-translate/src/annotator/bookkeeper.rsmajit/majit-translate/src/front/mir.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
| // the pending drain inside the call — makes the variant's | ||
| // payload classdefs order-independent, the same contract the | ||
| // struct-root loop above relies on. | ||
| let _ = self.getuniqueclassdef_for_enum_variant(&root, variant); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Do not discard the resolver error in the prologue pre-mint.
getuniqueclassdef_for_enum_variant now performs base materialization, variant interning, and payload-row projection. Each step returns Result. let _ = drops any failure. The doc comment above states that this pre-mint must leave every subtree member present and UNNUMBERED before assign_inheritance_ids. A swallowed error leaves the variant classdef absent. The single numbering pass then misses that subtree, and the later lazy mint produces the Skip-classified instantiation this method exists to prevent.
Record or propagate the error so the failure is visible.
♻️ Proposed change
- let _ = self.getuniqueclassdef_for_enum_variant(&root, variant);
+ if let Err(err) = self.getuniqueclassdef_for_enum_variant(&root, variant) {
+ self.warning(format!(
+ "pre_register_enum_variant_classes: {root}::{variant} failed to \
+ pre-mint before assign_inheritance_ids: {err}"
+ ));
+ }warning routes through the annotator backlink; use try_annotator guarding if the prologue can run without an attached annotator.
🤖 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 `@majit/majit-translate/src/annotator/bookkeeper.rs` at line 786, Handle the
Result returned by getuniqueclassdef_for_enum_variant in the prologue pre-mint
instead of discarding it with let _. Propagate the error, or record it through
the annotator backlink using try_annotator when no annotator may be attached,
while preserving the requirement that all variant subtree members are
materialized before assign_inheritance_ids.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch carries four commits on top of
origin/main. The top commit is the#[pyre_methods]signature port described below; the three beneath it areprior in-flight rtyper-legacy work (annotator enum-variant projection, front-end
type-vs-variant leaf harden bucket, getattr owner-stamp/getdictvalue residuals).
Summary
Ports upstream's
BuiltinCode/Signaturegateway to#[pyre_methods]-generatedshims so builtin keyword arguments resolve by name at call setup into positional
PY_NULL-padded slots, and drops thesplit_builtin_kwargsmarker-dict peel fromthe method wrapper preamble. This is the first slice of deleting the
__pyre_kw__marker-dict keyword ABI (a documented PRE-EXISTING-ADAPTATION).
#[pyre_methods]now builds a per-methodSignaturefrom its parameter tables andregisters every arm through a signature-aware constructor:
make_builtin_function_maybe_sig(default), the newmake_builtin_function_with_text_signature_and_sig(all-required instance arm), andthe new
make_new_descr_maybe_sig(__new__). Instance methods prependselfas apositional-only slot; a raw
&[PyObjectRef]whole-args method carriesNone. Themethod wrapper preamble then computes
__pyre_positional_countas the leadingnon-null run of
argsrather than peeling a trailing marker dict.134 method shims move off the marker ABI. Parity with upstream (keyword resolution at
BuiltinCode/Signaturesetup, no marker dict) is the primary result.Scope
make_builtin_function, which drops the macro-computed signature, so their preamblekeeps the marker-tolerant path. The single marker producer and
split_builtin_kwargsremain for null-signature functions and the ~50 hand-written direct callers.
#[pyre_function]follow-up is deliberately deferred: only 3 real standalonefunctions exist (all arity 0-1 that never receive keywords), and the macro cannot own
their external registration, so a marker-free function preamble would be fragile with
near-zero yield.
Test changes
Rewrites the base-owned jitcode_dispatch test
keyword_builtin_wrapper_finds_colored_argument_slice_item_descr(nowsignature_bound_wrapper_reads_argument_slice_with_distinct_item_descr): the__pyre_wrap_getrandbitswrapper no longer opens with thesplit_builtin_kwargscalland reads its argument array off the wrapper input register, so the test asserts the
entry is not an
inline_call_*and keeps the item-descr/length-descr distinctnessinvariant. De-stales the
wrapper_args_item_descr_indexdoc comment accordingly.Verification
cargo test --all --features dynasm: green (adds a dual-path parity test proving akeyword-only method binds identically positionally and by name).
python3 pyre/check.py(dynasm/cranelift/wasm): this branch introduces zero newfailures. The base commit is red on this host on 5 pre-existing fixtures — two
jit-stats snapshots not re-recorded by the rebase (
del_cellvar_walk_commit,pypy_type_surface) and three GC-crash / wrong-output fixtures (closure_per_call,list_length_hint_validate,make_function_inline). Each was confirmed base-owned bya branch-minus-commit control: with the 6 changed files restored to the parent commit,
all five reproduce identically. This branch touches no struct layout,
type_id, or GCsurface. jit-stats snapshots are intentionally not re-recorded.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests