Project

General

Profile

Bug #12855

Inconsistent keys identity in compare_by_identity Hash when using literals

Added by Eregon (Benoit Daloze) about 3 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
ruby -v:
ruby 2.2.2p95 (2015-04-13 revision 50295) [x86_64-linux]
[ruby-core:77684]

Description

This seems a regression since 2.2.
I would guess it's due to some optimization for having a string literal between []=.
That optimization should not trigger for compare_by_identity hashes, so both cases below are consistent.

h = {}.compare_by_identity
h['pear'] = 1
h['pear'] = 2

p h.size # => 1
p h


h = {}.compare_by_identity
k1 = 'pear'
h[k1] = 1
k2 = 'pear'
h[k2] = 2

p h.size # => 2
p h

Related issues

Related to Ruby master - Bug #9382: [patch] add opt_aref_str and opt_aset_strClosed01/08/2014Actions

Associated revisions

Revision ac9f8145
Added by Eregon (Benoit Daloze) almost 3 years ago

fix optimization for hash aset/aref with fstring

Patch by Eric Wong [ruby-core:78797].
I don't like the idea of making insns.def any bigger to support
a corner case, and "test_hash_aref_fstring_identity" shows
how contrived this is.

[ruby-core:78783] [Bug #12855]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@57278 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 57278
Added by Eregon (Benoit Daloze) almost 3 years ago

fix optimization for hash aset/aref with fstring

Patch by Eric Wong [ruby-core:78797].
I don't like the idea of making insns.def any bigger to support
a corner case, and "test_hash_aref_fstring_identity" shows
how contrived this is.

[ruby-core:78783] [Bug #12855]

Revision 57278
Added by Eregon (Benoit Daloze) almost 3 years ago

fix optimization for hash aset/aref with fstring

Patch by Eric Wong [ruby-core:78797].
I don't like the idea of making insns.def any bigger to support
a corner case, and "test_hash_aref_fstring_identity" shows
how contrived this is.

[ruby-core:78783] [Bug #12855]

Revision 57278
Added by Eregon (Benoit Daloze) almost 3 years ago

fix optimization for hash aset/aref with fstring

Patch by Eric Wong [ruby-core:78797].
I don't like the idea of making insns.def any bigger to support
a corner case, and "test_hash_aref_fstring_identity" shows
how contrived this is.

[ruby-core:78783] [Bug #12855]

Revision 57279
Added by nobu (Nobuyoshi Nakada) almost 3 years ago

adjust indent [ci skip]

Revision fd89be25
Added by naruse (Yui NARUSE) over 2 years ago

merge revision(s) 57278,57279: [Backport #12855]

    fix optimization for hash aset/aref with fstring

    Patch by Eric Wong [ruby-core:78797].
    I don't like the idea of making insns.def any bigger to support
    a corner case, and "test_hash_aref_fstring_identity" shows
    how contrived this is.

    [ruby-core:78783] [Bug #12855]
    adjust indent [ci skip]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_4@57848 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 57848
Added by naruse (Yui NARUSE) over 2 years ago

merge revision(s) 57278,57279: [Backport #12855]

fix optimization for hash aset/aref with fstring

Patch by Eric Wong [ruby-core:78797].
I don't like the idea of making insns.def any bigger to support
a corner case, and "test_hash_aref_fstring_identity" shows
how contrived this is.

[ruby-core:78783] [Bug #12855]
adjust indent [ci skip]

Revision bdedc17f
Added by usa (Usaku NAKAMURA) over 2 years ago

merge revision(s) 57278,57279: [Backport #12855]

    fix optimization for hash aset/aref with fstring

    Patch by Eric Wong [ruby-core:78797].
    I don't like the idea of making insns.def any bigger to support
    a corner case, and "test_hash_aref_fstring_identity" shows
    how contrived this is.

    [ruby-core:78783] [Bug #12855]
    adjust indent [ci skip]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_2@58099 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 58099
Added by usa (Usaku NAKAMURA) over 2 years ago

merge revision(s) 57278,57279: [Backport #12855]

fix optimization for hash aset/aref with fstring

Patch by Eric Wong [ruby-core:78797].
I don't like the idea of making insns.def any bigger to support
a corner case, and "test_hash_aref_fstring_identity" shows
how contrived this is.

[ruby-core:78783] [Bug #12855]
adjust indent [ci skip]

Revision 0d399220
Added by nagachika (Tomoyuki Chikanaga) over 2 years ago

merge revision(s) 57278,57279: [Backport #12855]

    fix optimization for hash aset/aref with fstring

    Patch by Eric Wong [ruby-core:78797].
    I don't like the idea of making insns.def any bigger to support
    a corner case, and "test_hash_aref_fstring_identity" shows
    how contrived this is.

    [ruby-core:78783] [Bug #12855]
    adjust indent [ci skip]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_3@58166 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 58166
Added by nagachika (Tomoyuki Chikanaga) over 2 years ago

merge revision(s) 57278,57279: [Backport #12855]

fix optimization for hash aset/aref with fstring

Patch by Eric Wong [ruby-core:78797].
I don't like the idea of making insns.def any bigger to support
a corner case, and "test_hash_aref_fstring_identity" shows
how contrived this is.

[ruby-core:78783] [Bug #12855]
adjust indent [ci skip]

History

Updated by jeremyevans0 (Jeremy Evans) about 3 years ago

While this was a behavior change between 2.1 and 2.2, I'm not sure I would consider it a regression. It seems unlikely anyone who uses compare_by_identity hashes would also be using string literals and want string literals keys to have different values.

If we do decide to revert to 2.1 behavior for string literals, at the very least we should make sure that on ruby 2.1+:

h = {}.compare_by_identity
h['pear'.freeze] = 1
h['pear'.freeze] = 2

p h.size # => 1
# because 'pear'.freeze.equal?('pear'.freeze)

and on ruby 2.3+:

# frozen-string-literal: true
# or when using --enable-frozen-string-literal
h = {}.compare_by_identity
h['pear'] = 1
h['pear'] = 2

p h.size # => 1
# because 'pear'.equal?('pear')

Updated by Eregon (Benoit Daloze) about 3 years ago

Jeremy Evans wrote:

While this was a behavior change between 2.1 and 2.2, I'm not sure I would consider it a regression.
It seems unlikely anyone who uses compare_by_identity hashes would also be using string literals and want string literals keys to have different values.

The main reason I consider it a bug is that it contradicts the very basic intuition that replacing a literal with an expression producing it has identical behavior.
For example, "z = #{3*4}" vs r = 12; "z = #{r}".
While of course a much smaller area of exposition,
I think there is no reason to introduce this change (and I'm fairly confident it was not intended).

Updated by shyouhei (Shyouhei Urabe) almost 3 years ago

  • Assignee set to normalperson (Eric Wong)
  • Status changed from Open to Assigned
#4

Updated by shyouhei (Shyouhei Urabe) almost 3 years ago

  • Related to Bug #9382: [patch] add opt_aref_str and opt_aset_str added

Updated by shyouhei (Shyouhei Urabe) almost 3 years ago

  • Assignee changed from normalperson (Eric Wong) to tmm1 (Aman Gupta)

We looked at this issue at today's developer meeting. It seems over-optimization as Benoit originally guessed. Let me assign this to Aman.

Updated by normalperson (Eric Wong) almost 3 years ago

Unintentional, yes, but maybe moot since frozen_string_literal
is going to be default in the future (maybe that case changed?)

Anyways, I hate this patch because it makes insns.def bigger,
and insns.def is already too big IMHO:

https://80x24.org/spew/20161222004752.4086-1-e@80x24.org/raw

Somebody else can commit; I hate it too much.

Updated by Eregon (Benoit Daloze) almost 3 years ago

Eric Wong wrote:

Unintentional, yes, but maybe moot since frozen_string_literal
is going to be default in the future (maybe that case changed?)

Anyways, I hate this patch because it makes insns.def bigger,
and insns.def is already too big IMHO:

https://80x24.org/spew/20161222004752.4086-1-e@80x24.org/raw

Somebody else can commit; I hate it too much.

Thank you for the patch!
I would like to commit it then, unless there is an objection.

IMHO optimizations have to be correct, even in edge cases.
Such instructions are already a trade-off of duplication and complexity for speed improvement,
so this small change in size does not matter much to me.
Optimizations need to be totally transparent to the user to be called as such.

#8

Updated by Eregon (Benoit Daloze) almost 3 years ago

  • Status changed from Assigned to Closed

Applied in changeset r57278.


fix optimization for hash aset/aref with fstring

Patch by Eric Wong [ruby-core:78797].
I don't like the idea of making insns.def any bigger to support
a corner case, and "test_hash_aref_fstring_identity" shows
how contrived this is.

[ruby-core:78783] [Bug #12855]

Updated by naruse (Yui NARUSE) over 2 years ago

  • Backport changed from 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN to 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: DONE

ruby_2_4 r57848 merged revision(s) 57278,57279.

Updated by nagachika (Tomoyuki Chikanaga) over 2 years ago

  • Backport changed from 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: DONE to 2.1: UNKNOWN, 2.2: REQUIRED, 2.3: REQUIRED, 2.4: DONE

Updated by usa (Usaku NAKAMURA) over 2 years ago

  • Backport changed from 2.1: UNKNOWN, 2.2: REQUIRED, 2.3: REQUIRED, 2.4: DONE to 2.1: UNKNOWN, 2.2: DONE, 2.3: REQUIRED, 2.4: DONE

ruby_2_2 r58099 merged revision(s) 57278,57279.

Updated by nagachika (Tomoyuki Chikanaga) over 2 years ago

  • Backport changed from 2.1: UNKNOWN, 2.2: DONE, 2.3: REQUIRED, 2.4: DONE to 2.1: UNKNOWN, 2.2: DONE, 2.3: DONE, 2.4: DONE

ruby_2_3 r58166 merged revision(s) 57278,57279.

Also available in: Atom PDF