From 84c480c0f2bd04de6b03b32e06203619d985e321 Mon Sep 17 00:00:00 2001 From: Ryo Nakamura Date: Thu, 10 Nov 2022 08:23:35 +0900 Subject: [PATCH] Support inline visibility definition on checking visibility --- .../change_support_visibility_definition.md | 1 + lib/rubocop/cop/mixin/visibility_help.rb | 45 ++++++- .../cop/layout/class_structure_spec.rb | 34 ++++- spec/rubocop/cop/visibility_help_spec.rb | 125 ++++++++++++++++++ 4 files changed, 196 insertions(+), 9 deletions(-) create mode 100644 changelog/change_support_visibility_definition.md create mode 100644 spec/rubocop/cop/visibility_help_spec.rb diff --git a/changelog/change_support_visibility_definition.md b/changelog/change_support_visibility_definition.md new file mode 100644 index 00000000000..a14ae1854d3 --- /dev/null +++ b/changelog/change_support_visibility_definition.md @@ -0,0 +1 @@ +* [#11171](https://github.com/rubocop/rubocop/pull/11171): Support inline visibility definition on checking visibility. ([@r7kamura][]) diff --git a/lib/rubocop/cop/mixin/visibility_help.rb b/lib/rubocop/cop/mixin/visibility_help.rb index fe106361ad9..b7b96056700 100644 --- a/lib/rubocop/cop/mixin/visibility_help.rb +++ b/lib/rubocop/cop/mixin/visibility_help.rb @@ -1,18 +1,43 @@ # frozen_string_literal: true +require 'set' + module RuboCop module Cop # Help methods for determining node visibility. module VisibilityHelp extend NodePattern::Macros - VISIBILITY_SCOPES = %i[private protected public].freeze + VISIBILITY_SCOPES = ::Set[:private, :protected, :public].freeze private def node_visibility(node) - scope = find_visibility_start(node) - scope&.method_name || :public + node_visibility_from_visibility_inline(node) || + node_visibility_from_visibility_block(node) || + :public + end + + def node_visibility_from_visibility_inline(node) + return unless node.def_type? + + node_visibility_from_visibility_inline_on_def(node) || + node_visibility_from_visibility_inline_on_method_name(node) + end + + def node_visibility_from_visibility_inline_on_def(node) + parent = node.parent + parent.method_name if visibility_inline_on_def?(parent) + end + + def node_visibility_from_visibility_inline_on_method_name(node) + node.right_siblings.reverse.find do |sibling| + visibility_inline_on_method_name?(sibling, method_name: node.method_name) + end&.method_name + end + + def node_visibility_from_visibility_block(node) + find_visibility_start(node)&.method_name end def find_visibility_start(node) @@ -21,7 +46,7 @@ def find_visibility_start(node) # Navigate to find the last protected method def find_visibility_end(node) - possible_visibilities = VISIBILITY_SCOPES - [node_visibility(node)] + possible_visibilities = VISIBILITY_SCOPES - ::Set[node_visibility(node)] right = node.right_siblings right.find do |child_node| possible_visibilities.include?(node_visibility(child_node)) @@ -30,7 +55,17 @@ def find_visibility_end(node) # @!method visibility_block?(node) def_node_matcher :visibility_block?, <<~PATTERN - (send nil? { :private :protected :public }) + (send nil? VISIBILITY_SCOPES) + PATTERN + + # @!method visibility_inline_on_def?(node) + def_node_matcher :visibility_inline_on_def?, <<~PATTERN + (send nil? VISIBILITY_SCOPES def) + PATTERN + + # @!method visibility_inline_on_method_name?(node, method_name:) + def_node_matcher :visibility_inline_on_method_name?, <<~PATTERN + (send nil? VISIBILITY_SCOPES (sym %method_name)) PATTERN end end diff --git a/spec/rubocop/cop/layout/class_structure_spec.rb b/spec/rubocop/cop/layout/class_structure_spec.rb index 22015755f98..214a3dde5e9 100644 --- a/spec/rubocop/cop/layout/class_structure_spec.rb +++ b/spec/rubocop/cop/layout/class_structure_spec.rb @@ -186,10 +186,6 @@ def initialize def some_public_method end - def other_public_method - end - - private :other_public_method def yet_other_public_method end @@ -199,6 +195,11 @@ def yet_other_public_method def some_protected_method end + def other_public_method + end + + private :other_public_method + private def some_private_method @@ -384,6 +385,31 @@ def bar end RUBY end + + it 'registers an offense and corrects public method after private method marked by its name' do + expect_offense(<<~RUBY) + class A + def foo + end + private :foo + + def bar + ^^^^^^^ `public_methods` is supposed to appear before `private_methods`. + end + end + RUBY + + expect_correction(<<~RUBY) + class A + def bar + end + + def foo + end + private :foo + end + RUBY + end end context 'initializer is private and comes after attribute macro' do diff --git a/spec/rubocop/cop/visibility_help_spec.rb b/spec/rubocop/cop/visibility_help_spec.rb new file mode 100644 index 00000000000..a77ddfc7567 --- /dev/null +++ b/spec/rubocop/cop/visibility_help_spec.rb @@ -0,0 +1,125 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::VisibilityHelp do + describe '#node_visibility' do + subject do + instance.__send__(:node_visibility, node) + end + + let(:instance) do + klass.new + end + + let(:klass) do + mod = described_class + Class.new do + include mod + end + end + + let(:node) do + processed_source.ast.each_node(:def).first + end + + let(:processed_source) do + parse_source(source) + end + + let(:source) do + raise NotImplementedError + end + + context 'without visibility block' do + let(:source) do + <<~RUBY + class A + def x; end + end + RUBY + end + + it { is_expected.to eq(:public) } + end + + context 'with visibility block public' do + let(:source) do + <<~RUBY + class A + public + + def x; end + end + RUBY + end + + it { is_expected.to eq(:public) } + end + + context 'with visibility block private' do + let(:source) do + <<~RUBY + class A + private + + def x; end + end + RUBY + end + + it { is_expected.to eq(:private) } + end + + context 'with visibility block private after public' do + let(:source) do + <<~RUBY + class A + public + + private + + def x; end + end + RUBY + end + + it { is_expected.to eq(:private) } + end + + context 'with inline public' do + let(:source) do + <<~RUBY + class A + public def x; end + end + RUBY + end + + it { is_expected.to eq(:public) } + end + + context 'with inline private' do + let(:source) do + <<~RUBY + class A + private def x; end + end + RUBY + end + + it { is_expected.to eq(:private) } + end + + context 'with inline private with symbol' do + let(:source) do + <<~RUBY + class A + def x; end + private :x + end + RUBY + end + + it { is_expected.to eq(:private) } + end + end +end