Skip to content

Fix REPL binding #424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
st0012 wants to merge 6 commits into from
Closed

Fix REPL binding #424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
494ba34
Fix REPL's _raised special local
st0012 Dec 6, 2021
91a8061
Fix typo
st0012 Dec 6, 2021
205004a
Remove unnecessary dup
st0012 Dec 6, 2021
2fb884b
Remove unnecessary FrameInfo#eval_binding
st0012 Dec 6, 2021
84e3fab
Add ThreadClient#eval_binding to provide consistent binding
st0012 Dec 6, 2021
27752e4
Propagate REPL binding locals back to original binding
st0012 Dec 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 0 additions & 16 deletions lib/debug/frame_info.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,22 +118,6 @@ def location_str
"#{pretty_path}:#{location.lineno}"
end

private def make_binding
__newb__ = self.self.instance_eval('binding')
self.local_variables.each{|var, val|
Copy link
Member Author

@st0012 st0012 Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't correct as local_variables only yields keys but no values. But the fact that we didn't spot this probably means it's never called anyway.

__newb__.local_variable_set(var, val)
}
__newb__
end

def eval_binding
if b = self.binding
b
elsif self.local_variables
make_binding
end
end

def local_variables
if lvars = self._local_variables
lvars
Expand Down
5 changes: 3 additions & 2 deletions lib/debug/server_dap.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,8 +639,9 @@ def process_dap args
frame = @target_frames[fid]
message = nil

if frame && (b = frame.binding)
b = b.dup
if frame
b = eval_binding

special_local_variables current_frame do |name, var|
b.local_variable_set(name, var) if /\%/ !~ name
end
Expand Down
34 changes: 25 additions & 9 deletions lib/debug/thread_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,29 +344,43 @@ def instance_eval_for_cmethod frame_self, src
frame_self.instance_eval(src)
end

def eval_binding
current_frame&.binding || TOPLEVEL_BINDING
end

SPECIAL_LOCAL_VARS = [
[:raised_exception, "_raised"],
[:return_value, "_return"],
]
SPECIAL_LOCAL_VAR_NAMES = SPECIAL_LOCAL_VARS.map { |_, n| n }

def frame_eval src, re_raise: false
@success_last_eval = false

b = current_frame&.eval_binding || TOPLEVEL_BINDING
b = b.dup
Copy link
Member Author

@st0012 st0012 Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some unknown reasons, I can't reproduce the polluted binding case anymore 🙈 . So I just remove the .dup call.

Copy link
Member Author

@st0012 st0012 Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's because the current implementation only defines them at the frame that has those values. So it's not easy or even impossible to leak them into the program.

For example, the _return is only defined at the end of the method definition, like

def foo
 # ...
end # <= here

And theoretically no user program will be able to access it, so there's no need to worry about it polluting the program.

Copy link
Collaborator

@ko1 ko1 Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without .dup, TOPLEVEL_BINDING will be affected with assignments.
Maybe it is not intended.

# the binding of original program
original_binding = eval_binding
# the binding used by debugger REPL that can have special locals
repl_binding = eval_binding.dup

special_local_variables current_frame do |name, var|
b.local_variable_set(name, var) if /\%/ !~ name
special_local_variables current_frame, binding: repl_binding do |name, var|
repl_binding.local_variable_set(name, var) if /\%/ !~ name
end

result = if b
f, _l = b.source_location
b.eval(src, "(rdbg)/#{f}")
result = if repl_binding
f, _l = repl_binding.source_location
repl_binding.eval(src, "(rdbg)/#{f}")
else
frame_self = current_frame.self
instance_eval_for_cmethod(frame_self, src)
end
@success_last_eval = true

# write non-special local variables back to the original binding
repl_binding.local_variables.each do |var|
next if SPECIAL_LOCAL_VAR_NAMES.include?(var)
original_binding.local_variable_set(var, repl_binding.local_variable_get(var))
end
Copy link
Member Author

@st0012 st0012 Dec 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After we dup the original binding, local variables changes made in the REPL won't be applied to it. So we need to manually propagate the values back to the original binding.

Copy link
Collaborator

@ko1 ko1 Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on master, it is propagated. which case do you considering?

[1, 5] in target.rb
=>   1| a = b = c = nil
     2| p a, b, c
     3|
     4|
     5| __END__
=>#0    <main> at target.rb:1
(rdbg) n    # next command
[1, 5] in target.rb
     1| a = b = c = nil
=>   2| p a, b, c
     3|
     4|
     5| __END__
=>#0    <main> at target.rb:2
(rdbg) eval a = b = c = :ok    # command
(rdbg) n    # next command
:ok
:ok
:ok

Copy link
Collaborator

@ko1 ko1 Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you mean:

[1, 4] in target.rb
     1|
=>   2| a = 1
     3|
     4| __END__
=>#0    <main> at target.rb:2
(rdbg) y = 50    # ruby
50
(rdbg) y
eval error: undefined local variable or method `y' for main:Object
  (rdbg)/target.rb:1:in `<main>'
nil

but y is only valid on this debug subsession (when continue, the y will be gone). hmm

Copy link
Collaborator

@ko1 ko1 Dec 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

byebug's example.

[master]$ byebug target.rb

[1, 10] in /mnt/c/ko1/src/rb/ruby-debug/target.rb
    1:
=>  2: a = 1
    3: b = 2
    4:
    5: __END__
    6:
    7: require 'foo'
    8:
    9: __END__
   10:
(byebug) y = 10
10
(byebug) p y
10
10
(byebug) n

[1, 10] in /mnt/c/ko1/src/rb/ruby-debug/target.rb
    1:
    2: a = 1
=>  3: b = 2
    4:
    5: __END__
    6:
    7: require 'foo'
    8:
    9: __END__
   10:
(byebug) y
*** NoMethodError Exception: undefined method `end_line=' for nil:NilClass


result

rescue Exception => e
Expand Down Expand Up @@ -442,10 +456,12 @@ def current_frame

## cmd: show

def special_local_variables frame
def special_local_variables frame, binding: nil
SPECIAL_LOCAL_VARS.each do |mid, name|
next unless frame&.send("has_#{mid}")
name = name.sub('_', '%') if frame.binding.local_variable_defined? name
binding ||= frame&.binding
next unless binding
name = name.sub('_', '%') if binding.local_variable_defined? name
yield name, frame.send(mid)
end
end
Expand Down
166 changes: 121 additions & 45 deletions test/debug/debugger_local_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,87 +1,163 @@
# frozen_string_literal: truk
# frozen_string_literal: true

require_relative '../support/test_case'

module DEBUGGER__
class DebuggerLocalsTest < TestCase
class RaisedTest < TestCase
class REPLLocalsTest < TestCase
def program
<<~RUBY
1| _rescued_ = 1111
2| foo rescue nil
3|
4| # check repl variable doesn't leak to the program
5| result = _rescued_ * 2
6| binding.b
1| a = 1
RUBY
end

def test_raised_is_accessible_from_repl
def test_locals_added_in_locals_are_accessible_between_evaluations
debug_code(program) do
type "catch Exception"
type "c"
type "_raised"
assert_line_text(/#<NameError: undefined local variable or method `foo' for main:Object/)
type "c"
type "result"
assert_line_text(/2222/)
type "y = 50"
type "y"
assert_line_text(/50/)
type "c"
end
end
end

def test_raised_is_accessible_from_command
debug_code(program) do
type "catch Exception pre: p _raised"
type "c"
assert_line_text(/#<NameError: undefined local variable or method `foo' for main:Object/)
type "c"
type "result"
assert_line_text(/2222/)
type "c"
class RaisedTest < TestCase
class RubyMethodTest < TestCase
def program
<<~RUBY
1| foo rescue nil
RUBY
end

def test_raised_is_accessible_from_repl
debug_code(program) do
type "catch Exception"
type "c"
type "_raised"
assert_line_text(/#<NameError: undefined local variable or method `foo' for main:Object/)
type "c"
end
end

def test_raised_is_accessible_from_command
debug_code(program) do
type "catch Exception pre: p _raised"
type "c"
assert_line_text(/#<NameError: undefined local variable or method `foo' for main:Object/)
type "c"
end
end
end

class CMethodTest < TestCase
def program
<<~RUBY
1| 1/0 rescue nil
RUBY
end

def test_raised_is_accessible_from_repl
debug_code(program) do
type "catch Exception"
type "c"
type "_raised"
assert_line_text(/ZeroDivisionError/)
type "c"
end
end

def test_raised_is_accessible_from_command
debug_code(program) do
type "catch Exception pre: p _raised"
type "c"
assert_line_text(/ZeroDivisionError/)
type "c"
end
end
end

class EncapsulationTest < TestCase
def program
<<~RUBY
1| 1/0 rescue nil
2| a = _raised
RUBY
end

def test_raised_doesnt_leak_to_program_binding
debug_code(program) do
type "catch StandardError"
type "c"
# stops for ZeroDivisionError
type "info"
type "_raised"
assert_line_text(/ZeroDivisionError/)
type "c"

# stops for NoMethodError because _raised is not defiend in the program
type "_raised"
assert_line_text(/NameError: undefined local variable or method `_raised' for main:Object/)
type "c"
end
end
end
end

class ReturnedTest < TestCase
def program
<<~RUBY
1| _return = 1111
2|
3| def foo
4| "foo"
5| end
6|
7| foo
8|
9| # check repl variable doesn't leak to the program
10| result = _return * 2
11|
12| binding.b
1| def foo
2| "foo"
3| end
4|
5| foo
RUBY
end

def test_returned_is_accessible_from_repl
debug_code(program) do
type "b 5"
type "b 3"
type "c"
type "_return + 'bar'"
assert_line_text(/"foobar"/)
type "c"
type "result"
assert_line_text(/2222/)
type "q!"
end
end

def test_returned_is_accessible_from_command
debug_code(program) do
type "b 5 pre: p _return + 'bar'"
type "b 3 pre: p _return + 'bar'"
type "c"
assert_line_text(/"foobar"/)
type "c"
type "result"
assert_line_text(/2222/)
type "q!"
end
end

class EncapsulationTest < TestCase
def program
<<~RUBY
1| def foo
2| "foo"
3| end
4|
5| foo
6| puts _return
RUBY
end

def test_raised_doesnt_leak_to_program_binding
debug_code(program) do
type "catch StandardError"
type "b 3"
type "c"
type "_return + 'bar'"
assert_line_text(/"foobar"/)
type "c"
# stops for NoMethodError because _return is not defiend in the program
type "_raised"
assert_line_text(/NameError: undefined local variable or method `_return' for main:Object/)
type "c"
end
end
end
end
Expand Down