Skip to content

Return nil from get_make_var when the variable is missing#22854

Open
thiagogenez wants to merge 1 commit into
Homebrew:mainfrom
thiagogenez:get-make-var-nilable
Open

Return nil from get_make_var when the variable is missing#22854
thiagogenez wants to merge 1 commit into
Homebrew:mainfrom
thiagogenez:get-make-var-nilable

Conversation

@thiagogenez

@thiagogenez thiagogenez commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

StringInreplaceExtension#get_make_var is declared sig { ... .returns(String) },
but it can return nil: its body is inreplace_string[regex, 1], and
String#[] returns nil when the pattern doesn't match — i.e. when the
requested 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.must to
satisfy the returns(String) signature. This makes the signature lie: callers
and static analysis treat the result as never-nil, and under runtime sig
checking the method raises TypeError: Passed nil into T.must instead of
returning nil. This is a public formula DSL method (@api public), so its
documented "may not find it" behaviour should be expressed in the type.

The fix declares returns(T.nilable(String)) and drops the T.must, restoring
the honest return type. This also follows AGENTS.md's guidance to avoid
T.must in favour of APIs that can return nil. No internal callers depend on
a non-nil return (brew typecheck stays green), and a new test covers the
missing-variable case.

Reproduce (with the new test added, against unpatched code):

brew tests --only=utils/string_inreplace_extension
#   => TypeError: Passed `nil` into T.must  (string_inreplace_extension.rb:82)

In normal brew runtime sig checks are disabled, so the call currently returns
nil silently — but the declared type still wrongly promises a non-nil
String, which is what this PR fixes.


  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include Hyperfine benchmarks.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • AI was used to generate or assist with generating this PR.

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 ran brew lgtm locally (all green). See
https://docs.brew.sh/Responsible-AI-Usage.


Copilot AI review requested due to automatic review settings June 22, 2026 23:31
@github-actions

Copy link
Copy Markdown

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_var to return T.nilable(String) and remove T.must so missing variables return nil.
  • 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.

@github-actions github-actions Bot reopened this Jun 22, 2026
@Bo98

Bo98 commented Jun 23, 2026

Copy link
Copy Markdown
Member

I guess this makes sense given it did use to return nil, but it's been 3 years now where nobody has noticed and it could now be argued that raising a error is a feature rather than a bug (albeit it should be a clearer ArgumentError given this is public API). Do you have a real world use case where get_make_var("invalid") returning nil would be useful?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants