Bug #18170
closedException#inspect should not include newlines
Description
Is this intentional?
p StandardError.new("foo\nbar")
#=>
# #<StandardError: foo
# bar>
I expect #inspect
returns a one-line string. How about returning #<StandardError: "foo\nbar">
or something?
Recently, multi-line error messages have been increasing by the introduction of did_you_mean and error_highlight. Printing an object that contains such an exception leads to a tricky output:
class Foo
def initialize
@exception = begin; exampl; rescue Exception; $!; end
end
def example
end
end
p Foo.new
#=>
# #<Foo:0x00007f15aeb4ba48 @exception=#<NameError: undefined local variable or method `exampl' for #<Foo:0x00007f15aeb4ba48 ...>
#
# @exception = begin; exampl; rescue Exception; $!; end
# ^^^^^^
# Did you mean? example>>
This issue was originally found by @ioquatix (Samuel Williams)
Updated by mame (Yusuke Endoh) about 3 years ago
- Status changed from Open to Assigned
- Assignee set to mame (Yusuke Endoh)
Matz said "give it a try". I'll create a PR
Updated by ioquatix (Samuel Williams) about 3 years ago
Thanks @mame (Yusuke Endoh).
Updated by mame (Yusuke Endoh) about 3 years ago
Created a PR: https://github.com/ruby/ruby/pull/4857
As expected, I had to modify some tests. Is this acceptable?
Updated by Eregon (Benoit Daloze) about 3 years ago
The quotes around the message feel redundant to me and likely to cause much more incompatibility.
The message is the main attribute of an exception, I don't think we need to quote it.
I'm neutral regarding escaping \n and non-printable characters.
Updated by mame (Yusuke Endoh) about 3 years ago
Eregon (Benoit Daloze) wrote in #note-4:
The quotes around the message feel redundant to me and likely to cause much more incompatibility.
Because we don't inspect an exception object so often, I think the redundancy is not much of a problem. I somewhat agree with the concern about incompatibility. Though it is not very admirable to depend on the return value of #inspect
, some changes of tests are actually needed for my PR.
I'm neutral regarding escaping \n and non-printable characters.
I think of three options.
- No change
- Apply String#inspect to the message (as my PR does)
- Apply String#inspect to the message, and drop the first and last quotes from the returned string
(3) is very ad-hoc but maybe will work well in many cases.
Updated by Eregon (Benoit Daloze) about 3 years ago
mame (Yusuke Endoh) wrote in #note-5:
Because we don't inspect an exception object so often, I think the redundancy is not much of a problem. I somewhat agree with the concern about incompatibility. Though it is not very admirable to depend on the return value of
#inspect
, some changes of tests are actually needed for my PR.
Right, the #inspect value should generally not be relied upon, but I feel something like rescue => e; p e
or log e.inspect
might not be so rare, especially when there was no Exception#full_message
(or users might not know about it).
Or even as a simple way to include the cause like rescue => e; raise FooException, "some error (#{e.inspect})
.
Exception#to_s
is not useful as it only returns the message.
Yet another case is in IRB:
> e
=> #<RuntimeError: my message>
is nice and I think what people expect.
> e
=> #<RuntimeError: "my message">
not so much.
So I think 1 or 3 is better.
Of course there is no guarantee general inspect
returns a single-line, so it seems best if usages of Exception#inspect or Exception#message handle multiple lines correctly.
Updated by ioquatix (Samuel Williams) about 3 years ago
I am strongly against (3), because I don't think we should be introducing ad-hoc behaviour.
I am strongly in favour of (2) and we should try to be as consistent with the rest of the implementations of #inspect
.
The quotes around the message feel redundant to me and likely to cause much more incompatibility.
Evidence?
Because we don't inspect an exception object so often, I think the redundancy is not much of a problem. I somewhat agree with the concern about incompatibility. Though it is not very admirable to depend on the return value of #inspect, some changes of tests are actually needed for my PR.
100% agree.
Updated by ioquatix (Samuel Williams) about 3 years ago
so it seems best if usages of Exception#inspect or Exception#message handle multiple lines correctly.
In my experience, this would break a ton of logging systems, e.g.
#!/usr/bin/env ruby
require 'logger'
exception = StandardError.new("foo\nbar")
logger = Logger.new($stderr)
logger.debug(exception)
We should investigate in more detail. @mame (Yusuke Endoh) how do you expect this to work with the new multi-line error messages?
Updated by Eregon (Benoit Daloze) about 3 years ago
ioquatix (Samuel Williams) wrote in #note-8:
so it seems best if usages of Exception#inspect or Exception#message handle multiple lines correctly.
In my experience, this would break a ton of logging systems, e.g.
There will always be objects with multiple-lines #inspect
.
Those system are broken if they think #inspect
only ever returns 1 line, there was never such a guarantee.
Updated by Eregon (Benoit Daloze) about 3 years ago
ioquatix (Samuel Williams) wrote in #note-7:
Evidence?
See the PR: https://github.com/ruby/ruby/pull/4857/files
For example minitest and irb tests are affected. Likely other test frameworks too.
Updated by ioquatix (Samuel Williams) about 3 years ago
I asked Jon Rowe from RSpec to check this. He monkey patched (3) into rspec and:
I monkey patched an equivalent fix into my local code, and nothing failed, so I’m 80% confident we don’t even match on inspect outputs in our specs
Followed by:
rspec isn’t broken by this but please don’t escape \n in message
Which is okay since we aren't proposing any changes to Exception#message
, only #inspect
.
I can also follow up with minitest authors if that's helpful.
Updated by matz (Yukihiro Matsumoto) about 3 years ago
I agree with the new behavior that wraps messages with newlines.
Matz.
Updated by mame (Yusuke Endoh) about 3 years ago
We discussed this ticket at the dev-meeting, and matz basically accepted as above.
The approved spec is very hacky. Only when the message includes new lines, Exception#inspect
applies inspect
to the message. Otherwise, it embeds the message as is.
p StandardError.new("foo\nbar") #=> #<StandardError:"foo\nbar">
p StandardError.new("foo bar") #=> #<StandardError: foo bar>
p StandardError.new("foo\\bar") #=> #<StandardError: foo\bar>
Note that there is no space between the colon and the double quote in the first line. This is a mark showing whether inspect
is used.
p StandardError.new("foo\nbar") #=> #<StandardError:"foo\nbar">
p StandardError.new('"foo\nbar"') #=> #<StandardError: "foo\nbar">
Parsing the result of #inspect
is never recommended, though. I'll update my PR later to follow the approved spec.
Updated by ioquatix (Samuel Williams) about 3 years ago
Note that there is no space between the colon and the double quote in the first line.
Why is that? Why is it necessary?
The approved spec is very hacky.
I agree it's ad-hoc and I dislike that we could not just call inspect on every message which would be consistent and straight forward to implement. The proposed solution seems both inefficient (need to scan string before printing) and inconsistent (now we still have two different formatting rules).
@matz (Yukihiro Matsumoto) what problem are we trying to prevent by not just applying #inspect
to every message?
Updated by mame (Yusuke Endoh) about 3 years ago
ioquatix (Samuel Williams) wrote in #note-14:
@matz (Yukihiro Matsumoto) what problem are we trying to prevent by not just applying
#inspect
to every message?
Incompatibility. In general, users should not depend on the return value of #inspect
. That being said, we don't want to introduce incompatibility unnecessarily. According to my PR, the simple approach actually required some modification to tests, so the incompatibility issue is not imaginary but real. We'd avoid the hassle here.
Considering this issue from a practical point of view, the problem occurs only when newlines are included. We don't have to espace other letters. So @matz (Yukihiro Matsumoto) chose to avoid an incompatibility as far as reasonably practicable.
@akr (Akira Tanaka) suggested the "no-space" hack to indicate whether #inspect
is applied or not, and @matz (Yukihiro Matsumoto) left to me whether the hack is introduced or not. I have no strong opinion about that, but just for the case, I'd like to make them distinguishable.
Updated by ioquatix (Samuel Williams) about 3 years ago
@mame (Yusuke Endoh) thanks for all your work on this issue.
Incompatibility. In general, users should not depend on the return value of #inspect. That being said, we don't want to introduce incompatibility unnecessarily. According to my PR, the simple approach actually required some modification to tests, so the incompatibility issue is not imaginary but real. We'd avoid the hassle here.
We tested the change with the entire RSpec code base and there was no failure. Because RSpec has specific error matching predicates which match the error, not the output of inspect. I imagine the same is true for Minitest and similar systems. I can't imagine there would be any systems that will directly match output of Exception#inspect
in actual real world tests, and even if there are, they are easily fixed.
Regarding the Ruby test failures, maybe the problem is that the test of the exact format is incorrect. Since we don't expect users to depend on exact format, why do our tests depend on the exact format? We can rework these tests to be more flexible.
we don't want to introduce incompatibility unnecessarily
I'm willing to break compatibility if it means a simpler and more consistent implementation, especially longer term. To this end, the current proposed change seems okay to me, but I wouldn't introduce any changes to spacing to determine the difference. As you said, we don't want users to parse the result of inspect, but by introducing spacing changes, you are indicating some change. Such spacing is also inconsistent with (all?) other implementations of #inspect
.
Updated by Eregon (Benoit Daloze) almost 3 years ago
I've been thinking about this again, and I'm not sure if
#<Foo:0x00007f15aeb4ba48 @exception=#<NameError: undefined local variable or method `exampl' for #<Foo:0x00007f15aeb4ba48 ...>
@exception = begin; exampl; rescue Exception; $!; end
^^^^^^
Did you mean? example>>
or
#<Foo:0x00007f15aeb4ba48 @exception=#<NameError: "undefined local variable or method `exampl' for #<Foo:0x00007f15aeb4ba48 ...>\n @exception = begin; exampl; rescue Exception; $!; end\n ^^^^^^\nDid you mean? example">>
is better.
The object's inspect is unreadable in both cases anyway.
For readability of the error, I believe the first is better.
And if the error was shown alone (common case, e.g. if an exception is shown in IRB or with p exception
), the first is clearly much better:
#<NameError: undefined local variable or method `exampl' for #<Foo:0x00007f15aeb4ba48 ...>
@exception = begin; exampl; rescue Exception; $!; end
^^^^^^
Did you mean? example>
vs
#<NameError: "undefined local variable or method `exampl' for #<Foo:0x00007f15aeb4ba48 ...>\n @exception = begin; exampl; rescue Exception; $!; end\n ^^^^^^\nDid you mean? example>
(that's completely unreadable)
I think needing to read the inspect
of an object containing an exception is pretty rare, and not worth making Exception#inspect
less readable (which can be very useful for debugging).
And there is an easy workaround, that object storing an exception can have a custom #inspect and something like:
#<ExceptionWrapper:0x00007f15aeb4ba48 for NameError with message:
undefined local variable or method `exampl' for #<Foo:0x00007f15aeb4ba48 ...>
@exception = begin; exampl; rescue Exception; $!; end
^^^^^^
Did you mean? example>
That's readable.
There might also be the need to disable did_you_mean and error_highlight for a given exception, I think that's a reasonable request, but also a different feature.
Maybe having something like Exception#original_message
seems a simple way to do that (the message, unaffected by did_you_mean/error_highlight/etc).
Or have Exception#message assemble different parts of the exception, and did_you_mean/error_highlight would use a proper hook to add extra data. Then Exception#message could accept some argument whether you want the extra data or not, or just a single line, etc.
Updated by ioquatix (Samuel Williams) almost 3 years ago
Here is what I want:
-
Exception.new(message).message
is alwaysmessage
. - Top level unhandled exception handler, which defaults to some well defined interface for printing exceptions.
- Well defined hooks for augmenting the exception printing out process.
-
Exception#inspect
should be exactly the default implementation provided byObject#inspect
.
In addition to this, in order to support printing exceptions to a format agonstic output, we need to make sure that meta-data attached to the exception is sufficiently well defined so as not to require implementation specific interfaces like RubyVM::AST.of
.
Anyway, I know that we can't have some/all of this, but I don't think it's unreasonable position to take.
Updated by Eregon (Benoit Daloze) almost 3 years ago
BTW it seems irb
disables error_highlight
somehow:
$ ruby -ve 'self.instance_off'
ruby 3.1.0dev (2021-11-08T13:15:21Z master bd2674ad33) [x86_64-linux]
-e:1:in `<main>': undefined method `instance_off' for main:Object (NoMethodError)
self.instance_off
^^^^^^^^^^^^^
Did you mean? instance_of?
vs
$ irb
irb(main):001:0> self.instance_off
(irb):1:in `<main>': undefined method `instance_off' for main:Object (NoMethodError)
Did you mean? instance_of?
from /home/eregon/prefix/ruby-master/lib/ruby/gems/3.1.0/gems/irb-1.3.8.pre.11/exe/irb:11:in `<top (required)>'
from /home/eregon/.rubies/ruby-master/bin/irb:25:in `load'
from /home/eregon/.rubies/ruby-master/bin/irb:25:in `<main>'
Updated by Eregon (Benoit Daloze) almost 3 years ago
ioquatix (Samuel Williams) wrote in #note-18:
Here is what I want:
Exception.new(message).message
is alwaysmessage
.
That's hard, e.g. for Errno#message.
I think having a way to get the original message or the message unaffected by did_you_mean/error_highlight
is good though.
- Top level unhandled exception handler, which defaults to some well defined interface for printing exceptions.
Isn't that already the case? The top level handler is basically $stderr.puts exc.full_message
.
- Well defined hooks for augmenting the exception printing out process.
Agreed, did_you_mean/error_highlight
should use a better hook than overriding message
unilaterally (or using set_message
).
Exception#inspect
should be exactly the default implementation provided byObject#inspect
.
Can't be, message
is not an instance variable so it wouldn't be shown at all.
In addition to this, in order to support printing exceptions to a format agonstic output, we need to make sure that meta-data attached to the exception is sufficiently well defined so as not to require implementation specific interfaces like
RubyVM::AST.of
.
+1
Anyway, I know that we can't have some/all of this, but I don't think it's unreasonable position to take.
I think it's a reasonable position and some of these have a decent chance to be accepted.
I don't see how changing/breaking Exception#inspect improves anything, so I suggest we don't change Exception#inspect.
Updated by Eregon (Benoit Daloze) almost 3 years ago
- Related to Feature #18296: Custom exception formatting should override `Exception#full_message`. added
Updated by mame (Yusuke Endoh) almost 3 years ago
Eregon (Benoit Daloze) wrote in #note-19:
BTW it seems
irb
disableserror_highlight
somehow:
Because irb uses Kernel#eval
to execute the input code, error_highlight cannot get the source code. Currently, there is a secret API, RubyVM.keep_script_lines = true
, which enables error_highlight in irb.
$ irb
irb(main):001:0> RubyVM.keep_script_lines = true
=> true
irb(main):002:0> 1.time
(irb):2:in `<main>': undefined method `time' for 1:Integer (NoMethodError)
1.time
^^^^^
Did you mean? times
from /home/mame/work/ruby/local/lib/ruby/gems/3.1.0/gems/irb-1.3.8.pre.11/exe/irb:11:in `<top (required)>'
from /home/mame/work/ruby/local/bin/irb:25:in `load'
from /home/mame/work/ruby/local/bin/irb:25:in `<main>'
irb(main):003:0>
Updated by mame (Yusuke Endoh) almost 3 years ago
I have updated my PR to follow the approved semantics. https://github.com/ruby/ruby/pull/4857
Updated by st0012 (Stan Lo) over 2 years ago
I think this change will affect many users' tests because test frameworks generally use exceptions for assertion messages.
For example, this is how test-unit implements assert_block
def assert_block(message="assert_block failed.")
_wrap_assertion do
if (! yield)
options = {}
if message.respond_to?(:user_message)
options[:user_message] = message.user_message
end
raise AssertionFailedError.new(message.to_s, options)
end
end
end
You can see it takes a custom message and then pass it into an exception, which will later be used to print failure info when the test failed.
And many developers (myself included) use multiple lines to format failure messages. Example from ruby/debug. So with this change, formatted test messages like those will be squashed into 1 line and become hard to read.
Updated by ioquatix (Samuel Williams) over 2 years ago
Can you show where a change to #inspect
will impact this code?
Updated by st0012 (Stan Lo) over 2 years ago
Sorry that the #inspect
call actually came from the ruby/debug
's own patch. But I still found an exception#inspect
occurrence in test-unit
here. It should be easily replaced with something like "#{exception.class}: #{exception.message}"
though.
And regarding the ruby/debug
test failures caused by the change, I'll update the test framework's patch so it won't fail for this again.
Updated by ioquatix (Samuel Williams) over 2 years ago
Thanks for your investigating and patches.
This looks like a great improvement.
Test frameworks should probably avoid making assertions on formatted output, so your proposal makes sense to me. Even better is something like assert_raises_exception(class, message){...}
so we decouple the class type (could be subclass) and message (could be pattern) from the actual string generated.
Updated by mame (Yusuke Endoh) over 2 years ago
- Status changed from Assigned to Closed
Applied in changeset git|9d927204e7b86eb00bfd07a060a6383139edf741.
error.c: Let Exception#inspect inspect its message
... only when the message string has a newline.
p StandardError.new("foo\nbar")
now prints `#<StandardError: "foo\nbar">'
instead of:
#<StandardError:
bar>
[Bug #18170]
Updated by ioquatix (Samuel Williams) about 2 years ago
I tested this on current Ruby head, and I can't seem to get the proposed new behaviour:
> p StandardError.new("foo\nbar")
#<StandardError: foo
bar>
Am I doing something wrong?
Updated by mame (Yusuke Endoh) about 2 years ago
After the commit, some tests of debug gem started failing, so I tentatively reverted it at https://github.com/ruby/ruby/commit/b9f030954a8a1572032f3548b39c5b8ac35792ce . And I forgot this issue. Sorry.
I think the issue has been already fixed in the side of debug gem at https://github.com/ruby/debug/commit/93991d84642a81018d3a41363484cbc043d1ec32 . I confirmed that make test-bundled-gems
now works on my machine. I'll reintroduce my commit.