[ruby/syntax_suggest] Fix sibling bug to #177

While #177 is reported as being caused by a comment, the underlying behavior is a problem due to the newline that we generated (from a comment). The prior commit fixed that problem by preserving whitespace before the comment. That guarantees that a block will form there from the frontier before it will be expanded there via a "neighbors" method. Since empty lines are valid ruby code, it will be hidden and be safe.

## Problem setup

This failure mode is not fixed by the prior commit, because the indentation is 0. To provide good results, we must make the algorithm less greedy. One heuristic/signal to follow is developer added newlines. If a developer puts a newline between code, it's more likely they're unrelated. For example:

```
port = rand(1000...9999)
stub_request(:any, "localhost:#{port}")

query = Cutlass::FunctionQuery.new(
  port: port
).call

expect(WebMock).to have_requested(:post, "localhost:#{port}").
  with(body: "{}")
```

This code is split into three chunks by the developer. Each are likely (but not guaranteed) to be intended to stand on their own (in terms of syntax). This behavior is good for scanning neighbors (same indent or higher) within a method, but bad for parsing neighbors across methods.

## Problem

Code is expanded to capture all neighbors, and then it decreases indent level which allows it to capture surrounding scope (think moving from within the method to also capturing the `def/end` definition. Once the indentation level has been increased, we go back to scanning neighbors, but now neighbors also contain keywords.

For example:

```
  1 def bark
  2
  3 end
  4
  5 def sit
  6 end
```

In this case if lines 4, 5, and 6 are in a block when it tries to expand neighbors it will expand up. If it stops after line 2 or 3 it may cause problems since there's a valid kw/end pair, but the block will be checked without it.

TLDR; It's good to stop scanning code after hitting a newline when you're in a method...it causes a problem scanning code between methods when everything inside of one of the methods is an empty line.

In this case it grabs the end on line 3 and since the problem was an extra end, the program now compiles correctly. It incorrectly assumes that the block it captured was causing the problem.

## Extra bit of context

One other technical detail is that after we've decided to stop scanning code for a new neighbor block expansion, we look around the block and grab any empty newlines. Basically adding empty newlines before of after a code block do not affect the parsing of that block.

## The fix

Since we know that this problem only happens when there's a newline inside of a method and we know this particular failure mode is due to having an invalid block (capturing an extra end, but not it's keyword) we have all the metadata we need to detect this scenario and correct it.

We know that the next line above our block must be code or empty (since we grabbed extra newlines). Same for code below it. We can count all the keywords and ends in the block. If they are balanced, it's likely (but not guaranteed) we formed the block correctly. If they're imbalanced, look above or below (depending on the nature of the imbalance), check to see if adding that line would balance the count.

This concept of balance and "leaning" comes from work in https://github.com/ruby/syntax_suggest/pull/152 and has proven useful, but not been formally introduced into the main branch.

## Outcome

Adding this extra check introduced no regressions and fixed the test case. It might be possible there's a mirror or similar problem that we're not handling. That will come out in time. It might also be possible that this causes a worse case in some code not under test. That too would come out in time.

One other possible concern to adding logic in this area (which is a hot codepath), is performance. This extra count check will be performed for every block. In general the two most helpful performance strategies I've found are reducing total number of blocks (therefore reducing overall N internal iterations) and making better matches (the parser to determine if a close block is valid or not is a major bottleneck. If we can split valid code into valid blocks, then it's only evaluated by the parser once, where as invalid code must be continuously re-checked by the parser until it becomes valid, or is determined to be the cause of the core problem.

This extra logic should very rarely result in a change, but when it does it should tend to produce slightly larger blocks (by one line) and more accurate blocks.

Informally it seems to have no impact on performance:

``
This branch:
DEBUG_DISPLAY=1 bundle exec rspec spec/ --format=failures  3.01s user 1.62s system 113% cpu 4.076 total
```

```
On main:
DEBUG_DISPLAY=1 bundle exec rspec spec/ --format=failures  3.02s user 1.64s system 113% cpu 4.098 total
```

https://github.com/ruby/syntax_suggest/commit/13739c6946

lib/syntax_suggest/around_block_scan.rb

@@ -61,7 +61,6 @@ module SyntaxSuggest
 
     def scan_while
       stop_next = false
-
       kw_count = 0
       end_count = 0
       index = before_lines.reverse_each.take_while do |line|
@@ -166,7 +165,55 @@ module SyntaxSuggest
       end
     end
 
-    def scan_neighbors
+    # Scanning is intentionally conservative because
+    # we have no way of rolling back an agressive block (at this time)
+    #
+    # If a block was stopped for some trivial reason, (like an empty line)
+    # but the next line would have caused it to be balanced then we
+    # can check that condition and grab just one more line either up or
+    # down.
+    #
+    # For example, below if we're scanning up, line 2 might cause
+    # the scanning to stop. This is because empty lines might
+    # denote logical breaks where the user intended to chunk code
+    # which is a good place to stop and check validity. Unfortunately
+    # it also means we might have a "dangling" keyword or end.
+    #
+    #   1 def bark
+    #   2
+    #   3 end
+    #
+    # If lines 2 and 3 are in the block, then when this method is
+    # run it would see it is unbalanced, but that acquiring line 1
+    # would make it balanced, so that's what it does.
+    def lookahead_balance_one_line
+      kw_count = 0
+      end_count = 0
+      lines.each do |line|
+        kw_count += 1 if line.is_kw?
+        end_count += 1 if line.is_end?
+      end
+
+      return self if kw_count == end_count # nothing to balance
+
+      # More ends than keywords, check if we can balance expanding up
+      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
+
+      # 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
+      end
+      self
+    end
+
+    def scan_neighbors_not_empty
       scan_while { |line| line.not_empty? && line.indent >= @orig_indent }
     end
 

lib/syntax_suggest/block_expand.rb

@@ -35,14 +35,31 @@ module SyntaxSuggest
       @code_lines = code_lines
     end
 
+    # Main interface. Expand current indentation, before
+    # expanding to a lower indentation
     def call(block)
       if (next_block = expand_neighbors(block))
-        return next_block
+        next_block
+      else
+        expand_indent(block)
       end
-
-      expand_indent(block)
     end
 
+    # Expands code to the next lowest indentation
+    #
+    # For example:
+    #
+    #   1 def dog
+    #   2   print "dog"
+    #   3 end
+    #
+    # If a block starts on line 2 then it has captured all it's "neighbors" (code at
+    # the same indentation or higher). To continue expanding, this block must capture
+    # lines one and three which are at a different indentation level.
+    #
+    # This method allows fully expanded blocks to decrease their indentation level (so
+    # they can expand to capture more code up and down). It does this conservatively
+    # as there's no undo (currently).
     def expand_indent(block)
       AroundBlockScan.new(code_lines: @code_lines, block: block)
         .skip(:hidden?)
@@ -51,14 +68,82 @@ module SyntaxSuggest
         .code_block
     end
 
+    # A neighbor is code that is at or above the current indent line.
+    #
+    # First we build a block with all neighbors. If we can't go further
+    # then we decrease the indentation threshold and expand via indentation
+    # i.e. `expand_indent`
+    #
+    # Handles two general cases.
+    #
+    # ## Case #1: Check code inside of methods/classes/etc.
+    #
+    # It's important to note, that not everything in a given indentation level can be parsed
+    # as valid code even if it's part of valid code. For example:
+    #
+    #   1 hash = {
+    #   2   name: "richard",
+    #   3   dog: "cinco",
+    #   4 }
+    #
+    # In this case lines 2 and 3 will be neighbors, but they're invalid until `expand_indent`
+    # is called on them.
+    #
+    # When we are adding code within a method or class (at the same indentation level),
+    # use the empty lines to denote the programmer intended logical chunks.
+    # Stop and check each one. For example:
+    #
+    #   1 def dog
+    #   2   print "dog"
+    #   3
+    #   4   hash = {
+    #   5 end
+    #
+    # If we did not stop parsing at empty newlines then the block might mistakenly grab all
+    # the contents (lines 2, 3, and 4) and report them as being problems, instead of only
+    # line 4.
+    #
+    # ## Case #2: Expand/grab other logical blocks
+    #
+    # Once the search algorithm has converted all lines into blocks at a given indentation
+    # it will then `expand_indent`. Once the blocks that generates are expanded as neighbors
+    # we then begin seeing neighbors being other logical blocks i.e. a block's neighbors
+    # may be another method or class (something with keywords/ends).
+    #
+    # For example:
+    #
+    #   1 def bark
+    #   2
+    #   3 end
+    #   4
+    #   5 def sit
+    #   6 end
+    #
+    # In this case if lines 4, 5, and 6 are in a block when it tries to expand neighbors
+    # it will expand up. If it stops after line 2 or 3 it may cause problems since there's a
+    # valid kw/end pair, but the block will be checked without it.
+    #
+    # We try to resolve this edge case with `lookahead_balance_one_line` below.
     def expand_neighbors(block)
-      expanded_lines = AroundBlockScan.new(code_lines: @code_lines, block: block)
+      neighbors = AroundBlockScan.new(code_lines: @code_lines, block: block)
         .skip(:hidden?)
         .stop_after_kw
-        .scan_neighbors
-        .scan_while { |line| line.empty? } # Slurp up empties
+        .scan_neighbors_not_empty
+
+      # Slurp up empties
+      with_empties = neighbors
+        .scan_while { |line| line.empty? }
+
+      # If next line is kw and it will balance us, take it
+      expanded_lines = with_empties
+        .lookahead_balance_one_line
         .lines
 
+      # Don't allocate a block if it won't be used
+      #
+      # If nothing was taken, return nil to indicate that status
+      # used in `def call` to determine if
+      # we need to expand up/out (`expand_indent`)
       if block.lines == expanded_lines
         nil
       else

lib/syntax_suggest/capture_code_context.rb

@@ -76,7 +76,6 @@ module SyntaxSuggest
     #     end
     #   end
     #
-    #
     def capture_falling_indent(block)
       AroundBlockScan.new(
         block: block,

spec/syntax_suggest/integration/syntax_suggest_spec.rb

@@ -234,5 +234,30 @@ module SyntaxSuggest
         > 10  end # extra end
       EOM
     end
+
+    it "space inside of a method" do
+      source = <<~'EOM'
+        class Dog       #  1
+          def bark      #  2
+
+          end           #  4
+
+          def sit       #  6
+            print "sit" #  7
+          end           #  8
+        end             #  9
+        end # extra end
+      EOM
+
+      io = StringIO.new
+      SyntaxSuggest.call(
+        io: io,
+        source: source
+      )
+      out = io.string
+      expect(out).to include(<<~EOM)
+        > 10  end # extra end
+      EOM
+    end
   end
 end

spec/syntax_suggest/unit/around_block_scan_spec.rb

@@ -13,7 +13,7 @@ module SyntaxSuggest
       code_lines = CodeLine.from_source(source)
       block = CodeBlock.new(lines: code_lines[1])
       expand = AroundBlockScan.new(code_lines: code_lines, block: block)
-        .scan_neighbors
+        .scan_neighbors_not_empty
 
       expect(expand.code_block.to_s).to eq(source)
       expand.scan_while { |line| false }
@@ -151,7 +151,7 @@ module SyntaxSuggest
       expand = AroundBlockScan.new(code_lines: code_lines, block: block)
       expand.skip(:empty?)
       expand.skip(:hidden?)
-      expand.scan_neighbors
+      expand.scan_neighbors_not_empty
 
       expect(expand.code_block.to_s).to eq(<<~EOM.indent(4))
 

spec/syntax_suggest/unit/block_expand_spec.rb

@@ -4,6 +4,36 @@ require_relative "../spec_helper"
 
 module SyntaxSuggest
   RSpec.describe BlockExpand do
+    it "empty line in methods" do
+      source_string = <<~EOM
+        class Dog        # index 0
+          def bark       # index 1
+
+          end            # index 3
+
+          def sit        # index 5
+            print "sit"  # index 6
+          end            # index 7
+        end              # index 8
+        end # extra end
+      EOM
+
+      code_lines = code_line_array(source_string)
+
+      sit = code_lines[4..7]
+      sit.each(&:mark_invisible)
+
+      block = CodeBlock.new(lines: sit)
+      expansion = BlockExpand.new(code_lines: code_lines)
+      block = expansion.expand_neighbors(block)
+
+      expect(block.to_s).to eq(<<~EOM.indent(2))
+          def bark       # index 1
+
+          end            # index 3
+      EOM
+    end
+
     it "captures multiple empty and hidden lines" do
       source_string = <<~EOM
         def foo

spec/syntax_suggest/unit/code_search_spec.rb

@@ -338,7 +338,6 @@ module SyntaxSuggest
             end
           EOM
           search.call
-          puts "done"
 
           expect(search.invalid_blocks.join).to eq(<<~'EOM')
             Foo.call do