[ruby/syntax_suggest] Refactor Scanner logic out of AroundBlockScan introduce history

AroundBlockScan started as a utility class that was meant to be used as a DSL for scanning and making new blocks. As logic got added to this class it became hard to reason about what exactly is being mutated when. I pulled the scanning logic out into it's own class which gives us a clean separation of concerns. This allowed me to remove a lot of accessors that aren't core to the logic provided by AroundBlockScan.

In addition to this refactor the ScanHistory class can snapshot a scan. This allows us to be more aggressive with scans in the future as we can now snapshot and rollback if it didn't turn out the way we wanted.

The change comes with a minor perf impact:

before: 5.092678   0.104299   5.196977 (  5.226494)
after: 5.128536   0.099871   5.228407 (  5.249542)

This represents a 0.996x change in speed (where 1x would be no change and 2x would be twice as fast). This is a 0.38% decrease in performance which is negligible. It's likely coming from the extra blocks being created while scanning. This is negligible and if the history feature works well we might be able to make better block decisions which is means fewer calls to ripper which is the biggest bottleneck.

While this doesn't fix https://github.com/ruby/syntax_suggest/issues/187 it's a good intermediate step that will hopefully make working on that issue easier.

https://github.com/ruby/syntax_suggest/commit/ad8487d8aa

4 files changed, 259 insertions, 151 deletions

diff --git a/lib/syntax_suggest/around_block_scan.rb b/lib/syntax_suggest/around_block_scan.rb
index fe63470dee..21f309bf11 100644
--- a/lib/syntax_suggest/around_block_scan.rb
+++ b/lib/syntax_suggest/around_block_scan.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require_relative "scan_history"
+
 module SyntaxSuggest
   # This class is useful for exploring contents before and after
   # a block
@@ -24,22 +26,17 @@ module SyntaxSuggest
   #   puts scan.before_index # => 0
   #   puts scan.after_index  # => 3
   #
-  # Contents can also be filtered using AroundBlockScan#skip
-  #
-  # To grab the next surrounding indentation use AroundBlockScan#scan_adjacent_indent
   class AroundBlockScan
     def initialize(code_lines:, block:)
       @code_lines = code_lines
-      @orig_before_index = block.lines.first.index
-      @orig_after_index = block.lines.last.index
       @orig_indent = block.current_indent
-      @skip_array = []
-      @after_array = []
-      @before_array = []
-      @stop_after_kw = false
 
-      @force_add_hidden = false
+      @stop_after_kw = false
       @force_add_empty = false
+      @force_add_hidden = false
+      @target_indent = nil
+
+      @scanner = ScanHistory.new(code_lines: code_lines, block: block)
     end
 
     # When using this flag, `scan_while` will
@@ -89,47 +86,34 @@ module SyntaxSuggest
     # stopping if we've found a keyword/end mis-match in one direction
     # or the other.
     def scan_while
-      stop_next = false
-      kw_count = 0
-      end_count = 0
-      index = before_lines.reverse_each.take_while do |line|
-        next false if stop_next
-        next true if @force_add_hidden && line.hidden?
-        next true if @force_add_empty && line.empty?
+      stop_next_up = false
+      stop_next_down = false
 
-        kw_count += 1 if line.is_kw?
-        end_count += 1 if line.is_end?
-        if @stop_after_kw && kw_count > end_count
-          stop_next = true
-        end
+      @scanner.scan(
+        up: ->(line, kw_count, end_count) {
+          next false if stop_next_up
+          next true if @force_add_hidden && line.hidden?
+          next true if @force_add_empty && line.empty?
 
-        yield line
-      end.last&.index
+          if @stop_after_kw && kw_count > end_count
+            stop_next_up = true
+          end
 
-      if index && index < before_index
-        @before_index = index
-      end
-
-      stop_next = false
-      kw_count = 0
-      end_count = 0
-      index = after_lines.take_while do |line|
-        next false if stop_next
-        next true if @force_add_hidden && line.hidden?
-        next true if @force_add_empty && line.empty?
+          yield line
+        },
+        down: ->(line, kw_count, end_count) {
+          next false if stop_next_down
+          next true if @force_add_hidden && line.hidden?
+          next true if @force_add_empty && line.empty?
 
-        kw_count += 1 if line.is_kw?
-        end_count += 1 if line.is_end?
-        if @stop_after_kw && end_count > kw_count
-          stop_next = true
-        end
+          if @stop_after_kw && end_count > kw_count
+            stop_next_down = true
+          end
 
-        yield line
-      end.last&.index
+          yield line
+        }
+      )
 
-      if index && index > after_index
-        @after_index = index
-      end
       self
     end
 
@@ -160,79 +144,104 @@ module SyntaxSuggest
     #     4    def eat
     #     5    end
     #
-    def capture_neighbor_context
+    def capture_before_after_kws
       lines = []
-      kw_count = 0
-      end_count = 0
-      before_lines.reverse_each do |line|
-        next if line.empty?
-        break if line.indent < @orig_indent
-        next if line.indent != @orig_indent
-
-        kw_count += 1 if line.is_kw?
-        end_count += 1 if line.is_end?
-        if kw_count != 0 && kw_count == end_count
-          lines << line
-          break
-        end
-
-        lines << line if line.is_kw? || line.is_end?
-      end
-
-      lines.reverse!
-
-      kw_count = 0
-      end_count = 0
-      after_lines.each do |line|
-        next if line.empty?
-        break if line.indent < @orig_indent
-        next if line.indent != @orig_indent
-
-        kw_count += 1 if line.is_kw?
-        end_count += 1 if line.is_end?
-        if kw_count != 0 && kw_count == end_count
-          lines << line
-          break
-        end
-
-        lines << line if line.is_kw? || line.is_end?
-      end
+      up_stop_next = false
+      down_stop_next = false
+      @scanner.commit_if_changed
 
+      lines = []
+      @scanner.scan(
+        up: ->(line, kw_count, end_count) {
+          break if up_stop_next
+          next true if line.empty?
+          break if line.indent < @orig_indent
+          next true if line.indent != @orig_indent
+
+          # If we're going up and have one complete kw/end pair, stop
+          if kw_count != 0 && kw_count == end_count
+            lines << line
+            break
+          end
+
+          lines << line if line.is_kw? || line.is_end?
+        },
+        down: ->(line, kw_count, end_count) {
+          break if down_stop_next
+          next true if line.empty?
+          break if line.indent < @orig_indent
+          next true if line.indent != @orig_indent
+
+          # if we're going down and have one complete kw/end pair,stop
+          if kw_count != 0 && kw_count == end_count
+            lines << line
+            break
+          end
+
+          lines << line if line.is_kw? || line.is_end?
+        }
+      )
+      @scanner.try_rollback
       lines
     end
 
     # Shows the context around code provided by "falling" indentation
     #
-    # Converts:
     #
+    # If this is the original code lines:
+    #
+    #   class OH
+    #     def hello
     #       it "foo" do
+    #     end
+    #   end
     #
-    # into:
+    # And this is the line that is captured
+    #
+    #       it "foo" do
+    #
+    # It will yield its surrounding context:
     #
     #   class OH
     #     def hello
-    #       it "foo" do
     #     end
     #   end
     #
+    # Example:
+    #
+    #   AroundBlockScan.new(
+    #       block: block,
+    #       code_lines: @code_lines
+    #   ).on_falling_indent do |line|
+    #     @lines_to_output << line
+    #   end
+    #
     def on_falling_indent
-      last_indent = @orig_indent
-      before_lines.reverse_each do |line|
-        next if line.empty?
-        if line.indent < last_indent
-          yield line
-          last_indent = line.indent
-        end
-      end
-
-      last_indent = @orig_indent
-      after_lines.each do |line|
-        next if line.empty?
-        if line.indent < last_indent
-          yield line
-          last_indent = line.indent
-        end
-      end
+      last_indent_up = @orig_indent
+      last_indent_down = @orig_indent
+
+      @scanner.commit_if_changed
+      @scanner.scan(
+        up: ->(line, _, _) {
+          next true if line.empty?
+
+          if line.indent < last_indent_up
+            yield line
+            last_indent_up = line.indent
+          end
+          true
+        },
+        down: ->(line, _, _) {
+          next true if line.empty?
+          if line.indent < last_indent_down
+            yield line
+            last_indent_down = line.indent
+          end
+          true
+        }
+      )
+      @scanner.try_rollback
+      self
     end
 
     # Scanning is intentionally conservative because
@@ -267,18 +276,24 @@ module SyntaxSuggest
       return self if kw_count == end_count # nothing to balance
 
       # More ends than keywords, check if we can balance expanding up
+      next_up = @scanner.next_up
+      next_down = @scanner.next_down
       if (end_count - kw_count) == 1 && next_up
-        return self unless next_up.is_kw?
-        return self unless next_up.indent >= @orig_indent
-
-        @before_index = next_up.index
+        if next_up.is_kw? && next_up.indent >= @orig_indent
+          @scanner.scan(
+            up: ->(line, _, _) { line == next_up },
+            down: ->(line, _, _) { false }
+          )
+        end
 
       # More keywords than ends, check if we can balance by expanding down
       elsif (kw_count - end_count) == 1 && next_down
-        return self unless next_down.is_end?
-        return self unless next_down.indent >= @orig_indent
-
-        @after_index = next_down.index
+        if next_down.is_end? && next_down.indent >= @orig_indent
+          @scanner.scan(
+            up: ->(line, _, _) { false },
+            down: ->(line) { line == next_down }
+          )
+        end
       end
       self
     end
@@ -286,19 +301,8 @@ module SyntaxSuggest
     # Finds code lines at the same or greater indentation and adds them
     # to the block
     def scan_neighbors_not_empty
-      scan_while { |line| line.not_empty? && line.indent >= @orig_indent }
-    end
-
-    # Returns the next line to be scanned above the current block.
-    # Returns `nil` if at the top of the document already
-    def next_up
-      @code_lines[before_index.pred]
-    end
-
-    # Returns the next line to be scanned below the current block.
-    # Returns `nil` if at the bottom of the document already
-    def next_down
-      @code_lines[after_index.next]
+      @target_indent = @orig_indent
+      scan_while { |line| line.not_empty? && line.indent >= @target_indent }
     end
 
     # Scan blocks based on indentation of next line above/below block
@@ -310,11 +314,12 @@ module SyntaxSuggest
     # the `def/end` lines surrounding a method.
     def scan_adjacent_indent
       before_after_indent = []
-      before_after_indent << (next_up&.indent || 0)
-      before_after_indent << (next_down&.indent || 0)
 
-      indent = before_after_indent.min
-      scan_while { |line| line.not_empty? && line.indent >= indent }
+      before_after_indent << (@scanner.next_up&.indent || 0)
+      before_after_indent << (@scanner.next_down&.indent || 0)
+
+      @target_indent = before_after_indent.min
+      scan_while { |line| line.not_empty? && line.indent >= @target_indent }
 
       self
     end
@@ -331,29 +336,12 @@ module SyntaxSuggest
     # Returns the lines matched by the current scan as an
     # array of CodeLines
     def lines
-      @code_lines[before_index..after_index]
-    end
-
-    # Gives the index of the first line currently scanned
-    def before_index
-      @before_index ||= @orig_before_index
-    end
-
-    # Gives the index of the last line currently scanned
-    def after_index
-      @after_index ||= @orig_after_index
-    end
-
-    # Returns an array of all the CodeLines that exist before
-    # the currently scanned block
-    private def before_lines
-      @code_lines[0...before_index] || []
+      @scanner.lines
     end
 
-    # Returns an array of all the CodeLines that exist after
-    # the currently scanned block
-    private def after_lines
-      @code_lines[after_index.next..-1] || []
+    # Managable rspec errors
+    def inspect
+      "#<#{self.class}:0x0000123843lol >"
     end
   end
 end
diff --git a/lib/syntax_suggest/block_expand.rb b/lib/syntax_suggest/block_expand.rb
index 8431d15edd..e85d286768 100644
--- a/lib/syntax_suggest/block_expand.rb
+++ b/lib/syntax_suggest/block_expand.rb
@@ -125,17 +125,20 @@ module SyntaxSuggest
     #
     # We try to resolve this edge case with `lookahead_balance_one_line` below.
     def expand_neighbors(block)
-      neighbors = AroundBlockScan.new(code_lines: @code_lines, block: block)
+      now = AroundBlockScan.new(code_lines: @code_lines, block: block)
+
+      # Initial scan
+      now
         .force_add_hidden
         .stop_after_kw
         .scan_neighbors_not_empty
 
       # Slurp up empties
-      with_empties = neighbors
+      now
         .scan_while { |line| line.empty? }
 
       # If next line is kw and it will balance us, take it
-      expanded_lines = with_empties
+      expanded_lines = now
         .lookahead_balance_one_line
         .lines
 
diff --git a/lib/syntax_suggest/capture_code_context.rb b/lib/syntax_suggest/capture_code_context.rb
index a618b2ec68..71f5b271b5 100644
--- a/lib/syntax_suggest/capture_code_context.rb
+++ b/lib/syntax_suggest/capture_code_context.rb
@@ -55,6 +55,10 @@ module SyntaxSuggest
         capture_falling_indent(block)
       end
 
+      sorted_lines
+    end
+
+    def sorted_lines
       @lines_to_output.select!(&:not_empty?)
       @lines_to_output.uniq!
       @lines_to_output.sort!
@@ -116,7 +120,7 @@ module SyntaxSuggest
       return unless block.visible_lines.count == 1
 
       around_lines = AroundBlockScan.new(code_lines: @code_lines, block: block)
-        .capture_neighbor_context
+        .capture_before_after_kws
 
       around_lines -= block.lines
 
diff --git a/lib/syntax_suggest/scan_history.rb b/lib/syntax_suggest/scan_history.rb
new file mode 100644
index 0000000000..c5664d0f65
--- /dev/null
+++ b/lib/syntax_suggest/scan_history.rb
@@ -0,0 +1,113 @@
+# frozen_string_literal: true
+
+module SyntaxSuggest
+  # Scans up/down from the given block
+  #
+  # You can snapshot a change by committing it and rolling back.
+  #
+  class ScanHistory
+    attr_reader :before_index, :after_index
+
+    def initialize(code_lines:, block:)
+      @code_lines = code_lines
+      @history = [block]
+      refresh_index
+    end
+
+    def commit_if_changed
+      if changed?
+        @history << CodeBlock.new(lines: @code_lines[before_index..after_index])
+        refresh_index
+      end
+
+      self
+    end
+
+    def try_rollback
+      if @history.length > 1
+        @history.pop
+        refresh_index
+      end
+
+      self
+    end
+
+    def changed?
+      @before_index != current.lines.first.index ||
+        @after_index != current.lines.last.index
+    end
+
+    # Iterates up and down
+    #
+    # Returns line, kw_count, end_count for each iteration
+    def scan(up:, down:)
+      kw_count = 0
+      end_count = 0
+
+      up_index = before_lines.reverse_each.take_while do |line|
+        kw_count += 1 if line.is_kw?
+        end_count += 1 if line.is_end?
+        up.call(line, kw_count, end_count)
+      end.last&.index
+
+      kw_count = 0
+      end_count = 0
+
+      down_index = after_lines.each.take_while do |line|
+        kw_count += 1 if line.is_kw?
+        end_count += 1 if line.is_end?
+        down.call(line, kw_count, end_count)
+      end.last&.index
+
+      @before_index = if up_index && up_index < @before_index
+        up_index
+      else
+        @before_index
+      end
+
+      @after_index = if down_index && down_index > @after_index
+        down_index
+      else
+        @after_index
+      end
+
+      self
+    end
+
+    def next_up
+      return nil if @before_index <= 0
+
+      @code_lines[@before_index - 1]
+    end
+
+    def next_down
+      return nil if @after_index >= @code_lines.length
+
+      @code_lines[@after_index + 1]
+    end
+
+    def lines
+      @code_lines[@before_index..@after_index]
+    end
+
+    private def before_lines
+      @code_lines[0...@before_index] || []
+    end
+
+    # Returns an array of all the CodeLines that exist after
+    # the currently scanned block
+    private def after_lines
+      @code_lines[@after_index.next..-1] || []
+    end
+
+    private def current
+      @history.last
+    end
+
+    private def refresh_index
+      @before_index = current.lines.first.index
+      @after_index = current.lines.last.index
+      self
+    end
+  end
+end