-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable types in Formula files #15057
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
# typed: false | ||
# typed: true | ||
# frozen_string_literal: true | ||
|
||
require "cache_store" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
# typed: false | ||
# typed: true | ||
# frozen_string_literal: true | ||
|
||
require "deprecate_disable" | ||
|
@@ -391,7 +391,7 @@ def audit_conflicts | |
"canonical name (#{conflicting_formula.name}) instead of #{conflict.name}" | ||
end | ||
|
||
reverse_conflict_found = false | ||
reverse_conflict_found = T.let(false, T::Boolean) | ||
conflicting_formula.conflicts.each do |reverse_conflict| | ||
reverse_conflict_formula = Formulary.factory(reverse_conflict.name) | ||
if tap.formula_renames.key?(reverse_conflict.name) || tap.aliases.include?(reverse_conflict.name) | ||
|
@@ -732,14 +732,14 @@ def audit_revision_and_version_scheme | |
current_revision = formula.revision | ||
current_url = formula.stable.url | ||
|
||
previous_version = nil | ||
previous_version_scheme = nil | ||
previous_revision = nil | ||
previous_version = T.let(nil, T.nilable(Version)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same deal here: why are these needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same deal as above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dduugg This isn't a boolean so would still like to try to understand this one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorbet will by default assign an lvar’s type as the type of the initial assignment, and use that to determine which methods are available and other types of analysis. Attempting to change the type will thus result an error. This can be allowed, however, if the type is is widened to include all assignments with a Does that make sense? If this is getting too esoteric, I can pause on the typing PRs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, all good, they are making the code better overall for sure! |
||
previous_version_scheme = T.let(nil, T.nilable(Integer)) | ||
previous_revision = T.let(nil, T.nilable(Integer)) | ||
|
||
newest_committed_version = nil | ||
newest_committed_checksum = nil | ||
newest_committed_revision = nil | ||
newest_committed_url = nil | ||
newest_committed_version = T.let(nil, T.nilable(Version)) | ||
newest_committed_checksum = T.let(nil, T.nilable(String)) | ||
newest_committed_revision = T.let(nil, T.nilable(Integer)) | ||
newest_committed_url = T.let(nil, T.nilable(String)) | ||
|
||
fv.rev_list("origin/HEAD") do |rev| | ||
begin | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
# typed: false | ||
# typed: true | ||
# frozen_string_literal: true | ||
|
||
require "utils/shell" | ||
|
@@ -7,6 +7,17 @@ | |
# | ||
# @api private | ||
module FormulaCellarChecks | ||
extend T::Sig | ||
extend T::Helpers | ||
|
||
abstract! | ||
|
||
sig { abstract.returns(Formula) } | ||
def formula; end | ||
|
||
sig { abstract.params(output: T.nilable(String)).void } | ||
def problem_if_output(output); end | ||
|
||
def check_env_path(bin) | ||
# warn the user if stuff was installed outside of their PATH | ||
return unless bin.directory? | ||
|
@@ -407,7 +418,7 @@ def cpuid_instruction?(file, objdump = "objdump") | |
end | ||
end | ||
|
||
has_cpuid_instruction = false | ||
has_cpuid_instruction = T.let(false, T::Boolean) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same |
||
Utils.popen_read(objdump, "--disassemble", file) do |io| | ||
until io.eof? | ||
instruction = io.readline.split("\t")[@instruction_column_index[objdump]]&.strip | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# typed: strict | ||
|
||
module FormulaCellarChecks | ||
include Kernel | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
# typed: false | ||
# typed: true | ||
# frozen_string_literal: true | ||
|
||
require "digest/md5" | ||
|
@@ -56,7 +56,7 @@ def self.clear_cache | |
namespace = Utils.deconstantize(klass.name) | ||
next if Utils.deconstantize(namespace) != name | ||
|
||
remove_const(Utils.demodulize(namespace)) | ||
remove_const(Utils.demodulize(namespace).to_sym) | ||
end | ||
end | ||
|
||
|
@@ -67,6 +67,7 @@ def self.clear_cache | |
module PathnameWriteMkpath | ||
refine Pathname do | ||
def write(content, offset = nil, **open_args) | ||
T.bind(self, Pathname) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this doing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorbet doesn't support refinements, so we need to explicitly tell sorbet that this def block is executed under the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
raise "Will not overwrite #{self}" if exist? && !offset && !open_args[:mode]&.match?(/^a\+?$/) | ||
|
||
dirname.mkpath | ||
|
@@ -129,7 +130,7 @@ def self.load_formula_from_path(name, path, flags:, ignore_errors:) | |
end | ||
|
||
def self.load_formula_from_api(name, flags:) | ||
namespace = "FormulaNamespaceAPI#{Digest::MD5.hexdigest(name)}" | ||
namespace = :"FormulaNamespaceAPI#{Digest::MD5.hexdigest(name)}" | ||
|
||
mod = Module.new | ||
remove_const(namespace) if const_defined?(namespace) | ||
|
@@ -268,6 +269,7 @@ def install | |
service_hash = Homebrew::Service.deserialize(service_hash) | ||
run_params = service_hash.delete(:run) | ||
service do | ||
T.bind(self, Homebrew::Service) | ||
if run_params.is_a?(Hash) | ||
run(**run_params) | ||
else | ||
|
@@ -306,7 +308,7 @@ def versioned_formulae_names | |
end | ||
end | ||
|
||
klass.loaded_from_api = true | ||
T.cast(klass, T.class_of(Formula)).loaded_from_api = true | ||
reitermarkus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
mod.const_set(class_s, klass) | ||
|
||
cache[:api] ||= {} | ||
|
@@ -351,7 +353,7 @@ def self.ensure_utf8_encoding(io) | |
|
||
def self.class_s(name) | ||
class_name = name.capitalize | ||
class_name.gsub!(/[-_.\s]([a-zA-Z0-9])/) { Regexp.last_match(1).upcase } | ||
class_name.gsub!(/[-_.\s]([a-zA-Z0-9])/) { T.must(Regexp.last_match(1)).upcase } | ||
class_name.tr!("+", "x") | ||
class_name.sub!(/(.)@(\d)/, "\\1AT\\2") | ||
class_name | ||
|
@@ -489,17 +491,20 @@ class FromUrlLoader < FormulaLoader | |
def initialize(url, from: nil) | ||
@url = url | ||
@from = from | ||
uri = URI(url) | ||
formula = File.basename(uri.path, ".rb") | ||
super formula, HOMEBREW_CACHE_FORMULA/File.basename(uri.path) | ||
uri_path = URI(url).path | ||
raise ArgumentError, "URL has no path component" unless uri_path | ||
|
||
formula = File.basename(uri_path, ".rb") | ||
super formula, HOMEBREW_CACHE_FORMULA/File.basename(uri_path) | ||
end | ||
|
||
def load_file(flags:, ignore_errors:) | ||
if @from != :formula_installer | ||
if %r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<formula_name>[\w+-.@]+).rb} =~ url | ||
match = url.match(%r{githubusercontent.com/[\w-]+/[\w-]+/[a-f0-9]{40}(?:/Formula)?/(?<name>[\w+-.@]+).rb}) | ||
if match | ||
raise UnsupportedInstallationMethod, | ||
"Installation of #{formula_name} from a GitHub commit URL is unsupported! " \ | ||
"`brew extract #{formula_name}` to a stable tap on GitHub instead." | ||
"Installation of #{match[:name]} from a GitHub commit URL is unsupported! " \ | ||
"`brew extract #{match[:name]}` to a stable tap on GitHub instead." | ||
elsif url.match?(%r{^(https?|ftp)://}) | ||
raise UnsupportedInstallationMethod, | ||
"Non-checksummed download of #{name} formula file from an arbitrary URL is unsupported! " \ | ||
|
@@ -512,8 +517,8 @@ def load_file(flags:, ignore_errors:) | |
curl_download url, to: path | ||
super | ||
rescue MethodDeprecatedError => e | ||
if %r{github.com/(?<user>[\w-]+)/(?<repo>[\w-]+)/} =~ url | ||
e.issues_url = "https://github.com/#{user}/#{repo}/issues/new" | ||
if (match_data = url.match(%r{github.com/(?<user>[\w-]+)/(?<repo>[\w-]+)/})) | ||
e.issues_url = "https://github.com/#{match_data[:user]}/#{match_data[:repo]}/issues/new" | ||
end | ||
raise | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# typed: strict | ||
|
||
module Formulary | ||
include Kernel | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is necessary to allow the type to widened from
FalseClass
toT.any(FalseClass, TrueClass)
(ruby does not have a native notion of aBoolean
type). This specific example is covered in the docs for the ensuing error. It's also touched on in the type annotations section.Sorry for not elaborating on this PR, there are already ~22 uses of this boolean lvar pattern on
master
, so I thought it might already be understood.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, don't expect you to preemptively elaborate.
This is one of the things I dislike about Sorbet, it feels weird that it's not more intuitive/permissive on Boolean usage like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, although I would characterize this as a bug in Ruby. (Mats is entrenched his no-Boolean-class position). The second link above gives the rationale for why Sorbet doesn’t special-case true/false values.