Remove OpenStruct from CLI::Args

Library/.rubocop.yml

@@ -408,10 +408,6 @@ Style/NumericLiterals:
 Style/OpenStructUse:
   Exclude:
     - "Taps/**/*"
-    # TODO: This is a pre-existing violation and should be corrected
-    #   to define methods so that call sites can be type-checked.
-    - "Homebrew/cli/args.rb"
-    - "Homebrew/cli/args.rbi"
 
 Style/OptionalBooleanParameter:
   AllowedMethods:

Library/Homebrew/cask/config.rb

@@ -42,6 +42,8 @@ module Cask
 
     sig { params(args: Homebrew::CLI::Args).returns(T.attached_class) }
     def self.from_args(args)
+      # FIXME: T.unsafe is a workaround for methods that are only defined when `cask_options`
+      # is invoked on the parser. (These could be captured by a DSL compiler instead.)
       args = T.unsafe(args)
       new(explicit: {
         appdir:               args.appdir,

Library/Homebrew/cli/args.rb

@@ -1,26 +1,23 @@
 # typed: strict
 # frozen_string_literal: true
 
-require "ostruct"
-
 module Homebrew
   module CLI
-    class Args < OpenStruct
+    class Args
       # Represents a processed option. The array elements are:
       #   0: short option name (e.g. "-d")
       #   1: long option name (e.g. "--debug")
       #   2: option description (e.g. "Print debugging information")
       #   3: whether the option is hidden
       OptionsType = T.type_alias { T::Array[[String, T.nilable(String), String, T::Boolean]] }
+
       sig { returns(T::Array[String]) }
-      attr_reader :options_only, :flags_only
+      attr_reader :options_only, :flags_only, :remaining
 
       sig { void }
       def initialize
         require "cli/named_args"
 
-        super
-
         @cli_args = T.let(nil, T.nilable(T::Array[String]))
         @processed_options = T.let([], OptionsType)
         @options_only = T.let([], T::Array[String])
@@ -30,30 +27,46 @@ module Homebrew
 
         # Can set these because they will be overwritten by freeze_named_args!
         # (whereas other values below will only be overwritten if passed).
-        self[:named] = NamedArgs.new(parent: self)
+        @named = T.let(NamedArgs.new(parent: self), T.nilable(NamedArgs))
-        self[:remaining] = []
+        @remaining = T.let([], T::Array[String])
       end
 
       sig { params(remaining_args: T::Array[T.any(T::Array[String], String)]).void }
-      def freeze_remaining_args!(remaining_args)
+      def freeze_remaining_args!(remaining_args) = @remaining.replace(remaining_args).freeze
-        self[:remaining] = remaining_args.freeze
-      end
 
       sig { params(named_args: T::Array[String], cask_options: T::Boolean, without_api: T::Boolean).void }
       def freeze_named_args!(named_args, cask_options:, without_api:)
         options = {}
-        options[:force_bottle] = true if self[:force_bottle?]
+        options[:force_bottle] = true if force_bottle?
-        options[:override_spec] = :head if self[:HEAD?]
+        options[:override_spec] = :head if self.HEAD?
         options[:flags] = flags_only unless flags_only.empty?
-        self[:named] = NamedArgs.new(
+        @named = T.let(
-          *named_args.freeze,
+          NamedArgs.new(*named_args.freeze, parent: self, cask_options:, without_api:, **options),
-          parent:       self,
+          T.nilable(NamedArgs),
-          cask_options:,
-          without_api:,
-          **options,
         )
       end
 
+      sig { returns(T.nilable(String)) }
+      def arch = nil
+
+      sig { returns(T::Boolean) }
+      def build_bottle? = false
+
+      sig { returns(T::Boolean) }
+      def build_from_source? = false
+
+      sig { returns(T::Boolean) }
+      def force_bottle? = false
+
+      sig { returns(T::Boolean) }
+      def HEAD? = false
+
+      sig { returns(T::Boolean) }
+      def include_test? = false
+
+      sig { returns(T.nilable(String)) }
+      def os = nil
+
       sig { params(processed_options: OptionsType).void }
       def freeze_processed_options!(processed_options)
         # Reset cache values reliant on processed_options
@@ -69,7 +82,7 @@ module Homebrew
       sig { returns(NamedArgs) }
       def named
         require "formula"
-        self[:named]
+        T.must(@named)
       end
 
       sig { returns(T::Boolean) }
@@ -77,7 +90,7 @@ module Homebrew
 
       sig { returns(T::Array[String]) }
       def build_from_source_formulae
-        if build_from_source? || self[:HEAD?] || self[:build_bottle?]
+        if build_from_source? || self.HEAD? || build_bottle?
           named.to_formulae.map(&:full_name)
         else
           []
@@ -109,9 +122,9 @@ module Homebrew
 
       sig { returns(T.nilable(Symbol)) }
       def only_formula_or_cask
-        if formula? && !cask?
+        if invoke_if_respond_to(:formula?) && !invoke_if_respond_to(:cask?)
           :formula
-        elsif cask? && !formula?
+        elsif invoke_if_respond_to(:cask?) && !invoke_if_respond_to(:formula?)
           :cask
         end
       end
@@ -174,24 +187,6 @@ module Homebrew
           end
         end.freeze
       end
-
-      sig { params(method_name: Symbol, _include_private: T::Boolean).returns(T::Boolean) }
-      def respond_to_missing?(method_name, _include_private = false)
-        @table.key?(method_name)
-      end
-
-      sig { params(method_name: Symbol, args: T.untyped).returns(T.untyped) }
-      def method_missing(method_name, *args)
-        return_value = super
-
-        # Once we are frozen, verify any arg method calls are already defined in the table.
-        # The default OpenStruct behaviour is to return nil for anything unknown.
-        if frozen? && args.empty? && !@table.key?(method_name)
-          raise NoMethodError, "CLI arg for `#{method_name}` is not declared for this command"
-        end
-
-        return_value
-      end
     end
   end
 end

Library/Homebrew/cli/args.rbi

@@ -14,30 +14,6 @@ class Homebrew::CLI::Args
   sig { returns(T::Boolean) }
   def quiet?; end
 
-  sig { returns(T::Array[String]) }
-  def remaining; end
-
   sig { returns(T::Boolean) }
   def verbose?; end
-
-  # FIXME: The methods below are not defined by Args, but are valid because Args inherits from OpenStruct
-  # We should instead be using type guards to check if the method is defined on the object before calling it
-
-  sig { returns(T.nilable(String)) }
-  def arch; end
-
-  sig { returns(T::Boolean) }
-  def build_from_source?; end
-
-  sig { returns(T::Boolean) }
-  def cask?; end
-
-  sig { returns(T::Boolean) }
-  def formula?; end
-
-  sig { returns(T::Boolean) }
-  def include_test?; end
-
-  sig { returns(T.nilable(String)) }
-  def os; end
 end

Library/Homebrew/cli/parser.rb

@@ -252,7 +252,7 @@ module Homebrew
         description = option_description(description, name, hidden:)
         process_option(name, description, type: :comma_array, hidden:)
         @parser.on(name, OptionParser::REQUIRED_ARGUMENT, Array, *wrap_option_desc(description)) do |list|
-          @args[option_to_name(name)] = list
+          @args.define_singleton_method(option_to_name(name)) { list }
         end
       end
 
@@ -277,7 +277,7 @@ module Homebrew
           # This odisabled should stick around indefinitely.
           odisabled "the `#{names.first}` flag", replacement unless replacement.nil?
           names.each do |name|
-            @args[option_to_name(name)] = option_value
+            @args.define_singleton_method(option_to_name(name)) { option_value }
           end
         end
 
@@ -558,24 +558,27 @@ module Homebrew
       def set_switch(*names, value:, from:)
         names.each do |name|
           @switch_sources[option_to_name(name)] = from
-          @args["#{option_to_name(name)}?"] = value
+          @args.define_singleton_method(:"#{option_to_name(name)}?") { value }
         end
       end
 
       sig { params(args: String).void }
       def disable_switch(*args)
         args.each do |name|
-          @args["#{option_to_name(name)}?"] = if name.start_with?("--[no-]")
+          result = if name.start_with?("--[no-]")
             nil
           else
             false
           end
+          @args.define_singleton_method(:"#{option_to_name(name)}?") { result }
         end
       end
 
       sig { params(name: String).returns(T::Boolean) }
       def option_passed?(name)
-        !!(@args[name.to_sym] || @args[:"#{name}?"])
+        [name.to_sym, :"#{name}?"].any? do |method|
+          @args.public_send(method) if @args.respond_to?(method)
+        end
       end
 
       sig { params(desc: String).returns(T::Array[String]) }
@@ -676,7 +679,7 @@ module Homebrew
           disable_switch(*args)
         else
           args.each do |name|
-            @args[option_to_name(name)] = nil
+            @args.define_singleton_method(option_to_name(name)) { nil }
           end
         end
 

Library/Homebrew/cmd/search.rb

@@ -108,7 +108,7 @@ module Homebrew
 
       sig { returns(T::Boolean) }
       def search_package_manager
-        package_manager = PACKAGE_MANAGERS.find { |name,| args[:"#{name}?"] }
+        package_manager = PACKAGE_MANAGERS.find { |name,| args.public_send(:"#{name}?") }
         return false if package_manager.nil?
 
         _, url = package_manager

Library/Homebrew/extend/os/linux/cli/parser.rb

@@ -11,13 +11,13 @@ module OS
 
         sig { void }
         def set_default_options
-          args["formula?"] = true if args.respond_to?(:formula?)
+          args.define_singleton_method(:formula?) { true } if args.respond_to?(:formula?)
         end
 
         sig { void }
         def validate_options
           return unless args.respond_to?(:cask?)
-          return unless args.cask?
+          return unless T.unsafe(args).cask?
 
           # NOTE: We don't raise an error here because we don't want
           #       to print the help page or a stack trace.

Library/Homebrew/sorbet/rbi/dsl/homebrew/cmd/tap_cmd.rbi

@@ -20,7 +20,7 @@ class Homebrew::Cmd::TapCmd::Args < Homebrew::CLI::Args
   sig { returns(T::Boolean) }
   def force?; end
 
-  sig { returns(T.nilable(String)) }
+  sig { returns(T::Boolean) }
   def force_auto_update?; end
 
   sig { returns(T::Boolean) }

Library/Homebrew/sorbet/rbi/dsl/homebrew/dev_cmd/audit.rbi

@@ -71,7 +71,7 @@ class Homebrew::DevCmd::Audit::Args < Homebrew::CLI::Args
   sig { returns(T.nilable(String)) }
   def os; end
 
-  sig { returns(T.nilable(String)) }
+  sig { returns(T::Boolean) }
   def signing?; end
 
   sig { returns(T::Boolean) }

Library/Homebrew/sorbet/tapioca/compilers/args.rb

@@ -39,11 +39,8 @@ module Tapioca
         end
       end
 
-      sig { params(parser: Homebrew::CLI::Parser).returns(T::Hash[Symbol, T.untyped]) }
+      sig { params(parser: Homebrew::CLI::Parser).returns(T::Array[Symbol]) }
-      def args_table(parser)
+      def args_table(parser) = parser.args.methods(false)
-        # we exclude non-args from the table, such as :named and :remaining
-        parser.instance_variable_get(:@args).instance_variable_get(:@table).except(:named, :remaining)
-      end
 
       sig { params(parser: Homebrew::CLI::Parser).returns(T::Array[Symbol]) }
       def comma_arrays(parser)
@@ -51,11 +48,11 @@ module Tapioca
               .filter_map { |k, v| parser.option_to_name(k).to_sym if v == :comma_array }
       end
 
-      sig { params(method_name: Symbol, value: T.untyped, comma_array_methods: T::Array[Symbol]).returns(String) }
+      sig { params(method_name: Symbol, comma_array_methods: T::Array[Symbol]).returns(String) }
-      def get_return_type(method_name, value, comma_array_methods)
+      def get_return_type(method_name, comma_array_methods)
         if comma_array_methods.include?(method_name)
           "T.nilable(T::Array[String])"
-        elsif [true, false].include?(value)
+        elsif method_name.end_with?("?")
           "T::Boolean"
         else
           "T.nilable(String)"
@@ -67,11 +64,11 @@ module Tapioca
       sig { params(klass: RBI::Scope, parser: Homebrew::CLI::Parser).void }
       def create_args_methods(klass, parser)
         comma_array_methods = comma_arrays(parser)
-        args_table(parser).each do |method_name, value|
+        args_table(parser).each do |method_name|
           method_name_str = method_name.to_s
           next if GLOBAL_OPTIONS.include?(method_name_str)
 
-          return_type = get_return_type(method_name, value, comma_array_methods)
+          return_type = get_return_type(method_name, comma_array_methods)
           klass.create_method(method_name_str, return_type:)
         end
       end

Library/Homebrew/test/abstract_command_spec.rb

@@ -22,7 +22,7 @@ RSpec.describe Homebrew::AbstractCommand do
       end
 
       it "allows access to args" do
-        expect(TestCat.new(["--bar", "baz"]).args[:bar]).to eq("baz")
+        expect(TestCat.new(["--bar", "baz"]).args.bar).to eq("baz")
       end
 
       it "raises on invalid args" do

Library/Homebrew/test/cask/upgrade_spec.rb

@@ -19,7 +19,11 @@ RSpec.describe Cask::Upgrade, :cask do
   let(:renamed_app) { Cask::CaskLoader.load("renamed-app") }
   let(:renamed_app_old_path) { renamed_app.config.appdir.join("OldApp.app") }
   let(:renamed_app_new_path) { renamed_app.config.appdir.join("NewApp.app") }
-  let(:args) { Homebrew::CLI::Args.new }
+  let(:args) do
+    parser = Homebrew::CLI::Parser.new(Homebrew::Cmd::Brew)
+    parser.cask_options
+    parser.args
+  end
 
   before do
     installed.each do |cask|

Library/Homebrew/test/sorbet/tapioca/compilers/args_spec.rb

@@ -18,36 +18,19 @@ RSpec.describe Tapioca::Compilers::Args do
 
   describe "#args_table" do
     it "returns a mapping of list args to default values" do
-      expect(compiler.args_table(list_parser).keys).to contain_exactly(
+      expect(compiler.args_table(list_parser)).to contain_exactly(
-        :"1?", :built_from_source?, :cask?, :casks?, :d?, :debug?,
+        :"1?", :built_from_source?, :cask?, :casks?, :d?, :debug?, :formula?, :formulae?, :full_name?, :h?, :help?,
-        :formula?, :formulae?, :full_name?, :h?, :help?,
+        :installed_as_dependency?, :installed_on_request?, :l?, :multiple?, :pinned?, :poured_from_bottle?, :q?,
-        :installed_as_dependency?, :installed_on_request?, :l?,
+        :quiet?, :r?, :t?, :v?, :verbose?, :versions?
-        :multiple?, :pinned?, :poured_from_bottle?, :q?, :quiet?,
-        :r?, :t?, :v?, :verbose?, :versions?
       )
     end
 
     it "returns a mapping of update-python-resources args to default values" do
-      expect(compiler.args_table(update_python_resources_parser)).to eq({
+      expect(compiler.args_table(update_python_resources_parser)).to contain_exactly(
-        d?:                        false,
+        :d?, :debug?, :exclude_packages, :extra_packages, :h?, :help?, :ignore_non_pypi_packages?,
-        debug?:                    false,
+        :install_dependencies?, :p?, :package_name, :print_only?, :q?, :quiet?, :s?, :silent?, :v?, :verbose?,
-        exclude_packages:          nil,
+        :version
-        extra_packages:            nil,
+      )
-        h?:                        false,
-        help?:                     false,
-        ignore_non_pypi_packages?: false,
-        install_dependencies?:     false,
-        p?:                        false,
-        package_name:              nil,
-        print_only?:               false,
-        q?:                        false,
-        quiet?:                    false,
-        s?:                        false,
-        silent?:                   false,
-        v?:                        false,
-        verbose?:                  false,
-        version:                   nil,
-      })
     end
   end
 
@@ -65,15 +48,15 @@ RSpec.describe Tapioca::Compilers::Args do
     let(:comma_arrays) { compiler.comma_arrays(update_python_resources_parser) }
 
     it "returns the correct type for switches" do
-      expect(compiler.get_return_type(:silent?, false, comma_arrays)).to eq("T::Boolean")
+      expect(compiler.get_return_type(:silent?, comma_arrays)).to eq("T::Boolean")
     end
 
     it "returns the correct type for flags" do
-      expect(compiler.get_return_type(:package_name, nil, comma_arrays)).to eq("T.nilable(String)")
+      expect(compiler.get_return_type(:package_name, comma_arrays)).to eq("T.nilable(String)")
     end
 
     it "returns the correct type for comma_arrays" do
-      expect(compiler.get_return_type(:extra_packages, nil, comma_arrays)).to eq("T.nilable(T::Array[String])")
+      expect(compiler.get_return_type(:extra_packages, comma_arrays)).to eq("T.nilable(T::Array[String])")
     end
   end
 end