Skip to content

Commit

Permalink
Handle ivars with thread-safe semantics
Browse files Browse the repository at this point in the history
  • Loading branch information
viralpraxis committed Sep 24, 2024
1 parent 480c556 commit 1b7ca2c
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 3 deletions.
2 changes: 2 additions & 0 deletions lib/rubocop-thread_safety.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

RuboCop::ThreadSafety::Inject.defaults!

require 'rubocop/cop/mixin/operation_with_threadsafe_result'

require 'rubocop/cop/thread_safety/instance_variable_in_class_method'
require 'rubocop/cop/thread_safety/class_and_module_attributes'
require 'rubocop/cop/thread_safety/mutable_class_instance_variable'
Expand Down
44 changes: 44 additions & 0 deletions lib/rubocop/cop/mixin/operation_with_threadsafe_result.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# frozen_string_literal: true

module RuboCop
module Cop
# Common functionality for checking if a well-known operation
# produces an object with thread-safe semantics.
module OperationWithThreadsafeResult
extend NodePattern::Macros

# @!method operation_produces_threadsafe_object?(node)
def_node_matcher :operation_produces_threadsafe_object?, <<~PATTERN
{
(send (const {nil? cbase} :Queue) :new ...)
(send
(const (const {nil? cbase} :ThreadSafe) {:Hash :Array})
:new ...)
(block
(send
(const (const {nil? cbase} :ThreadSafe) {:Hash :Array})
:new ...)
...)
(send (const (const {nil? cbase} :Concurrent) _) :new ...)
(block
(send (const (const {nil? cbase} :Concurrent) _) :new ...)
...)
(send (const (const (const {nil? cbase} :Concurrent) _) _) :new ...)
(block
(send
(const (const (const {nil? cbase} :Concurrent) _) _)
:new ...)
...)
(send
(const (const (const (const {nil? cbase} :Concurrent) _) _) _)
:new ...)
(block
(send
(const (const (const (const {nil? cbase} :Concurrent) _) _) _)
:new ...)
...)
}
PATTERN
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,10 @@ module ThreadSafety
# end
class MutableClassInstanceVariable < Base
extend AutoCorrector

include FrozenStringLiteral
include ConfigurableEnforcedStyle
include OperationWithThreadsafeResult

MSG = 'Freeze mutable objects assigned to class instance variables.'
FROZEN_STRING_LITERAL_TYPES_RUBY27 = %i[str dstr].freeze
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ module ThreadSafety
# end
# end
class RackMiddlewareInstanceVariable < Base
include OperationWithThreadsafeResult

MSG = 'Avoid instance variables in Rack middleware.'

RESTRICT_ON_SEND = %i[instance_variable_get instance_variable_set].freeze
Expand All @@ -62,10 +64,12 @@ def on_class(node)
return unless rack_middleware_like_class?(node)
return unless (application_variable = extract_application_variable_from_class_node(node))

safe_variables = extract_safe_variables_from_class_node(node)

node.each_node(:def) do |method_definition_node|
method_definition_node.each_node(:ivasgn, :ivar) do |ivar_node|
assignable, = ivar_node.to_a
next if assignable == application_variable
next if assignable == application_variable || safe_variables.include?(assignable)

add_offense ivar_node
end
Expand All @@ -85,6 +89,18 @@ def extract_application_variable_from_class_node(class_node)
.then { |node| constructor_method(node) }
.then { |variables| variables.first[1] if variables.first }
end

def extract_safe_variables_from_class_node(class_node)
class_node
.each_node(:def)
.find { |node| node.method?(:initialize) && node.arguments.size == 1 }
.then do |node|
node
.each_node(:ivasgn)
.select { |ivasgn_node| operation_produces_threadsafe_object?(ivasgn_node.to_a[1]) }
.map { _1.to_a[0] }
end
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,17 +136,21 @@ def call(env)
RUBY
end

it 'does not register an offense with thread-safe wrappers', skip: :FIXME do
expect_no_offenses(<<~RUBY)
it 'registers an offense with thread-safe wrappers' do
expect_offense(<<~RUBY)
class TestMiddleware
def initialize(app)
@app = app
@counter = Concurrent::AtomicReference.new(0)
@unsafe_counter = 0
^^^^^^^^^^^^^^^^^^^ Avoid instance variables in Rack middleware.
end
def call(env)
@app.call(env)
ensure
@unsafe_counter += 1
^^^^^^^^^^^^^^^ Avoid instance variables in Rack middleware.
@counter.update { |ref| ref + 1 }
end
end
Expand Down

0 comments on commit 1b7ca2c

Please sign in to comment.