Enable types in Formula files #15057

dduugg · 2023-03-25T15:50:51Z

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?

Enables typing in files relating to Formula. (After this change there are ~26 non-test files with typing disabled remaining)

Library/Homebrew/extend/object.rbi

Library/Homebrew/formulary.rb

reitermarkus · 2023-03-25T23:53:07Z

Library/Homebrew/formulary.rb

-      uri = URI(url)
-      formula = File.basename(uri.path, ".rb")
-      super formula, HOMEBREW_CACHE_FORMULA/File.basename(uri.path)
+      uri_path = T.must(URI(url).path)


I think this should just be an ArgumentError instead. The path is only guaranteed to be present since we only pass specific URLs to this.

Could I trouble you to be more specific in how you would like this to look? I'm not following where you want to raise an ArgumentError. (The T.must will raise a TypeError here on a falsy path, which I think is the correct error type, but also not something worth splitting hairs over.)

Something like

raise ArgumentError, "URL has no path component" unless uri.path

We'll probably never hit this, but semantically an argument error makes more sense to me here.

sorbet (rightfully) doesn't assume successive invocations of the same function will return the same type. so this would actually need to be something like:

uri_path = uri.path raise ArgumentError, "URL has no path component" unless uri_path # code that uses non-nil uri_path

which is a lot of code just to return a specific error type, even if I agreed with it. (Which I'm still not sold on – if this is not a TypeError, I'm not sure what would be.)

Semantically, it's an argument error caused by invalid input (i.e. a URL without path component), which in turn leads to a type error if left unhandled.

Agreed with @reitermarkus here

Library/Homebrew/formulary.rb

reitermarkus · 2023-03-25T23:59:46Z

Library/Homebrew/formulary.rb

    end

    def load_file(flags:, ignore_errors:)
      if @from != :formula_installer
-        if %r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<formula_name>[\w+-.@]+).rb} =~ url
+        if (md = %r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}.match(url))


I think we usually use match instead of md or match_data. May make sense to check for other outliers to make this more consistent.

Also flip the order here.

I can switch the order. The md here is an exception for readability, as otherwise this needs to span multiple lines. The regexp should arguably be a const anyway, but I try to minimize the refactoring not directly related to enabling typing (which is also why I preserve order by default).

Let's please try to avoid using single or double letter abbreviations like this that make things harder to read. match or match_data` are preferable.

dduugg · 2023-03-26T00:40:08Z

PTAL @reitermarkus

Library/Homebrew/formula_installer.rb

Library/Homebrew/formulary.rb

Library/Homebrew/ignorable.rbi

MikeMcQuaid

Thanks, nice work so far!

MikeMcQuaid · 2023-03-27T13:40:26Z

Library/Homebrew/formula_auditor.rb

@@ -391,7 +391,7 @@ def audit_conflicts
                  "canonical name (#{conflicting_formula.name}) instead of #{conflict.name}"
        end

-        reverse_conflict_found = false
+        reverse_conflict_found = T.let(false, T::Boolean)


Why is this needed?

This is necessary to allow the type to widened from FalseClass to T.any(FalseClass, TrueClass) (ruby does not have a native notion of a Boolean type). This specific example is covered in the docs for the ensuing error. It's also touched on in the type annotations section.

Sorry for not elaborating on this PR, there are already ~22 uses of this boolean lvar pattern on master, so I thought it might already be understood.

No worries, don't expect you to preemptively elaborate.

This is one of the things I dislike about Sorbet, it feels weird that it's not more intuitive/permissive on Boolean usage like this.

Fair, although I would characterize this as a bug in Ruby. (Mats is entrenched his no-Boolean-class position). The second link above gives the rationale for why Sorbet doesn’t special-case true/false values.

MikeMcQuaid · 2023-03-27T13:40:34Z

Library/Homebrew/formula_auditor.rb

-      previous_version = nil
-      previous_version_scheme = nil
-      previous_revision = nil
+      previous_version = T.let(nil, T.nilable(Version))


Same deal here: why are these needed?

Same deal as above

@dduugg This isn't a boolean so would still like to try to understand this one?

nil will just be NilClass, T.nilable(Version) is basically T.any(NilClass, Version).

Sorbet will by default assign an lvar’s type as the type of the initial assignment, and use that to determine which methods are available and other types of analysis.

Attempting to change the type will thus result an error. This can be allowed, however, if the type is is widened to include all assignments with a T.let statement on initial assignment. Similarly, Ruby has FalseClass and TrueClass and true/falseusually require widening to T::Boolean on initial assignment.

Does that make sense? If this is getting too esoteric, I can pause on the typing PRs.

I can pause on the typing PRs.

No, all good, they are making the code better overall for sure!

MikeMcQuaid · 2023-03-27T13:40:46Z

Library/Homebrew/formula_cellar_checks.rb

@@ -407,7 +418,7 @@ def cpuid_instruction?(file, objdump = "objdump")
      end
    end

-    has_cpuid_instruction = false
+    has_cpuid_instruction = T.let(false, T::Boolean)


MikeMcQuaid · 2023-03-27T13:41:03Z

Library/Homebrew/formulary.rb

@@ -67,6 +67,7 @@ def self.clear_cache
  module PathnameWriteMkpath
    refine Pathname do
      def write(content, offset = nil, **open_args)
+        T.bind(self, Pathname)


What is this doing?

sorbet doesn't support refinements, so we need to explicitly tell sorbet that this def block is executed under the Pathname class. Otherwise, it won't be able to locate definitions for exist? or dirname within the block.

MikeMcQuaid · 2023-03-27T13:41:40Z

Library/Homebrew/formulary.rb

    end

    def load_file(flags:, ignore_errors:)
      if @from != :formula_installer
-        if %r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<formula_name>[\w+-.@]+).rb} =~ url
+        if (md = url.match(%r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}))


Suggested change

if (md = url.match(%r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}))

if (match = url.match(%r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}))

or

Suggested change

if (md = url.match(%r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}))

if (match_data = url.match(%r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}))

dduugg · 2023-03-27T16:43:44Z

PTAL @MikeMcQuaid

MikeMcQuaid · 2023-03-28T07:56:19Z

Library/Homebrew/formula_auditor.rb

@@ -391,7 +391,7 @@ def audit_conflicts
                  "canonical name (#{conflicting_formula.name}) instead of #{conflict.name}"
        end

-        reverse_conflict_found = false
+        reverse_conflict_found = T.let(false, T::Boolean)


No worries, don't expect you to preemptively elaborate.

This is one of the things I dislike about Sorbet, it feels weird that it's not more intuitive/permissive on Boolean usage like this.

MikeMcQuaid · 2023-03-28T07:56:52Z

Library/Homebrew/formula_auditor.rb

-      previous_version = nil
-      previous_version_scheme = nil
-      previous_revision = nil
+      previous_version = T.let(nil, T.nilable(Version))


@dduugg This isn't a boolean so would still like to try to understand this one?

MikeMcQuaid · 2023-03-28T07:57:01Z

Library/Homebrew/formulary.rb

@@ -67,6 +67,7 @@ def self.clear_cache
  module PathnameWriteMkpath
    refine Pathname do
      def write(content, offset = nil, **open_args)
+        T.bind(self, Pathname)


MikeMcQuaid · 2023-03-28T07:57:27Z

Thanks again @dduugg! One question for post-merge that wasn't blocking.

Enable types in Formula files

7720485

dduugg commented Mar 25, 2023

View reviewed changes

Library/Homebrew/extend/object.rbi Outdated Show resolved Hide resolved

Library/Homebrew/formulary.rb Outdated Show resolved Hide resolved

reitermarkus reviewed Mar 25, 2023

View reviewed changes

Library/Homebrew/formulary.rb Show resolved Hide resolved

reitermarkus reviewed Mar 25, 2023

View reviewed changes

Library/Homebrew/formulary.rb Outdated Show resolved Hide resolved

reitermarkus reviewed Mar 25, 2023

View reviewed changes

reitermarkus reviewed Mar 26, 2023

View reviewed changes

Library/Homebrew/formula_installer.rb Outdated Show resolved Hide resolved

reitermarkus reviewed Mar 26, 2023

View reviewed changes

Library/Homebrew/formulary.rb Outdated Show resolved Hide resolved

dduugg mentioned this pull request Mar 26, 2023

Enable typing in Homebrew::Cleanup #15059

Merged

7 tasks

reitermarkus reviewed Mar 26, 2023

View reviewed changes

Library/Homebrew/ignorable.rbi Outdated Show resolved Hide resolved

Code review changes

1c411f6

dduugg force-pushed the enable-types branch from b44436c to 1c411f6 Compare March 26, 2023 15:58

reitermarkus approved these changes Mar 26, 2023

View reviewed changes

MikeMcQuaid reviewed Mar 27, 2023

View reviewed changes

Rename lvar

ccfc1be

MikeMcQuaid approved these changes Mar 28, 2023

View reviewed changes

MikeMcQuaid merged commit b97db86 into Homebrew:master Mar 28, 2023
22 checks passed

github-actions bot added the outdated PR was locked due to age label Apr 28, 2023

github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable types in Formula files #15057

Enable types in Formula files #15057

dduugg commented Mar 25, 2023 •

edited

reitermarkus Mar 25, 2023

dduugg Mar 26, 2023

reitermarkus Mar 26, 2023

reitermarkus Mar 26, 2023

dduugg Mar 26, 2023 •

edited

reitermarkus Mar 26, 2023

MikeMcQuaid Mar 27, 2023

reitermarkus Mar 25, 2023

reitermarkus Mar 26, 2023

dduugg Mar 26, 2023

MikeMcQuaid Mar 27, 2023

dduugg commented Mar 26, 2023

MikeMcQuaid left a comment

MikeMcQuaid Mar 27, 2023

dduugg Mar 27, 2023

MikeMcQuaid Mar 28, 2023

dduugg Mar 28, 2023

MikeMcQuaid Mar 27, 2023

dduugg Mar 27, 2023

MikeMcQuaid Mar 28, 2023

reitermarkus Mar 28, 2023

dduugg Mar 28, 2023

MikeMcQuaid Mar 28, 2023

MikeMcQuaid Mar 27, 2023

dduugg Mar 27, 2023

MikeMcQuaid Mar 27, 2023

dduugg Mar 27, 2023

MikeMcQuaid Mar 28, 2023

MikeMcQuaid Mar 27, 2023

dduugg commented Mar 27, 2023

MikeMcQuaid Mar 28, 2023

MikeMcQuaid Mar 28, 2023

MikeMcQuaid Mar 28, 2023

MikeMcQuaid commented Mar 28, 2023

	if (md = url.match(%r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}))
	if (match = url.match(%r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}))

	if (md = url.match(%r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}))
	if (match_data = url.match(%r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}))

Enable types in Formula files #15057

Enable types in Formula files #15057

Conversation

dduugg commented Mar 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg Mar 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg commented Mar 26, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Mar 28, 2023

dduugg commented Mar 25, 2023 •

edited

dduugg Mar 26, 2023 •

edited