-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Closed
Labels
feature requestgood first issueEasy task, suitable for newcomers to the projectEasy task, suitable for newcomers to the project
Description
Is your feature request related to a problem? Please describe.
One of the more confusing parts of Ruby is the ability to create a default Hash value pointing at a reference, like:
Hash.new([])
Hash.new({})
Hash.new("") # vs frozenThis behavior is seldom if ever intended, and leads to unexpected results.
Describe the solution you'd like
I had managed to create at least a decent skeleton start for this:
# Code
module RuboCop
module Cop
module Style
class HashNewReferenceValues < Cop
MSG = 'Do not use reference values in Hash constructors.'
def_node_matcher :value_in_constructor, <<~PATTERN
(send (const nil? :Hash) :new $(...))
PATTERN
def_node_matcher :non_reference_value, <<~PATTERN
{
# Literals
(int _) (float _) false true nil
# BigDecimal explicit
(send nil? :BigDecimal ...)
# Coercions to literals
(send _ { :to_i :to_f :to_d })
}
PATTERN
def_node_matcher :frozen_reference, <<~PATTERN
(send $... :freeze)
PATTERN
def on_send(node)
constructor_value = value_in_constructor(node)
return unless constructor_value
return if non_reference_value(constructor_value)
add_offense(node)
end
def extract_constructor_source(node)
constructor_value = value_in_constructor(node)
frozen_ref = frozen_reference(constructor_value)
return extract_frozen_source(frozen_ref) if frozen_ref
case constructor_value
when :array
"[]"
when :hash
"{}"
when RuboCop::AST::Node
constructor_value.source
else
raise ArgumentError, "Undefined case encountered"
end
end
def extract_frozen_source(node_collection)
node_collection.first.source
end
def autocorrect(node)
-> (corrector) do
ref_source = extract_constructor_source(node)
corrector.replace(node, "Hash.new { |h, k| h[k] = #{ref_source} }")
end
end
end
end
end
end
# Spec
RSpec.describe RuboCop::Cop::Style::HashNewReferenceValues, :config do
let(:cop) { described_class.new(config) }
[
"Hash.new(0)",
"Hash.new(false)",
"Hash.new(true)",
"Hash.new(nil)",
"Hash.new(BigDecimal('0.0'))",
"Hash.new(0.0)",
"Hash.new(0.0.to_d)",
"Hash.new(BigDecimal(0))",
"Hash.new { |h, k| h[k] = [] }",
].each do |good_source|
it "handles the literal value #{good_source}" do
expect_no_offenses(good_source)
end
end
it 'flags Array reference values' do
expect_offense(<<~RUBY)
Hash.new([])
^^^^^^^^^^^^ Do not use reference values in Hash constructors.
RUBY
expect_correction(<<~RUBY)
Hash.new { |h, k| h[k] = [] }
RUBY
end
it 'flags Hash reference values' do
expect_offense(<<~RUBY)
Hash.new({})
^^^^^^^^^^^^ Do not use reference values in Hash constructors.
RUBY
expect_correction(<<~RUBY)
Hash.new { |h, k| h[k] = {} }
RUBY
end
it 'handles frozen references' do
expect_offense(<<~RUBY)
Hash.new([].freeze)
^^^^^^^^^^^^^^^^^^^ Do not use reference values in Hash constructors.
RUBY
expect_correction(<<~RUBY)
Hash.new { |h, k| h[k] = [] }
RUBY
end
[
"Hash.new('some_string')",
"Hash.new(CONSTANT)",
].each do |bad_source|
it "handles additional bad sources like #{bad_source}" do
expect_offense(<<~RUBY)
#{bad_source}
#{'^' * bad_source.size} Do not use reference values in Hash constructors.
RUBY
extracted_source = cop.extract_constructor_source(RuboCop::ProcessedSource.new(bad_source, RUBY_VERSION.to_f).ast)
expect_correction(<<~RUBY)
Hash.new { |h, k| h[k] = #{extracted_source} }
RUBY
end
end
end...but wanted to run it by all of you first. It's not comprehensive, but it does serve as a good start for conversation.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
feature requestgood first issueEasy task, suitable for newcomers to the projectEasy task, suitable for newcomers to the project