fredag, januari 18, 2008

Ruby antipattern: Using eval without positioning information

I have noticed that the default way eval, instance_eval(String) and module_eval(String) almost never does what you want unless you supply positioning information. Oh, it will execute all right, and provided you have no bugs in your code, everything is fine. But sooner or later you or someone else will need to debug the code in that eval. In those cases it's highly annoying when the full error message is "(eval):1: redefining constant Foo". Sure, in most cases you can still get all the information. But why not just make sure that everything needed is there to trace the call?

I would recommend changing all places you have eval, or where using instance_eval or module_eval that takes a string argument, into the version that takes a file and line number:
eval("puts 'hello world'")
# becomes
eval("puts 'hello world'", binding, __FILE__, __LINE__)

"str".instance_eval("puts self")
# becomes
"str".instance_eval("puts self", __FILE__, __LINE__)

String.module_eval("A=1")
# becomes
String.module_eval("A=1", __FILE__, __LINE__)

7 kommentarer:

Unknown sa...

Adding positioning information also helps RCov more accurately report coverage.

Anonym sa...

just for fun, this "new eval" gives almost the same results:

module Kernel
alias_method :old_eval, :eval
def eval(string, bind = binding, filename=nil, line=nil)
if line.nil? || filename.nil?
my_caller = caller(1)[0].split(':')
filename = my_caller[1] if filename.nil?
line = my_caller[2].to_i if line.nil?
end
old_eval(string, bind, filename, line)
end
end

Anonym sa...

oops,
just for fun, this "new eval" gives almost the same results:

module Kernel
alias_method :old_eval, :eval
def eval(string, bind = binding, filename=nil, line=nil)
if line.nil? || filename.nil?
my_caller = caller(1)[0].split(':')
filename = my_caller[0] if filename.nil?
line = my_caller[1].to_i if line.nil?
end
old_eval(string, bind, filename, line)
end
end

Eric Promislow sa...

Too bad Kernel.binding is so expensive:


require 'benchmark'

def bare_eval(io)
io.puts 'hello world'
end

def binding_eval(io)
eval("io.puts 'hello world'", binding, __FILE__, __LINE__)
end

class FakeIO
def puts(s)
end
end
io = FakeIO.new
n = 1_000_000
Benchmark.bm do |x|
x.report { n.times {bare_eval(io)}}
x.report { n.times {binding_eval(io)}}
end

===>
user system total real
0.905000 0.000000 0.905000 ( 0.902000)
11.482000 0.016000 11.498000 ( 11.606000)


This is why debuggers, tracers, etc. built using set_trace_func are so slow.

Anonym sa...

Bear in mind if the code to evaluate is more that one line it will be worse than omitting __FILE__ and __LINE__

s = <<EOL

raise "Error!!!"
EOL
String.module_eval(s,__FILE__,__LINE__)

Outputs
lab.rb:7 Error!!! (TypeError)

The 3 preceeding lines of white space have been added to the current line given.

Ken B sa...

Kernel#eval does what it's supposed to if you omit the background information -- the call site is in the eval (the code string could have come from anywhere), and you can find the location of the eval in the backtrace. Lesson: always print backtraces when catching exceptions.

The file and line features are for when your code is loaded from some other file, for example Ruby Quiz #102 (Literate Ruby). If you look at my solution, you can see that it is possible to find the exact location of exceptions from the Literate Ruby files.

http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-talk/225745

Anonym sa...

Thanks Ken B