From 9e7cf24b5be8ed2b73207470dbd3d92a7208069c Mon Sep 17 00:00:00 2001 From: Chris Salzberg Date: Tue, 20 Feb 2024 14:40:53 +0900 Subject: [PATCH] Add Rubocop::Cops::Rails::ZeitwerkFriendlyConstant --- config/default.yml | 6 +- .../cop/rails/zeitwerk_friendly_constant.rb | 161 +++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + .../rails/zeitwerk_friendly_constant_spec.rb | 255 ++++++++++++++++++ 4 files changed, 422 insertions(+), 1 deletion(-) create mode 100644 lib/rubocop/cop/rails/zeitwerk_friendly_constant.rb create mode 100644 spec/rubocop/cop/rails/zeitwerk_friendly_constant_spec.rb diff --git a/config/default.yml b/config/default.yml index 4ad7a0d053..1d299037c1 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1218,7 +1218,11 @@ Rails/WhereNotWithMultipleConditions: Enabled: 'pending' Severity: warning VersionAdded: '2.17' - VersionChanged: '2.18' + +Rails/ZeitwerkFriendlyConstant: + Description: 'Ensure all constants defined in each file are independently loadable by Zeitwerk.' + Enabled: 'pending' + VersionAdded: '<>' # Accept `redirect_to(...) and return` and similar cases. Style/AndOr: diff --git a/lib/rubocop/cop/rails/zeitwerk_friendly_constant.rb b/lib/rubocop/cop/rails/zeitwerk_friendly_constant.rb new file mode 100644 index 0000000000..e651576d2f --- /dev/null +++ b/lib/rubocop/cop/rails/zeitwerk_friendly_constant.rb @@ -0,0 +1,161 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Ensures that every constant defined in a file matches the file name + # such a way that it is independently loadable by Zeitwerk. + # + # @example + # + # Good + # + # # /some/directory/foo.rb + # module Foo + # end + # + # # /some/directory/foo.rb + # module Foo + # module Bar + # end + # end + # + # # /some/directory/foo/bar.rb + # module Foo + # module Bar + # end + # end + # + # Bad + # + # # /some/directory/foo.rb + # module Bar + # end + # + # # /some/directory/foo/bar.rb + # module Foo + # module Bar + # end + # + # module Baz + # end + # end + # + class ZeitwerkFriendlyConstant < Base + MSG = 'Constant name does not match filename.' + CLASS_MESSAGE = 'Class name does not match filename.' + MODULE_MESSAGE = 'Module name does not match filename.' + INCOMPATIBLE_FILE_PATH_MESSAGE = 'Constant names are mutually incompatible with file path.' + + CONSTANT_NAME_MATCHER = /\A[[:upper:]_]*\Z/.freeze + CONSTANT_DEFINITION_TYPES = %i[module class casgn].freeze + + def relevant_file?(file) + super && (File.extname(file) == '.rb') + end + + def on_new_investigation + return if processed_source.blank? + + common_anchors = nil + + each_nested_constant(processed_source.ast) do |node, nesting| + anchors = nesting.anchors(path_segments) + + if anchors.empty? + add_offense(node, message: offense_message(node)) + else + common_anchors ||= anchors + + if (common_anchors &= anchors).empty? + # Add an offense if there is no common anchor among constants. + add_offense(node, message: INCOMPATIBLE_FILE_PATH_MESSAGE) + end + end + end + end + + private + + Nesting = Struct.new(:namespace) do + def push(node) + self.namespace += [node] + @constants = nil + end + + def constants + @constants ||= namespace.flat_map { |node| constant_name(node).split('::') } + end + + # For a nesting like ["Foo", "Bar"] and path segments ["", "Some", + # "Dir", "Foo", "Bar"], return an array of all possible "anchors" of the + # nesting within the segments, if any (in this case, [3]). + def anchors(path_segments) + (1..constants.length).each_with_object([]) do |i, anchors| + anchors << i if path_segments[(path_segments.size - i)..] == constants[0, i] + end + end + + def constant_name(node) + if (defined_module = node.defined_module) + defined_module.const_name + else + name = node.children[1].to_s + name = name.split('_').map(&:capitalize!).join if CONSTANT_NAME_MATCHER.match?(name) + name + end + end + end + # Traverse the AST from node and yield each constant, along with its + # nesting: an array of class/module names within which it is defined. + def each_nested_constant(node, nesting = Nesting.new([]), &block) + nesting.push(node) if constant_definition?(node) + + any_yielded = node.child_nodes.map do |child_node| + each_nested_constant(child_node, nesting.dup, &block) + end.any? + + # We only yield "leaves", i.e. constants that have no other nested + # constants within themselves. To do this we return true from this + # method if it itself has yielded, and only yield from parents if all + # recursive calls did not return true (i.e. they did not yield). + if !any_yielded && constant_definition?(node) + yield(node, nesting) + true + else + any_yielded + end + end + + def path_segments + @path_segments ||= processed_source.file_path.delete_suffix('.rb').split('/').map! { |dir| camelize(dir) } + end + + def constant_definition?(node) + CONSTANT_DEFINITION_TYPES.include?(node.type) + end + + def offense_message(node) + case node.type + when :module + MODULE_MESSAGE + when :class + CLASS_MESSAGE + end + end + + def camelize(path_segment) + path_segment.split('_').map! do |segment| + acronyms.key?(segment) ? acronyms[segment] : segment.capitalize + end.join + end + + def acronyms + @acronyms ||= cop_config['Acronyms'].to_h do |acronym| + [acronym.downcase, acronym] + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 0d92ff4ca5..700bc61c0e 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -138,3 +138,4 @@ require_relative 'rails/where_missing' require_relative 'rails/where_not' require_relative 'rails/where_not_with_multiple_conditions' +require_relative 'rails/zeitwerk_friendly_constant' diff --git a/spec/rubocop/cop/rails/zeitwerk_friendly_constant_spec.rb b/spec/rubocop/cop/rails/zeitwerk_friendly_constant_spec.rb new file mode 100644 index 0000000000..52a572b86b --- /dev/null +++ b/spec/rubocop/cop/rails/zeitwerk_friendly_constant_spec.rb @@ -0,0 +1,255 @@ +# frozen_string_literal: true + +RSpec.describe(RuboCop::Cop::Rails::ZeitwerkFriendlyConstant, :config) do + describe('Offending code') do + it 'Adds an offense when module name does not match filename' do + code = <<~RUBY + module Bar + ^^^^^^^^^^ Module name does not match filename. + end + RUBY + expect_offense(code, '/some/dir/foo.rb') + end + + it 'Adds an offense when any of multiple module names does not match filename' do + code = <<~RUBY + module Foo + end + + module Bar + ^^^^^^^^^^ Module name does not match filename. + end + RUBY + expect_offense(code, '/some/dir/foo.rb') + end + + it 'Adds an offense when nested class or module name does not match filename path' do + code = <<~RUBY + class Foo + module Bar + ^^^^^^^^^^ Module name does not match filename. + end + end + RUBY + expect_offense(code, '/some/dir/bar.rb') + end + + it 'Adds offenses to multiple constants that do not match filename' do + code = <<~RUBY + class Foo + module Bar + ^^^^^^^^^^ Module name does not match filename. + end + + module Baz + ^^^^^^^^^^ Module name does not match filename. + end + end + RUBY + expect_offense(code, '/some/dir/foo/qux.rb') + end + + it 'Adds an offense when nested class or module name in compact form does not match filename path' do + code = <<~RUBY + class Foo::Bar + ^^^^^^^^^^^^^^ Class name does not match filename. + end + RUBY + expect_offense(code, '/some/dir/bar.rb') + end + + it 'Adds an offense when parent namespace does not match defined module path' do + code = <<~RUBY + module Foo + class Baz + end + + module Bar + ^^^^^^^^^^ Module name does not match filename. + end + end + RUBY + expect_offense(code, '/some/dir/foo/baz.rb') + end + + it 'Adds an offense when namespace does not completely match module name' do + code = <<~RUBY + module Foo + class Bar + ^^^^^^^^^ Class name does not match filename. + end + end + RUBY + expect_offense(code, '/some/dir/foo/barbaz.rb') + end + + it 'Adds an offense for constants that are neither modules nor classes' do + code = <<~RUBY + module Foo + Bar = "mystring" + ^^^^^^^^^^^^^^^^ Constant name does not match filename. + end + RUBY + expect_offense(code, '/some/dir/foo/baz.rb') + end + + it 'Adds an offense for constant nested inside block that does not match filename' do + code = <<~RUBY + module Foo + begin + module Baz + ^^^^^^^^^^ Module name does not match filename. + end + end + end + RUBY + expect_offense(code, '/some/dir/foo/bar.rb') + end + + it 'Adds an offense for constants that have no common path' do + code = <<~RUBY + module Foo + end + + module Bar + module Foo + ^^^^^^^^^^ Constant names are mutually incompatible with file path. + end + end + RUBY + expect_offense(code, '/some/dir/bar/foo.rb') + end + + it 'Adds only one offense for constants that have no common path if one is already flagged' do + code = <<~RUBY + module Foo + ^^^^^^^^^^ Module name does not match filename. + end + + module Bar + end + RUBY + expect_offense(code, '/some/dir/bar.rb') + end + end + + describe('Non offending code') do + it 'Does not add an offense when module name matches filename' do + code = <<~RUBY + module Foo + end + RUBY + expect_no_offenses(code, '/some/dir/foo.rb') + end + + it 'Does not add an offense when nested module name matches filename path' do + code = <<~RUBY + module Foo + module Bar + end + end + RUBY + expect_no_offenses(code, '/some/dir/foo/bar.rb') + end + + it 'Does not add an offense when nested module name in compact form matches filename path' do + code = <<~RUBY + module Foo::Bar + end + RUBY + expect_no_offenses(code, '/some/dir/foo/bar.rb') + end + + it 'Does not add an offense when nested module name matches parent path' do + code = <<~RUBY + module Foo + module Bar + end + end + RUBY + expect_no_offenses(code, '/some/dir/foo.rb') + end + + it 'Does not add an offense for capitalized constants that match file name' do + code = <<~RUBY + module Foo + MY_VERSION_STRING = "mystring" + end + RUBY + expect_no_offenses(code, '/some/dir/foo/my_version_string.rb') + end + + it 'Does not add an offense when multiple nested module names matches parent path' do + code = <<~RUBY + module Foo + module Bar + end + + class Baz + end + end + RUBY + expect_no_offenses(code, '/some/dir/foo.rb') + end + + it 'Does not add an offense for more complex module names that match path' do + code = <<~RUBY + module FooBarBaz + end + RUBY + expect_no_offenses(code, '/some/dir/foo_bar_baz.rb') + end + + it 'Does not add an offense for file names with extra underscores' do + code = <<~RUBY + module FooBarBaz3 + end + RUBY + expect_no_offenses(code, '/some/dir/foo_bar_baz_3.rb') + end + + it 'Does not add an offense when filename matches namespace in multiple places' do + code = <<~RUBY + module Foo + module Bar + end + end + RUBY + expect_no_offenses(code, '/some/dir/foo/baz/foo/bar.rb') + end + + it 'Does not add an offense for constants nested inside block that matches filename' do + code = <<~RUBY + module Foo + begin + module Bar + end + end + end + RUBY + expect_no_offenses(code, '/some/dir/foo/bar.rb') + end + + it 'Does not add an offense for non-Ruby files' do + code = <<~RUBY + module Foo + end + RUBY + expect_no_offenses(code, '/some/dir/bar') + end + end + + describe('when config defines custom acronyms') do + let(:cop_config) { { 'Acronyms' => ['GCS'] } } + + it 'Does not add an offense when filename matches constant with custom acronym' do + code = <<~RUBY + module GCSCsvCollection + module Foo + end + end + RUBY + expect_no_offenses(code, '/some/dir/gcs_csv_collection/foo.rb') + end + end +end