Give every benchmark the same shape and lint it

.github/scripts/check-benchmarks.sh

@@ -1,7 +1,8 @@
 #!/bin/bash
-# The benchmarks-ok check. Passes when every benchmark job passed, or when
-# there was nothing to benchmark. Usage: check-benchmarks.sh <changes result> <rake result>
+# The benchmarks-ok check.
+# Passes when the lint passed and every benchmark job passed, or there was nothing to benchmark.
+# Usage: check-benchmarks.sh <changes result> <lint result> <rake result>
 set -e
 
-echo "changes: $1, rake: $2"
-[ "$1" = success ] && { [ "$2" = success ] || [ "$2" = skipped ]; }
+echo "changes: $1, lint: $2, rake: $3"
+[ "$1" = success ] && [ "$2" = success ] && { [ "$3" = success ] || [ "$3" = skipped ]; }

.github/scripts/lint-benchmarks.rb

@@ -0,0 +1,175 @@
+# Checks that every benchmark under code/ has the same shape, so they all run,
+# print a comparison, and are measured the same way:
+#
+# - every Benchmark.ips block calls x.compare!
+# - no custom timing (Benchmark.ips(20), x.time = 20, x.warmup = 5, x.config(time: 20) in any hash syntax),
+#   so every file uses the default
+# - no Benchmark.ips sits inside a method that never runs from the top of the file,
+#   which would benchmark nothing at all
+#
+# Usage: ruby .github/scripts/lint-benchmarks.rb [files...]  (needs Ruby 3.3+)
+require "prism"
+
+# Calls named `name` that have a block, not looking inside the ones found.
+def find_calls(node, name, found = [])
+  return found unless node
+
+  if node.is_a?(Prism::CallNode) && node.name == name && node.block
+    found << node
+  else
+    node.compact_child_nodes.each { |child| find_calls(child, name, found) }
+  end
+  found
+end
+
+def any_call?(node, &test)
+  return false unless node
+  return true if node.is_a?(Prism::CallNode) && test.call(node)
+
+  node.compact_child_nodes.any? { |child| any_call?(child, &test) }
+end
+
+TIMING_KEYS = %w[time warmup].freeze
+
+# x.time = 20, x.warmup = 5, or x.config with a time or warmup key,
+# in any hash syntax (time: 20, :time => 20).
+def sets_timing?(node)
+  any_call?(node) do |call|
+    next true if %i[time= warmup=].include?(call.name)
+    next false unless call.name == :config && call.arguments
+
+    call.arguments.arguments.any? do |arg|
+      next false unless arg.is_a?(Prism::KeywordHashNode) || arg.is_a?(Prism::HashNode)
+
+      arg.elements.any? do |element|
+        element.is_a?(Prism::AssocNode) && element.key.is_a?(Prism::SymbolNode) &&
+          TIMING_KEYS.include?(element.key.unescaped)
+      end
+    end
+  end
+end
+
+# Every method is known by its name, and by its scope
+# ("Foo#run", or "#run" at the top of the file).
+# A class method is also known by its full class name ("A::B.run"),
+# whether defined as `def self.run` or inside `class << self`,
+# so a call like `A.run` does not count as calling `C.run`.
+Definition = Struct.new(:keys, :node, :owner)
+
+def nested_name(owner, node)
+  [owner, node.constant_path.slice].compact.join("::")
+end
+
+def definitions(node, owner = nil, singleton = false, found = [])
+  return found unless node
+
+  case node
+  when Prism::ClassNode, Prism::ModuleNode
+    owner = nested_name(owner, node)
+    singleton = false
+  when Prism::SingletonClassNode
+    singleton = node.expression.is_a?(Prism::SelfNode)
+  when Prism::DefNode
+    class_method = owner && (singleton || node.receiver.is_a?(Prism::SelfNode))
+    keys = [node.name.to_s, "#{owner}##{node.name}"]
+    keys << "#{owner}.#{node.name}" if class_method
+    found << Definition.new(keys, node, owner)
+  end
+  node.compact_child_nodes.each { |child| definitions(child, owner, singleton, found) }
+  found
+end
+
+# Keys a call can reach:
+# - a call without a receiver reaches a method in the same class or at the
+#   top of the file when the file defines one, otherwise any method of that
+#   name (one from a parent class or an included module);
+# - `Foo.run` reaches "Foo.run" when the file defines it, otherwise any `run`;
+# - a call on another object (`Foo.new.run`) reaches any `run`;
+# - calls on a variable (x.report, x.compare!) are the benchmark's own API.
+def keys_of(call, owner, known)
+  receiver = call.receiver
+  case receiver
+  when Prism::LocalVariableReadNode
+    []
+  when nil, Prism::SelfNode
+    scoped = ["##{call.name}", *("#{owner}##{call.name}" if owner)]
+    scoped.intersect?(known) ? scoped : [call.name.to_s]
+  when Prism::ConstantReadNode, Prism::ConstantPathNode
+    key = "#{receiver.slice}.#{call.name}"
+    known.include?(key) ? [key] : [call.name.to_s]
+  else
+    [call.name.to_s]
+  end
+end
+
+# Keys of the methods called anywhere under `node`, from code in class
+# `owner`. With `top_level: true`, only calls outside any method:
+# the file's entry points.
+def called_keys(node, known, owner: nil, top_level: false, keys: [])
+  return keys unless node
+  return keys if top_level && node.is_a?(Prism::DefNode)
+
+  owner = nested_name(owner, node) if node.is_a?(Prism::ClassNode) || node.is_a?(Prism::ModuleNode)
+  keys.concat(keys_of(node, owner, known)) if node.is_a?(Prism::CallNode)
+  node.compact_child_nodes.each do |child|
+    called_keys(child, known, owner: owner, top_level: top_level, keys: keys)
+  end
+  keys
+end
+
+# Methods that run when the file runs: called from top-level code, or from a method that does.
+def reachable_methods(root, methods)
+  known = methods.flat_map { |m| m.keys.drop(1) }
+  keys = called_keys(root, known, top_level: true)
+  reached = []
+  loop do
+    newly = (methods - reached).select { |m| m.keys.intersect?(keys) }
+    break if newly.empty?
+
+    reached.concat(newly)
+    newly.each { |m| keys.concat(called_keys(m.node.body, known, owner: m.owner)) }
+  end
+  reached
+end
+
+def lint(file)
+  result = Prism.parse_file(file)
+  return ["does not parse: #{result.errors.first.message}"] if result.failure?
+
+  blocks = find_calls(result.value, :ips)
+  return ["no Benchmark.ips block"] if blocks.empty?
+
+  methods = definitions(result.value)
+  reached = reachable_methods(result.value, methods)
+  problems = []
+
+  blocks.each_with_index do |ips, index|
+    where = blocks.size > 1 ? "block #{index + 1} (line #{ips.location.start_line})" : "Benchmark.ips"
+
+    problems << "#{where}: no x.compare!" unless any_call?(ips.block) { |call| call.name == :compare! }
+    problems << "#{where}: remove the timing arguments, use the default" if ips.arguments
+    problems << "#{where}: remove the timing settings, use the default" if sets_timing?(ips.block)
+
+    # The innermost method around this Benchmark.ips, if any.
+    owner = methods.select do |m|
+      m.node.location.start_offset <= ips.location.start_offset && ips.location.end_offset <= m.node.location.end_offset
+    end.min_by { |m| m.node.location.length }
+
+    if owner && !reached.include?(owner)
+      name = owner.keys.last.delete_prefix("#")
+      problems << "#{where}: inside `def #{name}`, which never runs from the top of the file"
+    end
+  end
+  problems
+end
+
+files = ARGV.empty? ? Dir["code/**/*.rb"].sort : ARGV
+problems = files.flat_map { |file| lint(file).map { |problem| "#{file}: #{problem}" } }
+
+if problems.empty?
+  puts "All #{files.size} benchmark files have the expected shape."
+else
+  noun = problems.size == 1 ? "problem" : "problems"
+  puts problems, "", "#{problems.size} #{noun}, see \"Note on entry\" in CONTRIBUTING.md."
+  exit 1
+end

.github/workflows/benchmarks.yml

@@ -24,9 +24,21 @@ jobs:
         # A pull_request checks out a merge commit: HEAD^1 is the base.
         run: .github/scripts/pick-benchmarks.sh "${{ github.event_name == 'pull_request' && 'HEAD^1' || github.event.before }}"
 
+  # Every benchmark has the same shape (see lint-benchmarks.rb). Reads the
+  # files without running them, on the newest Ruby: the lint needs Prism.
+  lint:
+    runs-on: ubuntu-latest
+
+    steps:
+      - uses: actions/checkout@v4
+      - name: Lint benchmarks
+        run: docker compose run --rm -T --entrypoint ruby ruby_4.0 .github/scripts/lint-benchmarks.rb
+
+  # Waits for the lint: if it fails, the PR cannot merge, so the benchmark
+  # jobs would only hold the org's job slots for nothing.
   rake:
     name: rake (${{ matrix.ruby }}${{ matrix.variant && format('+{0}', matrix.variant) || '' }})
-    needs: changes
+    needs: [changes, lint]
     if: needs.changes.outputs.run == 'true'
     runs-on: ubuntu-latest
     timeout-minutes: 60
@@ -90,11 +102,11 @@ jobs:
   # The check to require on main. Passes when every benchmark job passed, or
   # when there was nothing to benchmark.
   benchmarks-ok:
-    needs: [changes, rake]
+    needs: [changes, lint, rake]
     if: always()
     runs-on: ubuntu-latest
 
     steps:
       - uses: actions/checkout@v4
       - name: Check the benchmark jobs
-        run: .github/scripts/check-benchmarks.sh ${{ needs.changes.result }} ${{ needs.rake.result }}
+        run: .github/scripts/check-benchmarks.sh ${{ needs.changes.result }} ${{ needs.lint.result }} ${{ needs.rake.result }}

CONTRIBUTING.md

@@ -35,6 +35,12 @@ Benchmark.ips do |x|
 end
 ```
 
+Keep that shape: end every `Benchmark.ips` block with `x.compare!`, keep the
+default timing (no `Benchmark.ips(20)`, `x.time = ...` or `x.config(time: ...)`),
+so every entry is measured the same way, and make sure
+the file actually calls `Benchmark.ips` when it runs
+(not only inside a method nothing calls). CI checks all three.
+
 Run your result:
 
 ```

code/enumerable/select-first-vs-detect.rb

@@ -10,7 +10,7 @@ def fast
   ARRAY.detect { |x| x.eql?(15) }
 end
 
-Benchmark.ips(20) do |x|
+Benchmark.ips do |x|
   x.report('Enumerable#select.first') { slow }
   x.report('Enumerable#detect') { fast }
   x.compare!