livecheck: replace T.unsafe with T.nilable

[cho-m]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

Follow up of thread at #16918 (comment)

[reitermarkus]

The point of having non-nilable arguments is to prevent explicitly passing nil as an argument, when in fact we want either a non-nilable argument or no argument at all.

Making the arguments nilable (rather than simply optional as is the case with T.unsafe) makes type-checking less meaningful.

[reitermarkus]

I guess for Integer we can use -1 instead of T.unsafe(nil), and for String we can use ''.

[MikeMcQuaid]

I guess for Integer we can use -1 instead of T.unsafe(nil), and for String we can use ''.

This seems better.

[apainintheneck]

I don't think it's worth making the type checking less strict just to clean up some ugly syntax.

[MikeMcQuaid]

I don't think it's worth making the type checking less strict

Using T.unsafe seems to be making type checking less strict by definition.

However, where I think we do agree is that having a method that looks like this smells a lot of "let's add types to an existing method rather than refactoring the method" (which is exactly what happened and why the types in this file are not very nicely handled).

[apainintheneck]

Using T.unsafe seems to be making type checking less strict by definition.

It's more strict for the arguments that get passed in but less strict from that point on for the default argument. I think any other String arguments will be evaluated normally though. Not entirely sure though.

However, where I think we do agree is that having a method that looks like this smells a lot of "let's add types to an existing method rather than refactoring the method" (which is exactly what happened and why the types in this file are not very nicely handled).

To be fair we can't really refactor these methods because they're public API at this point.

---

It looks like there are three other examples of this pattern in the code. I don't think the one in the source location file is needed at a glance.

Library/Homebrew/source_location.rb:    def initialize(line, column = T.unsafe(nil))
Library/Homebrew/tap.rb:  def self.fetch(user, repo = T.unsafe(nil))
Library/Homebrew/version.rb:  def respond_to?(method, include_all = T.unsafe(nil))

I'm fine with getting rid of it if we don't want to be as strict. As currently written it should cause errors when nil is passed as an argument to these methods. I don't think we should use the empty string and -1 argument hacks though. That's the worst option of the three.

It'd be nice to update the other ones too to keep it consistent.

Also, this pattern is used a lot in .rbi files but that doesn't mean it has to seep into the general codebase.

[MikeMcQuaid]

I'm fine with getting rid of it if we don't want to be as strict.

It'd be nice to update the other ones too to keep it consistent.

Also, this pattern is used a lot in .rbi files but that doesn't mean it has to seep into the general codebase.

Agreed with all of this 👍🏻

[cho-m]

Based on comments so far, there isn't a consensus on how to best handle. I see at least one disagreement with each approach.

I may just close this for now and revisit when we've gotten closer to fully typing all of brew and are left with narrowing/improving usage of escape hatches.

---

Perhaps along the boundary of a release where we can introduce some API changes (i.e. non-breaking for loading formulae/casks but potentially breaking for external usage of Livecheck) we can consider either splitting out setting/getting logic (I think getting logic is mainly internal) and/or having some indirection to handle the Livecheck DSL loading.

[MikeMcQuaid]

I may just close this for now and revisit when we've gotten closer to fully typing all of brew and are left with narrowing/improving usage of escape hatches.

I think that's reasonable, sorry for the wasted effort here ❤️.

I suspect eventually we'll want to move to Sorbet "strict" mode and that may define how we do this.