Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix offenses for InternalAffairs/OnSendWithoutOnCSend cop #76

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## master

- [#76](https://github.com/rubocop/rubocop-thread_safety/pull/76): Detect offenses when using safe navigation for `ThreadSafety/DirChdir`, `ThreadSafety/NewThread` and `ThreadSafety/RackMiddlewareInstanceVariable` cops. ([@viralpraxis](https://github.com/viralpraxis))
- [#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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ class ClassAndModuleAttributes < Base
...)
MATCHER

def on_send(node)
def on_send(node) # rubocop:disable InternalAffairs/OnSendWithoutOnCSend
return unless mattr?(node) || (!class_attribute_allowed? && class_attr?(node)) || singleton_attr?(node)

add_offense(node)
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/cop/thread_safety/class_instance_variable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def on_ivar(node)
end
alias on_ivasgn on_ivar

def on_send(node)
def on_send(node) # rubocop:disable InternalAffairs/OnSendWithoutOnCSend
return unless instance_variable_call?(node)
return unless class_method_definition?(node)
return if method_definition?(node)
Expand Down
14 changes: 10 additions & 4 deletions lib/rubocop/cop/thread_safety/dir_chdir.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ module ThreadSafety
# end
#
class DirChdir < Base
MESSAGE = 'Avoid using `%<module>s.%<method>s` due to its process-wide effect.'
MESSAGE = 'Avoid using `%<module>s%<dot>s%<method>s` due to its process-wide effect.'
RESTRICT_ON_SEND = %i[chdir cd].freeze

# @!method chdir?(node)
def_node_matcher :chdir?, <<~MATCHER
{
(send (const {nil? cbase} {:Dir :FileUtils}) :chdir ...)
(send (const {nil? cbase} :FileUtils) :cd ...)
({send csend} (const {nil? cbase} {:Dir :FileUtils}) :chdir ...)
({send csend} (const {nil? cbase} :FileUtils) :cd ...)
}
MATCHER

Expand All @@ -44,9 +44,15 @@ def on_send(node)

add_offense(
node,
message: format(MESSAGE, module: node.receiver.short_name, method: node.method_name)
message: format(
MESSAGE,
module: node.receiver.short_name,
method: node.method_name,
dot: node.loc.dot.source
)
)
end
alias on_csend on_send

private

Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/thread_safety/new_thread.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ class NewThread < Base

# @!method new_thread?(node)
def_node_matcher :new_thread?, <<~MATCHER
(send (const {nil? cbase} :Thread) {:new :fork :start} ...)
({send csend} (const {nil? cbase} :Thread) {:new :fork :start} ...)
MATCHER

def on_send(node)
new_thread?(node) { add_offense(node) }
end
alias on_csend on_send
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ def on_send(node)

add_offense node
end
alias on_csend on_send

private

Expand Down
2 changes: 1 addition & 1 deletion spec/rubocop/cop/thread_safety/dir_chdir_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::ThreadSafety::DirChdir, :config do
%w[Dir.chdir FileUtils.chdir FileUtils.cd].each do |expression|
%w[Dir.chdir Dir&.chdir FileUtils.chdir FileUtils.cd].each do |expression|
context "with `#{expression}` call" do
it 'registers an offense' do
expect_offense(<<~RUBY, expression: expression)
Expand Down
7 changes: 7 additions & 0 deletions spec/rubocop/cop/thread_safety/new_thread_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@
RUBY
end

it 'registers an offense for starting a new thread with safe navigation' do
expect_offense(<<~RUBY)
Thread&.new { do_work }
^^^^^^^^^^^ #{msg}
Comment on lines +50 to +51
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this construct does not make much sense in practice. But I thought that since technically this is still an issue to detect (Thread.new is the same as Thread&.new in runtime) it's easier to just do it

RUBY
end

it 'does not register an offense for calling new on other classes' do
expect_no_offenses('Other.new { do_work }')
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,25 @@ def call(env)
RUBY
end

it 'registers an offense with safe navigation' do
expect_offense(<<~RUBY)
class TestMiddleware
def initialize(app)
@app = app
foo = SomeClass.new
instance_variable_set(:counter, 1)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg}
end

def call(env)
@app.call(env)
instance_variable_get("@counter")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg}
end
end
RUBY
end

it 'registers no offenses' do
expect_no_offenses(<<~RUBY)
class TestMiddleware
Expand Down
Loading