Dedup add_lower_bound/add_upper_bound into add_var_bound - #4408
Open
nitishagar wants to merge 1 commit into
Open
Dedup add_lower_bound/add_upper_bound into add_var_bound#4408nitishagar wants to merge 1 commit into
nitishagar wants to merge 1 commit into
Conversation
`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 facebook#4292
Contributor
|
This pull request has been imported. If you are a Meta employee, you can view this in D114527565. (Because this pull request was imported automatically, there will not be any future comments.) |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
add_lower_boundandadd_upper_boundwere 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 singleis_upperbool — whichBoundsfield is the bound-side vs the opposite-side, theis_subsetargument order, and theis_uppervalue threaded throughget_new_bound.This extracts a private
add_var_bound(v, bound, is_upper, is_subset)holding the shared body, and reduces the two public functions to one-line delegations (false/true). This mirrors the establishedget_new_bound(..., is_upper: bool, ...)idiom already in this file, keeping the change idiomatic rather than introducing a one-use enum.Behavior preservation
The refactor is behavior-preserving: the helper is a pure relocation of the existing
add_lower_boundstatements with every divergence resolved by anis_upperbranch that reproduces the originaladd_upper_boundtext. Concretely preserved:is_subsetcallback can safely recurse intois_subset_eq, re-lock to write) — holding the lock acrossis_subsetwould deadlock.type_as_intvar_solution(&bound).expect(...)).(false, IntVar) => Nonedrop and the// TODO(issues/105)any-error fallback.The full
cargo test -p pyrefly --libsuite (7462 tests) passes unchanged.Fixes #4292
AI-generated: This PR was prepared by an AI agent (ZCode). The code has been reviewed and tested; the diff is a pure refactor with no intended behavior change, disclosed per the repository's AI-contribution guidelines.