diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e876cb..33051a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Change log +## master + +- [#73](https://github.com/rubocop/rubocop-thread_safety/pull/73): Add `AllowCallWithBlock` option to `ThreadSafety/DirChdir` cop. ([@viralpraxis](https://github.com/viralpraxis)) + ## 0.6.0 * [#59](https://github.com/rubocop/rubocop-thread_safety/pull/59): Rename `ThreadSafety::InstanceVariableInClassMethod` cop to `ThreadSafety::ClassInstanceVariable` to better reflect its purpose. ([@viralpraxis](https://github.com/viralpraxis)) diff --git a/config/default.yml b/config/default.yml index bb4af3c..46bc99e 100644 --- a/config/default.yml +++ b/config/default.yml @@ -35,6 +35,7 @@ ThreadSafety/NewThread: ThreadSafety/DirChdir: Description: Avoid using `Dir.chdir` due to its process-wide effect. Enabled: true + AllowCallWithBlock: false ThreadSafety/RackMiddlewareInstanceVariable: Description: Avoid instance variables in Rack middleware. diff --git a/docs/modules/ROOT/pages/cops_threadsafety.adoc b/docs/modules/ROOT/pages/cops_threadsafety.adoc index bb35f4b..6e68a4c 100644 --- a/docs/modules/ROOT/pages/cops_threadsafety.adoc +++ b/docs/modules/ROOT/pages/cops_threadsafety.adoc @@ -136,6 +136,8 @@ end |=== Avoid using `Dir.chdir` due to its process-wide effect. +If `AllowCallWithBlock` (disabled by default) option is enabled, +calling `Dir.chdir` with block will be allowed. [#examples-threadsafetydirchdir] === Examples @@ -149,6 +151,39 @@ Dir.chdir("/var/run") FileUtils.chdir("/var/run") ---- +[#allowcallwithblock_-false-_default_-threadsafetydirchdir] +==== AllowCallWithBlock: false (default) + +[source,ruby] +---- +# good +Dir.chdir("/var/run") do + puts Dir.pwd +end +---- + +[#allowcallwithblock_-true-threadsafetydirchdir] +==== AllowCallWithBlock: true + +[source,ruby] +---- +# bad +Dir.chdir("/var/run") do + puts Dir.pwd +end +---- + +[#configurable-attributes-threadsafetydirchdir] +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| AllowCallWithBlock +| `false` +| Boolean +|=== + [#threadsafetymutableclassinstancevariable] == ThreadSafety/MutableClassInstanceVariable diff --git a/lib/rubocop/cop/thread_safety/dir_chdir.rb b/lib/rubocop/cop/thread_safety/dir_chdir.rb index 6a13b12..12951b2 100644 --- a/lib/rubocop/cop/thread_safety/dir_chdir.rb +++ b/lib/rubocop/cop/thread_safety/dir_chdir.rb @@ -4,6 +4,8 @@ module RuboCop module Cop module ThreadSafety # Avoid using `Dir.chdir` due to its process-wide effect. + # If `AllowCallWithBlock` (disabled by default) option is enabled, + # calling `Dir.chdir` with block will be allowed. # # @example # # bad @@ -11,6 +13,19 @@ module ThreadSafety # # # bad # FileUtils.chdir("/var/run") + # + # @example AllowCallWithBlock: false (default) + # # good + # Dir.chdir("/var/run") do + # puts Dir.pwd + # end + # + # @example AllowCallWithBlock: true + # # bad + # Dir.chdir("/var/run") do + # puts Dir.pwd + # end + # class DirChdir < Base MESSAGE = 'Avoid using `%s.%s` due to its process-wide effect.' RESTRICT_ON_SEND = %i[chdir cd].freeze @@ -24,12 +39,19 @@ class DirChdir < Base MATCHER def on_send(node) - chdir?(node) do - add_offense( - node, - message: format(MESSAGE, module: node.receiver.short_name, method: node.method_name) - ) - end + return unless chdir?(node) + return if allow_call_with_block? && (node.block_argument? || node.parent&.block_type?) + + add_offense( + node, + message: format(MESSAGE, module: node.receiver.short_name, method: node.method_name) + ) + end + + private + + def allow_call_with_block? + !!cop_config['AllowCallWithBlock'] end end end diff --git a/spec/rubocop/cop/thread_safety/dir_chdir_spec.rb b/spec/rubocop/cop/thread_safety/dir_chdir_spec.rb index c916931..bb734bb 100644 --- a/spec/rubocop/cop/thread_safety/dir_chdir_spec.rb +++ b/spec/rubocop/cop/thread_safety/dir_chdir_spec.rb @@ -24,6 +24,33 @@ ^^^^^^^^^^^^^^^^^^^^^^^ #{msg} RUBY end + + it 'registers an offense with provided block' do + expect_offense(<<~RUBY) + Dir.chdir("/var/run") do + ^^^^^^^^^^^^^^^^^^^^^ #{msg} + puts Dir.pwd + end + RUBY + end + + it 'registers an offense with provided block with argument' do + expect_offense(<<~RUBY) + Dir.chdir("/var/run") do |dir| + ^^^^^^^^^^^^^^^^^^^^^ #{msg} + puts dir + end + RUBY + end + + it 'registers an offense with provided block argument' do + expect_offense(<<~RUBY) + def change_dir(&block) + Dir.chdir("/var/run", &block) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg} + end + RUBY + end end context 'with `FileUtils.chdir` method' do @@ -54,9 +81,46 @@ end end - context 'when received is not `Dir`' do + context 'when receiver is not `Dir`' do it 'does not register an offense' do expect_no_offenses 'chdir("/tmp")' end end + + context 'with `AllowCallWithBlock` config set to `true`' do + let(:cop_config) do + { 'Enabled' => true, 'AllowCallWithBlock' => true } + end + + it 'registers an offense' do + expect_offense(<<~RUBY) + Dir.chdir("/var/run") + ^^^^^^^^^^^^^^^^^^^^^ #{msg} + RUBY + end + + it 'does not register an offense with provided block' do + expect_no_offenses(<<~RUBY) + Dir.chdir("/var/run") do + puts Dir.pwd + end + RUBY + end + + it 'does not register an offense with provided block with argument' do + expect_no_offenses(<<~RUBY) + Dir.chdir("/var/run") do |dir| + puts dir + end + RUBY + end + + it 'does not register an offense with provided block argument' do + expect_no_offenses(<<~RUBY) + def change_dir(&block) + Dir.chdir("/var/run", &block) + end + RUBY + end + end end