From 64e481dff4f2aa985bef877357870a19fab67ab5 Mon Sep 17 00:00:00 2001 From: goose Date: Fri, 29 Mar 2024 11:22:11 +0700 Subject: [PATCH] [#499] Implement custom Rubocop rule to enforce class template --- .rubocop.yml | 4 + .template/addons/custom_cops/template.rb | 2 + rubocop/custom_cops/class_template.rb | 109 +++++++++ .../class_template/expression_category.rb | 97 ++++++++ .../expression_category_spec.rb | 230 ++++++++++++++++++ rubocop/spec/class_template_spec.rb | 119 +++++++++ .../required_inverse_of_relations_spec.rb | 2 +- 7 files changed, 562 insertions(+), 1 deletion(-) create mode 100644 rubocop/custom_cops/class_template.rb create mode 100644 rubocop/custom_cops/class_template/expression_category.rb create mode 100644 rubocop/spec/class_template/expression_category_spec.rb create mode 100644 rubocop/spec/class_template_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index a8fb856c..266ab331 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -3,6 +3,7 @@ require: - rubocop-rspec - rubocop-performance - ./rubocop/custom_cops/required_inverse_of_relations.rb + - ./rubocop/custom_cops/class_template.rb AllCops: Exclude: @@ -60,3 +61,6 @@ CustomCops/RequiredInverseOfRelations: Include: # Only Rails model files - !ruby/regexp /models\// + +CustomCops/ClassTemplate: + Enabled: true diff --git a/.template/addons/custom_cops/template.rb b/.template/addons/custom_cops/template.rb index aec9816f..356c330b 100644 --- a/.template/addons/custom_cops/template.rb +++ b/.template/addons/custom_cops/template.rb @@ -3,3 +3,5 @@ require 'fileutils' copy_file 'rubocop/custom_cops/required_inverse_of_relations.rb', mode: :preserve +directory 'rubocop/custom_cops/class_template', mode: :preserve +copy_file 'rubocop/custom_cops/class_template.rb', mode: :preserve diff --git a/rubocop/custom_cops/class_template.rb b/rubocop/custom_cops/class_template.rb new file mode 100644 index 00000000..12074add --- /dev/null +++ b/rubocop/custom_cops/class_template.rb @@ -0,0 +1,109 @@ +# frozen_string_literal: true + +require_relative 'class_template/expression_category' + +module CustomCops + class ClassTemplate < RuboCop::Cop::Base + include CustomCops::ExpressionCategory + + EXPRESSION_TYPE_ORDER = { + extend: 0, + include: 1, + constant_assignment: 2, + attribute_macro: 3, + other_macro: 4, + self_class_block: 5, + public_class_method: 5, + initialization: 6, + public_instance_method: 7, + alias: 8, + alias_method: 8, + protected_instance_method: 9, + private_instance_method: 10 + }.freeze + + # Scan class body + def on_class(node) + expressions = top_level_expressions(node) + + state = { function_visibility: :public, expression_types: [] } + expressions.each_with_object(state) { |expression, acc| process_expression(expression, acc) } + + validate_expressions_order(state[:expression_types]) + end + + private + + def top_level_expressions(class_node) + return [] unless class_node.body + + # Multi-expression body + return class_node.body.children if class_node.body.type == :begin + + # Single-expression body + [class_node.body] + end + + def process_expression(expression, acc) + category = categorize(expression) + + if %i[protected private].include?(category) + acc[:function_visibility] = category + else + category = "#{acc[:function_visibility]}_#{category}".to_sym if category == :instance_method + acc[:expression_types] << { category: category, expression: expression } + end + end + + def validate_expressions_order(expressions) + expressions = expressions.filter { _1[:category] != :unknown } + + expressions.each_cons(2) do |first, second| + next unless EXPRESSION_TYPE_ORDER[first[:category]] > EXPRESSION_TYPE_ORDER[second[:category]] + + add_offense( + second[:expression], + message: error_message(second[:category], expressions.pluck(:category)) + ) + end + end + + # Find the correct spot for the out of order expression + # rubocop:disable Metrics/MethodLength + def error_message(out_of_order_expression, expression_types) + expression_types.filter { _1 != out_of_order_expression } + .each_with_index do |expression, index| + error = if index.zero? + before_first_expression(expression, out_of_order_expression) + elsif index == expression_types.size - 1 + after_last_expression(expression, out_of_order_expression) + else + previous_expression = expression_types[index - 1] + in_between_expressions(previous_expression, expression, out_of_order_expression) + end + + return error if error + end + end + # rubocop:enable Metrics/MethodLength + + def before_first_expression(current_expression, out_of_order_expression) + return unless EXPRESSION_TYPE_ORDER[out_of_order_expression] < EXPRESSION_TYPE_ORDER[current_expression] + + "#{out_of_order_expression} should come before #{current_expression}." + end + + def after_last_expression(current_expression, out_of_order_expression) + return unless EXPRESSION_TYPE_ORDER[out_of_order_expression] > EXPRESSION_TYPE_ORDER[current_expression] + + "#{out_of_order_expression} should come after #{current_expression}." + end + + def in_between_expressions(previous_expression, current_expression, out_of_order_expression) + return unless EXPRESSION_TYPE_ORDER[out_of_order_expression] < EXPRESSION_TYPE_ORDER[current_expression] && + EXPRESSION_TYPE_ORDER[out_of_order_expression] > EXPRESSION_TYPE_ORDER[previous_expression] + + "#{out_of_order_expression} should come after #{previous_expression} and before #{current_expression}." + end + end +end diff --git a/rubocop/custom_cops/class_template/expression_category.rb b/rubocop/custom_cops/class_template/expression_category.rb new file mode 100644 index 00000000..725712ef --- /dev/null +++ b/rubocop/custom_cops/class_template/expression_category.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +require 'rubocop' + +module CustomCops + module ExpressionCategory + extend RuboCop::NodePattern::Macros + + ATTRIBUTE_MACROS = %i[attr_reader attr_writer attr_accessor].to_set + + CATEGORIES = %i[ + extend include constant_assignment attribute_macro alias_method protected private + other_macro self_class_block public_class_method initialization instance_method alias + ].freeze + + # extend SomeModule + def_node_matcher :extend?, <<~PATTERN + (send {nil? | self} :extend const) + PATTERN + + # include SomeModule + def_node_matcher :include?, <<~PATTERN + (send {nil? | self} :include const) + PATTERN + + # HELLO = 'world'.freeze + def_node_matcher :constant_assignment?, <<~PATTERN + (casgn nil? _ _) + PATTERN + + # attr_reader :foo, :bar + # attr_writer :baz + def_node_matcher :attribute_macro?, <<~PATTERN + (send {nil? | self} ATTRIBUTE_MACROS sym+) + PATTERN + + # validates :foo, presence: true + def_node_matcher :other_macro?, <<~PATTERN + (send {nil? | self} _ _+) + PATTERN + + # class << self + # def foo + # 'bar' + # end + # end + def_node_matcher :self_class_block?, <<~PATTERN + (sclass self _) + PATTERN + + # def self.foo + # "bar" + # end + def_node_matcher :public_class_method?, <<~PATTERN + (defs self _ args _) + PATTERN + + # def initialize(a, b) + # end + def_node_matcher :initialization?, <<~PATTERN + (def :initialize args _) + PATTERN + + # def method(a, b) + # end + def_node_matcher :instance_method?, <<~PATTERN + (def _ args _) + PATTERN + + # alias foo bar + # alias :foo :bar + def_node_matcher :alias?, <<~PATTERN + (alias sym sym) + PATTERN + + # alias_method :foo, :bar + # alias_method 'foo', 'bar' + def_node_matcher :alias_method?, <<~PATTERN + (send {nil? | self} :alias_method _ _) + PATTERN + + # protected + def_node_matcher :protected?, <<~PATTERN + (send {nil? | self} :protected) + PATTERN + + # private + def_node_matcher :private?, <<~PATTERN + (send {nil? | self} :private) + PATTERN + + # Categorize the expression of the class body + def categorize(expression) + CATEGORIES.find { |category| send(:"#{category}?", expression) } || :unknown + end + end +end diff --git a/rubocop/spec/class_template/expression_category_spec.rb b/rubocop/spec/class_template/expression_category_spec.rb new file mode 100644 index 00000000..6b08480b --- /dev/null +++ b/rubocop/spec/class_template/expression_category_spec.rb @@ -0,0 +1,230 @@ +# frozen_string_literal: true + +require 'rubocop' +require_relative '../../custom_cops/class_template/expression_category' + +describe CustomCops::ExpressionCategory, :config do + context 'given an include expression' do + it 'returns :include' do + expect(expression_category('include Foo')).to be(:include) + expect(expression_category('self.include Foo')).to be(:include) + end + end + + context 'given an extend expression' do + it 'returns :extend' do + expect(expression_category('extend Foo')).to be(:extend) + expect(expression_category('self.extend Foo')).to be(:extend) + end + end + + context 'given a constant assignment' do + it 'returns :constant_assignment' do + expect(expression_category("HELLO = 'world'.freeze")).to be(:constant_assignment) + expect(expression_category('HELLO = WORLD')).to be(:constant_assignment) + end + end + + context 'given an attribute macro' do + context 'given the attribute macro only has a single field' do + it 'returns :attribute_macro' do + expect(expression_category('attr_reader :hello')).to be(:attribute_macro) + expect(expression_category('self.attr_reader :hello')).to be(:attribute_macro) + + expect(expression_category('attr_writer :hello')).to be(:attribute_macro) + expect(expression_category('self.attr_writer :hello')).to be(:attribute_macro) + + expect(expression_category('attr_accessor :hello')).to be(:attribute_macro) + expect(expression_category('self.attr_accessor :hello')).to be(:attribute_macro) + end + end + + context 'given the attribute macro has multiple fields' do + it 'returns :attribute_macro' do + expect(expression_category('attr_reader :hello, :world')).to be(:attribute_macro) + expect(expression_category('self.attr_reader :hello, :world')).to be(:attribute_macro) + + expect(expression_category('attr_writer :hello, :world')).to be(:attribute_macro) + expect(expression_category('self.attr_writer :hello, :world')).to be(:attribute_macro) + + expect(expression_category('attr_accessor :hello, :world')).to be(:attribute_macro) + expect(expression_category('self.attr_accessor :hello, :world')).to be(:attribute_macro) + end + end + end + + context 'given a macro expression' do + it 'returns :other_macro' do + expect(expression_category('validates :name')).to be(:other_macro) + expect(expression_category("scope :in_advance, -> { where('started_at > ?', Time.current) }")).to be(:other_macro) + end + end + + context 'given a self class block' do + context 'given the block has no methods' do + it 'returns :self_class_block' do + code = <<~RUBY + class << self + end + RUBY + + expect(expression_category(code)).to be(:self_class_block) + end + end + + context 'given the block has ONE method' do + it 'returns :self_class_block' do + code = <<~RUBY + class << self + def foo + 'foo' + end + end + RUBY + + expect(expression_category(code)).to be(:self_class_block) + end + end + + context 'given the block has multiple methods' do + it 'returns :self_class_block' do + code = <<~RUBY + class << self + def foo + 'foo' + end + + def bar + 'bar' + end + end + RUBY + + expect(expression_category(code)).to be(:self_class_block) + end + end + end + + context 'given a public class method definition' do + context 'given the method has no arguments' do + it 'returns :public_class_method' do + code = <<~RUBY + def self.foo + "bar" + end + RUBY + + expect(expression_category(code)).to be(:public_class_method) + end + end + + context 'given the method has arguments' do + it 'returns :public_class_method' do + code = <<~RUBY + def self.foo(a, _b, c) + "bar" + end + RUBY + + expect(expression_category(code)).to be(:public_class_method) + end + end + end + + context 'given an initialize method definition' do + it 'returns :initialization' do + code = <<~RUBY + def initialize(a, _b, c) + @sum = a + c + end + RUBY + + expect(expression_category(code)).to be(:initialization) + end + end + + context 'given an instance method definition' do + context 'given the method has no arguments' do + it 'returns :instance_method' do + code = <<~RUBY + def foo + "bar" + end + RUBY + + expect(expression_category(code)).to be(:instance_method) + end + end + + context 'given the method has arguments' do + it 'returns :instance_method' do + code = <<~RUBY + def foo(a, _b, c) + "bar" + end + RUBY + + expect(expression_category(code)).to be(:instance_method) + end + end + end + + context 'given an alias expression' do + it 'returns :alias' do + code = <<~RUBY + alias foo bar + RUBY + + expect(expression_category(code)).to be(:alias) + end + end + + context 'given an alias_method expression' do + context 'given arguments are atoms' do + it 'returns :alias_method' do + code = <<~RUBY + alias_method :foo, :bar + RUBY + + expect(expression_category(code)).to be(:alias_method) + end + end + + context 'given arguments are strings' do + it 'returns :alias_method' do + code = <<~RUBY + alias_method 'foo', "bar_#{1 + 1}" + RUBY + + expect(expression_category(code)).to be(:alias_method) + end + end + end + + context 'given an protected statement' do + it 'returns :protected' do + code = <<~RUBY + protected + RUBY + + expect(expression_category(code)).to be(:protected) + end + end + + context 'given an private statement' do + it 'returns :private' do + code = <<~RUBY + private + RUBY + + expect(expression_category(code)).to be(:private) + end + end + + def expression_category(code) + dummy_class = Class.new { include CustomCops::ExpressionCategory } + source = RuboCop::ProcessedSource.new(code, RUBY_VERSION.to_f) + + dummy_class.new.categorize(source.ast) + end +end diff --git a/rubocop/spec/class_template_spec.rb b/rubocop/spec/class_template_spec.rb new file mode 100644 index 00000000..a8a02631 --- /dev/null +++ b/rubocop/spec/class_template_spec.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +require 'rubocop' +require 'rubocop/rspec/support' +require_relative '../custom_cops/class_template' + +RSpec.configure do |config| + config.include RuboCop::RSpec::ExpectOffense +end + +describe CustomCops::ClassTemplate, :config do + context 'given an out of order expression' do + it 'registers an offense' do + expect_offense(<<~RUBY) + class Test + include B + extend A + ^^^^^^^^ extend should come before include. + end + RUBY + + expect_offense(<<~RUBY) + class Test + extend A + include B + + def initialize + {foo: "bar"} + end + + CONSTANT = 1 + ^^^^^^^^^^^^ constant_assignment should come after include and before initialization. + end + RUBY + + expect_offense(<<~RUBY) + class Test + extend A + include B + + CONSTANT = 1 + + def initialize + {foo: "bar"} + end + + private + + def foo + "foo" + end + + def self.bar + ^^^^^^^^^^^^ public_class_method should come after constant_assignment and before initialization. + "bar" + end + end + RUBY + end + end + + context 'given NO out of order expression' do + it 'registers NO offense' do + expect_no_offenses(<<~RUBY) + class Test + extend A + include B + + CONSTANT = 1 + + def initialize + {foo: "bar"} + end + end + RUBY + + expect_no_offenses(<<~RUBY) + class Test + extend A + include B + + CONSTANT = 1 + + def self.bar + "bar" + end + + def initialize + {foo: "bar"} + end + + private + + def foo + "foo" + end + end + RUBY + end + end + + context 'given an unknown expression' do + it 'ignores it' do + expect_no_offenses(<<~RUBY) + class Test + extend A + include B + + def initialize + {foo: "bar"} + end + + # unknown expression + foo = "bar" + end + RUBY + end + end +end diff --git a/rubocop/spec/required_inverse_of_relations_spec.rb b/rubocop/spec/required_inverse_of_relations_spec.rb index d427e24c..f3b56af4 100644 --- a/rubocop/spec/required_inverse_of_relations_spec.rb +++ b/rubocop/spec/required_inverse_of_relations_spec.rb @@ -2,7 +2,7 @@ require 'rubocop' require 'rubocop/rspec/support' -require_relative '../cops/required_inverse_of_relations' +require_relative '../custom_cops/required_inverse_of_relations' RSpec.configure do |config| config.include RuboCop::RSpec::ExpectOffense