Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 28 additions & 20 deletions majit/majit-translate/src/annotator/bookkeeper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -756,26 +756,34 @@ impl Bookkeeper {
pairs.sort();

for (root, variant_names) in pairs {
// Materialize the discriminant-only base classdef before a
// variant references it through `getmro`; idempotent with the
// struct-root loop. `canonical_struct_name(root)` is the same
// spelling `intern_enum_variant_host` resolves the base under,
// so the pre-mint and the discriminant-narrowing resolver share
// one base lineage and the variant subtree numbers as one
// bracket.
let canon_root = majit_ir::descr::canonical_struct_name(&root);
let base = self.intern_class_by_qualname(&canon_root);
let _ = self.getuniqueclassdef(&base);
for variant in variant_names {
// The SAME interning primitive the discriminant-narrowing
// resolver ([`Self::getuniqueclassdef_for_enum_variant`])
// and the variant ctor arm (`flowspace_adapter`) use, so
// all three sites resolve ONE variant classdef under the
// `::`-qualified key — no `.`-vs-`::` split that would mint
// a second, distinct sibling the single numbering pass never
// reaches.
let variant_host = self.intern_enum_variant_host(&root, &variant);
let _ = self.getuniqueclassdef(&variant_host);
// Each variant is resolved through the canonical resolver, which
// materializes the discriminant-only base first
// ([`Self::getuniqueclassdef_for_struct_root`]) before interning
// the variant subclass — so the pre-mint and the
// discriminant-narrowing resolver share one base lineage and the
// variant subtree still numbers as one contiguous bracket.
for variant in &variant_names {
// Resolve each variant through the canonical resolver rather
// than a bare intern. It mints the discriminant-only base,
// interns the variant subclass under the same `::`-qualified
// key the discriminant-narrowing resolver and the variant ctor
// arm use (so all three resolve ONE variant classdef, no
// `.`-vs-`::` sibling split the single numbering pass misses),
// AND projects the variant's payload rows — draining any
// struct the payload first reaches — before it returns.
//
// The payload projection is what a bare intern skipped: a
// variant field typed `*mut PyObject` would otherwise stay an
// untyped FORCE shell until subject-flow narrowing first
// resolves it (`enum_variant_narrowing_knowntypedata` ->
// `getuniqueclassdef_for_enum_variant`), where a first-intern
// of the pointee struct mid-fixpoint generalises an
// already-annotated cell. Projecting here — at prologue time,
// before `assign_inheritance_ids` and any subject flow, with
// 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 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.

}
}
}
Expand Down
17 changes: 16 additions & 1 deletion majit/majit-translate/src/front/mir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1872,7 +1872,22 @@ fn harden_duplicate_leaf_metadata(
) {
let mut by_leaf: std::collections::HashMap<&str, Vec<&str>> = std::collections::HashMap::new();
for key in struct_fields.fields.keys() {
if let Some((_, leaf)) = key.rsplit_once("::") {
if let Some((head, leaf)) = key.rsplit_once("::") {
// A `{Enum}::{Variant}` key whose variant leaf ALSO names an
// enum base is not a peer type-leaf of that base: its bare
// alias is withdrawn by the variant-leaf pass below (keyed on
// the `Enum::Variant` tail), so admitting it into this
// type-leaf bucket lets the variant (`typedef::BytesSubArg::Buffer`,
// rows `[__pos_0]`) withdraw the same-named enum type's bare
// alias (`buffer::Buffer`, rows `[__discriminant]`) on a
// spurious row divergence. Restricting the skip to a leaf that
// is itself an enum base withdraws only the genuine
// type-vs-variant name collision, leaving every other
// variant-leaf bucket (whose withdrawal the resume numbering
// depends on) intact.
if struct_fields.is_enum_base(head) && struct_fields.is_enum_base(leaf) {
continue;
}
by_leaf.entry(leaf).or_default().push(key);
}
}
Expand Down
17 changes: 17 additions & 0 deletions pyre/pyre-interpreter/src/builtins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4257,6 +4257,23 @@ pub(crate) fn split_builtin_kwargs(args: &[PyObjectRef]) -> (&[PyObjectRef], Opt
(args, None)
}

/// 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
Comment on lines +4260 to +4274

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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-L1997
  • pyre/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.

}

/// Cold dictionary-strategy half of the flat builtin-keyword ABI.
///
/// The caller first proves `last` is a dict. Keeping the strategy dispatch
Expand Down
18 changes: 18 additions & 0 deletions pyre/pyre-interpreter/src/gateway.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1191,6 +1191,24 @@ pub fn make_builtin_function_with_text_signature(
function
}

/// `make_builtin_function_with_text_signature` that also records an argument
/// `Signature` (`Some` routes through the keyword-binding constructor; `None`
/// falls back to the positional-only one). Used by the `#[pyre_methods]`
/// all-required instance-method arm, which wants both the generated
/// `__text_signature__` for introspection and by-name keyword binding.
pub fn make_builtin_function_with_text_signature_and_sig(
name: &'static str,
func: BuiltinCodeFn,
text_signature: &'static str,
signature: Option<Signature>,
) -> PyObjectRef {
let function = make_builtin_function_maybe_sig(name, func, signature);
unsafe {
crate::function::fset_func_text_signature(function, pyre_object::w_str_new(text_signature));
}
function
}

/// Fixed-arity twin of `make_builtin_function_with_text_signature`.
pub fn make_builtin_function_with_arity_and_text_signature(
name: &'static str,
Expand Down
52 changes: 52 additions & 0 deletions pyre/pyre-interpreter/src/module/_random/macro_smoke.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,20 @@ impl Demo {
fn __reduce__(&self) -> PyObjectRef {
crate::pytuple![type_object(), crate::pytuple![], self.getstate()]
}
// A positional-or-keyword parameter plus a keyword-only default: the
// instance-method arm must build a `Signature` (`self` posonly, then
// `factor`, then a `marker_kwonly()` tail with `bias`) so the call path
// binds keywords by name and the wrapper preamble needs no marker-dict
// strip.
fn combine(
&self,
factor: i64,
#[kwonly]
#[default(0i64)]
bias: i64,
) -> i64 {
self.state as i64 * factor + bias
}
// `#[getter]` / `#[setter]` / `#[deleter]` GetSetProperty quad.
#[getter(doc = "raw 64-bit state as a signed int")]
fn raw_state(&self) -> i64 {
Expand Down Expand Up @@ -193,6 +207,44 @@ mod tests {
assert_eq!(Demo::from_obj(obj).expect("initialized Demo").state, 37);
}

/// A `#[pyre_methods]` instance method with a positional-or-keyword and a
/// keyword-only parameter binds identically whether the keyword arrives
/// positionally or by name through `bind_kwargs_to_signature` — the same
/// invariant the caller relies on to hand the wrapper a marker-free,
/// PY_NULL-padded scope.
#[test]
fn instance_method_binds_keyword_only_through_signature() {
crate::typedef::init_typeobjects();
let cls = type_object();
let obj = __pyre_wrap___new__(&[cls]).expect("synthesized __new__");
__pyre_wrap___init__(&[obj, w_int_new(7)]).expect("Demo.__init__");

// The `Signature` the instance-method arm derives for
// `combine(&self, factor, #[kwonly] bias)`: `self` positional-only,
// then `factor`, then a keyword-only `bias`.
let signature = crate::gateway::Signature::new(
vec!["self", "factor", "bias"],
None,
None,
/*kwonlyargcount*/ 1,
/*posonlyargcount*/ 1,
);
let bound = crate::call::bind_kwargs_to_signature(
&signature,
"combine",
&[obj, w_int_new(3)],
&[(rustpython_wtf8::Wtf8Buf::from("bias"), w_int_new(5))],
)
.expect("signature binding");
// 7 * 3 + 5 through the bound (marker-free, PY_NULL-padded) scope.
let via_keyword = __pyre_wrap_combine(&bound).expect("keyword-bound combine");
assert_eq!(unsafe { w_int_get_value(via_keyword) }, 26);

// The all-positional call omits `bias`, so its `#[default(0)]` applies.
let via_positional = __pyre_wrap_combine(&[obj, w_int_new(3)]).expect("positional combine");
assert_eq!(unsafe { w_int_get_value(via_positional) }, 21);
}

/// TypeDef ownership is process-global: another OS thread must observe
/// the exact same Python type object, not a fresh TLS allocation.
#[test]
Expand Down
10 changes: 7 additions & 3 deletions pyre/pyre-interpreter/src/objspace/std/mapdict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -690,12 +690,16 @@ pub unsafe fn instance_node_getdictvalue_checked(
ensure_mapdict_initialized(obj);
let inst = &mut *(obj as *mut pyre_object::W_ObjectObject);
let map = inst._get_mapdict_map();
let w_res = unsafe { node_read_checked(map, inst, name, DICT) };
let w = unsafe { node_read_checked(map, inst, name, DICT) }?;
// mapdict.py:846-847 getdictvalue → read → _direct_read (592-598): lazily
// migrate to boxed storage when the read attribute is unboxed and its class
// has frozen unboxing.
// has frozen unboxing. A raise in `read` unwinds before the migration tail
// (846-847 → 58 → 312-313 returns None on the miss path, never reaching
// _direct_read), so migration is skipped whenever the read raised;
// propagating the read error here mirrors that — and `maybe_migrate_to_boxed`
// re-derives the None on the raising path, so it is a no-op there regardless.
unsafe { maybe_migrate_to_boxed(map, inst, name, DICT) };
w_res
Ok(w)
}

/// `deldictvalue` routed to the mapdict node layer (mapdict.py:852-857
Expand Down
21 changes: 21 additions & 0 deletions pyre/pyre-interpreter/src/typedef.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2072,8 +2072,16 @@ unsafe fn retag_classmethod_descriptors(type_obj: PyObjectRef) {
/// it those carry no owner and report their errors under the bare method name,
/// where the namespace entry for the same method reports `type.name`.
///
/// `dont_look_inside`: reads the prebuilt `method_owner` table's per-type
/// `static OWNER` — a field-bearing frozen instance carrying a `fn` pointer,
/// which the annotator cannot model as a prebuilt constant. Making this a
/// residual-call boundary keeps the receiver-owner setup opaque to the JIT
/// so a lifting caller (`getattr_str_impl`'s bound-method assembly) is not
/// dragged into the unmodellable static read.
///
/// # Safety
/// `func` must be a valid, live function object.
#[majit_macros::dont_look_inside]
pub(crate) unsafe fn stamp_builtin_owner(func: PyObjectRef, type_name: &str) {
let Some(owner) = method_owner(type_name) else {
return;
Expand Down Expand Up @@ -2598,6 +2606,19 @@ pub(crate) fn make_new_descr_with_signature(
crate::make_builtin_function_as_builtin_with_signature("__new__", func, signature)
}

/// [`make_new_descr`] optionally carrying a `Signature`: `Some` binds keyword
/// arguments by name before the constructor runs; `None` (a variadic
/// whole-args `__new__`) keeps the positional-only carrier.
pub(crate) fn make_new_descr_maybe_sig(
func: fn(&[PyObjectRef]) -> Result<PyObjectRef, crate::PyError>,
signature: Option<crate::gateway::Signature>,
) -> PyObjectRef {
match signature {
Some(signature) => make_new_descr_with_signature(func, signature),
None => make_new_descr(func),
}
}

/// `typeobject.c tp_new_wrapper` — `__new__` takes the class to instantiate
/// as its first argument, so a call without one is rejected before the body
/// reads the value positionals behind it.
Expand Down
12 changes: 5 additions & 7 deletions pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2031,13 +2031,11 @@ pub(crate) fn walker_ec_leave(
/// descriptor. The first instruction's arraylen descriptor is deliberately
/// not interchangeable with the later getarrayitem descriptor.
pub(super) fn wrapper_args_item_descr_index(code: &[u8]) -> Option<u32> {
// Keyword-capable generated wrappers first call
// `split_builtin_kwargs(args)` before their arity/unwrap code. Its
// positional-slice result can therefore occupy a register other than the
// wrapper input r0, and register colouring can move it again between the
// arity check and unwrap. Generated gateways perform their argument
// extraction before entering the typed body, so the first Ref item read
// after the first slice-length read is the wrapper-argument descriptor.
// Generated gateways perform their argument extraction before entering the
// typed body, reading the slice length before any element. The first Ref
// item read after the first slice-length read is therefore the
// wrapper-argument descriptor, independent of which register colouring
// assigns to the slice.
let arraylen_pc = crate::jitcode_runtime::decoded_ops(code)
.find(|decoded| decoded.key == "arraylen_gc/rd>i")
.map(|decoded| decoded.pc)?;
Expand Down
41 changes: 18 additions & 23 deletions pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,45 +143,40 @@ fn builtin_wrapper_heapcache_uses_item_not_length_descr() {
}

#[test]
fn keyword_builtin_wrapper_finds_colored_argument_slice_item_descr() {
fn signature_bound_wrapper_reads_argument_slice_with_distinct_item_descr() {
let wrapper =
named_jitcode("__pyre_wrap_getrandbits").expect("getrandbits builtin wrapper jitcode");
let mut ops = crate::jitcode_runtime::decoded_ops(&wrapper.code);
let first = ops.next().expect("wrapper first op");
// The result colour is not pinned. `split_builtin_kwargs` returns the
// aggregate `(&[PyObjectRef], Option<PyObjectRef>)`; when the codewriter
// materializes that pair the entry call yields it by reference
// (`inline_call_r_r`), and when it inlines the body far enough to leave
// only the leading `args.is_empty()` test at the entry the same call
// yields that by value (`inline_call_r_i`). Both start the wrapper by
// splitting positional from keyword arguments, which is what this asserts.
let first = crate::jitcode_runtime::decoded_ops(&wrapper.code)
.next()
.expect("wrapper first op");
// `getrandbits` registers with a `Signature`, so the call path resolves
// keywords into positional PY_NULL-padded slots before the wrapper runs.
// There is no `split_builtin_kwargs(args)` peel at the entry, so the
// wrapper does not open with the splitter's `inline_call_*`; it reads its
// argument array directly.
assert!(
first.opname.starts_with("inline_call_"),
"keyword wrapper starts by splitting positional and keyword arguments, got {}",
!first.opname.starts_with("inline_call_"),
"signature-bound wrapper does not open with a keyword-split call, got {}",
first.key
);

let item_descr_index =
wrapper_args_item_descr_index(&wrapper.code).expect("wrapper item descriptor");
// Select by descr identity rather than by position. The inlined splitter
// reads `args.len()` off the wrapper's own `r0` before the split, so "the
// first `arraylen_gc`" names that read and not the positional slice's once
// the body inlines; the item descr names the slice under every inline
// depth.
// Select by descr identity rather than by position: the length read names
// the array itself and the item read names its elements, so the two carry
// distinct heap-cache descriptors even though both index the same slice.
let getitem = crate::jitcode_runtime::decoded_ops(&wrapper.code)
.find(|op| {
op.key == "getarrayitem_gc_r/rid>r"
&& item_pool_descr_index(&wrapper.code, op.pc + 3) == item_descr_index
})
.expect("keyword wrapper argument-slice item read");
.expect("wrapper argument-slice item read");
let slice_reg = wrapper.code[getitem.pc + 1];
assert_ne!(
slice_reg, 0,
"register coloring keeps the argument slice off r0 at extraction"
);
// With no keyword split the argument slice is the wrapper input itself
// (r0); the length read names the same register.
crate::jitcode_runtime::decoded_ops(&wrapper.code)
.find(|op| op.key == "arraylen_gc/rd>i" && wrapper.code[op.pc + 1] == slice_reg)
.expect("keyword wrapper reads the argument-slice length off the colored slice");
.expect("wrapper reads the argument-slice length off the same slice register");
}

/// Descr-pool index encoded little-endian at `at`, resolved to its
Expand Down
Loading
Loading