From 78dbb9f5e15329b4b780fe03c99ba057a29d80c7 Mon Sep 17 00:00:00 2001 From: Hartley McGuire Date: Sat, 11 Jan 2025 18:06:27 -0500 Subject: [PATCH] Add `railspect requires` for auditing `require`s This first commit sets up the command, a visitor to gather data, and a simple lint to demonstrate its use. This first lint ensures that "active_support/rails" is required in all frameworks (one was missing this require but this was addressed in a [previous commit][1]), and then checks that none of the requires in "active_support/rails" are called again (except in Active Support since it does not require "active_support/rails"). [1]: 2e920b0097dab768738ea20849dec8029f31e14e --- tools/rail_inspector/lib/rail_inspector.rb | 10 ++ .../rail_inspector/lib/rail_inspector/cli.rb | 9 ++ .../lib/rail_inspector/requires.rb | 92 +++++++++++++++++++ .../lib/rail_inspector/visitor/load.rb | 50 ++++++++++ .../test/rail_inspector/visitor/load_test.rb | 35 +++++++ 5 files changed, 196 insertions(+) create mode 100644 tools/rail_inspector/lib/rail_inspector/requires.rb create mode 100644 tools/rail_inspector/lib/rail_inspector/visitor/load.rb create mode 100644 tools/rail_inspector/test/rail_inspector/visitor/load_test.rb diff --git a/tools/rail_inspector/lib/rail_inspector.rb b/tools/rail_inspector/lib/rail_inspector.rb index 98dd03fe92550..4b17a907b4ff3 100644 --- a/tools/rail_inspector/lib/rail_inspector.rb +++ b/tools/rail_inspector/lib/rail_inspector.rb @@ -25,4 +25,14 @@ # SOFTWARE. module RailInspector + class << self + def frameworks(rails_path) + @frameworks ||= begin + spec = Gem::Specification.load(rails_path.join("rails.gemspec").to_s) + names = spec.dependencies.map(&:name) + names.delete("bundler") + names + end + end + end end diff --git a/tools/rail_inspector/lib/rail_inspector/cli.rb b/tools/rail_inspector/lib/rail_inspector/cli.rb index 68e171619a976..d201aee5fdae6 100644 --- a/tools/rail_inspector/lib/rail_inspector/cli.rb +++ b/tools/rail_inspector/lib/rail_inspector/cli.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "thor" +require_relative "../rail_inspector" module RailInspector class Cli < Thor @@ -30,5 +31,13 @@ def configuration(rails_path) checker.write! end + + desc "requires RAILS_PATH", "Check for autoloads being required" + option :autocorrect, type: :boolean, aliases: :a + def requires(rails_path) + require_relative "./requires" + + exit Requires.new(rails_path, options[:autocorrect]).call + end end end diff --git a/tools/rail_inspector/lib/rail_inspector/requires.rb b/tools/rail_inspector/lib/rail_inspector/requires.rb new file mode 100644 index 0000000000000..73f43563c1621 --- /dev/null +++ b/tools/rail_inspector/lib/rail_inspector/requires.rb @@ -0,0 +1,92 @@ +# frozen_string_literal: true + +require "active_support" +require "active_support/core_ext/string" +require "prism" + +require_relative "./visitor/load" + +module RailInspector + class Requires + def initialize(rails_path, autocorrect) + @loads = {} + @rails_path = Pathname.new(rails_path) + @exit = true + @autocorrect = autocorrect + end + + def call + populate_loads + + prevent_active_support_rails_requires + + @exit + end + + private + def populate_loads + current_file = nil + v = Visitor::Load.new { @loads[current_file] } + + @rails_path.glob("{#{frameworks.join(",")}}/lib/**/*.rb") do |file_pathname| + current_file = file_pathname.to_s + + @loads[current_file] = { requires: [], autoloads: [] } + + Prism.parse_file(current_file).value.accept(v) + end + end + + def prevent_active_support_rails_requires + frameworks.each do |framework| + next if framework == "activesupport" + + @rails_path.glob("#{framework}/lib/*.rb").each do |root_path| + root_requires = @loads[root_path.to_s][:requires] + next if root_requires.include?("active_support/rails") + + # required transitively + next if root_requires.include?("action_dispatch") + # action_pack namespace doesn't include any code + # arel does not depend on active_support at all + next if ["action_pack.rb", "arel.rb"].include?(root_path.basename.to_s) + + @exit = false + puts root_path + puts " + \"active_support/rails\" (framework root)" + end + end + + active_support_rails_requires = @loads["activesupport/lib/active_support/rails.rb"][:requires] + + duplicated_requires = {} + + @loads.each do |path, file_loads| + next if path.start_with? "activesupport" + + if active_support_rails_requires.intersect?(file_loads[:requires]) + duplicated_requires[path] = active_support_rails_requires.intersection(file_loads[:requires]) + end + end + + duplicated_requires.each do |path, offenses| + @exit = false + puts path + offenses.each do |duplicate_require| + puts " - #{duplicate_require} (active_support/rails)" + + next unless @autocorrect + + file = File.read(path) + file.gsub!("require \"#{duplicate_require}\"\n", "") + + File.write(path, file) + end + end + end + + def frameworks + RailInspector.frameworks(@rails_path) + end + end +end diff --git a/tools/rail_inspector/lib/rail_inspector/visitor/load.rb b/tools/rail_inspector/lib/rail_inspector/visitor/load.rb new file mode 100644 index 0000000000000..18d1e3c1763cd --- /dev/null +++ b/tools/rail_inspector/lib/rail_inspector/visitor/load.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require "prism" + +module RailInspector + module Visitor + class Load < Prism::Visitor + def initialize(&block) + @current_loads = block + @namespace_stack = [] + end + + def visit_module_node(node) + case node.constant_path + in Prism::ConstantReadNode[name:] + @namespace_stack << name + in Prism::ConstantPathNode + @namespace_stack << node.constant_path.full_name + end + + super + + @namespace_stack.pop + end + alias visit_class_node visit_module_node + + def visit_call_node(node) + case node.name + when :require + case node.arguments.arguments[0] + in Prism::StringNode[unescaped:] + @current_loads.call[:requires] << unescaped + else + # dynamic require, like "active_support/cache/#{store}" + end + when :autoload + case node.arguments.arguments + in [Prism::SymbolNode[unescaped:]] + namespaced_const = @namespace_stack.join("::") + namespaced_const << "::" << unescaped + + @current_loads.call[:autoloads] << namespaced_const.underscore + in [Prism::SymbolNode, Prism::StringNode[unescaped:]] + @current_loads.call[:autoloads] << unescaped + end + end + end + end + end +end diff --git a/tools/rail_inspector/test/rail_inspector/visitor/load_test.rb b/tools/rail_inspector/test/rail_inspector/visitor/load_test.rb new file mode 100644 index 0000000000000..0cc35fd015428 --- /dev/null +++ b/tools/rail_inspector/test/rail_inspector/visitor/load_test.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require "test_helper" +require "rail_inspector/visitor/load" + +class LoadTest < Minitest::Test + def test_finds_requires_and_autoloads + source = <<~FILE + require "a" + require "b" + + module D + require "k" + + autoload :L + autoload :M, "n/o" + + class E::F + require "p/q" + + autoload :G + autoload :H, "i/j" + end + end + FILE + + loads = { requires: [], autoloads: [] } + + visitor = RailInspector::Visitor::Load.new { loads } + Prism.parse(source).value.accept(visitor) + + assert_equal ["a", "b", "k", "p/q"], loads[:requires] + assert_equal ["d/l", "n/o", "d/e/f/g", "i/j"], loads[:autoloads] + end +end