diff --git a/majit/majit-translate/src/front/mir.rs b/majit/majit-translate/src/front/mir.rs index c15760b4ee4..e89dce29da5 100644 --- a/majit/majit-translate/src/front/mir.rs +++ b/majit/majit-translate/src/front/mir.rs @@ -23888,12 +23888,14 @@ mod tests { ); } - /// Array slicing keeps the residual RangeTo path because the general stop - /// has no length proof; mutable indexing is likewise residual because it - /// writes through a view. + /// Array slicing keeps the residual RangeTo path because a general stop has + /// no proof that `end <= slice.len()`. Both halves are asserted — the + /// residual call is present AND no `__getslice_rangeto` marker was planted + /// — so a lowering change that drops the call for an unrelated reason is a + /// failure rather than a pass. #[test] #[ignore] - fn call_function_impl_result_has_no_residual_array_index() { + fn call_function_impl_result_keeps_residual_array_index() { use crate::model::OpKind; let path = concat!( env!("CARGO_MANIFEST_DIR"), @@ -23902,19 +23904,31 @@ mod tests { let llbc = Llbc::load(path).expect("load real LLBC"); let graph = super::lower_function(&llbc, "call_function_impl_result") .expect("lower call_function_impl_result"); + let calls_path = |want: &[&str]| -> usize { + let want: Vec = want.iter().map(|s| s.to_string()).collect(); + graph + .blocks + .iter() + .flat_map(|b| &b.operations) + .filter(|op| { + matches!( + &op.kind, + OpKind::Call { + target: crate::model::CallTarget::FunctionPath { segments }, + .. + } if segments == &want + ) + }) + .count() + }; assert!( - graph.blocks.iter().flat_map(|b| &b.operations).any(|op| { - matches!( - &op.kind, - OpKind::Call { - target: crate::model::CallTarget::FunctionPath { segments }, - .. - } if segments - == &["core", "array", "", "index"] - .map(str::to_string) - ) - }), + calls_path(&["core", "array", "", "index"]) >= 1, "general RangeTo array index remains residual" ); + assert_eq!( + calls_path(&["__getslice_rangeto"]), + 0, + "no general RangeTo site is rewritten — the fold is declined" + ); } } diff --git a/majit/majit-translate/src/front/option_closure_select.rs b/majit/majit-translate/src/front/option_closure_select.rs index 44842b56f56..28231de9fad 100644 --- a/majit/majit-translate/src/front/option_closure_select.rs +++ b/majit/majit-translate/src/front/option_closure_select.rs @@ -486,20 +486,37 @@ fn emit_call_once( mod tests { use super::*; + /// The receiver's `Option` and the result's `Option` are separate + /// instantiations with separate owner roots. The fixture keeps them + /// distinct so an assertion on a constructed variant's `owner_root` fails + /// if the rewriter builds the result under the receiver's owners. + const RECV_OPTION: &str = "test::recv::Option"; + const RECV_SOME: &str = "test::recv::Option::Some"; + const RESULT_OPTION: &str = "test::result::Option"; + const RESULT_SOME: &str = "test::result::Option::Some"; + fn site(kind: ClosureCombinator, result_var: Variable) -> ClosureSelectSite { + site_with_result_niche(kind, result_var, false) + } + + fn site_with_result_niche( + kind: ClosureCombinator, + result_var: Variable, + result_niche: bool, + ) -> ClosureSelectSite { ClosureSelectSite { kind, result_var, - option_owner: "core::option::Option".into(), - some_owner: "core::option::Option::Some".into(), + option_owner: RECV_OPTION.into(), + some_owner: RECV_SOME.into(), call_once_owner: "test::closure".into(), payload_ty: ValueType::Int, call_result_ty: ValueType::Int, args_tuple_suffix: String::new(), niche: false, - result_option_owner: "core::option::Option".into(), - result_some_owner: "core::option::Option::Some".into(), - result_niche: false, + result_option_owner: RESULT_OPTION.into(), + result_some_owner: RESULT_SOME.into(), + result_niche, } } @@ -514,7 +531,7 @@ mod tests { .push_op_var( a, OpKind::Call { - target: CallTarget::method(method, Some("core::option::Option".into())), + target: CallTarget::method(method, Some(RECV_OPTION.into())), args: vec![opt, env], result_ty: ValueType::Int, }, @@ -544,6 +561,40 @@ mod tests { ) == 0 } + /// The distinct `owner_root`s of every `FieldWrite` naming `field_name`, + /// sorted. A built variant is spelled by its writes, so this is what tells + /// the result's owners apart from the receiver's. + fn field_write_owners(g: &FunctionGraph, field_name: &str) -> Vec { + let mut owners: Vec = g + .blocks + .iter() + .flat_map(|blk| &blk.operations) + .filter_map(|op| match &op.kind { + OpKind::FieldWrite { field, .. } if field.name == field_name => { + Some(field.owner_root.clone().unwrap_or_default()) + } + _ => None, + }) + .collect(); + owners.sort(); + owners.dedup(); + owners + } + + fn count_ctors(g: &FunctionGraph) -> usize { + count_calls( + g, + |t| matches!(t, CallTarget::SyntheticTransparentCtor { name, .. } if name == "Option"), + ) + } + + fn count_null_mut(g: &FunctionGraph) -> usize { + count_calls(g, |t| { + matches!(t, CallTarget::FunctionPath { segments } + if segments == &["core", "ptr", "null_mut"].map(str::to_string)) + }) + } + #[test] fn map_selects_some_call_wrapped_and_none() { let (g, a) = build_and_rewrite(ClosureCombinator::Map, "map"); @@ -558,11 +609,21 @@ mod tests { ); assert_eq!(g.blocks[a].exits.len(), 2, "A branches to Some/None arms"); // Two `Option` ctors: Some(f(x)) in the then arm, None in the else arm. - let ctors = count_calls( - &g, - |t| matches!(t, CallTarget::SyntheticTransparentCtor { name, .. } if name == "Option"), + assert_eq!(count_ctors(&g), 2, "map builds Some(U) and None"); + // Both built variants are keyed to the RESULT roots, never the + // receiver's: `map`'s `Option` is a different instantiation. + assert_eq!( + field_write_owners(&g, "__discriminant"), + vec![RESULT_OPTION], + "both built variants key the result enum root, not the receiver's" + ); + // The other `__pos_0` write is the closure's `(x,)` Args tuple, which is + // a real `Tuple` and unrelated to either `Option` instantiation. + assert_eq!( + field_write_owners(&g, "__pos_0"), + vec!["Tuple".to_string(), RESULT_SOME.to_string()], + "the Some(U) payload keys the result Some variant" ); - assert_eq!(ctors, 2, "map builds Some(U) and None"); } #[test] @@ -581,14 +642,90 @@ mod tests { "the Some arm calls the closure once" ); // Only the None arm builds an Option; the Some arm forwards the call. - let ctors = count_calls( - &g, - |t| matches!(t, CallTarget::SyntheticTransparentCtor { name, .. } if name == "Option"), + assert_eq!(count_ctors(&g), 1, "and_then builds only None"); + assert_eq!( + field_write_owners(&g, "__discriminant"), + vec![RESULT_OPTION], + "the built None keys the result enum root, not the receiver's" ); - assert_eq!(ctors, 1, "and_then builds only None"); assert_eq!(g.blocks[a].exits.len(), 2); } + /// A niche result `Option` is a one-word pointer: `Some(x)` is `x` and + /// `None` is null, so neither arm may build an aggregate. + fn build_and_rewrite_niche_result(kind: ClosureCombinator, method: &str) -> FunctionGraph { + let mut g = FunctionGraph::new("test_closure_select_niche_result"); + let a = g.startblock; + let opt = g.push_op_var(a, OpKind::ConstInt(0), true).unwrap(); + let env = g.push_op_var(a, OpKind::ConstInt(7), true).unwrap(); + let result = g + .push_op_var( + a, + OpKind::Call { + target: CallTarget::method(method, Some(RECV_OPTION.into())), + args: vec![opt, env], + result_ty: ValueType::Ref(None), + }, + true, + ) + .unwrap(); + let (b, _b_args) = g.create_block_with_arg_vars(1); + g.set_return(b, None); + g.set_goto(a, b, vec![result.clone()]); + let rewritten = + rewire_closure_select_call_sites(&mut g, &[site_with_result_niche(kind, result, true)]); + assert_eq!( + rewritten, 1, + "the niche-result {method} site must be rewritten" + ); + g + } + + #[test] + fn map_with_niche_result_builds_no_aggregate() { + let g = build_and_rewrite_niche_result(ClosureCombinator::Map, "map"); + assert!(residual_gone(&g, "map"), "residual map call removed"); + assert_eq!( + count_calls( + &g, + |t| matches!(t, CallTarget::Method { name, .. } if name == "call_once") + ), + 1, + "the Some arm still calls the closure once" + ); + assert_eq!( + count_ctors(&g), + 0, + "a niche Some(f(x)) is f(x) itself — no Option aggregate" + ); + assert_eq!( + field_write_owners(&g, "__discriminant"), + Vec::::new(), + "a niche Option has no discriminant field to write" + ); + assert_eq!(count_null_mut(&g), 1, "the None arm is the null pointer"); + } + + #[test] + fn and_then_with_niche_result_builds_no_aggregate() { + let g = build_and_rewrite_niche_result(ClosureCombinator::AndThen, "and_then"); + assert!( + residual_gone(&g, "and_then"), + "residual and_then call removed" + ); + assert_eq!( + count_ctors(&g), + 0, + "the Some arm forwards the closure's own Option — no aggregate" + ); + assert_eq!( + field_write_owners(&g, "__discriminant"), + Vec::::new(), + "a niche Option has no discriminant field to write" + ); + assert_eq!(count_null_mut(&g), 1, "the None arm is the null pointer"); + } + #[test] fn unwrap_or_else_forwards_payload_and_calls_on_none() { let (g, a) = build_and_rewrite(ClosureCombinator::UnwrapOrElse, "unwrap_or_else"); @@ -713,7 +850,7 @@ mod tests { .push_op_var( a, OpKind::Call { - target: CallTarget::method("map", Some("core::option::Option".into())), + target: CallTarget::method("map", Some(RECV_OPTION.into())), args: vec![opt, env], result_ty: ValueType::Int, }, diff --git a/majit/majit-translate/src/front/slice_index.rs b/majit/majit-translate/src/front/slice_index.rs index 99e25fb5a1f..f937b2b223a 100644 --- a/majit/majit-translate/src/front/slice_index.rs +++ b/majit/majit-translate/src/front/slice_index.rs @@ -313,36 +313,34 @@ fn rewire_one_slice_index_site( .map(|oi| (bi, oi)) }) .ok_or_else(|| format!("{name}: slice::index op vanished before rewrite"))?; - if let SliceIndexBounds::MinusOne { .. } = bounds { - graph.blocks[rb].operations[ri] = SpaceOperation { - result: Some(index_result), - kind: OpKind::Call { - target: CallTarget::FunctionPath { - segments: vec!["__getslice_minusone".to_string()], - }, - args: vec![slice], - result_ty: index_result_ty, - }, - }; - } else { - match bounds { - SliceIndexBounds::RangeFrom { start } => { - let synthetic_bound = graph.alloc_value_var(); - graph.blocks[rb].operations[ri] = SpaceOperation { - result: Some(index_result), - kind: OpKind::GetSlice { - args: vec![slice, start.clone(), synthetic_bound.clone()], - }, - }; - graph.blocks[rb].operations.insert( - ri, - SpaceOperation { - result: Some(synthetic_bound), - kind: OpKind::ConstNone, + match bounds { + SliceIndexBounds::MinusOne { .. } => { + graph.blocks[rb].operations[ri] = SpaceOperation { + result: Some(index_result), + kind: OpKind::Call { + target: CallTarget::FunctionPath { + segments: vec!["__getslice_minusone".to_string()], }, - ); - } - SliceIndexBounds::MinusOne { .. } => unreachable!(), + args: vec![slice], + result_ty: index_result_ty, + }, + }; + } + SliceIndexBounds::RangeFrom { start } => { + let synthetic_bound = graph.alloc_value_var(); + graph.blocks[rb].operations[ri] = SpaceOperation { + result: Some(index_result), + kind: OpKind::GetSlice { + args: vec![slice, start.clone(), synthetic_bound.clone()], + }, + }; + graph.blocks[rb].operations.insert( + ri, + SpaceOperation { + result: Some(synthetic_bound), + kind: OpKind::ConstNone, + }, + ); } } Ok(()) diff --git a/majit/majit-translate/src/tool/error.rs b/majit/majit-translate/src/tool/error.rs index 1c13e00a953..5e28a74b6be 100644 --- a/majit/majit-translate/src/tool/error.rs +++ b/majit/majit-translate/src/tool/error.rs @@ -255,6 +255,7 @@ pub fn offset2lineno(code: &HostCode, stopat: i64) -> u32 { fn no_source_lines( g: &std::cell::Ref<'_, crate::flowspace::model::FunctionGraph>, block: Option<&BlockRef>, + operindex: Option, ) -> Vec { /// Bound the listing: a blocked op's producer is normally a few ops away, /// and an unbounded dump would multiply the size of every record. @@ -270,13 +271,24 @@ fn no_source_lines( .collect::>() .join(", ") )); - for (i, op) in b.operations.iter().take(MAX_OPS).enumerate() { + // The window ENDS at the failing op, so a block whose failure sits past + // `MAX_OPS` still shows that op and the ops that produced its operands. + // Listing from 0 would drop both, which is the whole point of the dump. + let end = operindex + .map_or(b.operations.len(), |i| i.saturating_add(1)) + .min(b.operations.len()); + let start = end.saturating_sub(MAX_OPS); + if start > 0 { + out.push(format!(" … {start} earlier operation(s)")); + } + for (i, op) in b.operations[start..end].iter().enumerate() { + let i = start + i; out.push(format!(" op[{i}] {op}")); } - if b.operations.len() > MAX_OPS { + if end < b.operations.len() { out.push(format!( " … {} more operation(s)", - b.operations.len() - MAX_OPS + b.operations.len() - end )); } } @@ -357,23 +369,23 @@ pub fn source_lines1( // upstream: `source = graph.source`; attribute absent → ['no source!']. let source = match g.source() { Ok(s) => s, - Err(_) => return no_source_lines(&g, block), + Err(_) => return no_source_lines(&g, block, operindex), }; let filename = match g.filename() { Ok(s) => s, - Err(_) => return no_source_lines(&g, block), + Err(_) => return no_source_lines(&g, block, operindex), }; let startline = match g.startline() { Ok(n) => n, - Err(_) => return no_source_lines(&g, block), + Err(_) => return no_source_lines(&g, block, operindex), }; let func = match &g.func { Some(f) => f, - None => return no_source_lines(&g, block), + None => return no_source_lines(&g, block, operindex), }; let code = match func.code.as_deref() { Some(c) => c, - None => return no_source_lines(&g, block), + None => return no_source_lines(&g, block, operindex), }; let graph_lines: Vec<&str> = source.split('\n').collect(); @@ -823,6 +835,54 @@ mod tests { ); } + /// A graph with no Python source falls back to the operation dump. The + /// window must END at the failing op: listing from index 0 would show the + /// first `MAX_OPS` ops and drop both the failing op and its producer, which + /// is the only context the dump exists to supply. + #[test] + fn no_source_lines_window_ends_at_the_failing_operation() { + let startblock = Block::shared(Vec::new()); + { + let mut b = startblock.borrow_mut(); + for _ in 0..60 { + b.operations + .push(crate::flowspace::model::SpaceOperation::new( + "newtuple", + Vec::new(), + Hlvalue::Variable(Variable::new()), + )); + } + } + // `graph.func` is None, so every accessor errors and `source_lines1` + // takes the `no_source_lines` fallback. + let graph = FunctionGraph::new("f", startblock.clone()); + let graph_ref: GraphRef = Rc::new(RefCell::new(graph)); + let out = source_lines1(&graph_ref, Some(&startblock), Some(55), None, false, 0); + let listed: Vec<&String> = out.iter().filter(|l| l.contains("op[")).collect(); + + assert!( + out.iter().any(|l| l.starts_with(" op[55] ")), + "the failing op is listed: {out:?}" + ); + assert!( + out.iter().any(|l| l.starts_with(" op[54] ")), + "its producer context is listed: {out:?}" + ); + assert!( + !out.iter().any(|l| l.starts_with(" op[56] ")), + "the window stops at the failing op: {out:?}" + ); + assert_eq!(listed.len(), 40, "the listing stays bounded: {out:?}"); + assert!( + out.iter().any(|l| l == " … 16 earlier operation(s)"), + "the elided head is reported: {out:?}" + ); + assert!( + out.iter().any(|l| l == " … 4 more operation(s)"), + "the elided tail is reported: {out:?}" + ); + } + #[test] fn format_annotations_uses_upstream_style_somevalue_rendering() { let ann = RPythonAnnotator::new(None, None, None, false);