diff --git a/Library/.rubocop.yml b/Library/.rubocop.yml index 46e926e83a..96c2f71e1f 100644 --- a/Library/.rubocop.yml +++ b/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: diff --git a/Library/Homebrew/cask/config.rb b/Library/Homebrew/cask/config.rb index e34e441797..ae8c2539fb 100644 --- a/Library/Homebrew/cask/config.rb +++ b/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, diff --git a/Library/Homebrew/cli/args.rb b/Library/Homebrew/cli/args.rb index f391a8726e..bd80f8325a 100644 --- a/Library/Homebrew/cli/args.rb +++ b/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) - 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[:force_bottle] = true if 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, parent: self, cask_options:, without_api:, **options), + T.nilable(NamedArgs), ) 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 diff --git a/Library/Homebrew/cli/args.rbi b/Library/Homebrew/cli/args.rbi index 11451fdb55..7c161fd39b 100644 --- a/Library/Homebrew/cli/args.rbi +++ b/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 diff --git a/Library/Homebrew/cli/parser.rb b/Library/Homebrew/cli/parser.rb index b1a9465ee4..9c5eba6ee6 100644 --- a/Library/Homebrew/cli/parser.rb +++ b/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 diff --git a/Library/Homebrew/cmd/search.rb b/Library/Homebrew/cmd/search.rb index 9d0e6cf6bd..7429daec5e 100644 --- a/Library/Homebrew/cmd/search.rb +++ b/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 diff --git a/Library/Homebrew/extend/os/linux/cli/parser.rb b/Library/Homebrew/extend/os/linux/cli/parser.rb index e5fd59e146..c4f2d7554e 100644 --- a/Library/Homebrew/extend/os/linux/cli/parser.rb +++ b/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. diff --git a/Library/Homebrew/sorbet/rbi/dsl/homebrew/cmd/tap_cmd.rbi b/Library/Homebrew/sorbet/rbi/dsl/homebrew/cmd/tap_cmd.rbi index 2372f2207a..63c82b8607 100644 --- a/Library/Homebrew/sorbet/rbi/dsl/homebrew/cmd/tap_cmd.rbi +++ b/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) } diff --git a/Library/Homebrew/sorbet/rbi/dsl/homebrew/dev_cmd/audit.rbi b/Library/Homebrew/sorbet/rbi/dsl/homebrew/dev_cmd/audit.rbi index 146719a333..9939567b9c 100644 --- a/Library/Homebrew/sorbet/rbi/dsl/homebrew/dev_cmd/audit.rbi +++ b/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) } diff --git a/Library/Homebrew/sorbet/tapioca/compilers/args.rb b/Library/Homebrew/sorbet/tapioca/compilers/args.rb index b3ad828b48..bf3ee2e77e 100644 --- a/Library/Homebrew/sorbet/tapioca/compilers/args.rb +++ b/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]) } - def args_table(parser) - # 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 args_table(parser) = parser.args.methods(false) 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) } - def get_return_type(method_name, value, comma_array_methods) + sig { params(method_name: Symbol, comma_array_methods: T::Array[Symbol]).returns(String) } + 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 diff --git a/Library/Homebrew/test/abstract_command_spec.rb b/Library/Homebrew/test/abstract_command_spec.rb index c6669c842b..3c24bb6b83 100644 --- a/Library/Homebrew/test/abstract_command_spec.rb +++ b/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 diff --git a/Library/Homebrew/test/cask/upgrade_spec.rb b/Library/Homebrew/test/cask/upgrade_spec.rb index 3b5455fcc4..d2bf61b2b9 100644 --- a/Library/Homebrew/test/cask/upgrade_spec.rb +++ b/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| diff --git a/Library/Homebrew/test/sorbet/tapioca/compilers/args_spec.rb b/Library/Homebrew/test/sorbet/tapioca/compilers/args_spec.rb index ee50a912a1..7761721b29 100644 --- a/Library/Homebrew/test/sorbet/tapioca/compilers/args_spec.rb +++ b/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( - :"1?", :built_from_source?, :cask?, :casks?, :d?, :debug?, - :formula?, :formulae?, :full_name?, :h?, :help?, - :installed_as_dependency?, :installed_on_request?, :l?, - :multiple?, :pinned?, :poured_from_bottle?, :q?, :quiet?, - :r?, :t?, :v?, :verbose?, :versions? + expect(compiler.args_table(list_parser)).to contain_exactly( + :"1?", :built_from_source?, :cask?, :casks?, :d?, :debug?, :formula?, :formulae?, :full_name?, :h?, :help?, + :installed_as_dependency?, :installed_on_request?, :l?, :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({ - d?: false, - debug?: false, - exclude_packages: nil, - 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, - }) + expect(compiler.args_table(update_python_resources_parser)).to contain_exactly( + :d?, :debug?, :exclude_packages, :extra_packages, :h?, :help?, :ignore_non_pypi_packages?, + :install_dependencies?, :p?, :package_name, :print_only?, :q?, :quiet?, :s?, :silent?, :v?, :verbose?, + :version + ) 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