Skip to content

Prevent Hash.new with reference values like Hash.new([]) #11013

@baweaver

Description

@baweaver

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 frozen

This 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions