rubocop: Avoid duplicate arch-specific versions (and more) in casks#21724
rubocop: Avoid duplicate arch-specific versions (and more) in casks#21724
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the RuboCop::Cop::Cask::OnSystemConditionals cop to autocorrect casks that redundantly nest arch-specific blocks when the version is identical (and to improve/extend existing sha256-nesting simplifications), reducing manual maintainer edits in homebrew-cask.
Changes:
- Add a new simplification pass to collapse
on_intel/on_armblocks when both contain identicalversion(and consolidatesha256appropriately). - Tighten
sha256-nesting autocorrect by requiring the arch blocks to be siblings and removing whole lines on correction. - Extend specs and Sorbet RBI signatures to cover the new pattern matching.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Library/Homebrew/rubocops/cask/on_system_conditionals.rb | Adds simplify_arch_version_stanzas, adjusts sha256 simplification, and introduces an AST search for version+sha pairs. |
| Library/Homebrew/rubocops/cask/on_system_conditionals.rbi | Adds Sorbet signature for version_and_sha256_on_arch_stanzas. |
| Library/Homebrew/test/rubocops/cask/on_system_conditionals_spec.rb | Updates offense messaging expectations and adds new test cases for identical arch-version scenarios (including nested on_os). |
Files not reviewed (1)
- Library/Homebrew/rubocops/cask/on_system_conditionals.rbi: Language not supported
Comments suppressed due to low confidence (1)
Library/Homebrew/rubocops/cask/on_system_conditionals.rb:92
simplify_arch_version_stanzashas the same “last one wins” behavior assimplify_sha256_stanzas: it overwritesnodes[:arm]/nodes[:intel]for every match and then processes at most one pair. This will miss additional offending pairs (e.g., multipleon_osblocks each containing anon_intel/on_armpair) and can also miss the only offending pair if a later non-matching block overwrites one side. Suggest grouping byblock_node.parent(or walking sibling blocks) and applying the simplification per parent scope.
def simplify_arch_version_stanzas
nodes = {}
version_and_sha256_on_arch_stanzas(cask_body) do |block_node, arch_method, version_value, sha256_value|
arch = arch_method.to_s.delete_prefix("on_").to_sym
nodes[arch] = { node: block_node, version_value:, sha256_value: }
end
return if !nodes.key?(:arm) || !nodes.key?(:intel)
return if nodes[:arm][:node].parent != nodes[:intel][:node].parent
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Looks good when 🟢 and you're happy, thanks @issyl0!
- Casks sometimes end up with `on_arm`/`on_intel` blocks whose version values are identical, such as if at some point they released an ARM-only version which had to have its own stanza. - Example fixes that could be done by this cop: https://github.com/Homebrew/homebrew-cask/pull/ 252975. Co-authored-by: Issy Long <me@issylong.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
- Add a sibling check so that `on_arm`/`on_intel` blocks inside `on_os` blocks (e.g. `on_sonoma :or_newer`) are still flagged when they contain identical or simplifiable stanzas, because nested blocks are harder to reason about. Co-authored-by: Issy Long <me@issylong.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
- Avoid a last-match-wins bug when multiple `on_arm`/`on_intel` pairs exist in different scopes. Grouping matches by parent lets us detect and autocorrect every eligible pair instead of only the final pair encountered. - Add regression tests for multiple eligible `on_arch` pairs across sibling `on_os` blocks and for a later lone `on_arch` block after a correctable pair. Co-authored-by: Issy Long <me@issylong.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
- Autocorrecting previously replaced or removed whole `on_arch` blocks could delete user-authored comments that are not represented in the AST. Keep reporting offenses, but avoid autocorrection when comments fall within the affected block ranges. Co-authored-by: Issy Long <me@issylong.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
3a4bf51 to
d433677
Compare
|
For d433677 I'm not sure if there actually are any, but it seemed legit as an idea to check for. |
brew lgtm(style, typechecking and tests) with your changes locally?AI was used to generate or assist with generating this PR. Please specify below how you used AI to help you, and what steps you have taken to manually verify the changes.
Out of curiosity I gave the task to Copilot with context from the PR with the manual fixes, plus the Slack thread that requested the RuboCop rule for this.
Immensely unsatisfying compared to doing it myself (@issyl0), to be honest, although much more efficient for getting started!
Supersedes rubocop: Avoid duplicate arch-specific versions in casks #21723 which got into a weird state.
on_arm/on_intelblocks whoseversionvalues are identical, such as if at some point they released an ARM-only version which had to have its own stanza.