Project

General

Profile

Actions

Bug #18170

open

Exception#inspect should not include newlines

Added by mame (Yusuke Endoh) about 1 month ago. Updated about 11 hours ago.

Status:
Assigned
Priority:
Normal
Target version:
-
[ruby-core:105276]

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 1 month ago

  • Assignee set to mame (Yusuke Endoh)
  • Status changed from Open to Assigned

Matz said "give it a try". I'll create a PR

Updated by mame (Yusuke Endoh) about 1 month 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 1 month 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 1 month 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.

  1. No change
  2. Apply String#inspect to the message (as my PR does)
  3. 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) 30 days 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) 29 days 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) 29 days 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) 29 days 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) 29 days 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) 27 days 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) 5 days ago

I agree with the new behavior that wraps messages with newlines.

Matz.

Updated by mame (Yusuke Endoh) about 17 hours 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 16 hours 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 11 hours 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.

Actions

Also available in: Atom PDF