Update rubocop to 1.89.0 and rubocop-sorbet to 0.14.0 - #23452
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Homebrew鈥檚 vendored RuboCop to 1.89.0 (and json to 2.21.2) and applies the resulting style/autocorrect changes across the codebase, including removing now-unnecessary RuboCop suppressions and adjusting a call site to avoid an Enumerator#intersect? runtime error.
Changes:
- Bump vendored
rubocopto 1.89.0 andjsonto 2.21.2 (including lockfile + bundler setup paths). - Re-enable/lean on
Style/ArrayIntersectby updating affected call sites to useintersect?safely. - Remove obsolete
Layout/HashAlignmentsuppression comments and adjust new offenses from the RuboCop update; disableStyle/IfUnlessModifierdue to new behaviour.
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/vulns/identify.rb | Adjust indentation for multiline chained call. |
| Library/Homebrew/vendor/bundle/bundler/setup.rb | Update vendored load paths for json 2.21.2 and rubocop 1.89.0. |
| Library/Homebrew/test/install_steps_spec.rb | Switch from manual intersection+empty check to Array#intersect?. |
| Library/Homebrew/tap_auditor.rb | Remove redundant parentheses around hash literal passed to <<. |
| Library/Homebrew/sorbet/rbi/gems/rubocop@1.89.0.rbi | Update RuboCop RBI to match 1.89.0 API surface. |
| Library/Homebrew/Gemfile.lock | Bump json and rubocop versions and checksums. |
| Library/Homebrew/formula.rb | Remove now-dead Layout/HashAlignment suppression on delegate calls. |
| Library/Homebrew/formula_auditor.rb | Remove redundant parentheses around hash literals passed to <<. |
| Library/Homebrew/dev-cmd/tap-new.rb | Remove now-unnecessary duplicate assignment workaround for Ruby warnings. |
| Library/Homebrew/cask/artifact/abstract_uninstall.rb | Avoid calling intersect? on the Enumerator from each_filename by converting to an array. |
| Library/.rubocop.yml | Remove Style/ArrayIntersect disable; disable Style/IfUnlessModifier. |
馃挕 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Avoid a postfixed conditional being obscured by a long preceding statement. | ||
| Style/IfUnlessModifier: | ||
| Enabled: false |
Can you say more about this one? Some examples would be good. |
|
@MikeMcQuaid sure thing! unless quiet
opoo "Not upgrading #{cask.token}, the installed version is not below the minimum version #{version}"
endafter: opoo "Not upgrading #{cask.token}, the installed version is not below the minimum version #{version}" unless quietHappy to keep the cop if you prefer the correction (or want to use it selectively), but I found the corrections to excessively obscure a postfixed conditional. |
|
@dduugg yeh I agree not ideal on long lines. Worth filing an issue on this though I think about respecting the existing LineLength or at least allowing customisability? |
The `zap` stanza ended with a `\` followed by a blank line, so the continuation joined nothing. RuboCop 1.89.0 flags this as `Style/RedundantLineContinuation`, which fails `brew style homebrew/cask` on Homebrew/brew#23452. Claude-Session: https://claude.ai/code/session_01AdSQcU5MmY4VWiFdfrPeu2
Also updates rubocop-sorbet (0.13.2 -> 0.14.0) and json (2.21.1 -> 2.21.2), which rubocop depends on. Two suppressions can go away thanks to bug fixes in this release: - rubocop/rubocop#15438 and rubocop/rubocop#15442 stop `Style/ArrayIntersect` firing when the `include?` receiver in a block is not an array literal, so the cop is re-enabled. It still cannot guarantee the *outer* receiver is an `Array`, and in `cask/artifact/abstract_uninstall.rb` that receiver is the `Enumerator` from `Pathname#each_filename`, which has no `intersect?`. Convert with `to_a` there rather than taking the autocorrect verbatim. - rubocop/rubocop#15452 fixes the `Layout/HashAlignment` false positive on multi-line hash keys, so the two `delegate` disable comments in `formula.rb` are dead and removed. One suppression is added: rubocop/rubocop#15493 makes `Style/IfUnlessModifier` respect `Layout/LineLength`'s exemptions rather than its `Max`. Several of our exemption patterns are unescaped regexes (`"#{version."` matches `#{version}`, `#{version_text}` and `#{version_info[...]}`), so the cop demanded modifier form for statements that then ran to 179 characters. Disable the cop. Reported upstream as rubocop/rubocop#15531. rubocop-sorbet 0.14.0 widens `Sorbet/SetterReturnType` to cover setters taking splat and keyword arguments, so several `depends_on` and ENV setters switch to `.void`. Under `HOMEBREW_SORBET_RUNTIME` a `.void` method returns the sentinel `T::Private::Types::Void::VOID`, which breaks the two callers that consumed a setter's return value. Sorbet does not catch either statically: - `DependsOn#load` stored the result of `send(:"#{key}=", ...)` into the delegated hash, so the whole `depends_on` stanza became `VOID`. Read the value back through the matching reader instead. - `Superenv#cc=` assigned `super` to `HOMEBREW_CC`, raising `TypeError: no implicit conversion of Module into String`. Assign from `val`. Drop `extend/ENV/shared.rbi`, which existed to widen `[]=` to accept `Pathname` and `PATH`. Without it `[]=` resolves to Sorbet's own `ENVClass#[]=`, which takes `T.nilable(String)`, so the ENV assignments convert explicitly with `to_s`/`&.to_s`. Both `Pathname` and `PATH` define `to_str`, so Ruby was already converting implicitly and behaviour is unchanged. The remaining changes are new offenses in this release: - `Lint/UselessAssignment` (rubocop/rubocop#12269) catches the `root_url = root_url =` workaround in `dev-cmd/tap-new.rb`. The Ruby `assigned but unused variable` warning it silenced no longer fires on our vendored Ruby, so drop it. - `Style/RedundantParentheses` (rubocop/rubocop#15472) and `Layout/MultilineMethodCallIndentation` autocorrections. The redundant `T.let` on `GitHub::API::ERRORS` is also dropped. Claude-Session: https://claude.ai/code/session_01AdSQcU5MmY4VWiFdfrPeu2
|
I think I鈥檓 coming around to the idea of disabling the new sorbet cop that requires setters to be void. It removes some nice ergonomics without much upside. |
| Enabled: true | ||
|
|
||
| # Removes some ergonomic use of setter return values, without much upside. | ||
| Sorbet/SetterReturnType: |
There was a problem hiding this comment.
Sorry to be annoying: honestly I think I prefer the enabled version above? I think it's a bit more explicit and intuitive.
Updates
rubocopfrom 1.88.2 to 1.89.0 andrubocop-sorbetfrom 0.13.2 to 0.14.0, along withjson(2.21.1 to 2.21.2) which rubocop depends on.rubocop 1.89.0
Two suppressions can go away thanks to bug fixes in this release:
none? { |s| str.include?(s) }into a TypeError聽rubocop/rubocop#15438 and Fix an error for Style/ArrayIntersect cop聽rubocop/rubocop#15442 stopStyle/ArrayIntersectfiring when theinclude?receiver in a block is not an array literal, so the cop is re-enabled. It still cannot guarantee the outer receiver is anArray, and incask/artifact/abstract_uninstall.rbthat receiver is theEnumeratorreturned byPathname#each_filename, which has nointersect?. That call site converts withto_arather than taking the autocorrect verbatim, which would have raisedNoMethodError.Layout/HashAlignmentfalse positive on multi-line hash keys, so the twodelegatedisable comments informula.rbare now dead code and removed.One suppression is added. rubocop/rubocop#15493 makes
Style/IfUnlessModifierrespectLayout/LineLength's exemptions instead of itsMaxwhen deciding whether the modifier form fits. Several of our exemption patterns are unescaped regexes ("#{version."matches#{version},#{version_text}and#{version_info[...]}), so the cop demanded modifier form for statements that then ran to 179 characters. There is no config toggle for the new behaviour, so the cop is disabled. See the discussion below for the four affected sites.This is reported upstream as rubocop/rubocop#15531, with a minimal reproduction, asking that the modifier cops keep checking the joined line against
Maxor that the new behaviour be made configurable.Style/WhileUntilModifierandStyle/GuardClauseare affected by the same change, but neither produces offenses here today, so both stay enabled.The rest are new offenses in this release:
Lint/UselessAssignment(Lint/UselessAssignment vs postfix if聽rubocop/rubocop#12269) catches theroot_url = root_url =workaround indev-cmd/tap-new.rb. The Rubyassigned but unused variablewarning it silenced no longer fires on our vendored Ruby 4.0.6, so it is dropped.Style/RedundantParentheses(VerifyStyle/RedundantParenthesescorrections by reparsing聽rubocop/rubocop#15472) informula_auditor.rbandtap_auditor.rb, plus aLayout/MultilineMethodCallIndentationcorrection invulns/identify.rb.rubocop-sorbet 0.14.0
This widens
Sorbet/SetterReturnTypeto cover setters taking splat and keyword arguments, so severaldepends_onand ENV setters switch to.void.Under
HOMEBREW_SORBET_RUNTIMEa.voidmethod returns the sentinelT::Private::Types::Void::VOIDrather than the value the body produced. That breaks the two callers that consumed a setter's return value, neither of which Sorbet catches statically.Cask::DSL::DependsOn#loadstored the setter result into the delegated hash backing the whole stanza:so every
depends_onvalue becameVOID, producingRequired (1): T::Private::Types::Void::VOID (cask)andundefined method 'keys' for module T::Private::Types::Void::VOID. It now calls the setter for its side effect and reads the value back through the matching reader, which returns the same object the setter previously returned.Superenv#cc=consumed the return value ofsuper:which raises once
SharedEnvExtension#cc=becomes.void:It now calls
superand assigns fromval.A
.voidreturn can only leak throughsend,superor an implicit return, since assignment syntax always evaluates to the right-hand side. Those two were the only consumers.ENV.clangand friends now returnVOIDbecause theCOMPILERS.eachblocks end onsend(:cxx=, ...), but nothing reads those returns, and there are noextend/osoverrides ofcc=,cxx=or[]=.Dropping
extend/ENV/shared.rbi[]=is a setter, so the widenedSorbet/SetterReturnTypeapplies to the shim too and its return type had to become.void:The whole point of the generic was to return the assigned value's type, which a
voidreturn makes pointless. That left a hand-written shim whose only remaining job was widening the parameter to acceptPathnameandPATH, so I found it best to drop it and be explicit at the affected call sites instead.Without the shim,
[]=resolves to Sorbet's ownENVClass#[]=, which takesT.nilable(String). The ENV assignments now convert explicitly:&.to_swhere the value is nilable (T.nilable(PATH)),.to_swhere it is not (PATH,Pathname, or a|| ""that already removed nil).This is not a behaviour change. Both
PathnameandPATHdefineto_str, soENV["X"] = some_pathwas already converting implicitly, and the explicit calls just make it visible to the typechecker.Also drops the redundant
T.letonGitHub::API::ERRORS.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Claude Code (Opus 5) ran
brew vendor-gems --update,brew style --fixandbrew typecheck --update, then drafted this description. I directed the handling of each cop change, including the decision to disableStyle/IfUnlessModifierrather than accept its autocorrections. Several autocorrections were rejected after checking them against the vendored Ruby rather than trustingbrew typecheck, which passes clean on all of them:Pathname#each_filename.respond_to?(:intersect?)returnsfalse, and bothVOIDbugs above were reproduced underHOMEBREW_SORBET_RUNTIME. TheDependsOn#loadfix was checked red/green againstbrew tests --only=cask/info. The obsoletetap-new.rbworkaround was confirmed by reproducing the original pattern underruby -w.brew lgtm --onlineand the fullbrew testssuite pass locally.