From 7049faf1442ec16c9f392f03e9a367b568fcb446 Mon Sep 17 00:00:00 2001 From: Shunsuke Shibayama Date: Sat, 27 May 2023 23:07:41 +0900 Subject: [PATCH] fix: eliminate unsafe type-var updating --- crates/erg_common/shared.rs | 2 + crates/erg_compiler/context/compare.rs | 80 ++++++++++------------- crates/erg_compiler/context/generalize.rs | 6 +- crates/erg_compiler/ty/free.rs | 56 ++++------------ crates/erg_compiler/ty/mod.rs | 13 +--- 5 files changed, 54 insertions(+), 103 deletions(-) diff --git a/crates/erg_common/shared.rs b/crates/erg_common/shared.rs index ac50b8437..07e9d9463 100644 --- a/crates/erg_common/shared.rs +++ b/crates/erg_common/shared.rs @@ -62,11 +62,13 @@ impl Shared { } #[inline] + #[track_caller] pub fn borrow(&self) -> Ref<'_, T> { RefCell::borrow(&self.0) } #[inline] + #[track_caller] pub fn borrow_mut(&self) -> RefMut<'_, T> { RefCell::borrow_mut(&self.0) } diff --git a/crates/erg_compiler/context/compare.rs b/crates/erg_compiler/context/compare.rs index 2bace8639..dd66667ba 100644 --- a/crates/erg_compiler/context/compare.rs +++ b/crates/erg_compiler/context/compare.rs @@ -459,54 +459,46 @@ impl Context { // => ?P.undoable_link(Int) // => Mul Int :> Int (FreeVar(lfv), rhs) => { - match &*lfv.borrow() { - FreeKind::Linked(t) | FreeKind::UndoableLinked { t, .. } => { - self.supertype_of(t, rhs) - } - FreeKind::Unbound { constraint: _, .. } - | FreeKind::NamedUnbound { constraint: _, .. } => { - if let Some((_sub, sup)) = lfv.get_subsup() { - lfv.forced_undoable_link(rhs); - let res = self.supertype_of(&sup, rhs); - lfv.undo(); - res - } else if let Some(lfvt) = lfv.get_type() { - // e.g. lfv: ?L(: Int) is unreachable - // but - // ?L(: Array(Type, 3)) :> Array(Int, 3) - // => Array(Type, 3) :> Array(Typeof(Int), 3) - // => true - let rhs_meta = self.meta_type(rhs); - self.supertype_of(&lfvt, &rhs_meta) - } else { - // constraint is uninitialized - log!(err "constraint is uninitialized: {lfv}/{rhs}"); - true - } - } + if let FreeKind::Linked(t) | FreeKind::UndoableLinked { t, .. } = &*lfv.borrow() { + return self.supertype_of(t, rhs); + } + if let Some((_sub, sup)) = lfv.get_subsup() { + lfv.undoable_link(rhs); + let res = self.supertype_of(&sup, rhs); + lfv.undo(); + res + } else if let Some(lfvt) = lfv.get_type() { + // e.g. lfv: ?L(: Int) is unreachable + // but + // ?L(: Array(Type, 3)) :> Array(Int, 3) + // => Array(Type, 3) :> Array(Typeof(Int), 3) + // => true + let rhs_meta = self.meta_type(rhs); + self.supertype_of(&lfvt, &rhs_meta) + } else { + // constraint is uninitialized + log!(err "constraint is uninitialized: {lfv}/{rhs}"); + true } } - (lhs, FreeVar(rfv)) => match &*rfv.borrow() { - FreeKind::Linked(t) | FreeKind::UndoableLinked { t, .. } => { - self.supertype_of(lhs, t) + (lhs, FreeVar(rfv)) => { + if let FreeKind::Linked(t) | FreeKind::UndoableLinked { t, .. } = &*rfv.borrow() { + return self.supertype_of(lhs, t); } - FreeKind::Unbound { constraint: _, .. } - | FreeKind::NamedUnbound { constraint: _, .. } => { - if let Some((sub, _sup)) = rfv.get_subsup() { - rfv.forced_undoable_link(lhs); - let res = self.supertype_of(lhs, &sub); - rfv.undo(); - res - } else if let Some(rfvt) = rfv.get_type() { - let lhs_meta = self.meta_type(lhs); - self.supertype_of(&lhs_meta, &rfvt) - } else { - // constraint is uninitialized - log!(err "constraint is uninitialized: {lhs}/{rfv}"); - true - } + if let Some((sub, _sup)) = rfv.get_subsup() { + rfv.undoable_link(lhs); + let res = self.supertype_of(lhs, &sub); + rfv.undo(); + res + } else if let Some(rfvt) = rfv.get_type() { + let lhs_meta = self.meta_type(lhs); + self.supertype_of(&lhs_meta, &rfvt) + } else { + // constraint is uninitialized + log!(err "constraint is uninitialized: {lhs}/{rfv}"); + true } - }, + } (Record(lhs), Record(rhs)) => { for (l_k, l_t) in lhs.iter() { if let Some((r_k, r_t)) = rhs.get_key_value(l_k) { diff --git a/crates/erg_compiler/context/generalize.rs b/crates/erg_compiler/context/generalize.rs index 3263173de..27cfd63ec 100644 --- a/crates/erg_compiler/context/generalize.rs +++ b/crates/erg_compiler/context/generalize.rs @@ -148,7 +148,7 @@ impl Generalizer { if sub == sup { let t = self.generalize_t(sub, uninit); let res = FreeVar(fv); - res.forced_link(&t); + res.link(&t); res } else if sup != Obj && !self.qnames.contains(&fv.unbound_name().unwrap()) @@ -532,10 +532,10 @@ impl<'c, 'q, 'l, L: Locational> Dereferencer<'c, 'q, 'l, L> { fv.dummy_link(); } (true, false) => { - fv.forced_undoable_link(&super_t); + fv.undoable_link(&super_t); } (false, true | false) => { - fv.forced_undoable_link(&sub_t); + fv.undoable_link(&sub_t); } } let res = self.validate_subsup(sub_t, super_t); diff --git a/crates/erg_compiler/ty/free.rs b/crates/erg_compiler/ty/free.rs index 61d75d5a5..6caddb3ac 100644 --- a/crates/erg_compiler/ty/free.rs +++ b/crates/erg_compiler/ty/free.rs @@ -601,9 +601,11 @@ impl LimitedDisplay for Free { } impl Free { + #[track_caller] pub fn borrow(&self) -> Ref<'_, FreeKind> { self.0.borrow() } + #[track_caller] pub fn borrow_mut(&self) -> RefMut<'_, FreeKind> { self.0.borrow_mut() } @@ -614,15 +616,6 @@ impl Free { pub fn forced_as_ref(&self) -> &FreeKind { unsafe { self.as_ptr().as_ref() }.unwrap() } - pub fn force_replace(&self, new: FreeKind) { - // prevent linking to self - if addr_eq!(*self.borrow(), new) { - return; - } - unsafe { - *self.0.as_ptr() = new; - } - } pub fn can_borrow(&self) -> bool { self.0.can_borrow() } @@ -756,6 +749,7 @@ impl Free { Self(Shared::new(FreeKind::Linked(t))) } + #[track_caller] pub fn replace(&self, to: FreeKind) { // prevent linking to self if self.is_linked() && addr_eq!(*self.borrow(), to) { @@ -766,6 +760,7 @@ impl Free { /// returns linked type (panic if self is unbounded) /// NOTE: check by `.is_linked` before call + #[track_caller] pub fn crack(&self) -> Ref<'_, T> { Ref::map(self.borrow(), |f| match f { FreeKind::Linked(t) | FreeKind::UndoableLinked { t, .. } => t, @@ -775,6 +770,7 @@ impl Free { }) } + #[track_caller] pub fn crack_constraint(&self) -> Ref<'_, Constraint> { Ref::map(self.borrow(), |f| match f { FreeKind::Linked(_) | FreeKind::UndoableLinked { .. } => panic!("the value is linked"), @@ -817,6 +813,7 @@ impl Free { impl Free { /// SAFETY: use `Type/TyParam::link` instead of this. /// This method may cause circular references. + #[track_caller] pub(super) fn link(&self, to: &T) { // prevent linking to self if self.is_linked() && addr_eq!(*self.crack(), *to) { @@ -825,19 +822,7 @@ impl Free { self.borrow_mut().replace(to.clone()); } - /// NOTE: Do not use this except to rewrite circular references. - /// No reference to any type variable may be left behind when rewriting. - /// However, `get_subsup` is safe because it does not return references. - pub(super) fn forced_link(&self, to: &T) { - // prevent linking to self - if self.is_linked() && addr_eq!(*self.crack(), *to) { - return; - } - unsafe { - self.as_ptr().as_mut().unwrap().replace(to.clone()); - } - } - + #[track_caller] pub fn undoable_link(&self, to: &T) { if self.is_linked() && addr_eq!(*self.crack(), *to) { panic!("link to self"); @@ -850,29 +835,12 @@ impl Free { *self.borrow_mut() = new; } - /// NOTE: Do not use this except to rewrite circular references. - /// No reference to any type variable may be left behind when rewriting. - /// However, `get_subsup` is safe because it does not return references. - pub fn forced_undoable_link(&self, to: &T) { - if self.is_linked() && addr_eq!(*self.crack(), *to) { - panic!("link to self"); - } - let prev = self.clone_inner(); - let new = FreeKind::UndoableLinked { - t: to.clone(), - previous: Box::new(prev), - }; - self.force_replace(new); - } - pub fn undo(&self) { - match &*self.borrow() { - FreeKind::UndoableLinked { previous, .. } => { - let prev = *previous.clone(); - self.force_replace(prev); - } + let prev = match &*self.borrow() { + FreeKind::UndoableLinked { previous, .. } => *previous.clone(), _other => panic!("cannot undo"), - } + }; + self.replace(prev); } pub fn unwrap_unbound(self) -> (Option, usize, Constraint) { @@ -922,7 +890,7 @@ impl Free { impl Free { pub fn dummy_link(&self) { - self.forced_undoable_link(&T::default()); + self.undoable_link(&T::default()); } } diff --git a/crates/erg_compiler/ty/mod.rs b/crates/erg_compiler/ty/mod.rs index cf69f23eb..0754e9fc9 100644 --- a/crates/erg_compiler/ty/mod.rs +++ b/crates/erg_compiler/ty/mod.rs @@ -2326,7 +2326,7 @@ impl Type { pub fn qvars(&self) -> Set<(Str, Constraint)> { match self { - Self::FreeVar(fv) if fv.is_linked() => fv.crack().qvars(), + Self::FreeVar(fv) if fv.is_linked() => fv.unsafe_crack().qvars(), Self::FreeVar(fv) if !fv.constraint_is_uninited() => { let base = set! {(fv.unbound_name().unwrap(), fv.constraint().unwrap())}; if let Some((sub, sup)) = fv.get_subsup() { @@ -2973,17 +2973,6 @@ impl Type { _ => panic!("{self} is not a free variable"), } } - - pub(crate) fn forced_link(&self, to: &Type) { - if self.addr_eq(to) { - return; - } - match self { - Self::FreeVar(fv) => fv.forced_link(to), - Self::Refinement(refine) => refine.t.forced_link(to), - _ => panic!("{self} is not a free variable"), - } - } } pub struct ReplaceTable<'t> {