Merge pull request #17030 from Homebrew/strict-parser

Enable strict typing in `CLI::Parser`

Library/Homebrew/abstract_command.rb

@@ -26,6 +26,9 @@ module Homebrew
       sig { params(name: String).returns(T.nilable(T.class_of(AbstractCommand))) }
       def command(name) = subclasses.find { _1.command_name == name }
 
+      sig { returns(T::Boolean) }
+      def dev_cmd? = T.must(name).start_with?("Homebrew::DevCmd")
+
       sig { returns(CLI::Parser) }
       def parser = CLI::Parser.new(self, &@parser_block)
 

Library/Homebrew/brew.rb

@@ -136,9 +136,9 @@ begin
   end
 rescue UsageError => e
   require "help"
-  Homebrew::Help.help cmd, remaining_args: args.remaining, usage_error: e.message
+  Homebrew::Help.help cmd, remaining_args: args&.remaining, usage_error: e.message
 rescue SystemExit => e
-  onoe "Kernel.exit" if args.debug? && !e.success?
+  onoe "Kernel.exit" if args&.debug? && !e.success?
   $stderr.puts Utils::Backtrace.clean(e) if args&.debug? || ARGV.include?("--debug")
   raise
 rescue Interrupt
@@ -146,7 +146,7 @@ rescue Interrupt
   exit 130
 rescue BuildError => e
   Utils::Analytics.report_build_error(e)
-  e.dump(verbose: args&.verbose?)
+  e.dump(verbose: args&.verbose? || false)
 
   if OS.unsupported_configuration?
     $stderr.puts "#{Tty.bold}Do not report this issue: you are running in an unsupported configuration.#{Tty.reset}"

Library/Homebrew/cli/args.rbi

@@ -14,6 +14,9 @@ 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
 end

Library/Homebrew/cli/parser.rb

@@ -1,4 +1,4 @@
-# typed: true
+# typed: strict
 # frozen_string_literal: true
 
 require "abstract_command"
@@ -9,15 +9,31 @@ require "optparse"
 require "set"
 require "utils/tty"
 
-COMMAND_DESC_WIDTH = 80
-OPTION_DESC_WIDTH = 45
-HIDDEN_DESC_PLACEHOLDER = "@@HIDDEN@@"
-
 module Homebrew
   module CLI
     class Parser
-      attr_reader :processed_options, :hide_from_man_page, :named_args_type
+      # FIXME: Enable cop again when https://github.com/sorbet/sorbet/issues/3532 is fixed.
+      # rubocop:disable Style/MutableConstant
+      ArgType = T.type_alias { T.any(NilClass, Symbol, T::Array[String], T::Array[Symbol]) }
+      OptionsType = T.type_alias { T::Array[[String, T.nilable(String), T.nilable(String), String, T::Boolean]] }
+      # rubocop:enable Style/MutableConstant
+      HIDDEN_DESC_PLACEHOLDER = "@@HIDDEN@@"
+      SYMBOL_TO_USAGE_MAPPING = T.let({
+        text_or_regex: "<text>|`/`<regex>`/`",
+        url:           "<URL>",
+      }.freeze, T::Hash[Symbol, String])
+      private_constant :ArgType, :HIDDEN_DESC_PLACEHOLDER, :SYMBOL_TO_USAGE_MAPPING
 
+      sig { returns(OptionsType) }
+      attr_reader :processed_options
+
+      sig { returns(T::Boolean) }
+      attr_reader :hide_from_man_page
+
+      sig { returns(ArgType) }
+      attr_reader :named_args_type
+
+      sig { params(cmd_path: Pathname).returns(T.nilable(CLI::Parser)) }
       def self.from_cmd_path(cmd_path)
         cmd_args_method_name = Commands.args_method_name(cmd_path)
         cmd_name = cmd_args_method_name.to_s.delete_suffix("_args").tr("_", "-")
@@ -39,6 +55,7 @@ module Homebrew
         end
       end
 
+      sig { returns(T::Array[[Symbol, String, { description: String }]]) }
       def self.global_cask_options
         [
           [:flag, "--appdir=", {
@@ -119,49 +136,52 @@ module Homebrew
         ]
       end
 
+      sig { params(option: String).returns(String) }
+      def self.option_to_name(option)
+        option.sub(/\A--?(\[no-\])?/, "").tr("-", "_").delete("=")
+      end
+
       sig {
         params(cmd: T.nilable(T.class_of(Homebrew::AbstractCommand)), block: T.nilable(T.proc.bind(Parser).void)).void
       }
       def initialize(cmd = nil, &block)
-        @parser = OptionParser.new
-
-        @parser.summary_indent = " " * 2
-
+        @parser = T.let(OptionParser.new, OptionParser)
+        @parser.summary_indent = "  "
         # Disable default handling of `--version` switch.
         @parser.base.long.delete("version")
-
         # Disable default handling of `--help` switch.
         @parser.base.long.delete("help")
 
-        @args = Homebrew::CLI::Args.new
+        @args = T.let(Homebrew::CLI::Args.new, Homebrew::CLI::Args)
 
         if cmd
-          @command_name = cmd.command_name
-          @is_dev_cmd = cmd.name&.start_with?("Homebrew::DevCmd")
+          @command_name = T.let(cmd.command_name, String)
+          @is_dev_cmd = T.let(cmd.dev_cmd?, T::Boolean)
         else
           # FIXME: remove once commands are all subclasses of `AbstractCommand`:
           # Filter out Sorbet runtime type checking method calls.
-          cmd_location = T.must(caller_locations).select do |location|
+          cmd_location = caller_locations.select do |location|
             T.must(location.path).exclude?("/gems/sorbet-runtime-")
           end.fetch(1)
-          @command_name = T.must(cmd_location.label).chomp("_args").tr("_", "-")
-          @is_dev_cmd = T.must(cmd_location.absolute_path).start_with?(Commands::HOMEBREW_DEV_CMD_PATH)
+          @command_name = T.let(T.must(cmd_location.label).chomp("_args").tr("_", "-"), String)
+          @is_dev_cmd = T.let(T.must(cmd_location.absolute_path).start_with?(Commands::HOMEBREW_DEV_CMD_PATH),
+                              T::Boolean)
         end
 
-        @constraints = []
-        @conflicts = []
-        @switch_sources = {}
-        @processed_options = []
-        @non_global_processed_options = []
-        @named_args_type = nil
-        @max_named_args = nil
-        @min_named_args = nil
-        @named_args_without_api = false
-        @description = nil
-        @usage_banner = nil
-        @hide_from_man_page = false
-        @formula_options = false
-        @cask_options = false
+        @constraints = T.let([], T::Array[[String, String]])
+        @conflicts = T.let([], T::Array[T::Array[String]])
+        @switch_sources = T.let({}, T::Hash[String, Symbol])
+        @processed_options = T.let([], OptionsType)
+        @non_global_processed_options = T.let([], T::Array[[String, ArgType]])
+        @named_args_type = T.let(nil, T.nilable(ArgType))
+        @max_named_args = T.let(nil, T.nilable(Integer))
+        @min_named_args = T.let(nil, T.nilable(Integer))
+        @named_args_without_api = T.let(false, T::Boolean)
+        @description = T.let(nil, T.nilable(String))
+        @usage_banner = T.let(nil, T.nilable(String))
+        @hide_from_man_page = T.let(false, T::Boolean)
+        @formula_options = T.let(false, T::Boolean)
+        @cask_options = T.let(false, T::Boolean)
 
         self.class.global_options.each do |short, long, desc|
           switch short, long, description: desc, env: option_to_name(long), method: :on_tail
@@ -172,6 +192,10 @@ module Homebrew
         generate_banner
       end
 
+      sig {
+        params(names: String, description: T.nilable(String), replacement: T.untyped, env: T.untyped,
+               depends_on: T.nilable(String), method: Symbol, hidden: T::Boolean, disable: T::Boolean).void
+      }
       def switch(*names, description: nil, replacement: nil, env: nil, depends_on: nil,
                  method: :on, hidden: false, disable: false)
         global_switch = names.first.is_a?(Symbol)
@@ -201,32 +225,22 @@ module Homebrew
       end
       alias switch_option switch
 
-      def value_for_env(env)
-        return if env.blank?
-
-        method_name = :"#{env}?"
-        if Homebrew::EnvConfig.respond_to?(method_name)
-          Homebrew::EnvConfig.public_send(method_name)
-        else
-          ENV.fetch("HOMEBREW_#{env.upcase}", nil)
-        end
-      end
-      private :value_for_env
-
+      sig { params(text: T.nilable(String)).returns(T.nilable(String)) }
       def description(text = nil)
         return @description if text.blank?
 
         @description = text.chomp
       end
 
+      sig { params(text: String).void }
       def usage_banner(text)
         @usage_banner, @description = text.chomp.split("\n\n", 2)
       end
 
-      def usage_banner_text
-        @parser.banner
-      end
+      sig { returns(T.nilable(String)) }
+      def usage_banner_text = @parser.banner
 
+      sig { params(name: String, description: T.nilable(String), hidden: T::Boolean).void }
       def comma_array(name, description: nil, hidden: false)
         name = name.chomp "="
         description = option_description(description, name, hidden:)
@@ -236,6 +250,10 @@ module Homebrew
         end
       end
 
+      sig {
+        params(names: String, description: T.nilable(String), replacement: T.any(Symbol, String, NilClass),
+               depends_on: T.nilable(String), hidden: T::Boolean).void
+      }
       def flag(*names, description: nil, replacement: nil, depends_on: nil, hidden: false)
         required, flag_type = if names.any? { |name| name.end_with? "=" }
           [OptionParser::REQUIRED_ARGUMENT, :required_flag]
@@ -262,18 +280,15 @@ module Homebrew
         end
       end
 
+      sig { params(options: String).returns(T::Array[T::Array[String]]) }
       def conflicts(*options)
         @conflicts << options.map { |option| option_to_name(option) }
       end
 
-      def self.option_to_name(option)
-        option.sub(/\A--?(\[no-\])?/, "")
-              .tr("-", "_")
-              .delete("=")
-      end
-
+      sig { params(option: String).returns(String) }
       def option_to_name(option) = self.class.option_to_name(option)
 
+      sig { params(name: String).returns(String) }
       def name_to_option(name)
         if name.length == 1
           "-#{name}"
@@ -282,10 +297,12 @@ module Homebrew
         end
       end
 
+      sig { params(names: String).returns(T.nilable(String)) }
       def option_to_description(*names)
         names.map { |name| name.to_s.sub(/\A--?/, "").tr("-", " ") }.max
       end
 
+      sig { params(description: T.nilable(String), names: String, hidden: T::Boolean).returns(String) }
       def option_description(description, *names, hidden: false)
         return HIDDEN_DESC_PLACEHOLDER if hidden
         return description if description.present?
@@ -293,6 +310,10 @@ module Homebrew
         option_to_description(*names)
       end
 
+      sig {
+        params(argv: T::Array[String], ignore_invalid_options: T::Boolean)
+          .returns([T::Array[String], T::Array[String]])
+      }
       def parse_remaining(argv, ignore_invalid_options: false)
         i = 0
         remaining = []
@@ -328,9 +349,7 @@ module Homebrew
         [remaining, non_options]
       end
 
-      # @return [Args] The actual return type is `Args`, but since `Args` uses `method_missing` to handle options, the
-      #   `sig` annotates this as returning `T.untyped` to avoid spurious type errors.
-      sig { params(argv: T::Array[String], ignore_invalid_options: T::Boolean).returns(T.untyped) }
+      sig { params(argv: T::Array[String], ignore_invalid_options: T::Boolean).returns(Args) }
       def parse(argv = ARGV.freeze, ignore_invalid_options: false)
         raise "Arguments were already parsed!" if @args_parsed
 
@@ -381,7 +400,7 @@ module Homebrew
         @args.freeze_processed_options!(@processed_options)
         @args.freeze
 
-        @args_parsed = true
+        @args_parsed = T.let(true, T.nilable(TrueClass))
 
         if !ignore_invalid_options && @args.help?
           puts generate_help_text
@@ -391,12 +410,15 @@ module Homebrew
         @args
       end
 
+      sig { void }
       def set_default_options; end
 
+      sig { void }
       def validate_options; end
 
+      sig { returns(String) }
       def generate_help_text
-        Formatter.format_help_text(@parser.to_s, width: COMMAND_DESC_WIDTH)
+        Formatter.format_help_text(@parser.to_s, width: Formatter::COMMAND_DESC_WIDTH)
                  .gsub(/\n.*?@@HIDDEN@@.*?(?=\n)/, "")
                  .sub(/^/, "#{Tty.bold}Usage: brew#{Tty.reset} ")
                  .gsub(/`(.*?)`/m, "#{Tty.bold}\\1#{Tty.reset}")
@@ -406,11 +428,12 @@ module Homebrew
                  end
       end
 
+      sig { void }
       def cask_options
         self.class.global_cask_options.each do |args|
-          options = args.pop
+          options = T.cast(args.pop, T::Hash[Symbol, String])
           send(*args, **options)
-          conflicts "--formula", args.last
+          conflicts "--formula", args[1]
         end
         @cask_options = true
       end
@@ -422,7 +445,7 @@ module Homebrew
 
       sig {
         params(
-          type:        T.any(NilClass, Symbol, T::Array[String], T::Array[Symbol]),
+          type:        ArgType,
           number:      T.nilable(Integer),
           min:         T.nilable(Integer),
           max:         T.nilable(Integer),
@@ -442,9 +465,9 @@ module Homebrew
 
         if type == :none
           @max_named_args = 0
-        elsif number.present?
+        elsif number
           @min_named_args = @max_named_args = number
-        elsif min.present? || max.present?
+        elsif min || max
           @min_named_args = min
           @max_named_args = max
         end
@@ -459,11 +482,7 @@ module Homebrew
 
       private
 
-      SYMBOL_TO_USAGE_MAPPING = {
-        text_or_regex: "<text>|`/`<regex>`/`",
-        url:           "<URL>",
-      }.freeze
-
+      sig { returns(String) }
       def generate_usage_banner
         command_names = ["`#{@command_name}`"]
         aliases_to_skip = %w[instal uninstal]
@@ -519,6 +538,7 @@ module Homebrew
         "#{command_names.join(", ")}#{options}#{named_args}"
       end
 
+      sig { returns(String) }
       def generate_banner
         @usage_banner ||= generate_usage_banner
 
@@ -530,6 +550,7 @@ module Homebrew
         BANNER
       end
 
+      sig { params(names: String, value: T.untyped, from: Symbol).void }
       def set_switch(*names, value:, from:)
         names.each do |name|
           @switch_sources[option_to_name(name)] = from
@@ -537,6 +558,7 @@ module Homebrew
         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-]")
@@ -547,14 +569,17 @@ module Homebrew
         end
       end
 
+      sig { params(name: String).returns(T::Boolean) }
       def option_passed?(name)
-        @args[name.to_sym] || @args[:"#{name}?"]
+        !!(@args[name.to_sym] || @args[:"#{name}?"])
       end
 
+      sig { params(desc: String).returns(T::Array[String]) }
       def wrap_option_desc(desc)
-        Formatter.format_help_text(desc, width: OPTION_DESC_WIDTH).split("\n")
+        Formatter.format_help_text(desc, width: Formatter::OPTION_DESC_WIDTH).split("\n")
       end
 
+      sig { params(name: String, depends_on: T.nilable(String)).returns(T.nilable(T::Array[[String, String]])) }
       def set_constraints(name, depends_on:)
         return if depends_on.nil?
 
@@ -563,6 +588,7 @@ module Homebrew
         @constraints << [primary, secondary]
       end
 
+      sig { void }
       def check_constraints
         @constraints.each do |primary, secondary|
           primary_passed = option_passed?(primary)
@@ -577,6 +603,7 @@ module Homebrew
         end
       end
 
+      sig { void }
       def check_conflicts
         @conflicts.each do |mutually_exclusive_options_group|
           violations = mutually_exclusive_options_group.select do |option|
@@ -596,6 +623,7 @@ module Homebrew
         end
       end
 
+      sig { void }
       def check_invalid_constraints
         @conflicts.each do |mutually_exclusive_options_group|
           @constraints.each do |p, s|
@@ -606,12 +634,14 @@ module Homebrew
         end
       end
 
+      sig { void }
       def check_constraint_violations
         check_invalid_constraints
         check_conflicts
         check_constraints
       end
 
+      sig { params(args: T::Array[String]).void }
       def check_named_args(args)
         types = Array(@named_args_type).filter_map do |type|
           next type if type.is_a? Symbol
@@ -631,6 +661,7 @@ module Homebrew
         raise exception if exception
       end
 
+      sig { params(args: String, type: Symbol, hidden: T::Boolean).void }
       def process_option(*args, type:, hidden: false)
         option, = @parser.make_switch(args)
         @processed_options.reject! { |existing| existing.second == option.long.first } if option.long.first.present?
@@ -651,6 +682,7 @@ module Homebrew
         @non_global_processed_options << [option.long.first || option.short.first, type]
       end
 
+      sig { params(argv: T::Array[String]).returns([T::Array[String], T::Array[String]]) }
       def split_non_options(argv)
         if (sep = argv.index("--"))
           [argv.take(sep), argv.drop(sep + 1)]
@@ -659,6 +691,7 @@ module Homebrew
         end
       end
 
+      sig { params(argv: T::Array[String]).returns(T::Array[Formula]) }
       def formulae(argv)
         argv, non_options = split_non_options(argv)
 
@@ -681,12 +714,26 @@ module Homebrew
         end.uniq(&:name)
       end
 
+      sig { params(argv: T::Array[String]).returns(T::Boolean) }
       def only_casks?(argv)
         argv.include?("--casks") || argv.include?("--cask")
       end
+
+      sig { params(env: T.any(NilClass, String, Symbol)).returns(T.untyped) }
+      def value_for_env(env)
+        return if env.blank?
+
+        method_name = :"#{env}?"
+        if Homebrew::EnvConfig.respond_to?(method_name)
+          Homebrew::EnvConfig.public_send(method_name)
+        else
+          ENV.fetch("HOMEBREW_#{env.upcase}", nil)
+        end
+      end
     end
 
     class OptionConstraintError < UsageError
+      sig { params(arg1: String, arg2: String, missing: T::Boolean).void }
       def initialize(arg1, arg2, missing: false)
         message = if missing
           "`#{arg2}` cannot be passed without `#{arg1}`."
@@ -698,14 +745,15 @@ module Homebrew
     end
 
     class OptionConflictError < UsageError
+      sig { params(args: T::Array[String]).void }
       def initialize(args)
-        args_list = args.map { Formatter.option(_1) }
-                        .join(" and ")
+        args_list = args.map { Formatter.option(_1) }.join(" and ")
         super "Options #{args_list} are mutually exclusive."
       end
     end
 
     class InvalidConstraintError < UsageError
+      sig { params(arg1: String, arg2: String).void }
       def initialize(arg1, arg2)
         super "`#{arg1}` and `#{arg2}` cannot be mutually exclusive and mutually dependent simultaneously."
       end

Library/Homebrew/dev-cmd/irb.rb

@@ -6,21 +6,25 @@ require "formulary"
 require "cask/cask_loader"
 
 class String
+  # @!visibility private
   def f(*args)
     require "formula"
     Formulary.factory(self, *args)
   end
 
+  # @!visibility private
   def c(config: nil)
     Cask::CaskLoader.load(self, config:)
   end
 end
 
 class Symbol
+  # @!visibility private
   def f(*args)
     to_s.f(*args)
   end
 
+  # @!visibility private
   def c(config: nil)
     to_s.c(config:)
   end

Library/Homebrew/help.rb

@@ -116,7 +116,7 @@ module Homebrew
       help_lines = command_help_lines(path)
       return if help_lines.blank?
 
-      Formatter.format_help_text(help_lines.join, width: COMMAND_DESC_WIDTH)
+      Formatter.format_help_text(help_lines.join, width: Formatter::COMMAND_DESC_WIDTH)
                .sub("@hide_from_man_page ", "")
                .sub(/^\* /, "#{Tty.bold}Usage: brew#{Tty.reset} ")
                .gsub(/`(.*?)`/m, "#{Tty.bold}\\1#{Tty.reset}")

Library/Homebrew/test/abstract_command_spec.rb

@@ -67,7 +67,6 @@ RSpec.describe Homebrew::AbstractCommand do
           filename = File.basename(file, ".rb")
           require(file)
           command = described_class.command(filename)
-          dir = command.name.start_with?("Homebrew::DevCmd") ? "dev-cmd" : "cmd"
           expect(Pathname(File.join(__dir__, "../#{dir}/#{command.command_name}.rb"))).to exist
         end
       end

Library/Homebrew/utils/formatter.rb

@@ -7,6 +7,9 @@ require "utils/tty"
 #
 # @api private
 module Formatter
+  COMMAND_DESC_WIDTH = 80
+  OPTION_DESC_WIDTH = 45
+
   def self.arrow(string, color: nil)
     prefix("==>", string, color)
   end