+ builder.rb: warn on duplicate hash key literals.

This commit tracks upstream commits ruby/ruby@37eb5e7 and ruby/ruby@9f3888d.
whitequark · Nov 19, 2021 · 984da87 · 984da87
1 parent 57bd371
commit 984da87
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 0 deletions.
diff --git a/lib/parser/builders/default.rb b/lib/parser/builders/default.rb
@@ -538,6 +538,26 @@ def kwsplat(dstar_t, arg)
     end
 
     def associate(begin_t, pairs, end_t)
+      0.upto(pairs.length - 1) do |i|
+        (i + 1).upto(pairs.length - 1) do |j|
+          key1, = *pairs[i]
+          key2, = *pairs[j]
+
+          # keys have to be simple nodes, MRI ignores equal composite keys like
+          # `{ a(1) => 1, a(1) => 1 }`
+          case key1.type
+          when :sym, :str, :int, :float
+            if key1 == key2
+              diagnostic :warning, :duplicate_hash_key, nil, key2.loc.expression
+            end
+          when :rational, :complex, :regexp
+            if @parser.version >= 31 && key1 == key2
+              diagnostic :warning, :duplicate_hash_key, nil, key2.loc.expression
+            end
+          end
+        end
+      end
+
       n(:hash, [ *pairs ],
         collection_map(begin_t, pairs, end_t))
     end

diff --git a/lib/parser/messages.rb b/lib/parser/messages.rb
@@ -80,6 +80,7 @@ module Parser
 
     # Parser warnings
     :useless_else            => 'else without rescue is useless',
+    :duplicate_hash_key      => 'key is duplicated and overwritten',
 
     # Parser errors that are not Ruby errors
     :invalid_encoding        => 'literal contains escape sequences incompatible with UTF-8',

diff --git a/test/test_parser.rb b/test/test_parser.rb
@@ -10598,4 +10598,78 @@ def test_assignment_to_numparam_via_pattern_matching
       %q{                ~~ location},
       SINCE_2_7)
   end
+
+  def test_warn_on_duplicate_hash_key
+    # symbol
+    assert_diagnoses(
+      [:warning, :duplicate_hash_key],
+      %q{ { :foo => 1, :foo => 2 } },
+      %q{              ^^^^ location},
+      ALL_VERSIONS)
+
+    # string
+    assert_diagnoses(
+      [:warning, :duplicate_hash_key],
+      %q{ { "foo" => 1, "foo" => 2 } },
+      %q{               ^^^^^ location},
+      ALL_VERSIONS)
+
+    # small number
+    assert_diagnoses(
+      [:warning, :duplicate_hash_key],
+      %q{ { 1000 => 1, 1000 => 2 } },
+      %q{              ^^^^ location},
+      ALL_VERSIONS)
+
+    # float
+    assert_diagnoses(
+      [:warning, :duplicate_hash_key],
+      %q{ { 1.0 => 1, 1.0 => 2 } },
+      %q{             ^^^ location},
+      ALL_VERSIONS)
+
+    # bignum
+    assert_diagnoses(
+      [:warning, :duplicate_hash_key],
+      %q{ { 1_000_000_000_000_000_000 => 1, 1_000_000_000_000_000_000 => 2 } },
+      %q{                                   ^^^^^^^^^^^^^^^^^^^^^^^^^ location},
+      ALL_VERSIONS)
+
+    # rational (tRATIONAL exists starting from 2.7)
+    refute_diagnoses(%q{ { 1.0r => 1, 1.0r => 2 } },
+      SINCE_2_1 - SINCE_3_1)
+
+    assert_diagnoses(
+      [:warning, :duplicate_hash_key],
+      %q{ { 1.0r => 1, 1.0r => 2 } },
+      %q{              ~~~~ location},
+      SINCE_3_1)
+
+    # complex (tIMAGINARY exists starting from 2.7)
+    refute_diagnoses(%q{ { 1.0i => 1, 1.0i => 2 } },
+      SINCE_2_1 - SINCE_3_1)
+
+    assert_diagnoses(
+      [:warning, :duplicate_hash_key],
+      %q{ { 1.0i => 1, 1.0i => 2 } },
+      %q{              ~~~~ location},
+      SINCE_3_1)
+
+    # small float
+    assert_diagnoses(
+      [:warning, :duplicate_hash_key],
+      %q{ { 1.72723e-77 => 1, 1.72723e-77 => 2 } },
+      %q{                     ~~~~~~~~~~~ location},
+      ALL_VERSIONS)
+
+    # regexp
+    refute_diagnoses(%q{ { /foo/ => 1, /foo/ => 2 } },
+      ALL_VERSIONS - SINCE_3_1)
+
+    assert_diagnoses(
+      [:warning, :duplicate_hash_key],
+      %q{ { /foo/ => 1, /foo/ => 2 } },
+      %q{               ~~~~~ location},
+      SINCE_3_1)
+  end
 end