Bug #4352

[patch] Fix eval(s, b) backtrace; make eval(s, b) consistent with eval(s)

Added by James M. Lawrence about 3 years ago. Updated 3 months ago.

[ruby-core:35027]
Status:Assigned
Priority:Normal
Assignee:Yukihiro Matsumoto
Category:core
Target version:current: 2.2.0
ruby -v:ruby 1.9.3dev (2011-02-01 trunk 30751) [i386-darwin9.8.0] Backport:

Description

=begin
def ex_message
begin
yield
rescue => e
p e.message
end
end

exmessage { eval('raise') }
ex
message { eval('raise', binding) }

eval('def f ; end')
p method(:f).sourcelocation
eval('def g ; end', binding)
p method(:g).source
location


Without patch:
"(eval):1:in `block in ': "
""
["(eval)", 1]
["eval_test.rb", 14]

With patch:
"(eval):1:in block in <main>': "
"(eval):1:in
block in ': "
["(eval)", 1]
["(eval)", 1]

Knowing the line of an error inside eval is useful. Passing a binding
shouldn't discard that information. Present behavior is even wrong:
there's no line 10 in this file.


eval %{

# .. code ...
raise

}, binding


Without patch:
/Users/jlawrence/tmp/raiser.rb:10:in <main>': unhandled exception
from /Users/jlawrence/tmp/raiser.rb:7:in
eval'
from /Users/jlawrence/tmp/raiser.rb:7:in `'

With patch:
/Users/jlawrence/tmp/raiser.rb:7:in eval': (eval):4:in': (RuntimeError)
from /Users/jlawrence/tmp/raiser.rb:7:in eval'
from /Users/jlawrence/tmp/raiser.rb:7:in
'
=end

eval.patch Magnifier (1.75 KB) James M. Lawrence, 02/01/2011 11:31 AM


Related issues

Related to ruby-trunk - Bug #4487: require_relative fails in an eval'ed file Assigned 03/10/2011
Related to ruby-trunk - Bug #4379: [patch] eval(s, b, "(eval)", n) discards location Assigned 02/08/2011

History

#1 Updated by James M. Lawrence about 3 years ago

=begin
I came across this issue when I noticed that sourcelocation gives non-useful info when a binding is passed to eval. Thus the patch fixes two somewhat different problems: eval backtrace and sourcelocation. If changing the backtrace is inappropriate then a separate bug for source_location should be filed.
=end

#2 Updated by Yusuke Endoh about 3 years ago

=begin
Hi,

2011/2/1 James M. Lawrence redmine@ruby-lang.org:

Knowing the line of an error inside eval is useful. Passing a binding
shouldn't discard that information.

I understand you, but the behavior is intended.

A binding also has its own information of filename and lineno.
Some people ( ) think that binding's
lineno information is more important than eval's information, and that
eval shouldn't discard the binding's informantion.

# foo.rb
eval("p [FILE, LINE]", binding) #=> expected: ["foo.rb", 2]

In addition, the behavior is compatible to 1.8.

Present behavior is even wrong:

there's no line 10 in this file.

eval %{

# .. code ...
raise

}, binding

Without patch:
/Users/jlawrence/tmp/raiser.rb:10:in <main>': unhandled exception
from /Users/jlawrence/tmp/raiser.rb:7:in
eval'
from /Users/jlawrence/tmp/raiser.rb:7:in `'

With patch:
/Users/jlawrence/tmp/raiser.rb:7:in eval': (eval):4:in': ?(RuntimeError)
from /Users/jlawrence/tmp/raiser.rb:7:in eval'
from /Users/jlawrence/tmp/raiser.rb:7:in
'

It is indeed confusing, but it can be understood as follows:

1) Here are actual linenos. The binding has its own lineno at which the
method is called:

1: eval %{
2:
3: # .. code ...
4: raise
5:
6:
7: }, binding

2) binding virtually overwrites linenos so that the eval'ing code starts
with the binding's own lineno (that is, Line 7):

1: eval %{ # ( 7)
2: # ( 8)
3: # .. code ... # ( 9)
4: raise # (10)
5: # (11)
6: # (12)
7: }, binding # (13)

3) an exception is raised at (virtual) Line 10.

You can exploit this behavior to know the lineno more directly:

1: # foo.rb
2: b, s = binding, %{
3:
4:
5: # .. code ...
6: raise # <- HERE!!
7:
8:
9: }, b
10: eval s, b #=> foo.rb:6:in `': unhandled exception

I guess eval should have received binding as the first argument ;-)

eval binding, %{
...
}

--
Yusuke Endoh mame@tsg.ne.jp

=end

#3 Updated by James M. Lawrence about 3 years ago

=begin
Thank you for that detailed explanation. The problem for me is the
connection to source_location, which should be usable by tools.

Shouldn't sourcelocation give the file and line of a method or block
definition? If so then source
location is bugged when the binding
argument is passed.

The seemingly simplest solution is to make the backtrace and
source_location consistent. It only takes a 4-line patch.

I might have thought otherwise if virtual line numbers served some
human purpose, but I just see them as confusing. Tools are even more
confused.

Should a new bug for source_location be created?
=end

#4 Updated by Yusuke Endoh about 3 years ago

=begin
Hi,

2011/2/2 James M. Lawrence redmine@ruby-lang.org:

Thank you for that detailed explanation. The problem for me is the
connection to source_location, which should be usable by tools.

What kind of tools are you talking about?
Even if a binding location is discarded, we can still fake FILE
and LINE without using a binding:

eval <<-END, nil, "/etc/passwd", 1
def foo
end
END
p method(:foo).source_location #=> ["/etc/passwd", 1]

So, source_location user should know and accept the fact that the
information is not trustable.
Why do you think only a binding location as a problem?

--
Yusuke Endoh mame@tsg.ne.jp

=end

#5 Updated by James M. Lawrence about 3 years ago

=begin

Why do you think only a binding location as a problem?

The initial problem I encountered was

eval %{def g ; end}, nil, "(eval)", 99
p method(:g).source_location #=> ["(eval)", 99]

eval %{def f ; end}, binding, "(eval)", 99
p method(:f).source_location #=> ["prob.rb", 1]

I needed source_location to be ["(eval)", 99] in the latter case,
which is solved by the patch. This could be also be done by making
eval recognize the difference between an empty file argument and an
"(eval)" file argument, however the problem seemed more fundamental.

eval should not implicitly slurp file/line info from the binding.
Creating a "dishonest" backtrace is something that should be done
explicitly by the user, as in

eval(s, b, *b.source_location)

In the last the example in the original post, the existence of
raiser.rb:10 is an outright falsehood. Pointing to a nonexistent line
number should not be the default behavior of eval(s, b).

Since sourcelocation claims to be "the ruby source filename and line
number containing this proc", I was thinking that source
location
could give the "true" location, ignoring the file/line "lies" passed
to eval. But this was wrong--indeed I want the "lies" to be reflected
in source_location. My problem is with eval implicitly grabbing
file/line from the binding, which is just what the patch solves.

As mentioned above, this would be nice too:

class Binding
def sourcelocation
eval "[
FILE, _LINE__]"
end
end

diff --git a/proc.c b/proc.c
index 7b1d147..0042caf 100644
--- a/proc.c
+++ b/proc.c
@@ -305,6 +305,24 @@ binding_clone(VALUE self)
return bindval;
}

+/*
+ * call-seq:
+ * binding.sourcelocation -> [String, Fixnum]
+ *
+ * Returns the ruby source filename and line number associated with
+ * this binding.
+ */
+static VALUE
+binding
sourcelocation(VALUE self)
+{
+ rb
bindingt *src;
+ VALUE loc[2];
+ GetBindingPtr(self, src);
+ loc[0] = rb
strdupfrozen(src->filename);
+ loc[1] = INT2FIX(src->lineno);
+ return rb
arynew4(2, loc);
+}
+
VALUE
rb
bindingnew(void)
{
@@ -2227,6 +2245,7 @@ Init
Binding(void)
rbundefmethod(CLASSOF(rbcBinding), "new");
rbdefinemethod(rbcBinding, "clone", bindingclone, 0);
rbdefinemethod(rbcBinding, "dup", bindingdup, 0);
+ rbdefinemethod(rbcBinding, "sourcelocation", bindingsourcelocation, 0);
rbdefinemethod(rbcBinding, "eval", bindeval, -1);
rbdefineglobalfunction("binding", rbf_binding, 0);
}

=end

#6 Updated by Yusuke Endoh about 3 years ago

=begin
Hi,

2011/2/3 Rocky Bernstein rockyb@rubyforge.org:

See also
http://ola-bini.blogspot.com/2008/01/ruby-antipattern-using-eval-without.html.
It is called an "anti-pattern" there which I guess is used in a derogatory
fashion.

I'd like to positively call it a "best practice" :-)

A place where setting the file and line is used is in template systems like
merb or erb where the file the user works on is not a Ruby file but a
template file that expands to a Ruby file. In that situation, disregarding
the expanded Ruby file is convenient since there can only be one location
attached in the Ruby implementation.
However I don't see why a templating system couldn't also provide an
expanded Ruby file for debugging problems much as one can get the
C-preprocessed output for a C program when one wants.

I'm not sure that I could understand you.
Are you saying that erb users should debug their erb code by looking
the erb-generated Ruby code? I don't think that it is user-friendly.

FYI, erb offers ERB#src which provides a generated Ruby code. However,
I don't want to encourage users to read and debug the generated code.

But shouldn't we try to address the location problem head on in the Ruby
implementation? I suspect it too late to try to change Ruby MRI 1.x, but
perhaps in Ruby 2.x some thought could be given to how to address.?

Yes, it is good to improve the spec in 2.0.
Before that, we must first check the use case.
For creating what kind of tools, what kind of information do you need?

If the fact that source_location is not trustable is a a concern, then
perhaps setting the file and line parameters on eval can be disallowed at
some level greater than 0 and less than 4 which is where eval() is
disallowed totally.?

We should not ignore erb-like use case. Just prohibiting the file and
line parameters is too strict. Unless Ruby provides an alternative
feature, like #line of C-preprocessor.

BTW, is it ok for you that source_location returns ["(eval)", 1]?
It does not allow to distinguish whether it is executed in Kernel#eval
or the source code file is named "(eval)".
"-" (for stdin) and "-e" (for -e option) seem to have the same problem.

--
Yusuke Endoh mame@tsg.ne.jp

=end

#7 Updated by Yusuke Endoh about 3 years ago

=begin
Hi,

2011/2/3 James M. Lawrence redmine@ruby-lang.org:

The initial problem I encountered was

eval %{def g ; end}, nil, "(eval)", 99
p method(:g).source_location #=> ["(eval)", 99]

eval %{def f ; end}, binding, "(eval)", 99
p method(:f).source_location #=> ["prob.rb", 1]

Hmm. It is indeed irritating for Kernel#eval to prefer implicit filename
of a binding to explicitly-specified filename.
This is because the current implementation is uncool, like:

def eval(src, binding, filename = "(eval)", lineno = 1)
filename = binding.filename if filename == "(eval)"
...
end

Kernel#eval uses a string "(eval)" when the filename is not specified
explicitly. And then, it replaces it with implicit filename of binding
when the given filename is equal to "(eval)" with string comparison.

Even so, I hesitate to change the behavior in 1.9.x for compatibility
reason.

Since sourcelocation claims to be "the ruby source filename and line
number containing this proc", I was thinking that source
location
could give the "true" location, ignoring the file/line "lies" passed
to eval.

Honestly, I understand your expectation. It is of course acceptable to
fix it in 2.0. But it would require a major modification because the
current implementation itself does NOT know the "true" location. The
information is discarded at the parse time.

--
Yusuke Endoh mame@tsg.ne.jp

=end

#8 Updated by James M. Lawrence about 3 years ago

=begin
Yusuke Endoh:

Since sourcelocation claims to be "the ruby source filename and line
number containing this proc", I was thinking that source
location
could give the "true" location, ignoring the file/line "lies" passed
to eval.

Honestly, I understand your expectation. It is of course acceptable to
fix it in 2.0. But it would require a major modification because the
current implementation itself does NOT know the "true" location. The
information is discarded at the parse time.

Did you misunderstand? It's not my expectation--the next sentence said
it was wrong, and indeed I rely on the current behavior.

My expectation is that overriding file/line for eval can only be done
explicitly, never implicitly through the binding. That's what the
patch does. What did you think of my argument for that?

=end

#9 Updated by Yusuke Endoh about 3 years ago

=begin
Hi,

2011/2/3 James M. Lawrence redmine@ruby-lang.org:

Yusuke Endoh:

Since sourcelocation claims to be "the ruby source filename and line
number containing this proc", I was thinking that source
location
could give the "true" location, ignoring the file/line "lies" passed
to eval.

Honestly, I understand your expectation.  It is of course acceptable to
fix it in 2.0.  But it would require a major modification because the
current implementation itself does NOT know the "true" location.  The
information is discarded at the parse time.

Did you misunderstand? It's not my expectation--the next sentence said
it was wrong, and indeed I rely on the current behavior.

My expectation is that overriding file/line for eval can only be done
explicitly, never implicitly through the binding.

I see. Sorry for my misunderstanding.
But anyway, it is difficult to meet your expectation in 1.9.
Inheriting file/line from a binding is actually intended, not a
bug, even though it is confusing.
We cannot change any spec in 1.9, unless it is considered a bug.

However, it might be considered a bug for Kernel#eval to ignore
an explicitly specified "(eval)", I think.

Does that answer you?

--
Yusuke Endoh mame@tsg.ne.jp

=end

#10 Updated by James M. Lawrence about 3 years ago

=begin
Yusuke Endoh:

However, it might be considered a bug for Kernel#eval to ignore
an explicitly specified "(eval)", I think.

Does that answer you?

Yes, thanks. Redmine doesn't have a category for bugs that are fixed
by a feature request :). As Rocky explains there are some deeper
issues involved, but it still amounts to a feature request. I've filed
a new bug for "(eval)" . Sorry for the confusion.

=end

#11 Updated by Yui NARUSE almost 3 years ago

  • Status changed from Open to Assigned
  • Assignee set to Yusuke Endoh

#12 Updated by Yusuke Endoh almost 3 years ago

  • Assignee changed from Yusuke Endoh to Yukihiro Matsumoto

Hi, matz

This ticket includes some design concerns.

The source_location seems important information not only for
human users, but also for tools, such as a debugger, lint,
coverage measurement, etc.
James and Rocky seem to be interested in the aspect of tools,
and dislike eval, which fakes the information.

1) Is the feature that fakes source_location really needed?
I guess that the feature is designed for erb-like application,
but C-style "#line" may be better.

2) What source_location should be used when only binding is
given?

eval("FILE", TOPLEVEL_BINDING) #=> "" or "(eval)"?

Some people expect "" , and others expect
"(eval)".

3) Which sourcelocation should be used when binding and
explicit source
location are both given?

eval("FILE", TOPLEVEL_BINDING, "foo") #=> "" or "foo"?

Currently "foo" is returned.

4) Should 3) be applied even if "(eval)" is given explicitly?

eval("FILE", TOPLEVEL_BINDING, "(eval)") #=> "" or "(eval)"?

Currently "" is returned.

5) Shouldn't ruby have two-type information, one for human and
one for tools? The former is used for backtrace. The latter
is used for tools. eval can fake only the former information.

6) If 5) is accepted, which information should be used by
require_relative? See #4487.

Thanks,

Yusuke Endoh mame@tsg.ne.jp

#13 Updated by Koichi Sasada about 1 year ago

  • Target version changed from 2.0.0 to 2.1.0

Time up for 2.0.0.

Matz, could you check this ticket?

#14 Updated by Hiroshi SHIBATA 3 months ago

  • Target version changed from 2.1.0 to current: 2.2.0

Also available in: Atom PDF