Replace throw/catch control flow with block return values#22015
Replace throw/catch control flow with block return values#22015MikeMcQuaid merged 4 commits intomainfrom
Conversation
Ruby's throw/catch is a non-local control flow mechanism (a form of goto). Replace all three uses with idiomatic Ruby patterns: - Dependency: remove catch(:action) from `action`, use the block's return value directly. Callers signal actions via `next` with named constants (PRUNE, SKIP, KEEP_BUT_PRUNE_RECURSIVE_DEPS). - Requirement: remove catch(:prune) from `prune?`, check the block's return value against the PRUNE constant. - Livecheck: extract nested loop into `find_version_update_revision` helper and use `return` for early exit. Remove the now-unnecessary `prune`, `skip`, and `keep_but_prune_recursive_deps` class methods and update all callers and type signatures.
Document why the trailing nil in language/python.rb is necessary (block sig requires T.nilable(Symbol), not Array) and why Dependency::PRUNE works in the polymorphic Requirement context in dependencies_helpers.rb (both constants are :prune).
There was a problem hiding this comment.
Pull request overview
This PR removes local throw/catch control flow from dependency/requirement expansion and replaces it with idiomatic block return values, centralizing action constants in Dependable and refactoring livecheck’s history scan to use early returns.
Changes:
- Replace
Dependency.prune/skip/keep_but_prune_recursive_depsandRequirement.prunethrow-based helpers with block return values (Dependable::PRUNE,Dependable::SKIP,Dependable::KEEP_BUT_PRUNE_RECURSIVE_DEPS). - Unify action constants in
Dependableand update Sorbet block signatures to reflect returned action values. - Refactor livecheck revision scanning into
find_version_update_revision.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/dependency.rb | Removes catch(:action) control flow; switches to block return action symbols. |
| Library/Homebrew/requirement.rb | Removes catch(:prune); prune? now checks returned action symbol and returns boolean. |
| Library/Homebrew/dependable.rb | Introduces shared action constants PRUNE/SKIP/KEEP_BUT_PRUNE_RECURSIVE_DEPS. |
| Library/Homebrew/formula.rb | Updates recursive dependency/requirement block type signatures; updates runtime dep expansion to return actions. |
| Library/Homebrew/cask_dependent.rb | Updates recursive dep/req block type signatures to return action symbols. |
| Library/Homebrew/dependencies_helpers.rb | Updates expansion filtering logic to return Dependable::* actions. |
| Library/Homebrew/formula_installer.rb | Updates requirement/dependency expansion blocks to return Dependable::* actions; adds casts for typed blocks. |
| Library/Homebrew/build.rb | Updates pruning logic in recursive dep/req expansion to return Dependable::* actions; adds cast. |
| Library/Homebrew/language/python.rb | Updates pruning logic to return Dependable::PRUNE and ensures nil return for type safety. |
| Library/Homebrew/livecheck/livecheck.rb | Extracts revision-search helper and replaces throw/catch early-exit with return. |
| Library/Homebrew/dev-cmd/test.rb | Updates pruning logic in recursive_dependencies block to return Dependable::PRUNE. |
| Library/Homebrew/dev-cmd/unbottled.rb | Updates pruning logic in Dependency.expand block to return Dependable::PRUNE. |
| Library/Homebrew/extend/os/linux/cmd/update-report.rb | Updates pruning logic in Dependency.expand block to return Dependable::PRUNE. |
| Library/Homebrew/test/dependency_expansion_spec.rb | Updates specs to use action constants returned from the block. |
| Library/Homebrew/test/formula_spec.rb | Updates requirement pruning in spec to return Dependable::PRUNE. |
| Library/Homebrew/test_bot/test_formulae.rb | Updates pruning in recursive_dependencies block to return Dependable::PRUNE. |
| Library/Homebrew/test_bot/formulae_dependents.rb | Updates pruning/skip logic in dependency expansion blocks to return Dependable::* actions. |
| Library/Homebrew/test_bot/formulae.rb | Updates dependency expansion filtering to return Dependable::* actions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9b67db0 to
d3eb2ee
Compare
PRUNE, SKIP, and KEEP_BUT_PRUNE_RECURSIVE_DEPS were defined separately on Dependency and Requirement with identical values. This coupling was fragile — if either constant changed, the polymorphic code in dependencies_helpers.rb would silently break. Move the constants to the shared Dependable module (included by both classes) so there is a single source of truth, and update all callers to reference Dependable:: directly.
f2418b7 to
d1ef9b6
Compare
Add explicit nil return in formula_installer.rb where the else branch returns an Array from <<, violating the block's declared T.nilable(Symbol) return type. Use T::Sig::WithoutRuntime for Formula#recursive_requirements (matching recursive_dependencies) since CaskDependent may not be initialized yet.
d1ef9b6 to
5833a05
Compare
|
(I'm not married to the content of this PR, but I do personally consider it a readability / comprehensibility net win. No offense taken if I'm alone on this one though.) |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Yeh, I agree, this is much nicer and more intuitive for non-Rubyists. Thanks!
Summary
Ruby's
throw/catchis a non-local control flow mechanism (a form of goto). In this codebase, it was never used for true multi-frame unwinding; everythrowoccurred exactly one frame above its matchingcatch. Block return values handle that case directly and more simply, so this PR replaces all three uses with idiomatic Ruby patterns:catch(:action)fromaction, use the block's return value directly. Callers signal actions vianextwith named constants (PRUNE,SKIP,KEEP_BUT_PRUNE_RECURSIVE_DEPS). Remove theprune,skip, andkeep_but_prune_recursive_depsclass methods.catch(:prune)fromprune?, check the block's return value against thePRUNEconstant. Remove thepruneclass method.find_version_update_revisionhelper and usereturnfor early exit instead ofthrow :version_update_revision_found.PRUNE,SKIP, andKEEP_BUT_PRUNE_RECURSIVE_DEPSinto the sharedDependablemodule (included by bothDependencyandRequirement) so there is a single source of truth. Previously these were defined separately on each class with identical values, which was fragile for the polymorphic code independencies_helpers.rb.Type signature improvements:
.void(incorrect, since the return value was communicated via side-channelthrow) to.returns(T.nilable(Symbol)).Formula#recursive_requirementsblock param changed fromT.untypedto a fully specified proc type. This was only possible because the block's return value is now explicit rather than communicated viathrow.CaskDependent#recursive_requirementsblock sig corrected from an incorrect.returns(CaskDependent::Requirement)to.returns(T.nilable(Symbol)).Dependency.actionreturn type changed fromT.nilable(T.any(Integer, Symbol))toT.nilable(Symbol), removing a staleIntegerthat was an artifact of thecatchmechanism.Requirement#prune?return type tightened fromT.nilable(T::Boolean)toT::Boolean.Test plan
brew lgtm --onlinepasses (style, typecheck, and tests)brew unbottledandbrew testcommands behave as beforebrew lgtm(style, typechecking and tests) with your changes locally?brew lgtm --online.