Add Cached module to simplify caching code.

[reitermarkus]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

Also added a separate version of Cachable#cache which takes a block and automatically freezes the cached value.

[apainintheneck]

I'm not sure that it's intuitive to include and extend the same module in different places in the codebase especially if we're planning on doing both in the same class. It might be worth thinking up some way of differentiating the instance cache from the class/module cache.

Interestingly enough we do have this pattern in a few places in the codebase but it's never truly an instance cache.

$ cd $(brew --repo)
$ git grep --no-index -C 4 'include Cachable'
Library/Homebrew/api/cask.rb-    #
Library/Homebrew/api/cask.rb-    # @api private
Library/Homebrew/api/cask.rb-    module Cask
Library/Homebrew/api/cask.rb-      class << self
Library/Homebrew/api/cask.rb:        include Cachable
Library/Homebrew/api/cask.rb-
Library/Homebrew/api/cask.rb-        private :cache
Library/Homebrew/api/cask.rb-
Library/Homebrew/api/cask.rb-        sig { params(token: String).returns(Hash) }
--
Library/Homebrew/api/formula.rb-    #
Library/Homebrew/api/formula.rb-    # @api private
Library/Homebrew/api/formula.rb-    module Formula
Library/Homebrew/api/formula.rb-      class << self
Library/Homebrew/api/formula.rb:        include Cachable
Library/Homebrew/api/formula.rb-
Library/Homebrew/api/formula.rb-        private :cache
Library/Homebrew/api/formula.rb-
Library/Homebrew/api/formula.rb-        sig { params(name: String).returns(Hash) }
--
Library/Homebrew/readall.rb-#
Library/Homebrew/readall.rb-# @api private
Library/Homebrew/readall.rb-module Readall
Library/Homebrew/readall.rb-  class << self
Library/Homebrew/readall.rb:    include Cachable
Library/Homebrew/readall.rb-    include SystemCommand::Mixin
Library/Homebrew/readall.rb-
Library/Homebrew/readall.rb-    # TODO: remove this once the `MacOS` module is undefined on Linux
Library/Homebrew/readall.rb-    MACOS_MODULE_REGEX = /\b(MacOS|OS::Mac)(\.|::)\b/

[reitermarkus]

not sure that it's intuitive to include and extend the same module

I disagree, that's just how Ruby works, but it is actually quite surprising that all existing usages so far are class-level caches.

[MikeMcQuaid]

Looks good other than BracesRequiredMethods change.

Relatedly: I appreciate the renewed enthusiasm @reitermarkus but would appreciate if you can prioritise e.g. fixing the newly flaky tests from previous refactors (see BuildPulse or Slack). Thanks!

[MikeMcQuaid]

Too many breaking refactors recently. Let's hold off on this until after the next stable tag, please.

[apainintheneck]

not sure that it's intuitive to include and extend the same module

I disagree, that's just how Ruby works, but it is actually quite surprising that all existing usages so far are class-level caches.

Fair enough. Maybe I'm overthinking this.

[apainintheneck]

There are a few places in the code that use cache.fetch(:key) { logic } and those can probably be cleaned up after this change too.

[MikeMcQuaid]

Applied the one change I cared about (which will likely break syntax job). Once syntax fixed up: 👍🏻 to merge (although, as mentioned elsewhere, please do so when around for 2-4 hours afterwards to fix up any reported regressions).

[apainintheneck]

@reitermarkus You're crazy for trying to solve this with meta programming but I really like the result so I guess you had the right approach. I think that this can be simplified a bit be restricting the arity of methods that can be cached. It'd be really simple to add a runtime check to restrict this to methods without arguments with method.arity.zero?; doing it purely in tests is also possible but a little trickier.

[apainintheneck]

Would it be possible to use the included or extended callback to simplify this a little bit?

Ideally all that'd be necessary here would be to add include Cached or extend Cached and everything would be set up.

[reitermarkus]

It may be possible, but does Sorbet support following this?

[apainintheneck]

No idea. @dduugg do you happen to know if this works as expected? We do use the included callback in one part of the code but the methods are generated so they are not typed by Sorbet.

brew/Library/Homebrew/extend/on_system.rb

Lines 142 to 162 in 6a9c9c0

	sig { params(_base: Class).void }
	def self.included(_base)
	raise "Do not include `OnSystem` directly. Instead, include `OnSystem::MacOSAndLinux` or `OnSystem::MacOSOnly`"
	end
	module MacOSAndLinux
	sig { params(base: Class).void }
	def self.included(base)
	OnSystem.setup_arch_methods(base)
	OnSystem.setup_base_os_methods(base)
	OnSystem.setup_macos_methods(base)
	end
	end
	module MacOSOnly
	sig { params(base: Class).void }
	def self.included(base)
	OnSystem.setup_arch_methods(base)
	OnSystem.setup_macos_methods(base)
	end
	end

[dduugg]

I won't have the bandwidth to look at this specific code in detail anytime soon, but often the answer to making included hook functionality sorbet-compatible is to use mixes_in_class_methods: https://sorbet.org/docs/abstract#interfaces-and-the-included-hook

[apainintheneck]

Thanks @dduugg! That's very helpful.

[apainintheneck]

I'm not sure it really makes sense for cached methods to take arguments so this can probably be simplified a bit more. None of the cache methods in Tap take arguments right now. At the very least, I think it's a bad idea to allow cached methods to take blocks. I don't think it's be terribly difficult to add tests to make sure that methods are defined that way too.

[apainintheneck]

Also, I'm a bit surprised that you can use a block as a hash key. I guess I'd just never considered even attempting that.

[MikeMcQuaid]

I'm not sure it really makes sense for cached methods to take arguments so this can probably be simplified a bit more. None of the cache methods in Tap take arguments right now. At the very least, I think it's a bad idea to allow cached methods to take blocks.

Agreed.

[reitermarkus]

Actually it does make sense, for example Tap::_fetch will cache it based on user and repo.

[reitermarkus]

The block may not be needed though.

[MikeMcQuaid]

Let's remove this functionality unless it's needed in 3+ places.

[apainintheneck]

There is also a pattern in the code to create complex hash keys by concatenating strings with dashes. I'm not sure if this is preferable though to just using arrays of objects. Does anyone have strong feelings either way?

[apainintheneck]

What changed to make this necessary?

[reitermarkus]

Actually returning super in CoreTap#remote when HOMEBREW_NO_INSTALL_FROM_API is set.

[MikeMcQuaid]

Given (more) recent user regressions around refactoring and how this PR is adding more code than it's removing I want to step back and have a discussion about the motivation here.

I am not convinced this makes the code significantly easier to follow or that this is not going to cause more bugs.

[MikeMcQuaid]

Why is a new module needed now? Is this only used once?

[reitermarkus]

Why is a new module needed now?

To make the code more readable and easier to maintain, i.e. not having to manually keep track of which cached variables need to be cleared.

Is this only used once?

So far only in Tap, but it should be possible to replace Cachable with this.

[MikeMcQuaid]

I think it'd be good to use this in 2+ places if it's added here to verify the API is sufficiently generic and better test the edges.

[apainintheneck]

So far only in Tap, but it should be possible to replace Cachable with this.

I think having both Cachable and Cached is a bit confusing especially if they are used in the same module. The long-term goal should be to only have one. I prefer the simplicity of Cachable in most cases. Cached is not as flexible and that means once we try to cache methods that take arguments the interface is not as clean and it's possible that we might need to add helper methods which actually add complexity.

I like the approach and I think the code is cool but I'm not sure it's worth the cost here from a maintainability point of view.

As a quick comparison of Cachable and Cached, I'll show some examples of things that would be easier and things that would be harder.

# Now (on master)
def git_head
  raise TapUnavailableError, name unless installed?

  @git_head ||= git_repo.head_ref
end

# Cached
cached def git_head
  raise TapUnavailableError, name unless installed?

  git_repo.head_ref
end

# Cachable
def git_head
  raise TapUnavailableError, name unless installed?

  cache[:git_head] ||= git_repo.head_ref
end

These functions are all similar complexity. The advantage of caching is being able to clear the cache with one easy method which would be provided by both Cached and Cachable.

Note: Cached and Cachable are not exactly equivalent here. Cached will only be evaluated once while Cachable will check if the tap is installed each time though the head ref will be cached.

Here's another example.

# Now (on master)
def audit_exceptions
  @audit_exceptions ||= begin
    ensure_installed!
    super
  end
end

# Cached
cached def audit_exceptions
  ensure_installed!
  super
end

# Cachable
def audit_exceptions
  cache[:audit_exceptions] ||= begin
    ensure_installed!
    super
  end
end

The complexity is not much different in this case either. I would argue that it's also easier to understand at a glance since it uses basic Ruby patterns as well but that's very subjective.

One fine example, that's a bit different.

# Cachable (on master)
def self.fetch(user, repo)
  ...

  cache_key = "#{user}/#{repo}".downcase
  cache.fetch(cache_key) { |key| cache[key] = Tap.new(user, repo) }
end

# Cached
def self.fetch(user, repo)
  ...

  _fetch(user, repo)
end

private_class_method cached_class_method def self._fetch(user, repo)
  new(user, repo)
end

The Tap.fetch method shows that Cached is a not as flexible and in this case that means we need to create an additional method. One method is not the end of the world, of course, but it does suggest that this doesn't always simplify the code.

I feel like the simplest approach would just be to use Cachable without any modifications.

[MikeMcQuaid]

The long-term goal should be to only have one.

I agree with most of the above except: I don't think this should be "long-term", I would really like to avoid having both existing separately for any time/at all.

No longer applies

[reitermarkus]

restrict this to methods without arguments with method.arity.zero?

I tried this in the beginning, but that actually only works when not using Sorbet at runtime because Sorbet will always wrap all methods with *args.

[apainintheneck]

restrict this to methods without arguments with method.arity.zero?

I tried this in the beginning, but that actually only works when not using Sorbet at runtime because Sorbet will always wrap all methods with *args.

Lovely... it sometimes feels like I'm fighting Sorbet when I propose generic solutions.

[apainintheneck]

It looks like you can get the arity and parameters for a method wrapped by Sorbet by using the T::Utils.signature_for_method helper. Not sure if that's really needed here but at least it's interesting.

brew(main):011:0> Formulary.method(:factory).arity
=> -1
brew(main):012:0> Formulary.method(:factory).parameters
=> [[:rest, :args], [:keyrest, :**], [:block, :blk]]
brew(main):013:0> T::Utils.signature_for_method(Formulary.method(:factory)).req_arg_count
=> 1
brew(main):014:0> T::Utils.signature_for_method(Formulary.method(:factory)).parameters
=>
[[:req, :ref],
 [:opt, :spec],
 [:key, :alias_path],
 [:key, :from],
 [:key, :warn],
 [:key, :force_bottle],
 [:key, :flags],
 [:key, :ignore_errors]]

Another way to get around it is by disabling runtime checks but I doubt we want to do that.

I found that by looking here: https://sorbet.org/docs/runtime#tsigwithoutruntimesig

Edit: There is also a T::Utils.arity(method) helper as well.

[apainintheneck]

If possible, I think we should avoid making this a pattern. Ideally the original method would be able to be cached directly.

[apainintheneck]

I'll throw another tangentially related idea out there. Maybe it'd be good to somehow programmatically collect all classes and modules that mix in Cachable and then clear those caches in-between tests. That would make it harder to forget to clear the cache of some module. This probably doesn't need to happen at the instance level though. To be clear, I'm not suggesting that this should be included in this PR. I'm just thinking out loud.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[MikeMcQuaid]

Passing on this, sorry.