Merge pull request #18847 from Homebrew/no-ostruct

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 @@ def self.defaults
 
     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,41 @@ def initialize
 
         # 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)
-        self[:remaining] = []
+        @named = T.let(NamedArgs.new(parent: self), T.nilable(NamedArgs))
+        @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)
-        self[:remaining] = remaining_args.freeze
-      end
+      def freeze_remaining_args!(remaining_args) = @remaining.replace(remaining_args).freeze
 
       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[:override_spec] = :head if self[:HEAD?]
-        options[:flags] = flags_only unless flags_only.empty?
-        self[:named] = NamedArgs.new(
-          *named_args.freeze,
-          parent:       self,
-          cask_options:,
-          without_api:,
-          **options,
+        @named = T.let(
+          NamedArgs.new(
+            *named_args.freeze,
+            cask_options:,
+            flags:         flags_only,
+            force_bottle:  @table[:force_bottle?] || false,
+            override_spec: @table[:HEAD?] ? :head : nil,
+            parent:        self,
+            without_api:,
+          ),
+          T.nilable(NamedArgs),
         )
       end
 
+      sig { params(name: Symbol, value: T.untyped).void }
+      def set_arg(name, value)
+        @table[name] = value
+      end
+
+      sig { override.params(_blk: T.nilable(T.proc.params(x: T.untyped).void)).returns(T.untyped) }
+      def tap(&_blk)
+        return super if block_given? # Object#tap
+
+        @table[:tap]
+      end
+
       sig { params(processed_options: OptionsType).void }
       def freeze_processed_options!(processed_options)
         # Reset cache values reliant on processed_options
@@ -69,15 +77,15 @@ def freeze_processed_options!(processed_options)
       sig { returns(NamedArgs) }
       def named
         require "formula"
-        self[:named]
+        T.must(@named)
       end
 
       sig { returns(T::Boolean) }
       def no_named? = named.empty?
 
       sig { returns(T::Array[String]) }
       def build_from_source_formulae
-        if build_from_source? || self[:HEAD?] || self[:build_bottle?]
+        if @table[:build_from_source?] || @table[:HEAD?] || @table[:build_bottle?]
           named.to_formulae.map(&:full_name)
         else
           []
@@ -86,7 +94,7 @@ def build_from_source_formulae
 
       sig { returns(T::Array[String]) }
       def include_test_formulae
-        if include_test?
+        if @table[:include_test?]
           named.to_formulae.map(&:full_name)
         else
           []
@@ -109,9 +117,9 @@ def context
 
       sig { returns(T.nilable(Symbol)) }
       def only_formula_or_cask
-        if formula? && !cask?
+        if @table[:formula?] && !@table[:cask?]
           :formula
-        elsif cask? && !formula?
+        elsif @table[:cask?] && !@table[:formula?]
           :cask
         end
       end
@@ -120,7 +128,7 @@ def only_formula_or_cask
       def os_arch_combinations
         skip_invalid_combinations = false
 
-        oses = case (os_sym = os&.to_sym)
+        oses = case (os_sym = @table[:os]&.to_sym)
         when nil
           [SimulateSystem.current_os]
         when :all
@@ -131,7 +139,7 @@ def os_arch_combinations
           [os_sym]
         end
 
-        arches = case (arch_sym = arch&.to_sym)
+        arches = case (arch_sym = @table[:arch]&.to_sym)
         when nil
           [SimulateSystem.current_arch]
         when :all
@@ -174,24 +182,6 @@ def cli_args
           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 @@ def comma_array(name, description: nil, hidden: false)
         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
+          set_args_method(option_to_name(name).to_sym, list)
         end
       end
 
@@ -277,7 +277,7 @@ def flag(*names, description: nil, replacement: nil, depends_on: nil, hidden: fa
           # 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
+            set_args_method(option_to_name(name).to_sym, option_value)
           end
         end
 
@@ -286,6 +286,17 @@ def flag(*names, description: nil, replacement: nil, depends_on: nil, hidden: fa
         end
       end
 
+      sig { params(name: Symbol, value: T.untyped).void }
+      def set_args_method(name, value)
+        @args.set_arg(name, value)
+        return if @args.respond_to?(name)
+
+        @args.define_singleton_method(name) do
+          # We cannot reference the ivar directly due to https://github.com/sorbet/sorbet/issues/8106
+          instance_variable_get(:@table).fetch(name)
+        end
+      end
+
       sig { params(options: String).returns(T::Array[T::Array[String]]) }
       def conflicts(*options)
         @conflicts << options.map { |option| option_to_name(option) }
@@ -558,24 +569,27 @@ def generate_banner
       def set_switch(*names, value:, from:)
         names.each do |name|
           @switch_sources[option_to_name(name)] = from
-          @args["#{option_to_name(name)}?"] = value
+          set_args_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
+          set_args_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 +690,7 @@ def process_option(*args, type:, hidden: false)
           disable_switch(*args)
         else
           args.each do |name|
-            @args[option_to_name(name)] = nil
+            set_args_method(option_to_name(name).to_sym, nil)
           end
         end
 

Library/Homebrew/cmd/search.rb

@@ -108,7 +108,7 @@ def print_regex_help
 
       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/dev-cmd/audit.rb

@@ -125,7 +125,7 @@ def run
 
         audit_formulae, audit_casks = Homebrew.with_no_api_env do # audit requires full Ruby source
           if args.tap
-            Tap.fetch(T.must(args.tap)).then do |tap|
+            Tap.fetch(args.tap).then do |tap|
               [
                 tap.formula_files.map { |path| Formulary.factory(path) },
                 tap.cask_files.map { |path| Cask::CaskLoader.load(path) },

Library/Homebrew/dev-cmd/bump.rb

@@ -86,7 +86,7 @@ def run
               Formulary.factory(qualified_name)
             end
           elsif args.tap
-            tap = Tap.fetch(T.must(args.tap))
+            tap = Tap.fetch(args.tap)
             raise UsageError, "`--tap` requires `--auto` for official taps." if tap.official?
 
             formulae = args.cask? ? [] : tap.formula_files.map { |path| Formulary.factory(path) }

Library/Homebrew/dev-cmd/livecheck.rb

@@ -60,7 +60,7 @@ def run
 
         formulae_and_casks_to_check = Homebrew.with_no_api_env do
           if args.tap
-            tap = Tap.fetch(T.must(args.tap))
+            tap = Tap.fetch(args.tap)
             formulae = args.cask? ? [] : tap.formula_files.map { |path| Formulary.factory(path) }
             casks = args.formula? ? [] : tap.cask_files.map { |path| Cask::CaskLoader.load(path) }
             formulae + casks

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

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