From 4c4edf50a6a69a614dd2d606d4b07fbad0896b7a Mon Sep 17 00:00:00 2001 From: Aaron Orenstein Date: Fri, 17 Feb 2023 13:00:26 -0800 Subject: [PATCH] Fix closure locals Summary: Closure bodies implicitly load their locals from the closure class during function prelude - we need to emulate that behavior. Reviewed By: davidpichardie Differential Revision: D42443146 fbshipit-source-id: 9bb9e0e0ed6fb24db66a1679c4024fdd5af2eb62 --- .../hack/src/hackc/ir/conversions/textual/class.rs | 9 +++- hphp/hack/src/hackc/ir/conversions/textual/func.rs | 4 +- .../hackc/ir/conversions/textual/lower/class.rs | 60 +++++++++++++++++++++- .../src/hackc/ir/conversions/textual/lower/func.rs | 35 +++++++++++++ hphp/hack/src/hackc/ir/ir_core/func_builder.rs | 38 ++++++++++++++ hphp/hack/src/hackc/test/infer/closure.hack | 42 +++++++++++++++ 6 files changed, 184 insertions(+), 4 deletions(-) create mode 100644 hphp/hack/src/hackc/test/infer/closure.hack diff --git a/hphp/hack/src/hackc/ir/conversions/textual/class.rs b/hphp/hack/src/hackc/ir/conversions/textual/class.rs index 3cf0a73347c..380f771eaa4 100644 --- a/hphp/hack/src/hackc/ir/conversions/textual/class.rs +++ b/hphp/hack/src/hackc/ir/conversions/textual/class.rs @@ -77,8 +77,10 @@ impl<'a, 'b, 'c> ClassState<'a, 'b, 'c> { unit_state: &'a mut UnitState, class: ir::Class<'c>, ) -> Self { - let needs_factory = - !class.flags.is_interface() && !class.flags.is_trait() && !class.flags.is_enum(); + let needs_factory = !class.flags.is_interface() + && !class.flags.is_trait() + && !class.flags.is_enum() + && !class.flags.contains(ir::Attr::AttrIsClosureClass); ClassState { class, needs_factory, @@ -114,6 +116,7 @@ impl ClassState<'_, '_, '_> { } if self.needs_factory { + // TODO: This should be the parameters from the constructor. self.write_factory(std::iter::empty())?; } @@ -362,6 +365,7 @@ impl ClassState<'_, '_, '_> { class: &self.class, is_static, strings: Arc::clone(&self.unit_state.strings), + flags: method.flags, }); let func = lower::lower_func( method.func, @@ -395,6 +399,7 @@ impl ClassState<'_, '_, '_> { class: &self.class, is_static, strings: Arc::clone(&self.unit_state.strings), + flags: method.flags, }); func::write_func( self.txf, diff --git a/hphp/hack/src/hackc/ir/conversions/textual/func.rs b/hphp/hack/src/hackc/ir/conversions/textual/func.rs index 34a7b6d3857..de331956739 100644 --- a/hphp/hack/src/hackc/ir/conversions/textual/func.rs +++ b/hphp/hack/src/hackc/ir/conversions/textual/func.rs @@ -28,6 +28,7 @@ use ir::Instr; use ir::InstrId; use ir::LocId; use ir::LocalId; +use ir::MethodFlags; use ir::SpecialClsRef; use ir::StringInterner; use ir::UnitBytesId; @@ -923,8 +924,8 @@ impl<'a, 'b, 'c> FuncState<'a, 'b, 'c> { } pub(crate) fn write_todo(&mut self, msg: &str) -> Result { - trace!("TODO: {}", msg); textual_todo! { + message = ("TODO: {msg}"), let target = format!("$todo.{msg}"); self.fb.call(&target, ()) } @@ -979,6 +980,7 @@ pub(crate) struct MethodInfo<'a> { pub(crate) class: &'a ir::Class<'a>, pub(crate) is_static: IsStatic, pub(crate) strings: Arc, + pub(crate) flags: MethodFlags, } impl MethodInfo<'_> { diff --git a/hphp/hack/src/hackc/ir/conversions/textual/lower/class.rs b/hphp/hack/src/hackc/ir/conversions/textual/lower/class.rs index cd158eae676..158844dc542 100644 --- a/hphp/hack/src/hackc/ir/conversions/textual/lower/class.rs +++ b/hphp/hack/src/hackc/ir/conversions/textual/lower/class.rs @@ -1,10 +1,23 @@ use std::sync::Arc; +use ir::instr; +use ir::Attr; use ir::Class; +use ir::Coeffects; +use ir::Constant; +use ir::FuncBuilder; +use ir::Instr; +use ir::LocalId; +use ir::MemberOpBuilder; +use ir::Method; +use ir::MethodFlags; +use ir::MethodId; +use ir::Param; use ir::StringInterner; +use ir::Visibility; use log::trace; -pub(crate) fn lower_class<'a>(class: Class<'a>, _strings: Arc) -> Class<'a> { +pub(crate) fn lower_class<'a>(mut class: Class<'a>, strings: Arc) -> Class<'a> { if !class.ctx_constants.is_empty() { textual_todo! { trace!("TODO: class.ctx_constants"); @@ -17,5 +30,50 @@ pub(crate) fn lower_class<'a>(class: Class<'a>, _strings: Arc) - } } + if class.flags.contains(Attr::AttrIsClosureClass) { + create_default_closure_constructor(&mut class, strings); + } + class } + +fn create_default_closure_constructor<'a>(class: &mut Class<'a>, strings: Arc) { + let name = MethodId::from_str("__construct", &strings); + + let func = FuncBuilder::build_func(Arc::clone(&strings), |fb| { + let loc = fb.add_loc(class.src_loc.clone()); + fb.func.loc_id = loc; + + // '$this' parameter is implied. + + for prop in &class.properties { + fb.func.params.push(Param { + name: prop.name.id, + is_variadic: false, + is_inout: false, + is_readonly: false, + user_attributes: Vec::new(), + ty: prop.type_info.clone(), + default_value: None, + }); + + let lid = LocalId::Named(prop.name.id); + let value = fb.emit(Instr::Hhbc(instr::Hhbc::CGetL(lid, loc))); + MemberOpBuilder::base_h(loc).emit_set_m_pt(fb, prop.name, value); + } + + let null = fb.emit_constant(Constant::Null); + fb.emit(Instr::ret(null, loc)); + }); + + let method = Method { + attributes: Vec::new(), + attrs: Attr::AttrNone, + coeffects: Coeffects::default(), + flags: MethodFlags::empty(), + func, + name, + visibility: Visibility::Public, + }; + class.methods.push(method); +} diff --git a/hphp/hack/src/hackc/ir/conversions/textual/lower/func.rs b/hphp/hack/src/hackc/ir/conversions/textual/lower/func.rs index 8165bfdefca..1d0f88608b0 100644 --- a/hphp/hack/src/hackc/ir/conversions/textual/lower/func.rs +++ b/hphp/hack/src/hackc/ir/conversions/textual/lower/func.rs @@ -5,8 +5,13 @@ use std::sync::Arc; +use ir::instr::Hhbc; use ir::Func; use ir::FuncBuilder; +use ir::Instr; +use ir::LocalId; +use ir::MemberOpBuilder; +use ir::MethodFlags; use ir::StringInterner; use log::trace; @@ -22,6 +27,14 @@ pub(crate) fn lower_func<'a>( ir::print::DisplayFunc::new(&func, true, &strings) ); + // In a closure we implicitly load all the properties as locals - so start + // with that as a prelude to all entrypoints. + if let Some(method_info) = method_info.as_ref() { + if method_info.flags.contains(MethodFlags::IS_CLOSURE_BODY) { + load_closure_vars(&mut func, method_info, &strings); + } + } + // Start by 'unasync'ing the Func. ir::passes::unasync(&mut func); trace!( @@ -50,3 +63,25 @@ pub(crate) fn lower_func<'a>( func } + +fn load_closure_vars(func: &mut Func<'_>, method_info: &MethodInfo<'_>, strings: &StringInterner) { + let mut instrs = Vec::new(); + + let loc = func.loc_id; + + for prop in &method_info.class.properties { + // Property names are the variable names without the '$'. + let prop_str = strings.lookup_bstr(prop.name.id); + let mut var = prop_str.to_vec(); + var.insert(0, b'$'); + let lid = LocalId::Named(strings.intern_bytes(var)); + + let iid = func.alloc_instr(Instr::MemberOp( + MemberOpBuilder::base_h(loc).query_pt(prop.name), + )); + instrs.push(iid); + instrs.push(func.alloc_instr(Instr::Hhbc(Hhbc::SetL(iid.into(), lid, loc)))); + } + + func.block_mut(Func::ENTRY_BID).iids.splice(0..0, instrs); +} diff --git a/hphp/hack/src/hackc/ir/ir_core/func_builder.rs b/hphp/hack/src/hackc/ir/ir_core/func_builder.rs index c6cba884039..3a256d0b72f 100644 --- a/hphp/hack/src/hackc/ir/ir_core/func_builder.rs +++ b/hphp/hack/src/hackc/ir/ir_core/func_builder.rs @@ -28,6 +28,7 @@ use crate::InstrIdMap; use crate::LocId; use crate::LocalId; use crate::MOpMode; +use crate::PropId; use crate::QueryMOp; use crate::ReadonlyOp; use crate::StringInterner; @@ -464,6 +465,10 @@ impl MemberOpBuilder { mop } + pub fn base_h(loc: LocId) -> Self { + Self::new(BaseOp::BaseH { loc }) + } + pub fn isset_ec(mut self, key: ValueId) -> MemberOp { self.operands.push(key); let loc = self.base_op.loc_id(); @@ -495,4 +500,37 @@ impl MemberOpBuilder { let mop = self.query_ec(key); builder.emit(Instr::MemberOp(mop)) } + + pub fn query_pt(self, key: PropId) -> MemberOp { + let loc = self.base_op.loc_id(); + self.final_op(FinalOp::QueryM { + key: MemberKey::PT(key), + readonly: ReadonlyOp::Any, + query_m_op: QueryMOp::CGet, + loc, + }) + } + + pub fn emit_query_pt(self, builder: &mut FuncBuilder<'_>, key: PropId) -> ValueId { + let mop = self.query_pt(key); + builder.emit(Instr::MemberOp(mop)) + } + + pub fn set_m_pt(mut self, key: PropId, value: ValueId) -> MemberOp { + let loc = self.base_op.loc_id(); + let key = MemberKey::PT(key); + let readonly = ReadonlyOp::Any; + self.operands.push(value); + self.final_op(FinalOp::SetM { key, readonly, loc }) + } + + pub fn emit_set_m_pt( + self, + builder: &mut FuncBuilder<'_>, + key: PropId, + value: ValueId, + ) -> ValueId { + let mop = self.set_m_pt(key, value); + builder.emit(Instr::MemberOp(mop)) + } } diff --git a/hphp/hack/src/hackc/test/infer/closure.hack b/hphp/hack/src/hackc/test/infer/closure.hack new file mode 100644 index 00000000000..a7c637e27bb --- /dev/null +++ b/hphp/hack/src/hackc/test/infer/closure.hack @@ -0,0 +1,42 @@ +// RUN: %hackc compile-infer %s | FileCheck %s + +// TEST-CHECK-BAL: define Closure$closure1$static.__invoke +// CHECK: define Closure$closure1$static.__invoke($this: *Closure$closure1$static) : *HackMixed { +// CHECK: local $x: *void +// CHECK: #b0: +// CHECK: n0 = &$this +// CHECK: n1 = $builtins.hack_string("x") +// CHECK: n2 = $builtins.hack_dim_field_get(n0, n1) +// CHECK: n3: *HackMixed = load n2 +// CHECK: store &$x <- n3: *HackMixed +// CHECK: n4: *HackMixed = load &$x +// CHECK: n5 = $builtins.hhbc_print(n4) +// CHECK: ret n5 +// CHECK: } + +// TEST-CHECK-BAL: define Closure$closure1.__construct +// CHECK: define Closure$closure1.__construct($this: *Closure$closure1, x: *HackMixed) : *HackMixed { +// CHECK: #b0: +// CHECK: n0: *HackMixed = load &x +// CHECK: n1 = &$this +// CHECK: n2 = $builtins.hack_string("x") +// CHECK: n3 = $builtins.hack_dim_field_get(n1, n2) +// CHECK: store n3 <- n0: *HackMixed +// CHECK: ret null +// CHECK: } + +// TEST-CHECK-BAL: define $root.closure1 +// CHECK: define $root.closure1($this: *void, $x: *HackString) : *HackMixed { +// CHECK: local $y: *void +// CHECK: #b0: +// CHECK: n0: *HackMixed = load &$x +// CHECK: n1 = __sil_allocate() +// CHECK: n2 = Closure$closure1.__construct(n1, n0) +// CHECK: store &$y <- n1: *HackMixed +// CHECK: n3: *HackMixed = load &$y +// CHECK: ret n3 +// CHECK: } +function closure1(string $x): mixed { + $y = () ==> print($x); + return $y; +} -- 2.11.4.GIT