Merge pull request #19475 from Homebrew/typed-system-command

refactor: Enable strict typing in download_strategy

Library/Homebrew/cask/installer.rb

@@ -374,7 +374,7 @@ on_request: true)
 
       graph = ::Utils::TopologicalHash.graph_package_dependencies(@cask)
 
-      raise CaskSelfReferencingDependencyError, @cask.token if graph[@cask].include?(@cask)
+      raise CaskSelfReferencingDependencyError, @cask.token if graph.fetch(@cask).include?(@cask)
 
       ::Utils::TopologicalHash.graph_package_dependencies(primary_container.dependencies, graph)
 

Library/Homebrew/cask/url.rb

@@ -1,4 +1,4 @@
-# typed: true # rubocop:todo Sorbet/StrictSigil
+# typed: strict
 # frozen_string_literal: true
 
 require "source_location"
@@ -7,9 +7,25 @@ require "utils/curl"
 module Cask
   # Class corresponding to the `url` stanza.
   class URL < SimpleDelegator
+    BlockReturn = T.type_alias do
+      T.any(URI::Generic, String, [T.any(URI::Generic, String), T::Hash[Symbol, T.untyped]])
+    end
+
     class DSL
-      attr_reader :uri, :tag, :branch, :revisions, :revision,
+      sig { returns(T.any(URI::Generic, String)) }
-                  :trust_cert, :cookies, :header, :data, :only_path
+      attr_reader :uri
+
+      sig { returns(T.nilable(T::Array[String])) }
+      attr_reader :revisions
+
+      sig { returns(T.nilable(T::Boolean)) }
+      attr_reader :trust_cert
+
+      sig { returns(T.nilable(T::Hash[String, String])) }
+      attr_reader :cookies, :data
+
+      sig { returns(T.nilable(T.any(String, T::Array[String]))) }
+      attr_reader :header
 
       sig { returns(T.nilable(T.any(URI::Generic, String))) }
       attr_reader :referer
@@ -20,11 +36,11 @@ module Cask
       sig { returns(T.nilable(T.any(Symbol, String))) }
       attr_reader :user_agent
 
-      sig { returns(T.any(T::Class[T.anything], Symbol, NilClass)) }
+      sig { returns(T.any(T::Class[AbstractDownloadStrategy], Symbol, NilClass)) }
       attr_reader :using
 
       sig { returns(T.nilable(String)) }
-      attr_reader :verified
+      attr_reader :tag, :branch, :revision, :only_path, :verified
 
       extend Forwardable
       def_delegators :uri, :path, :scheme, :to_s
@@ -35,7 +51,7 @@ module Cask
           # @api public
           verified:   T.nilable(String),
           # @api public
-          using:      T.any(Class, Symbol, NilClass),
+          using:      T.any(T::Class[AbstractDownloadStrategy], Symbol, NilClass),
           # @api public
           tag:        T.nilable(String),
           # @api public
@@ -59,41 +75,29 @@ module Cask
         ).void
       }
       def initialize(
-        uri,
+        uri, verified: nil, using: nil, tag: nil, branch: nil, revisions: nil, revision: nil, trust_cert: nil,
-        verified: nil,
+        cookies: nil, referer: nil, header: nil, user_agent: nil, data: nil, only_path: nil
-        using: nil,
-        tag: nil,
-        branch: nil,
-        revisions: nil,
-        revision: nil,
-        trust_cert: nil,
-        cookies: nil,
-        referer: nil,
-        header: nil,
-        user_agent: nil,
-        data: nil,
-        only_path: nil
       )
-        @uri = URI(uri)
+        @uri = T.let(URI(uri), T.any(URI::Generic, String))
 
         header = Array(header) unless header.nil?
 
         specs = {}
-        specs[:verified]   = @verified   = verified
+        specs[:verified]   = @verified   = T.let(verified, T.nilable(String))
-        specs[:using]      = @using      = using
+        specs[:using]      = @using      = T.let(using, T.any(T::Class[AbstractDownloadStrategy], Symbol, NilClass))
-        specs[:tag]        = @tag        = tag
+        specs[:tag]        = @tag        = T.let(tag, T.nilable(String))
-        specs[:branch]     = @branch     = branch
+        specs[:branch]     = @branch     = T.let(branch, T.nilable(String))
-        specs[:revisions]  = @revisions  = revisions
+        specs[:revisions]  = @revisions  = T.let(revisions, T.nilable(T::Array[String]))
-        specs[:revision]   = @revision   = revision
+        specs[:revision]   = @revision   = T.let(revision, T.nilable(String))
-        specs[:trust_cert] = @trust_cert = trust_cert
+        specs[:trust_cert] = @trust_cert = T.let(trust_cert, T.nilable(T::Boolean))
-        specs[:cookies]    = @cookies    = cookies
+        specs[:cookies]    = @cookies    = T.let(cookies, T.nilable(T::Hash[String, String]))
-        specs[:referer]    = @referer    = referer
+        specs[:referer]    = @referer    = T.let(referer, T.nilable(T.any(URI::Generic, String)))
-        specs[:headers]    = @header     = header
+        specs[:headers]    = @header     = T.let(header, T.nilable(T.any(String, T::Array[String])))
-        specs[:user_agent] = @user_agent = user_agent || :default
+        specs[:user_agent] = @user_agent = T.let(user_agent || :default, T.nilable(T.any(Symbol, String)))
-        specs[:data]       = @data       = data
+        specs[:data]       = @data       = T.let(data, T.nilable(T::Hash[String, String]))
-        specs[:only_path]  = @only_path  = only_path
+        specs[:only_path]  = @only_path  = T.let(only_path, T.nilable(String))
 
-        @specs = specs.compact
+        @specs = T.let(specs.compact, T::Hash[Symbol, T.untyped])
       end
     end
 
@@ -115,9 +119,10 @@ module Cask
         sig { returns(URI::Generic) }
         attr_accessor :url
 
+        sig { params(str: String, url: URI::Generic).void }
         def initialize(str, url)
           super(str)
-          @url = url
+          @url = T.let(url, URI::Generic)
         end
       end
 
@@ -125,19 +130,18 @@ module Cask
         params(
           uri:   T.nilable(T.any(URI::Generic, String)),
           dsl:   ::Cask::DSL,
-          block: T.proc.params(arg0: T.all(String, PageWithURL))
+          block: T.proc.params(arg0: T.all(String, PageWithURL)).returns(BlockReturn),
-                       .returns(T.any(T.any(URI::Generic, String), [T.any(URI::Generic, String), Hash])),
         ).void
       }
       def initialize(uri, dsl:, &block)
-        @uri = uri
+        @uri = T.let(uri, T.nilable(T.any(URI::Generic, String)))
-        @dsl = dsl
+        @dsl = T.let(dsl, ::Cask::DSL)
-        @block = block
+        @block = T.let(block, T.proc.params(arg0: T.all(String, PageWithURL)).returns(BlockReturn))
 
         odeprecated "cask `url do` blocks" if @block
       end
 
-      sig { returns(T.any(T.any(URI::Generic, String), [T.any(URI::Generic, String), Hash])) }
+      sig { returns(BlockReturn) }
       def call
         if @uri
           result = ::Utils::Curl.curl_output("--fail", "--silent", "--location", @uri.to_s)
@@ -158,9 +162,8 @@ module Cask
       sig {
         params(
           uri:   T.any(URI::Generic, String),
-          block: T.proc.params(arg0: T.all(String, PageWithURL))
+          block: T.proc.params(arg0: T.all(String, PageWithURL)).returns(BlockReturn),
-                       .returns(T.any(T.any(URI::Generic, String), [T.any(URI::Generic, String), Hash])),
+        ).returns(BlockReturn)
-        ).returns(T.any(T.any(URI::Generic, String), [T.any(URI::Generic, String), Hash]))
       }
       def url(uri, &block)
         self.class.new(uri, dsl: @dsl, &block).call
@@ -169,6 +172,10 @@ module Cask
       # This allows calling DSL methods from inside a `url` block.
       #
       # @api public
+      sig {
+        override.params(method: Symbol, args: T.untyped, block: T.nilable(T.proc.returns(T.untyped)))
+                .returns(T.anything)
+      }
       def method_missing(method, *args, &block)
         if @dsl.respond_to?(method)
           @dsl.public_send(method, *args, &block)
@@ -177,8 +184,9 @@ module Cask
         end
       end
 
-      def respond_to_missing?(method, include_all)
+      sig { override.params(method_name: T.any(Symbol, String), include_private: T::Boolean).returns(T::Boolean) }
-        @dsl.respond_to?(method, include_all) || super
+      def respond_to_missing?(method_name, include_private = false)
+        @dsl.respond_to?(method_name, include_private) || super
       end
     end
 
@@ -186,7 +194,7 @@ module Cask
       params(
         uri:             T.nilable(T.any(URI::Generic, String)),
         verified:        T.nilable(String),
-        using:           T.any(Class, Symbol, NilClass),
+        using:           T.any(T::Class[AbstractDownloadStrategy], Symbol, NilClass),
         tag:             T.nilable(String),
         branch:          T.nilable(String),
         revisions:       T.nilable(T::Array[String]),
@@ -200,30 +208,13 @@ module Cask
         only_path:       T.nilable(String),
         caller_location: Thread::Backtrace::Location,
         dsl:             T.nilable(::Cask::DSL),
-        block:           T.nilable(
+        block:           T.nilable(T.proc.params(arg0: T.all(String, BlockDSL::PageWithURL)).returns(BlockReturn)),
-          T.proc.params(arg0: T.all(String, BlockDSL::PageWithURL))
-                .returns(T.any(T.any(URI::Generic, String), [T.any(URI::Generic, String), Hash])),
-        ),
       ).void
     }
     def initialize(
-      uri = nil,
+      uri = nil, verified: nil, using: nil, tag: nil, branch: nil, revisions: nil, revision: nil, trust_cert: nil,
-      verified: nil,
+      cookies: nil, referer: nil, header: nil, user_agent: nil, data: nil, only_path: nil,
-      using: nil,
+      caller_location: caller_locations.fetch(0), dsl: nil, &block
-      tag: nil,
-      branch: nil,
-      revisions: nil,
-      revision: nil,
-      trust_cert: nil,
-      cookies: nil,
-      referer: nil,
-      header: nil,
-      user_agent: nil,
-      data: nil,
-      only_path: nil,
-      caller_location: T.must(caller_locations).fetch(0),
-      dsl: nil,
-      &block
     )
       super(
         if block
@@ -233,27 +224,13 @@ module Cask
             DSL.new(uri2, **options)
           end
         else
-          DSL.new(
+          DSL.new(T.must(uri), verified:, using:, tag:, branch:, revisions:, revision:, trust_cert:, cookies:,
-            T.must(uri),
+          referer:, header:, user_agent:, data:, only_path:)
-            verified:,
-            using:,
-            tag:,
-            branch:,
-            revisions:,
-            revision:,
-            trust_cert:,
-            cookies:,
-            referer:,
-            header:,
-            user_agent:,
-            data:,
-            only_path:,
-          )
         end
       )
 
-      @from_block = !block.nil?
+      @from_block = T.let(!block.nil?, T::Boolean)
-      @caller_location = caller_location
+      @caller_location = T.let(caller_location, Thread::Backtrace::Location)
     end
 
     sig { returns(Homebrew::SourceLocation) }
@@ -284,10 +261,10 @@ module Cask
     def raw_url_line
       return @raw_url_line if defined?(@raw_url_line)
 
-      @raw_url_line = Pathname(T.must(@caller_location.path))
+      @raw_url_line = T.let(Pathname(T.must(@caller_location.path))
                       .each_line
                       .drop(@caller_location.lineno - 1)
-                      .first
+                      .first, T.nilable(String))
     end
   end
 end

Library/Homebrew/sorbet/rbi/dsl/cask/url.rbi

@@ -56,7 +56,7 @@ class Cask::URL
   sig { returns(T.nilable(T.any(::String, ::Symbol))) }
   def user_agent; end
 
-  sig { returns(T.nilable(T.any(::Symbol, T::Class[T.anything]))) }
+  sig { returns(T.nilable(T.any(::Symbol, T::Class[::AbstractDownloadStrategy]))) }
   def using; end
 
   sig { returns(T.nilable(::String)) }

Library/Homebrew/test/cask/tab_spec.rb

@@ -124,8 +124,10 @@ RSpec.describe Cask::Tab, :cask do
     specify "with all types of dependencies" do
       cask = Cask::CaskLoader.load("with-depends-on-everything")
 
-      unar = instance_double(Formula, full_name: "unar", version: "1.2", revision: 0, pkg_version: "1.2",
+      unar = Class.new(Formula) do
-                             deps: [], requirements: [])
+        url "my_url"
+        version "1.2"
+      end.new("unar", Pathname.new(__FILE__).expand_path, :stable)
       expect(Formulary).to receive(:factory).with("unar").and_return(unar)
 
       expected_hash = {

Library/Homebrew/test/download_strategies/abstract_spec.rb

@@ -3,7 +3,7 @@
 require "download_strategy"
 
 RSpec.describe AbstractDownloadStrategy do
-  subject(:strategy) { described_class.new(url, name, version, **specs) }
+  subject(:strategy) { Class.new(described_class).new(url, name, version, **specs) }
 
   let(:specs) { {} }
   let(:name) { "foo" }

Library/Homebrew/test/download_strategies/detector_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe DownloadStrategyDetector do
   describe "::detect" do
     subject(:strategy_detector) { described_class.detect(url, strategy) }
 
-    let(:url) { Object.new }
+    let(:url) { "invalidurl" }
     let(:strategy) { nil }
 
     context "when given Git URL" do

Library/Homebrew/test/download_strategies/subversion_spec.rb

@@ -17,6 +17,8 @@ RSpec.describe SubversionDownloadStrategy do
       it "adds the appropriate svn args" do
         expect(strategy).to receive(:system_command!)
           .with("svn", hash_including(args: array_including("--trust-server-cert", "--non-interactive")))
+          .and_return(instance_double(SystemCommand::Result))
+
         strategy.fetch
       end
     end
@@ -27,6 +29,7 @@ RSpec.describe SubversionDownloadStrategy do
       it "adds svn arguments for :revision" do
         expect(strategy).to receive(:system_command!)
           .with("svn", hash_including(args: array_including_cons("-r", "10")))
+          .and_return(instance_double(SystemCommand::Result))
 
         strategy.fetch
       end

Library/Homebrew/test/download_strategies/vcs_spec.rb

@@ -9,7 +9,7 @@ RSpec.describe VCSDownloadStrategy do
   describe "#cached_location" do
     it "returns the path of the cached resource" do
       allow_any_instance_of(described_class).to receive(:cache_tag).and_return("foo")
-      downloader = described_class.new(url, "baz", version)
+      downloader = Class.new(described_class).new(url, "baz", version)
       expect(downloader.cached_location).to eq(HOMEBREW_CACHE/"baz--foo")
     end
   end

Library/Homebrew/url.rb

@@ -1,16 +1,20 @@
-# typed: true # rubocop:todo Sorbet/StrictSigil
+# typed: strict
 # frozen_string_literal: true
 
 require "version"
 
 class URL
-  attr_reader :specs, :using
+  sig { returns(T::Hash[Symbol, T.untyped]) }
+  attr_reader :specs
+
+  sig { returns(T.any(NilClass, Symbol, T::Class[AbstractDownloadStrategy])) }
+  attr_reader :using
 
   sig { params(url: String, specs: T::Hash[Symbol, T.untyped]).void }
   def initialize(url, specs = {})
-    @url = url.freeze
+    @url = T.let(url.freeze, String)
-    @specs = specs.dup
+    @specs = T.let(specs.dup, T::Hash[Symbol, T.untyped])
-    @using = @specs.delete(:using)
+    @using = T.let(@specs.delete(:using), T.any(NilClass, Symbol, T::Class[AbstractDownloadStrategy]))
     @specs.freeze
   end
 
@@ -19,13 +23,14 @@ class URL
     @url
   end
 
-  sig { returns(T.class_of(AbstractDownloadStrategy)) }
+  sig { returns(T::Class[AbstractDownloadStrategy]) }
   def download_strategy
-    @download_strategy ||= DownloadStrategyDetector.detect(@url, @using)
+    @download_strategy ||=
+      T.let(DownloadStrategyDetector.detect(@url, @using), T.nilable(T::Class[AbstractDownloadStrategy]))
   end
 
   sig { returns(Version) }
   def version
-    @version ||= Version.detect(@url, **@specs)
+    @version ||= T.let(Version.detect(@url, **@specs), T.nilable(Version))
   end
 end

Library/Homebrew/utils/curl.rb

@@ -211,7 +211,7 @@ module Utils
     end
 
     sig {
-      params(
+      overridable.params(
         args:         String,
         print_stdout: T.any(T::Boolean, Symbol),
         options:      T.untyped,
@@ -264,7 +264,7 @@ module Utils
       curl(*args, **options)
     end
 
-    sig { params(args: String, options: T.untyped).returns(SystemCommand::Result) }
+    sig { overridable.params(args: String, options: T.untyped).returns(SystemCommand::Result) }
     def curl_output(*args, **options)
       curl_with_workarounds(*args, print_stderr: false, show_output: true, **options)
     end

Library/Homebrew/utils/github/artifacts.rb

@@ -31,14 +31,14 @@ class GitHubArtifactDownloadStrategy < AbstractFileDownloadStrategy
     @token = T.let(token, String)
   end
 
-  sig { params(timeout: T.nilable(Integer)).void }
+  sig { override.params(timeout: T.any(Float, Integer, NilClass)).void }
   def fetch(timeout: nil)
     ohai "Downloading #{url}"
     if cached_location.exist?
       puts "Already downloaded: #{cached_location}"
     else
       begin
-        Utils::Curl.curl("--location", "--create-dirs", "--output", temporary_path, url,
+        Utils::Curl.curl("--location", "--create-dirs", "--output", temporary_path.to_s, url,
                          "--header", "Authorization: token #{@token}",
                          secrets: [@token],
                          timeout:)
@@ -46,7 +46,7 @@ class GitHubArtifactDownloadStrategy < AbstractFileDownloadStrategy
         raise CurlDownloadStrategyError, url
       end
       cached_location.dirname.mkpath
-      temporary_path.rename(cached_location)
+      temporary_path.rename(cached_location.to_s)
     end
 
     symlink_location.dirname.mkpath

Library/Homebrew/utils/topological_hash.rb

@@ -1,4 +1,4 @@
-# typed: true # rubocop:todo Sorbet/StrictSigil
+# typed: strict
 # frozen_string_literal: true
 
 require "tsort"
@@ -6,11 +6,18 @@ require "tsort"
 module Utils
   # Topologically sortable hash map.
   class TopologicalHash < Hash
+    extend T::Generic
     include TSort
 
+    CaskOrFormula = T.type_alias { T.any(Cask::Cask, Formula) }
+
+    K = type_member { { fixed: CaskOrFormula } }
+    V = type_member { { fixed: T::Array[CaskOrFormula] } }
+    Elem = type_member(:out) { { fixed: [CaskOrFormula, T::Array[CaskOrFormula]] } }
+
     sig {
       params(
-        packages:    T.any(Cask::Cask, Formula, T::Array[T.any(Cask::Cask, Formula)]),
+        packages:    T.any(CaskOrFormula, T::Array[CaskOrFormula]),
         accumulator: TopologicalHash,
       ).returns(TopologicalHash)
     }
@@ -20,14 +27,15 @@ module Utils
       packages.each do |cask_or_formula|
         next if accumulator.key?(cask_or_formula)
 
-        if cask_or_formula.is_a?(Cask::Cask)
+        case cask_or_formula
+        when Cask::Cask
           formula_deps = cask_or_formula.depends_on
                                         .formula
                                         .map { |f| Formula[f] }
           cask_deps = cask_or_formula.depends_on
                                      .cask
                                      .map { |c| Cask::CaskLoader.load(c, config: nil) }
-        else
+        when Formula
           formula_deps = cask_or_formula.deps
                                         .reject(&:build?)
                                         .reject(&:test?)
@@ -35,6 +43,8 @@ module Utils
           cask_deps = cask_or_formula.requirements
                                      .filter_map(&:cask)
                                      .map { |c| Cask::CaskLoader.load(c, config: nil) }
+        else
+          T.absurd(cask_or_formula)
         end
 
         accumulator[cask_or_formula] = formula_deps + cask_deps
@@ -48,10 +58,12 @@ module Utils
 
     private
 
+    sig { params(block: T.proc.params(arg0: K).void).void }
     def tsort_each_node(&block)
       each_key(&block)
     end
 
+    sig { params(node: K, block: T.proc.params(arg0: CaskOrFormula).void).returns(V) }
     def tsort_each_child(node, &block)
       fetch(node).each(&block)
     end