Feature #6130: inspect using to_s is pain

• Status: Closed
• Assignee: mame (Yusuke Endoh)
• Target version: 2.0.0

[trans (Thomas Sawyer)]

Every time I define #to_s on a class I have to remember to also redefine #inspect. It's annoying and has led me to waste hours debugging b/c the errors that can occur from it often seem strange and unrelated.

I think #inspect should have an independent definition that outputs the class name, object_id and when possible instance variable settings, and should have nothing to do with #to_s by default. We developers can always alias it as such if it is appropriate.

The only exception should be for classes that have literal representation in Ruby, such as Array, String, Hash, etc. In those cases the #inspect should give the literal representation.

[mame (Yusuke Endoh)]

• Status changed from Open to Assigned
• Assignee set to matz (Yukihiro Matsumoto)

Thomas, you meant:

class Foo
def to_s; "foo"; end
end
x = Foo.new

p "== #{ x } ==" #=> "== foo ==", as you want

p x #=> "foo", but you want: #Foo:0xXXXX

, right? If so I agree, though it may be difficult in 2.0.

--
Yusuke Endoh [e-mail]

[trans (Thomas Sawyer)]

Yep.

[Eregon (Benoit Daloze)]

mame (Yusuke Endoh) wrote:

If so I agree, though it may be difficult in 2.0.

I also agree, default #inspect is nice and should not be overridden by #to_s.

If not possible for 2.0, I think it should still be considered later.

[matz (Yukihiro Matsumoto)]

OK, I accept. The time for it would be upto the maintainer.

Matz.

[mame (Yusuke Endoh)]

• Assignee changed from matz (Yukihiro Matsumoto) to mame (Yusuke Endoh)

That's great to hear!

As 2.0 release manager, I'm positive to import this change in 2.0.

I think it does not violate 2.0 compatibility policy; the format
of a return string of #inspect is not specially specified. This
changes only the format.

However, it should be postponed to 3.0 if it affects any actual
existing products. Please let me if you know such a case.

I'd like to hear your opinion,

--
Yusuke Endoh [e-mail]

[mame (Yusuke Endoh)]

Please let me if you know such a case.
Oops. Please let me know such a case if you know.

[mame (Yusuke Endoh)]

I'm not sure if it is trivial to fix this issue.
Could anyone please create a patch and study the behavior?

--
Yusuke Endoh [e-mail]

[Eregon (Benoit Daloze)]

mame (Yusuke Endoh) wrote:

I'm not sure if it is trivial to fix this issue.
Could anyone please create a patch and study the behavior?

I've been working on this and wondered what to do if there is no instance variable. Should it call (dynamically) #to_s? or always use Kernel#to_s which gives the class name and pointer (which would be called anyway if #to_s was not overridden) ?

That's the first solution, use the nice #inspect output whenever there are instance variables.
Another, lighter way would be to use #to_s for internal structures (and T_DATA, so classes defined in C extensions), but never for other types (all classes defined in Ruby).

What do you think?

[Eregon (Benoit Daloze)]

• File 0001-object.c-rb_obj_inspect-Kernel-inspect-improve-consi.patch 0001-object.c-rb_obj_inspect-Kernel-inspect-improve-consi.patch added

Eregon (Benoit Daloze) wrote:

I've been working on this and wondered what to do if there is no instance variable. Should it call (dynamically) #to_s? or always use Kernel#to_s which gives the class name and pointer (which would be called anyway if #to_s was not overridden) ?

After some thoughts I believe it would not be worth changing the behavior if it was not fully consistent.
So I propose this patch.

test-all pass with only two modifications in pretty_print tests, which relied on #inspect calling #to_s.
The internal structures have been updated so #inspect is an alias of #to_s when it was not defined.
The last modification is to define #inspect on the toplevel self (only #to_s was defined).

Of course, tests for the new behavior need to be added (I'll do later if the idea is approved).

What do you think?

[mame (Yusuke Endoh)]

Benoit, thank you!

After some thoughts I believe it would not be worth changing the behavior if it was not fully consistent.

Agreed.

So I propose this patch.

I'll review. Please wait!

test-all pass with only two modifications in pretty_print tests, which relied on #inspect calling #to_s.

Good point. I was missing pretty_print.
It must be also changed. Akr, do you have time?

--
Yusuke Endoh [e-mail]

[mame (Yusuke Endoh)]

Hello,

So I propose this patch.

I'll review. Please wait!

Looks good to me. I'll commit it, except the change of test/test_pp.rb. Thanks!
(It surprised me that your patch also passes rubyspec; rubyspec has no test for this behavior?)

Tanaka-san, I made a patch for pp. Unless there is no objection, I'll commit this too.

diff --git a/lib/pp.rb b/lib/pp.rb
index 94269ab..6e0c797 100644
--- a/lib/pp.rb
+++ b/lib/pp.rb
@@ -265,8 +265,7 @@ class PP < PrettyPrint
module ObjectMixin
# 1. specific pretty_print
# 2. specific inspect

• 3. specific to_s

• 4. generic pretty_print

• 3. generic pretty_print

  A default pretty printing method for general objects.

  It calls #pretty_print_instance_variables to list instance variables.

@@ -283,18 +282,10 @@ class PP < PrettyPrint
inspect_method = method_method.call(:inspect)
rescue NameError
end

•  begin
  

•    to_s_method = method_method.call(:to_s)
  

•  rescue NameError
  

•  end
   if inspect_method && /\(Kernel\)#/ !~ inspect_method.inspect
     q.text self.inspect
   elsif !inspect_method && self.respond_to?(:inspect)
     q.text self.inspect
  

•  elsif to_s_method && /\(Kernel\)#/ !~ to_s_method.inspect
  

•    q.text self.to_s
  

•  elsif !to_s_method && self.respond_to?(:to_s)
  

•    q.text self.to_s
   else
     q.pp_object(self)
   end
  

diff --git a/test/test_pp.rb b/test/test_pp.rb
index fe65287..acd3e83 100644
--- a/test/test_pp.rb
+++ b/test/test_pp.rb
@@ -118,7 +118,6 @@ class PPInspectTest < Test::Unit::TestCase
def a.to_s() "aaa" end
result = PP.pp(a, '')
assert_equal("#{a.inspect}\n", result)

• assert_equal("aaa\n", result)
  end
  end

--
Yusuke Endoh [e-mail]

[mame (Yusuke Endoh)]

Oh, yeah, Benoit. I'm happy if you write tests for the new behavior :-)
Do you have a commit bit already?

--
Yusuke Endoh [e-mail]

[Eregon (Benoit Daloze)]

Hello,
mame (Yusuke Endoh) wrote:

Looks good to me. I'll commit it, except the change of test/test_pp.rb. Thanks!

Thank you for reviewing!

Oh, yeah, Benoit. I'm happy if you write tests for the new behavior :-)
Do you have a commit bit already?

No, I don't.

I'll write these tests ASAP.

(It surprised me that your patch also passes rubyspec; rubyspec has no test for this behavior?)

I'd be happy to add some tests to rubyspec too.

[Eregon (Benoit Daloze)]

I started writing the tests:
https://github.com/eregon/ruby/commit/f9ac4cd1daadd7c37c722c79a36296f3d161058b

I'll improve them tomorrow and write the ones for rubyspec too.

[Eregon (Benoit Daloze)]

• File 0002-test-ruby-test_object.rb-test_inspect-add-tests-for-.patch 0002-test-ruby-test_object.rb-test_inspect-add-tests-for-.patch added

I attach the updated tests (sorry about the delay :/).
Please comment is something is wrong.

After speaking with Yusuke, it seems best to not include a (complete) specification for #inspect in RubySpec yet, because the exact #inspect format might be an implementation detail.

However I'd like to add the behavior change (not calling #to_s anymore) to RubySpec after this has been merged.

[drbrain (Eric Hodel)]

If there are no objections I will commit this Monday (unless Benoit gets a commit bit sooner)

[Eregon (Benoit Daloze)]

• File 0002-test-ruby-test_object.rb-test_inspect-add-tests-for-.patch 0002-test-ruby-test_object.rb-test_inspect-add-tests-for-.patch added

Updated patch for the tests according to nobu's comment in https://github.com/eregon/ruby/commit/f9ac4cd1daadd7c37c722c79a36296f3d161058b#commitcomment-1490653.

[mame (Yusuke Endoh)]

I suggested giving Benoit a commit bit at the developer meeting (7/21),
and matz agreed.

Benoit, could you send your PGP public key to cvs-admin AT ruby-lang.org?
See the following procedure in detail:

http://bugs.ruby-lang.org/projects/ruby/wiki/CommitterHowto#What-to-do-for-registering-you-as-a-committer

Then, please commit your patch yourself.

I'd like to thank you for your continuing contribution!

--
Yusuke Endoh [e-mail]

[Eregon (Benoit Daloze)]

• Status changed from Assigned to Closed
• % Done changed from 0 to 100

This issue was solved with changeset r36699.
Thomas, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

Kernel#inspect: improve consistency and do not call #to_s.

A class can now benefit from the nice default #inspect even if it
defines #to_s. Also, there is no more unexpected change in #inspect
result. Internal structures have been adapted so they don't rely
on the removed behavior (#inspect calling overridden #to_s).

• object.c (rb_obj_inspect): Kernel#inspect: do not call #to_s.
• test/ruby/test_object.rb (test_inspect): add tests for Kernel#inspect.
• bignum.c, io.c, numeric.c, object.c, proc.c, vm.c (Init_*):
  alias #inspect to #to_s where it was expected.
  [ruby-core:43238][Feature #6130]

[Eregon (Benoit Daloze)]

Hi,
mame (Yusuke Endoh) wrote:

I suggested giving Benoit a commit bit at the developer meeting (7/21),
and matz agreed.
[...]
Then, please commit your patch yourself.

I'd like to thank you for your continuing contribution!

I just committed as r36699 and I committed your PP patch as r36700.

Thank you for proposing it!