From 61ee6a15bed82c8ae0f9c8d3ecb44972c6d7124a Mon Sep 17 00:00:00 2001 From: Nitish Agarwal <1592163+nitishagar@users.noreply.github.com> Date: Sun, 2 Aug 2026 18:17:52 +0530 Subject: [PATCH] Dedup add_lower_bound/add_upper_bound into add_var_bound `add_lower_bound` and `add_upper_bound` were near-perfect mirrors: identical two-phase lock structure, identical `(res.is_ok(), quantified_kind)` match (including the IntVar arm), and identical fallbacks. They differed only in ways fully derivable from a single `is_upper` bool: which `Bounds` field is the bound-side vs the opposite-side, the `is_subset` argument order, and the `is_upper` value threaded through `get_new_bound`. Extract a private `add_var_bound(v, bound, is_upper, is_subset)` holding the shared body, and reduce the two public functions to one-line delegations (`false` / `true`). This mirrors the established `get_new_bound(..., is_upper: bool, ...)` idiom already accepted in this file, keeping the change idiomatic rather than introducing a one-use enum. The refactor is behavior-preserving: the helper is a pure relocation of the existing `add_lower_bound` statements with every divergence resolved by an `is_upper` branch that reproduces the original `add_upper_bound` text. The two-phase lock pattern (read under lock, drop guard, validate without lock so the `is_subset` callback can safely recurse into `is_subset_eq`, re-lock to write) is preserved verbatim, as are the IntVar normalization expect, the `(false, IntVar) => None` drop, and the `// TODO(issues/105)` any-error fallback. Public signatures are unchanged, so all five callers need no edits. Fixes #4292 --- pyrefly/lib/solver/solver.rs | 141 ++++++++++++++++++----------------- 1 file changed, 72 insertions(+), 69 deletions(-) diff --git a/pyrefly/lib/solver/solver.rs b/pyrefly/lib/solver/solver.rs index cbfee524ce..d4d535626f 100644 --- a/pyrefly/lib/solver/solver.rs +++ b/pyrefly/lib/solver/solver.rs @@ -2088,24 +2088,59 @@ impl Solver { Ok(()) } - pub fn add_lower_bound( + /// Shared core of [`Self::add_lower_bound`] (`is_upper == false`) and + /// [`Self::add_upper_bound`] (`is_upper == true`). + /// + /// The two public functions are near-perfect mirrors: every divergence is + /// mechanically derivable from `is_upper` — which `Bounds` field is the + /// bound-side vs the opposite-side, the `is_subset` argument order, and the + /// `is_upper` bool threaded through `get_new_bound` (the same idiom + /// [`Self::get_new_bound`] already uses). + /// + /// Preserves the two-phase lock pattern of the originals: read under the + /// lock, drop the guard, validate without the lock (the `is_subset` callback + /// recurses into `is_subset_eq` which re-locks `variables`), then re-lock to + /// write. Holding the lock across `is_subset` would deadlock. + fn add_var_bound( &self, v: Var, bound: Type, + is_upper: bool, is_subset: &mut dyn FnMut(&Type, &Type) -> Result<(), SubsetError>, ) -> Result<(), SubsetError> { let lock = self.variables.lock(); let e = lock.get(v); - let (first_bound, upper_bound, res, quantified_kind) = match &*e { + // The bound-side field holds bounds on the same side being added; the + // opposite-side field (`bounds.upper` for a lower bound, `bounds.lower` + // for an upper bound) is what the new bound must be consistent with. + let (first_bound, opp_bound, res, quantified_kind) = match &*e { Variable::Quantified { quantified: _, bounds, } | Variable::Unwrap(bounds) => ( - bounds.lower.first().cloned(), - self.get_current_bound(bounds.upper.clone()), + (if is_upper { + &bounds.upper + } else { + &bounds.lower + }) + .first() + .cloned(), + self.get_current_bound(if is_upper { + bounds.lower.clone() + } else { + bounds.upper.clone() + }), if let Variable::Quantified { quantified, .. } = &*e { - self.validate_bound_consistency(&bound, &bounds.lower, quantified.kind()) + self.validate_bound_consistency( + &bound, + if is_upper { + &bounds.upper + } else { + &bounds.lower + }, + quantified.kind(), + ) } else { Ok(()) }, @@ -2120,7 +2155,17 @@ impl Solver { drop(e); drop(lock); let res = res.and_then(|_| { - upper_bound.map_or(Ok(()), |upper_bound| is_subset(&bound, &upper_bound)) + // For a lower bound we check `bound <: opp_bound` (the existing upper + // bound); for an upper bound we check `opp_bound <: bound` (the + // existing lower bound). Either way the new bound must be consistent + // with the opposite-side bound via transitivity. + opp_bound.map_or(Ok(()), |opp_bound| { + if is_upper { + is_subset(&opp_bound, &bound) + } else { + is_subset(&bound, &opp_bound) + } + }) }); let new_bound = match (res.is_ok(), quantified_kind) { (true, Some(QuantifiedKind::IntVar)) => Some( @@ -2129,12 +2174,12 @@ impl Solver { // `validate_bound_consistency` accepted this bound, so the // same IntVar normalization must succeed before storing it. type_as_intvar_solution(&bound) - .expect("successful IntVar lower-bound check must normalize"), - false, + .expect("successful IntVar bound check must normalize"), + is_upper, is_subset, ), ), - (true, _) => Some(self.get_new_bound(first_bound, bound, false, is_subset)), + (true, _) => Some(self.get_new_bound(first_bound, bound, is_upper, is_subset)), (false, Some(QuantifiedKind::IntVar)) => None, (false, _) => { // TODO(https://github.com/facebook/pyrefly/issues/105): don't throw away the bound. @@ -2148,78 +2193,36 @@ impl Solver { quantified: _, bounds, } - | Variable::Unwrap(bounds) => self.add_bound(&mut bounds.lower, new_bound), + | Variable::Unwrap(bounds) => self.add_bound( + if is_upper { + &mut bounds.upper + } else { + &mut bounds.lower + }, + new_bound, + ), _ => {} } } res } + pub fn add_lower_bound( + &self, + v: Var, + bound: Type, + is_subset: &mut dyn FnMut(&Type, &Type) -> Result<(), SubsetError>, + ) -> Result<(), SubsetError> { + self.add_var_bound(v, bound, false, is_subset) + } + pub fn add_upper_bound( &self, v: Var, bound: Type, is_subset: &mut dyn FnMut(&Type, &Type) -> Result<(), SubsetError>, ) -> Result<(), SubsetError> { - let lock = self.variables.lock(); - let e = lock.get(v); - let (first_bound, lower_bound, res, quantified_kind) = match &*e { - Variable::Quantified { - quantified: _, - bounds, - } - | Variable::Unwrap(bounds) => ( - bounds.upper.first().cloned(), - self.get_current_bound(bounds.lower.clone()), - if let Variable::Quantified { quantified, .. } = &*e { - self.validate_bound_consistency(&bound, &bounds.upper, quantified.kind()) - } else { - Ok(()) - }, - if let Variable::Quantified { quantified, .. } = &*e { - Some(quantified.kind()) - } else { - None - }, - ), - _ => return Ok(()), - }; - drop(e); - drop(lock); - let res = res.and_then(|_| { - lower_bound.map_or(Ok(()), |lower_bound| is_subset(&lower_bound, &bound)) - }); - let new_bound = match (res.is_ok(), quantified_kind) { - (true, Some(QuantifiedKind::IntVar)) => Some( - self.get_new_bound( - first_bound, - // `validate_bound_consistency` accepted this bound, so the - // same IntVar normalization must succeed before storing it. - type_as_intvar_solution(&bound) - .expect("successful IntVar upper-bound check must normalize"), - true, - is_subset, - ), - ), - (true, _) => Some(self.get_new_bound(first_bound, bound, true, is_subset)), - (false, Some(QuantifiedKind::IntVar)) => None, - (false, _) => { - // TODO(https://github.com/facebook/pyrefly/issues/105): don't throw away the bound. - Some(NewBound::AddBound(Type::any_error())) - } - }; - let lock = self.variables.lock(); - if let Some(new_bound) = new_bound { - match &mut *lock.get_mut(v) { - Variable::Quantified { - quantified: _, - bounds, - } - | Variable::Unwrap(bounds) => self.add_bound(&mut bounds.upper, new_bound), - _ => {} - } - } - res + self.add_var_bound(v, bound, true, is_subset) } /// Get current bound from a set of bounds of an unfinished variable.