Return nil from get_make_var when the variable is missing#22854
Return nil from get_make_var when the variable is missing#22854thiagogenez wants to merge 1 commit into
Conversation
|
Thanks for your pull request. This has been closed because it appears to use an incomplete or outdated pull request template. Please edit this pull request to fill in the current pull request template. This workflow will reopen this pull request automatically once the template is complete. Do not open a new pull request for this. |
There was a problem hiding this comment.
Pull request overview
This PR restores the historical (and documented) behavior of the public formula DSL method StringInreplaceExtension#get_make_var by returning nil when a requested Makefile variable is absent, rather than raising due to a T.must introduced for strict typing.
Changes:
- Update
get_make_varto returnT.nilable(String)and removeT.mustso missing variables returnnil. - Clarify the method’s docstring to reflect the nil-on-missing behavior.
- Add an RSpec example covering the missing-variable case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
Library/Homebrew/utils/string_inreplace_extension.rb |
Restores get_make_var’s nil-on-missing behavior by making the return type nilable and removing T.must. |
Library/Homebrew/test/utils/string_inreplace_extension_spec.rb |
Adds test coverage to ensure get_make_var returns nil when the requested variable is not present. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I guess this makes sense given it did use to return |
StringInreplaceExtension#get_make_varis declaredsig { ... .returns(String) },but it can return
nil: its body isinreplace_string[regex, 1], andString#[]returnsnilwhen the pattern doesn't match — i.e. when therequested make variable is absent. The docstring even says "Finds the specified
variable", which implies it may not find one.
A 2023 strict-typing change (c7369b7) wrapped the result in
T.musttosatisfy the
returns(String)signature. This makes the signature lie: callersand static analysis treat the result as never-
nil, and under runtime sigchecking the method raises
TypeError: Passed nil into T.mustinstead ofreturning
nil. This is a public formula DSL method (@api public), so itsdocumented "may not find it" behaviour should be expressed in the type.
The fix declares
returns(T.nilable(String))and drops theT.must, restoringthe honest return type. This also follows
AGENTS.md's guidance to avoidT.mustin favour of APIs that can returnnil. No internal callers depend ona non-nil return (
brew typecheckstays green), and a new test covers themissing-variable case.
Reproduce (with the new test added, against unpatched code):
In normal
brewruntime sig checks are disabled, so the call currently returnsnilsilently — but the declared type still wrongly promises a non-nilString, which is what this PR fixes.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?This PR was written with assistance from Claude Code (Claude Opus 4.8). I
reviewed all generated changes, verified the issue with a red/green test (under
runtime sig checking the old code raises
TypeError: Passed nil into T.must;the fix returns
nil), and ranbrew lgtmlocally (all green). Seehttps://docs.brew.sh/Responsible-AI-Usage.