Feature #6130

inspect using to_s is pain

Added by Thomas Sawyer about 2 years ago. Updated over 1 year ago.

[ruby-core:43238]
Status:Closed
Priority:Normal
Assignee:Yusuke Endoh
Category:core
Target version:2.0.0

Description

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, objectid and when possible instance variable settings, and should have nothing to do with #tos 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.

0001-object.c-rb_obj_inspect-Kernel-inspect-improve-consi.patch Magnifier (7.48 KB) Benoit Daloze, 04/11/2012 07:56 AM

0002-test-ruby-test_object.rb-test_inspect-add-tests-for-.patch Magnifier (1.44 KB) Benoit Daloze, 05/26/2012 10:29 PM

0002-test-ruby-test_object.rb-test_inspect-add-tests-for-.patch Magnifier (1.44 KB) Benoit Daloze, 06/22/2012 07:36 PM


Related issues

Duplicates ruby-trunk - Bug #4453: Overriding #to_s changes #inspect Closed 03/01/2011

Associated revisions

Revision 36699
Added by Benoit Daloze over 1 year ago

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

A class can now benefit from the nice default #inspect even if it
defines #tos. 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 (rbobjinspect): Kernel#inspect: do not call #to_s.
  • test/ruby/testobject.rb (testinspect): add tests for Kernel#inspect.
  • bignum.c, io.c, numeric.c, object.c, proc.c, vm.c (Init*): alias #inspect to #tos where it was expected. [Feature #6130]

Revision 36700
Added by Benoit Daloze over 1 year ago

update PP with recent Kernel#inspect change. Patch by Yusuke Endoh.

  • lib/pp.rb (class PP): do not call #to_s anymore, as #inspect no more does.
  • test/test_pp.rb (class PPInspectTest): remove related assertion. [Feature #6130]

Revision 36701
Added by Yui NARUSE over 1 year ago

Revert r36699 and r36700. [Feature #6130]

Revert "Kernel#inspect: improve consistency and do not call #to_s."
Revert "update PP with recent Kernel#inspect change. Patch by Yusuke Endoh."

r36699 cause test-all failure on test/drb/testdrb.rb and
test/drb/test
drbssl.rb. Run test-all before commit.

Moreover its ChangeLog formst is wrong: see CommitterHowto
https://bugs.ruby-lang.org/projects/ruby/wiki/CommitterHowto#ChangeLog

Revision 36709
Added by Benoit Daloze over 1 year ago

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

  • object.c (rbobjinspect): Kernel#inspect: do not call #tos. A class can now benefit from the nice default #inspect even if it defines #tos. Also, there is no more unexpected change in #inspect result.
  • NEWS: Add note about the change.
  • bignum.c, io.c, numeric.c, object.c, proc.c, vm.c (Init*): Adapt internal structures (by aliasing #inspect to #tos) so they don't rely on the removed behavior (#inspect calling overridden #to_s).
  • test/ruby/testobject.rb (testinspect): add tests for Kernel#inspect.
  • lib/pp.rb (class PP): do not call #to_s anymore, as #inspect no more does (mame).
  • test/test_pp.rb (class PPInspectTest): remove related assertion (mame). [Feature #6130]
  • test/drb/drbtest.rb (DRbCore#teardown, DRbAry#teardown): adapt DRb tests with the new change (shirosaki). [Bug #6866]

History

#1 Updated by Yusuke Endoh about 2 years ago

  • Status changed from Open to Assigned
  • Assignee set to 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 mame@tsg.ne.jp

#2 Updated by Thomas Sawyer about 2 years ago

Yep.

#3 Updated by Benoit Daloze about 2 years ago

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.

#4 Updated by Yukihiro Matsumoto about 2 years ago

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

Matz.

#5 Updated by Yusuke Endoh about 2 years ago

  • Assignee changed from Yukihiro Matsumoto to 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 mame@tsg.ne.jp

#6 Updated by Yusuke Endoh about 2 years ago

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

#7 Updated by Yusuke Endoh about 2 years ago

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

Yusuke Endoh mame@tsg.ne.jp

#8 Updated by Benoit Daloze about 2 years ago

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) #tos? or always use Kernel#tos 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 #tos for internal structures (and TDATA, so classes defined in C extensions), but never for other types (all classes defined in Ruby).

What do you think?

#9 Updated by Benoit Daloze about 2 years ago

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) #tos? or always use Kernel#tos 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 prettyprint tests, which relied on #inspect calling #tos.
The internal structures have been updated so #inspect is an alias of #tos 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?

#10 Updated by Yusuke Endoh about 2 years ago

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 prettyprint tests, which relied on #inspect calling #tos.

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

Yusuke Endoh mame@tsg.ne.jp

#11 Updated by Yusuke Endoh about 2 years ago

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 prettyprint
# 2. specific inspect
- # 3. specific to
s
- # 4. generic prettyprint
+ # 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
inspectmethod = methodmethod.call(:inspect)
rescue NameError
end
- begin
- tosmethod = methodmethod.call(:tos)
- rescue NameError
- end
if inspectmethod && /(Kernel)#/ !~ inspectmethod.inspect
q.text self.inspect
elsif !inspectmethod && self.respondto?(:inspect)
q.text self.inspect
- elsif tosmethod && /(Kernel)#/ !~ tosmethod.inspect
- q.text self.tos
- elsif !to
smethod && self.respondto?(:tos)
- q.text self.to
s
else
q.ppobject(self)
end
diff --git a/test/test
pp.rb b/test/testpp.rb
index fe65287..acd3e83 100644
--- a/test/test
pp.rb
+++ b/test/testpp.rb
@@ -118,7 +118,6 @@ class PPInspectTest < Test::Unit::TestCase
def a.to
s() "aaa" end
result = PP.pp(a, '')
assertequal("#{a.inspect}\n", result)
- assert
equal("aaa\n", result)
end
end

Yusuke Endoh mame@tsg.ne.jp

#12 Updated by Yusuke Endoh about 2 years ago

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

Yusuke Endoh mame@tsg.ne.jp

#13 Updated by Benoit Daloze about 2 years ago

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.

#14 Updated by Benoit Daloze about 2 years ago

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

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

#15 Updated by Benoit Daloze almost 2 years ago

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.

#16 Updated by Eric Hodel almost 2 years ago

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

#18 Updated by Yusuke Endoh over 1 year ago

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 mame@tsg.ne.jp

#19 Updated by Benoit Daloze over 1 year ago

  • 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 #tos. 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 (rbobjinspect): Kernel#inspect: do not call #to_s.
  • test/ruby/testobject.rb (testinspect): add tests for Kernel#inspect.
  • bignum.c, io.c, numeric.c, object.c, proc.c, vm.c (Init*): alias #inspect to #tos where it was expected. [Feature #6130]

#20 Updated by Benoit Daloze over 1 year ago

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!

Also available in: Atom PDF