[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

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
+      stop_next_up = false
-      kw_count = 0
+      stop_next_down = false
-      end_count = 0
+
-      index = before_lines.reverse_each.take_while do |line|
+      @scanner.scan(
-        next false if stop_next
+        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?
 
-        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
+            stop_next_up = true
           end
 
           yield line
-      end.last&.index
+        },
-
+        down: ->(line, kw_count, end_count) {
-      if index && index < before_index
+          next false if stop_next_down
-        @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?
 
-        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
+            stop_next_down = true
           end
 
           yield line
-      end.last&.index
+        }
+      )
 
-      if index && index > after_index
-        @after_index = index
-      end
       self
     end
 
@@ -160,54 +144,51 @@ module SyntaxSuggest
     #     4    def eat
     #     5    end
     #
-    def capture_neighbor_context
+    def capture_before_after_kws
       lines = []
-      kw_count = 0
+      up_stop_next = false
-      end_count = 0
+      down_stop_next = false
-      before_lines.reverse_each do |line|
+      @scanner.commit_if_changed
-        next if line.empty?
-        break if line.indent < @orig_indent
-        next if line.indent != @orig_indent
 
-        kw_count += 1 if line.is_kw?
+      lines = []
-        end_count += 1 if line.is_end?
+      @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?
-      end
+        },
-
+        down: ->(line, kw_count, end_count) {
-      lines.reverse!
+          break if down_stop_next
-
+          next true if line.empty?
-      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
+          next true if line.indent != @orig_indent
 
-        kw_count += 1 if line.is_kw?
+          # if we're going down and have one complete kw/end pair,stop
-        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
+        }
-
+      )
+      @scanner.try_rollback
       lines
     end
 
     # Shows the context around code provided by "falling" indentation
     #
-    # Converts:
     #
-    #       it "foo" do
+    # If this is the original code lines:
-    #
-    # into:
     #
     #   class OH
     #     def hello
@@ -215,24 +196,52 @@ module SyntaxSuggest
     #     end
     #   end
     #
+    # And this is the line that is captured
+    #
+    #       it "foo" do
+    #
+    # It will yield its surrounding context:
+    #
+    #   class OH
+    #     def hello
+    #     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
+      last_indent_up = @orig_indent
-      before_lines.reverse_each do |line|
+      last_indent_down = @orig_indent
-        next if line.empty?
-        if line.indent < last_indent
-          yield line
-          last_indent = line.indent
-        end
-      end
 
-      last_indent = @orig_indent
+      @scanner.commit_if_changed
-      after_lines.each do |line|
+      @scanner.scan(
-        next if line.empty?
+        up: ->(line, _, _) {
-        if line.indent < last_indent
+          next true if line.empty?
+
+          if line.indent < last_indent_up
             yield line
-          last_indent = line.indent
+            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?
+        if next_up.is_kw? && next_up.indent >= @orig_indent
-        return self unless next_up.indent >= @orig_indent
+          @scanner.scan(
-
+            up: ->(line, _, _) { line == next_up },
-        @before_index = next_up.index
+            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?
+        if next_down.is_end? && next_down.indent >= @orig_indent
-        return self unless next_down.indent >= @orig_indent
+          @scanner.scan(
-
+            up: ->(line, _, _) { false },
-        @after_index = next_down.index
+            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 }
+      @target_indent = @orig_indent
-    end
+      scan_while { |line| line.not_empty? && line.indent >= @target_indent }
-
-    # 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]
     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
+      before_after_indent << (@scanner.next_up&.indent || 0)
-      scan_while { |line| line.not_empty? && line.indent >= indent }
+      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]
+      @scanner.lines
     end
 
-    # Gives the index of the first line currently scanned
+    # Managable rspec errors
-    def before_index
+    def inspect
-      @before_index ||= @orig_before_index
+      "#<#{self.class}:0x0000123843lol >"
-    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] || []
-    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
   end
 end

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
 

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
 

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

spec/syntax_suggest/integration/syntax_suggest_spec.rb

@@ -21,9 +21,10 @@ module SyntaxSuggest
             filename: file
           )
         end
+      end
+
       debug_display(io.string)
       debug_display(benchmark)
-      end
 
       expect(io.string).to include(<<~'EOM')
              6  class SyntaxTree < Ripper

spec/syntax_suggest/unit/around_block_scan_spec.rb

@@ -4,6 +4,43 @@ require_relative "../spec_helper"
 
 module SyntaxSuggest
   RSpec.describe AroundBlockScan do
+    it "on_falling_indent" do
+      source = <<~'EOM'
+        class OH
+          def lol
+            print 'lol
+          end
+
+          def hello
+            it "foo" do
+          end
+
+          def yolo
+            print 'haha'
+          end
+        end
+      EOM
+
+      code_lines = CleanDocument.new(source: source).call.lines
+      block = CodeBlock.new(lines: code_lines[6])
+
+      lines = []
+      AroundBlockScan.new(
+        block: block,
+        code_lines: code_lines
+      ).on_falling_indent do |line|
+        lines << line
+      end
+      lines.sort!
+
+      expect(lines.join).to eq(<<~'EOM')
+        class OH
+          def hello
+          end
+        end
+      EOM
+    end
+
     it "continues scan from last location even if scan is false" do
       source = <<~'EOM'
         print 'omg'
@@ -104,8 +141,8 @@ module SyntaxSuggest
       expand = AroundBlockScan.new(code_lines: code_lines, block: block)
       expand.scan_while { true }
 
-      expect(expand.before_index).to eq(0)
+      expect(expand.lines.first.index).to eq(0)
-      expect(expand.after_index).to eq(6)
+      expect(expand.lines.last.index).to eq(6)
       expect(expand.code_block.to_s).to eq(source_string)
     end
 

spec/syntax_suggest/unit/capture_code_context_spec.rb

@@ -4,6 +4,32 @@ require_relative "../spec_helper"
 
 module SyntaxSuggest
   RSpec.describe CaptureCodeContext do
+    it "capture_before_after_kws two" do
+      source = <<~'EOM'
+        class OH
+
+          def hello
+
+          def hai
+          end
+        end
+      EOM
+
+      code_lines = CleanDocument.new(source: source).call.lines
+      block = CodeBlock.new(lines: code_lines[2])
+
+      display = CaptureCodeContext.new(
+        blocks: [block],
+        code_lines: code_lines
+      )
+      display.capture_before_after_kws(block)
+      expect(display.sorted_lines.join).to eq(<<~'EOM'.indent(2))
+        def hello
+        def hai
+        end
+      EOM
+    end
+
     it "capture_before_after_kws" do
       source = <<~'EOM'
         def sit