Change semantics of finish

This is a backwards incompatible change that changes the meaning of the
`finish` command. `finish n` means now literally "finish _n_ frames from
current position and then stop", which makes more sense than the old
"run program until frame number _n_".

The `Context#stop_return` method has been removed from the API because
it was just added to support `byebug` statements at the end of blocks
and now `Context#step_out` can perfectly handle this.

Now the API now can be easily extended in the future to support
breakpoints at the end of method, which could be useful to inspect the return
value, for example.

pry-byebug will need to be upgraded to support this.

ext/byebug/byebug.c

@@ -160,7 +160,7 @@ call_at_catchpoint(VALUE context_obj, debug_context_t *dc, VALUE exp)
 static VALUE
 call_at_return(VALUE context_obj, debug_context_t *dc, VALUE file, VALUE line)
 {
-  dc->stop_reason = CTX_STOP_BREAKPOINT;
+  CTX_FL_UNSET(dc, CTX_FL_STOP_ON_RET);
   return call_at(context_obj, dc, rb_intern("at_return"), 2, file, line);
 }
 
@@ -241,6 +241,9 @@ call_event(VALUE trace_point, void *data)
 
   dc->calced_stack_size++;
 
+  if (CTX_FL_TEST(dc, CTX_FL_STOP_ON_RET))
+    dc->steps_out = dc->steps_out <= 0 ? -1 : dc->steps_out + 1;
+
   EVENT_COMMON
 
   breakpoint = Qnil;
@@ -270,7 +273,11 @@ return_event(VALUE trace_point, void *data)
 
   EVENT_COMMON
 
-  if (dc->calced_stack_size + 1 == dc->before_frame)
+  if (dc->steps_out == 1)
+  {
+    dc->steps = 1;
+  }
+  else if ((dc->steps_out == 0) && (CTX_FL_TEST(dc, CTX_FL_STOP_ON_RET)))
   {
     VALUE file, line;
 
@@ -280,11 +287,7 @@ return_event(VALUE trace_point, void *data)
     call_at_return(context, dc, file, line);
   }
 
-  if (dc->calced_stack_size + 1 == dc->after_frame)
-  {
-    reset_stepping_stop_points(dc);
-    dc->steps = 1;
-  }
+  dc->steps_out = dc->steps_out <= 0 ? -1 : dc->steps_out - 1;
 
   cleanup(dc);
 }

ext/byebug/byebug.h

@@ -12,6 +12,7 @@
 #define CTX_FL_SUSPEND      (1<<5) /* thread currently suspended             */
 #define CTX_FL_TRACING      (1<<6) /* call at_tracing method                 */
 #define CTX_FL_WAS_RUNNING  (1<<7) /* thread was previously running          */
+#define CTX_FL_STOP_ON_RET  (1<<8) /* can stop on method 'end'               */
 
 /* macro functions */
 #define CTX_FL_TEST(c,f)  ((c)->flags & (f))
@@ -34,11 +35,10 @@ typedef struct {
   VALUE thread;
   int thnum;
 
-  int dest_frame;
-  int lines;                   /* # of lines in dest_frame before stopping    */
-  int steps;                   /* # of steps before stopping                  */
-  int after_frame;             /* stop right after returning from this frame  */
-  int before_frame;            /* stop right before returning from this frame */
+  int dest_frame;              /* next stop's frame if stopped by next     */
+  int lines;                   /* # of lines in dest_frame before stopping */
+  int steps;                   /* # of steps before stopping               */
+  int steps_out;               /* # of returns before stopping             */
 
   VALUE last_file;
   VALUE last_line;

ext/byebug/context.c

@@ -12,8 +12,7 @@ reset_stepping_stop_points(debug_context_t *context)
   context->dest_frame   = -1;
   context->lines        = -1;
   context->steps        = -1;
-  context->after_frame  = -1;
-  context->before_frame = -1;
+  context->steps_out    = -1;
 }
 
 /*
@@ -421,22 +420,35 @@ Context_step_into(int argc, VALUE *argv, VALUE self)
  *  call-seq:
  *    context.step_out(frame)
  *
- *  Stops after frame number +frame+ is activated. Implements +finish+ and
- *  +next+ commands.
+ *  Stops after +n_frames+ frames are finished. Implements +finish+ and
+ *  +next+ commands. +force+ parameter (if true) ensures that the cursor will
+ *  stop in the specified frame even when there's no more instructions to run.
+ *  In that case, it will stop when the return event for that frame is
+ *  triggered.
  */
 static VALUE
-Context_step_out(VALUE self, VALUE frame)
+Context_step_out(int argc, VALUE *argv, VALUE self)
 {
+  int n_args, n_frames;
+  VALUE v_frames, v_force;
   debug_context_t *context;
 
+  n_args = rb_scan_args(argc, argv, "02", &v_frames, &v_force);
+  n_frames = n_args == 0 ? 1 : FIX2INT(v_frames);
+  v_force = (n_args < 2) ? Qfalse : v_force;
+
   Data_Get_Struct(self, debug_context_t, context);
 
-  if (FIX2INT(frame) < 0 || FIX2INT(frame) >= context->calced_stack_size)
+  if (n_frames < 0 || n_frames >= context->calced_stack_size)
     rb_raise(rb_eRuntimeError, "Stop frame is out of range.");
 
-  context->after_frame = context->calced_stack_size - FIX2INT(frame);
+  context->steps_out = n_frames;
+  if (RTEST(v_force))
+    CTX_FL_SET(context, CTX_FL_STOP_ON_RET);
+  else
+    CTX_FL_UNSET(context, CTX_FL_STOP_ON_RET);
 
-  return frame;
+  return Qnil;
 }
 
 /*
@@ -474,27 +486,6 @@ Context_step_over(int argc, VALUE *argv, VALUE self)
   return Qnil;
 }
 
-/*
- *  call-seq:
- *    context.stop_return(frame)
- *
- *  Stops before frame number +frame+ is activated. Useful when you enter the
- *  debugger after the last statement in a method.
- */
-static VALUE
-Context_stop_return(VALUE self, VALUE frame)
-{
-  debug_context_t *context;
-
-  Data_Get_Struct(self, debug_context_t, context);
-  if (FIX2INT(frame) < 0 || FIX2INT(frame) >= context->calced_stack_size)
-    rb_raise(rb_eRuntimeError, "Stop frame is out of range.");
-
-  context->before_frame = context->calced_stack_size - FIX2INT(frame);
-
-  return frame;
-}
-
 static void
 context_suspend_0(debug_context_t *context)
 {
@@ -639,9 +630,8 @@ Init_context(VALUE mByebug)
   rb_define_method(cContext, "resume"           , Context_resume           ,  0);
   rb_define_method(cContext, "calced_stack_size", Context_calced_stack_size,  0);
   rb_define_method(cContext, "step_into"        , Context_step_into        , -1);
-  rb_define_method(cContext, "step_out"         , Context_step_out         ,  1);
+  rb_define_method(cContext, "step_out"         , Context_step_out         , -1);
   rb_define_method(cContext, "step_over"        , Context_step_over        , -1);
-  rb_define_method(cContext, "stop_return"      , Context_stop_return      ,  1);
   rb_define_method(cContext, "stop_reason"      , Context_stop_reason      ,  0);
   rb_define_method(cContext, "suspend"          , Context_suspend          ,  0);
   rb_define_method(cContext, "suspended?"       , Context_is_suspended     ,  0);

lib/byebug.rb

@@ -181,16 +181,13 @@ class Exception
 
 module Kernel
   #
-  # Enters byebug after _steps_into_ line events and _steps_out_ return events
-  # occur. Before entering byebug startup, the init script is read.
+  # Enters byebug right before (or right after if _before_ is false) return
+  # events occur. Before entering byebug the init script is read.
   #
-  def byebug(steps_into = 1, steps_out = 2)
+  def byebug(steps_out = 1, before = true)
     Byebug.start
     Byebug.run_init_script(StringIO.new)
-    if Byebug.current_context.calced_stack_size > 2
-      Byebug.current_context.stop_return steps_out if steps_out >= 1
-    end
-    Byebug.current_context.step_into steps_into if steps_into >= 0
+    Byebug.current_context.step_out(steps_out, before)
   end
 
   alias_method :debugger, :byebug

lib/byebug/commands/finish.rb

@@ -9,14 +9,12 @@ def regexp
     end
 
     def execute
-      if not @match[1]
-        frame_pos = @state.frame_pos
-      else
-        max_frame = Context.stack_size - @state.frame_pos
-        frame_pos = get_int(@match[1], "finish", 0, max_frame-1, 0)
-        return nil unless frame_pos
-      end
-      @state.context.step_out frame_pos
+      max_frames = Context.stack_size - @state.frame_pos
+      n_frames = get_int(@match[1], "finish", 0, max_frames - 1, 1)
+      return nil unless n_frames
+
+      force = n_frames == 0 ? true : false
+      @state.context.step_out(@state.frame_pos + n_frames, force)
       @state.frame_pos = 0
       @state.proceed
     end
@@ -27,14 +25,11 @@ def names
       end
 
       def description
-        %{fin[ish][ frame-number]\tExecute until selected stack frame returns.
-
-          If no frame number is given, we run until the currently selected frame
-          returns. The currently selected frame starts out the most-recent frame
-          or 0 if no frame positioning (e.g "up", "down" or "frame") has been
-          performed.
+        %{fin[ish][ n_frames]\tExecute until frame returns.
 
-          If a frame number is given we run until that frame returns.}
+          If no number is given, we run until the current frame returns. If a
+          number of frames `n_frames` is given, then we run until `n_frames`
+          return from the current position.}
       end
     end
   end

lib/byebug/commands/trace.rb

@@ -19,7 +19,7 @@ def execute
           if @match[3] && @match[3] !~ /(:?no)?stop/
             errmsg "expecting \"stop\" or \"nostop\"; got \"#{@match[3]}\"\n"
           else
-            dbg_cmd = (@match[3] && @match[3] !~ /nostop/) ? 'byebug(1,0)' : ''
+            dbg_cmd = (@match[3] && @match[3] !~ /nostop/) ? 'byebug(1, false)' : ''
           end
           eval("trace_var(:\"\$#{varname}\") do |val|
                   print \"traced global variable '#{varname}' has value '\#{val}'\"\n

test/finish_test.rb

@@ -18,22 +18,22 @@ def d
 class TestFinish < TestDsl::TestCase
   before { enter "break #{__FILE__}:14", 'cont' }
 
-  it 'must stop at the next frame by default' do
+  it 'must stop after current frame is finished when without arguments' do
     enter 'finish'
     debug_file('finish') { state.line.must_equal 11 }
   end
 
-  it 'must stop at the #0 frame by default' do
+  it 'must stop before current frame finishes if 0 specified as argument' do
     enter 'finish 0'
-    debug_file('finish') { state.line.must_equal 11 }
+    debug_file('finish') { state.line.must_equal 15 }
   end
 
-  it 'must stop at the specified frame' do
+  it 'must stop after current frame is finished if 1 specified as argument' do
     enter 'finish 1'
-    debug_file('finish') { state.line.must_equal 7 }
+    debug_file('finish') { state.line.must_equal 11 }
   end
 
-  it 'must stop at the next frame if the current frame was changed' do
+  it 'must behave consistenly even if current frame has been changed' do
     enter 'up', 'finish'
     debug_file('finish') { state.line.must_equal 7 }
   end