Merge pull request #22283 from Homebrew/rubocop-public-api-checks

Move API checks into RuboCop

Author: MikeMcQuaid

.github/workflows/tests.yml

@@ -65,10 +65,6 @@ jobs:
 
       - run: brew typecheck
 
-      - name: Check public API lists are in sync
-        working-directory: ${{ steps.set-up-homebrew.outputs.repository-path }}
-        run: brew ruby Library/Homebrew/github_actions_utils/public_api_check.rb
-
   tap-syntax:
     name: tap syntax
     needs: syntax

Library/Homebrew/.rubocop.yml

@@ -37,7 +37,7 @@ Naming/MethodParameterName:
     - uv
 
 # Only enforce documentation for public APIs.
-# Checked by the tests.yml syntax job
+# Include list checked by Homebrew/PublicApiDocumentation.
 Style/Documentation:
   AllowedConstants:
     - Homebrew

Library/Homebrew/github_actions_utils/public_api_check.rb

@@ -11,9 +11,13 @@ module Homebrew
       # definitions.
       #
       # Both cookbook method lists live in {ApiAnnotationHelper} and are
-      # validated by `.github/check_cookbook_method_lists.rb` in CI.
+      # validated by this cop.
       class PublicApiCookbook < Base
         MSG = "Method `%<method>s` is referenced in the %<cookbook>s but is not annotated with `@api public`."
+        MISSING_FORMULA_LIST_MSG = "Formula Cookbook references methods missing from " \
+                                   "`FORMULA_COOKBOOK_METHODS`: %<methods>s."
+        MISSING_CASK_LIST_MSG = "Method `%<method>s` is annotated with `@api public` in `%<file>s` but is " \
+                                "missing from `CASK_COOKBOOK_METHODS`."
 
         sig { void }
         def on_new_investigation
@@ -23,12 +27,61 @@ def on_new_investigation
           return if file_path.nil?
 
           relative_path = file_path.sub(%r{.*/Library/Homebrew/}, "")
+
+          if relative_path == "rubocops/shared/api_annotation_helper.rb"
+            missing_formula = (HOMEBREW_LIBRARY_PATH.parent.parent/"docs/Formula-Cookbook.md").read
+                              .scan(
+                                %r{/rubydoc/\w+(?:/\w+)*\.html#(\w+[!?]?)-(?:class|instance)_method},
+                              )
+                              .flatten -
+                              ApiAnnotationHelper::FORMULA_COOKBOOK_METHODS.keys
+            missing_formula.sort!
+
+            if missing_formula.any?
+              add_offense(
+                processed_source.ast&.each_descendant(:casgn)&.find do |node|
+                  node.const_name == "FORMULA_COOKBOOK_METHODS"
+                end || processed_source.ast || processed_source.buffer.source_range,
+                message: format(
+                  MISSING_FORMULA_LIST_MSG,
+                  methods: missing_formula.map { |method| "`#{method}`" }.join(", "),
+                ),
+              )
+            end
+
+            return
+          end
+
           api_public_targets = build_api_public_targets
 
           check_cookbook_methods(ApiAnnotationHelper::FORMULA_COOKBOOK_METHODS,
                                  "Formula Cookbook", relative_path, api_public_targets)
           check_cookbook_methods(ApiAnnotationHelper::CASK_COOKBOOK_METHODS,
                                  "Cask Cookbook", relative_path, api_public_targets)
+
+          return unless %w[cask/dsl.rb cask/cask.rb cask/dsl/version.rb].include?(relative_path)
+
+          cookbook_methods = ApiAnnotationHelper::CASK_COOKBOOK_METHODS.keys.to_set
+          lines = processed_source.lines
+
+          processed_source.comments.each do |comment|
+            next unless ["# @api public", "@api public"].include?(comment.text.strip)
+
+            (1..5).each do |offset|
+              target_line = lines[comment.loc.line - 1 + offset]&.strip
+              break if target_line.blank?
+
+              match = target_line.match(/\A(?:def\s+(?:self\.)?|attr_reader\s+:|attr_accessor\s+:)(\w+[!?]?)/) ||
+                      target_line.match(/\Adelegate\s+(\w+[!?]?):/)
+              next if match.nil?
+
+              method_name = match[1].to_s
+              break if cookbook_methods.include?(method_name)
+
+              add_offense(comment, message: format(MISSING_CASK_LIST_MSG, method: method_name, file: relative_path))
+              break
+            end
+          end
         end
 
         private
@@ -37,7 +90,7 @@ def on_new_investigation
         # preceded by an `@api public` annotation in their doc block.
         # Walks forward from each `@api public` comment to find the next
         # def/attr_reader/delegate, matching only the immediately following
-        # definition — not one 20 lines away.
+        # definition; not one 20 lines away.
         sig { returns(T::Set[Integer]) }
         def build_api_public_targets
           targets = T.let(Set.new, T::Set[Integer])

Library/Homebrew/rubocops/public_api_cookbook.rb

@@ -26,16 +26,36 @@ module Homebrew
       # ```
       class PublicApiDocumentation < Base
         MSG = "`@api public` methods must have a descriptive YARD comment, not just the annotation."
+        MISSING_INCLUDE_MSG = "`%<file>s` contains `@api public` but is missing from `Style/Documentation.Include`."
+        EXTRA_INCLUDE_MSG = "`%<file>s` is included in `Style/Documentation.Include` but does not contain " \
+                            "`@api public`."
 
         sig { void }
         def on_new_investigation
           super
 
-          processed_source.comments.each do |comment|
+          comments = processed_source.comments
+          comments.each do |comment|
             next unless api_public_comment?(comment)
 
             add_offense(comment) unless descriptive_comment_preceding?(comment)
           end
+
+          documentation_include = config.dig("Style/Documentation", "Include")
+          file_path = processed_source.file_path
+          return if documentation_include.nil? || file_path.nil?
+
+          api_public_comments = comments.select { |comment| api_public_comment?(comment) }
+          relative_path = file_path.sub(%r{.*/Library/Homebrew/}, "")
+          included = Array(documentation_include).include?(relative_path)
+          if api_public_comments.any? && !included
+            add_offense(api_public_comments.first, message: format(MISSING_INCLUDE_MSG, file: relative_path))
+          elsif api_public_comments.empty? && included
+            add_offense(
+              processed_source.ast || processed_source.buffer.source_range,
+              message: format(EXTRA_INCLUDE_MSG, file: relative_path),
+            )
+          end
         end
 
         private

Library/Homebrew/rubocops/public_api_documentation.rb

@@ -22,7 +22,7 @@ module ApiAnnotationHelper
 
       # Methods documented in docs/Formula-Cookbook.md mapped to their
       # defining source files (relative to Library/Homebrew/).
-      # Validated by `github_actions_utils/public_api_check.rb`
+      # Validated by `Homebrew/PublicApiCookbook`
       # against rubydoc links in `docs/Formula-Cookbook.md`.
       FORMULA_COOKBOOK_METHODS = T.let({
         "cd"                    => "extend/pathname.rb",
@@ -59,7 +59,7 @@ module ApiAnnotationHelper
 
       # Methods documented in docs/Cask-Cookbook.md mapped to their
       # defining source files (relative to Library/Homebrew/).
-      # Validated by `github_actions_utils/public_api_check.rb`
+      # Validated by `Homebrew/PublicApiCookbook`
       # against `@api public` annotations in cask source files.
       CASK_COOKBOOK_METHODS = T.let({
         "after_comma"          => "cask/dsl/version.rb",

Library/Homebrew/rubocops/shared/api_annotation_helper.rb

@@ -8,6 +8,7 @@
 
   let(:formula_path) { "formula.rb" }
   let(:cask_dsl_path) { "cask/dsl.rb" }
+  let(:helper_path) { "rubocops/shared/api_annotation_helper.rb" }
 
   context "when a cookbook-referenced method lacks `@api public`" do
     it "reports an offense for a formula method" do
@@ -82,4 +83,48 @@ def desc; end
       RUBY
     end
   end
+
+  context "when a formula cookbook method is missing from the helper list" do
+    before do
+      (mktmpdir/"docs").tap do |docs|
+        docs.mkpath
+        (docs/"Formula-Cookbook.md").write <<~MARKDOWN
+          [`new_api`](/rubydoc/Formula.html#new_api-instance_method)
+        MARKDOWN
+
+        stub_const("HOMEBREW_LIBRARY_PATH", docs.parent/"Library/Homebrew")
+      end
+
+      stub_const("RuboCop::Cop::ApiAnnotationHelper::FORMULA_COOKBOOK_METHODS", {})
+    end
+
+    it "reports an offense" do
+      expect_offense(<<~RUBY, helper_path)
+        module ApiAnnotationHelper
+          FORMULA_COOKBOOK_METHODS = {}.freeze
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Homebrew/PublicApiCookbook: Formula Cookbook references methods missing from `FORMULA_COOKBOOK_METHODS`: `new_api`.
+        end
+      RUBY
+    end
+  end
+
+  context "when a public cask method is missing from the helper list" do
+    before do
+      stub_const("RuboCop::Cop::ApiAnnotationHelper::CASK_COOKBOOK_METHODS", {})
+    end
+
+    it "reports an offense" do
+      expect_offense(<<~RUBY, cask_dsl_path)
+        module Cask
+          module DSL
+            # The new stanza.
+            #
+            # @api public
+            ^^^^^^^^^^^^^ Homebrew/PublicApiCookbook: Method `new_stanza` is annotated with `@api public` in `cask/dsl.rb` but is missing from `CASK_COOKBOOK_METHODS`.
+            def new_stanza; end
+          end
+        end
+      RUBY
+    end
+  end
 end

Library/Homebrew/test/rubocops/public_api_cookbook_spec.rb

@@ -93,4 +93,34 @@ def prefix; end
       RUBY
     end
   end
+
+  context "when a public API file is missing from `Style/Documentation.Include`" do
+    subject(:cop) do
+      described_class.new(RuboCop::Config.new("Style/Documentation" => { "Include" => [] }))
+    end
+
+    it "reports an offense" do
+      expect_offense(<<~RUBY, "public_api.rb")
+        # The public method.
+        #
+        # @api public
+        ^^^^^^^^^^^^^ `public_api.rb` contains `@api public` but is missing from `Style/Documentation.Include`.
+        def foo; end
+      RUBY
+    end
+  end
+
+  context "when a documented API file has no public API annotations" do
+    subject(:cop) do
+      described_class.new(RuboCop::Config.new("Style/Documentation" => { "Include" => ["stale.rb"] }))
+    end
+
+    it "reports an offense" do
+      expect_offense(<<~RUBY, "stale.rb")
+        class Stale
+        ^^^^^^^^^^^ `stale.rb` is included in `Style/Documentation.Include` but does not contain `@api public`.
+        end
+      RUBY
+    end
+  end
 end