Project

General

Profile

Actions

Bug #20433

closed

Hash.inspect for some hash returns syntax invalid representation

Added by tompng (tomoya ishida) 6 months ago. Updated 7 days ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:117564]

Description

For these hashes, Hash.inspect returns a syntax invalid representation:

{ :a! => 1 } # {:a!=>1}
{ :a? => 1 } # {:a?=>1}
{ :* => 1 }  # {:*=>1}
{ :== => 1 } # {:===>1}
{ :< => 1 }  # {:<=>1}

eval(hash.inspect) will raise SyntaxError.
Although inspect does not guarantee that the result can be eval-ed, it'd be confusing for these few cases.

Maybe related to https://bugs.ruby-lang.org/issues/20235

Updated by byroot (Jean Boussier) 6 months ago

An easy fix would be for Hash#inspect to include spaces around =>. It would also read much nicer in my opinion.

Updated by nobu (Nobuyoshi Nakada) 6 months ago

Or use {a!: 1} for symbol keys?

Updated by nobu (Nobuyoshi Nakada) 6 months ago

Ditto for pp.rb.

Updated by jeremyevans0 (Jeremy Evans) 5 months ago

I submitted a pull request for the most backwards compatible change, which only uses spaces around => for symbols ending in [!?*=<]: https://github.com/ruby/ruby/pull/10737

Updated by Eregon (Benoit Daloze) 5 months ago

I think always having spaces would help readability.
If we don't want to change that, then how about simply quoting these symbols that need it?

Updated by Dan0042 (Daniel DeLorme) 5 months ago

If we don't want to change that, then how about simply quoting these symbols that need it?

+1

Updated by mame (Yusuke Endoh) 4 months ago

Discussed at the dev meeting.

In conclusion, @matz (Yukihiro Matsumoto) wanted to change the return value of Hash#inspect significantly and estimate its compatibility impact:

{ :key => 42 }  #=> {key: 42}     # if the key is a symbol
{ :== => 42 }   #=> {"==": 42}    # if the key is a symbol and quotes are needed
{ "str" => 42 } #=> {"str" => 42} # otherwise (note that `=>` is surrounded by spaces)

# when keys are mixed
{ :== => 1, :key => 2, "str" => 3 } # {"==": 1, key: 2, "str" => 3}

Actually, some solutions were discussed.

Solution 1: insert a space before => only when needed.

{ :key => 1 } # {:key=>1}
{ :== => 1 }  # {:== =>1}
{ :a! => 1 }  # {:a! =>1}

Solution 2: insert spaces before and after => consistently

{ :key => 1 } # {:key => 1}
{ :== => 1 }  # {:== => 1}
{ :a! => 1 }  # {:a! => 1}

Solution 3: quote keys only when needed

{ :== => 1 }  # {:"=="=>1}
{ :a! => 1 }  # {:"a!"=>1}
{ :key => 1 } # {:key=>1}

Matz said he likes Solution 2. However, this has a compatibility issue. It may break some existing test assertions.

Matz had had the idea of denoting symbol keys by a colon in Hash#inspect. And the incompatibility impacts of Solution 2 and of symbol keys by colons are expected to be about the same. Rather than introducing the incompatibilities in two steps, Matz said he wants to change it at once.

Updated by tompng (tomoya ishida) 4 months ago

I created a proof of concept patch for colon style inspect https://github.com/ruby/ruby/pull/10924

Updated by tompng (tomoya ishida) 4 months ago

I investigated the impact to ruby ci.

Need to update these tests. The required change was within the expected range.

bootstraptest
test/coverage
test/error_highlight
test/rdoc
test/reline
test/ruby
test/rubygems
spec/bundler
spec/ruby

Two bundled gem tests failed.

minitest (4 failure)
debug (2 failure)

All of them passes by only changing the expected string/regexp from key=>value to key => value or :key=>value to key: value.

Test against several ruby versions

Test of mspec, default gems and bundled gems needs to pass in several ruby versions.
In most case, changing expected = "message {1=>2}" to expected = "message #{{1=>2}.inspect}" was enough.

There are some case that needs a little more effort like this.

# spec/ruby/core/string/modulo_spec.rb
{ a: obj }.send(@method).should =~ /^\{:a=>#<MockObject:0x[0-9a-f]+>\}$/

# spec/ruby/library/net-http/http/send_request_spec.rb
response.body.should include('"Referer"=>"' + referer + '"')

# minitst/minitest test/minitest/test_minitest_mock.rb
exp = "expected foo(:kw=>false) => nil, got [foo(:kw=>true) => nil]"

@matz (Yukihiro Matsumoto) For your information

Updated by matz (Yukihiro Matsumoto) 3 months ago

IMO, this is a good chance to upgrade Hash#inspect format to adopt current Hash usage.

  • symbol keys should be represented by "key:"
  • ambiguous keys should be wrapped by (double) quotation marks
  • spaces should be placed after ":" and around "=>".

I'd propose that we try new format for release candidates. If serious compatibility issues happen (although I don't believe we will see), we re-consider this change again.

Matz.

Actions #11

Updated by zverok (Victor Shepelev) 3 months ago

ambiguous keys should be wrapped by (double) quotation marks

BTW, if the change is so radical, can we also (start to) adopt Python’s flexible quotation marks? (For now, only for Hash keys, but once, maybe, for strings):

print("test 'quotes'".__repr__())
#=> "test 'quotes'"

print('test "quotes"'.__repr__())
#=> 'test "quotes"'

I.e. make it so for Hash keys:

{"'": true, '"': false}.inspect
#=> {"'": true, '"': false}

Updated by vo.x (Vit Ondruch) 3 months ago

matz (Yukihiro Matsumoto) wrote in #note-10:

IMO, this is a good chance to upgrade Hash#inspect format to adopt current Hash usage.

  • symbol keys should be represented by "key:"

I wish we stayed with the hash rockets, sigh

Updated by byroot (Jean Boussier) 9 days ago

@tompng (tomoya ishida) @mame (Yusuke Endoh) what is the status on this? I see https://github.com/ruby/ruby/pull/10924 has been merged, should this ticket be closed?

I personally love this change, but I have a lot of tests to update as a result, so I'd like to know if this is certain this change will be part of 3.4.0 before I start the work of fixing it all.

Updated by mame (Yusuke Endoh) 9 days ago

  • Status changed from Open to Closed

Yes, I have merged it since @matz (Yukihiro Matsumoto) said at the dev meeting today that he still wants to try this change in the next preview release. So closing.

However, it may be reverted if the impact of the incompatibility is too large.

Updated by byroot (Jean Boussier) 7 days ago

To provide some datapoint, here's the impact on Rails test suite: https://github.com/rails/rails/pull/53202

I still love the change, and it really wasn't much work to fix, but I fear we may hear some complaints.

Actions

Also available in: Atom PDF

Like0
Like1Like0Like0Like0Like0Like0Like0Like0Like0Like1Like0Like0Like0Like0Like1