Skip to content

Conversation

@st0012
Copy link
Member

@st0012 st0012 commented Dec 5, 2021

Specs:

  • Like irb's _ variable, it stores the value returned by the
    previously evaluated expression in the REPL.
  • The default value is nil.
  • All the threads under the same session share the previously evaluated
    value. So when switching to other threads, you can use _ to get the value
    returned by the previous thread.
  • If the previous evaluation caused an exception, its result won't be stored.

@ko1
Copy link
Collaborator

ko1 commented Dec 14, 2021

I'm afraid to conflict with already existing parameter _.
Interestingly, binding.irb and binding.pry has same issue.

]$ ruby -r pry target.rb

From: /mnt/c/ko1/src/rb/ruby-debug/target.rb:3 Object#foo:

    2: def foo _
 => 3:   binding.pry
    4: end

[1] pry(main)> p _
nil
=> nil
[2] pry(main)> 42
=> 42
[3] pry(main)> _
=> 42

@st0012
Copy link
Member Author

st0012 commented Dec 14, 2021

I know this behavior. But from my experience, _ in normal Ruby program usually means "won't be used value". So I think this problem is negligible in most cases and aligning user experience with irb and pry is more important. Take myself for example, because I switch between the debugger and Rails console everyday, I almost see this everyday too:

(rdbg) _    # ruby
eval error: undefined local variable or method `_' for main:Object
  (rdbg)/target.rb:1:in `<main>'
nil

And if we do get an issue report for this behavior, I think it'd also happen in irb too. We can then discuss an approach with the irb team and come up with a consistent approach to address it.

@ko1
Copy link
Collaborator

ko1 commented Dec 17, 2021

I decided to postpone this feature.
Reasons:

  • already existing _ variable should not be overridden. debugger should be able to see all variables.
  • Previous value of this patch is managed by ThreadClient object, so if the thread context is switched, the previous value is not "previous" in the literal sense.
  • I want to introduce previous values like gcc:
(gdb) p 1
$1 = 1
(gdb) p 2+3
$2 = 5
(gdb) p $1 + $2
$3 = 6

$[n] is not acceptable on Ruby, so we need to invent new keywords.$_[n] ($_1, ...) is one idea but a few gems use this kind of global variables:

ko1@aluminium:~$ gem-codesearch '\$_\d+\b'
/srv/gems/ruby-compiler-0.1.1/vendor/ruby/test/ruby/test_rubyoptions.rb:      write_file("test-script", "$_0 = $0.dup; Process.setproctitle('hello world'); $0 == $_0 or Process.setproctitle('$0 changed!'); sleep 60")
/srv/gems/syntheticore-perlize-0.1.5/perlize.rb:            $_2 = arg if $_1
/srv/gems/syntheticore-perlize-0.1.5/perlize.rb:            $_1 = arg if $_1.nil?
/srv/gems/syntheticore-perlize-0.1.5/perlize.rb:            $__ = [$_1, $_2].compact.last
/srv/gems/syntheticore-perlize-0.1.5/perlize.rb:            $_1 = nil if $_2.nil?
/srv/gems/syntheticore-perlize-0.1.5/perlize.rb:            $_2 = nil
/srv/gems/syntheticore-perlize-0.1.5/perlize.rb:            $__ = [$_1, $_2].compact.last

Anyway, last value expression should be consistent with them.

@st0012
Copy link
Member Author

st0012 commented Dec 18, 2021

already existing _ variable should not be overridden. debugger should be able to see all variables.

Perhaps we can use the same approach for _raised? Which is not assigning the value when _ is set by the user, but the rest of the time it'll be set. I think it's a balance between keeping the (probably few) user's program behavior while giving a consistent experience for most users.

Previous value of this patch is managed by ThreadClient object, so if the thread context is switched, the previous value is not "previous" in the literal sense.

I don't do concurrent programming often so I'm not sure what to expect in such case. But I imagine a shared value will be more complicated to manage than the current solution? If that's true, I'd go with the current approach and modify it when we receive more feedback.

I want to introduce previous values like gcc:

I think it's a good idea. But I also hope to have _ as well.

@ko1
Copy link
Collaborator

ko1 commented Dec 19, 2021

If that's true, I'd go with the current approach and modify it when we receive more feedback.

It is difficult to change such basic design.

@st0012
Copy link
Member Author

st0012 commented Dec 21, 2021

@ko1 ok, here's my plan to implement it:

  1. To make the value shared, we need to keep it in Session.
  2. The _ variable needs be accessible in the frame binding to serve usages like a = _ + 5.
  3. Given the above requirements, whenever a line of code is evaluated, the debugger needs to:
    1. Return the value to Session with event! method. This is simple.
    2. The session should then dispatch that value to all thread clients with @th_clients.each_value { |tc| tc << [....]}
    3. Before a ThreadClient#frame_eval is called, it needs to assign the value to the duplicated binding like we do for SPECIAL_LOCALS.

Wdyt?

Specs:

- Like `irb`'s `_` variable, it stores the value returned by the
  previously evaluated expression in the REPL.
- The default value is `nil`.
- All the threads under the same session share the previously evaluated
  value. So when switching to other threads, you can use `_` to get the value
  returned by the previous thread.
- If the previous evluation caused an exception, its result won't be
  stored.
@st0012 st0012 force-pushed the support-underscore-variable branch from de190bb to 25ef5fc Compare January 1, 2022 17:07
@st0012
Copy link
Member Author

st0012 commented Jan 1, 2022

@ko1 I've addressed this 2 issues:

already existing _ variable should not be overridden. debugger should be able to see all variables.
Previous value of this patch is managed by ThreadClient object, so if the thread context is switched, the previous value is not "previous" in the literal sense.

And regarding:

I want to introduce previous values like gcc:

I think it could use more discussion and can be addressed in another PR.

@st0012 st0012 mentioned this pull request Jan 28, 2022
end
@success_last_eval = true

event! :sync_prev_evaled, result
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please send SESSION's method. I'll make it.

@st0012
Copy link
Member Author

st0012 commented Mar 20, 2022

@ko1 if #503 is merged, we can use internal data channel and metadata to pass things around, which means the new events added here won't be needed. do you want to hold this PR's merge for that?

@ko1
Copy link
Collaborator

ko1 commented Mar 20, 2022

Considering about this feature, there are same issues on multiple processes like ractors and we can not solve the issue.
On multi-process mode, the confusion can be bigger but I agree it is useful feature.

  • inter-threads: share the value with _
  • inter-ractors: share the value between a ractor? share the value if it is shareable? Q. what happens in another case?
  • inter-process: Q. what happens?

For Q:
(1) Use the last value in a ractor/process
(2) Do not set _ if the last value is not evaluated on different ractor/process

I need to check other debuggers (and similar systems)

@st0012
Copy link
Member Author

st0012 commented Apr 9, 2022

I don't have experience on debugging multi-process with a debugger, but I think it makes sense to not setting _ in a new thread/ractor.

And regarding the implementation, I think we can use the same metadata/internal info approach I proposed in #503. So after an expression is evaluated, its value will be sent back to the SESSION as part of the internal info. And every ThreadClient will receive the last-evaled value through commands' metadata. That way we won't need to introduce any new event/command types. Wdyt?

@ko1
Copy link
Collaborator

ko1 commented Apr 17, 2022

Could you survey other REPL/debuggers?

@st0012 st0012 closed this Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants