Fix nested pseudo pair case

The problem here is that we still have a remove_pseudo_pair situation
two of these lines are valid when paired with above/below, one is not
however when you look for the next `above` it shows `setup_language_pack_environment(`
which is correct, but the below returns `bundle_default_without: "development:test"`
which is technically correct, but not useful to us, we need that node's below


The fix was to re-group the invalid nodes with the original above/below

The downside of this approch is that it may violate expectations of above/below guarantees. The original document can no longer be re-created. It makes things very convenient though so this seems like the right path forward.

It also seems that `invalid_inside_split_pair` and `remove_pseudo_pair` are essentially the same thing, but one has captured the leaning blocks inside of the node's parents while the other simply has them as an above/below reference. 

We might be able to simplify something later. I'm pleased with this result, it isolates exactly just the failing line.


192 examples, 4 failures, 1 pending

Failed examples:

rspec ./spec/integration/ruby_command_line_spec.rb:46 # Requires with ruby cli detects require error and adds a message with auto mode
rspec ./spec/unit/indent_search_spec.rb:1034 # DeadEnd::IndentSearch doesn't scapegoat rescue
rspec ./spec/unit/indent_tree_spec.rb:721 # DeadEnd::IndentTree finds random pipe (|) wildly misindented
rspec ./spec/unit/indent_tree_spec.rb:1052 # DeadEnd::IndentTree syntax_tree.rb.txt for performance validation


It seems like if the fix here was in diagnose, that I need to have a better set of diagnose tests. Considering there are none! It seems like it would be a good idea to start there next time. We can collect cases from the existing indent_tree_spec.

Ultimately we need to assert the indent tree shape/properties rather than what we're currently doing with walking/diagnosing/searching in the name of testing. However there's a dependency resolution problem. We are testing a tree structure so we need an easy way to assert properties of that structure. But changes to the structure change it's properties. In short until we have a working solution the properties we desire won't be 100% clear.

Author: schneems

lib/dead_end/block_node.rb

@@ -6,7 +6,7 @@ module DeadEnd
   # A block node keeps a reference to the block above it
   # and below it. In addition a block can "capture" another
   # block. Block nodes are treated as immutable(ish) so when that happens
-  # a new node is created that contains a refernce to all the blocks it was
+  # a new node is created that contains a reference to all the blocks it was
   # derived from. These are known as a block's "parents".
   #
   # If you walk the parent chain until it ends you'll end up with nodes
@@ -33,7 +33,7 @@ class BlockNode
     #
     #   block = BlockNode.from_blocks([parents[0], parents[2]])
     #   expect(block.leaning).to eq(:equal)
-    def self.from_blocks(parents)
+    def self.from_blocks(parents, above: nil, below: nil)
       lines = []
       while parents.length == 1 && parents.first.parents.any?
         parents = parents.first.parents
@@ -47,8 +47,9 @@ def self.from_blocks(parents)
         block.delete
       end
 
-      above = parents.first.above
-      below = parents.last.below
+      above ||= parents.first.above
+      below ||= parents.last.below
+
 
       parents = [] if parents.length == 1
 

lib/dead_end/diagnose_node.rb

@@ -17,15 +17,15 @@ module DeadEnd
   # The algorithm here is tightly coupled to the nodes produced by the current IndentTree
   # implementation.
   #
-  #
   # Possible problem states:
   #
   # - :self - The block holds no parents, if it holds a problem its in the current node.
   #
   # - :invalid_inside_split_pair - An invalid block is splitting two valid leaning blocks, return the middle.
   #
   # - :remove_pseudo_pair - Multiple invalid blocks in isolation are present, but when paired with external leaning
-  #   blocks above and below they become valid. Remove these and group the leftovers together. i.e. `else/ensure/rescue`.
+  #   blocks above and below they become valid. Remove these and group the leftovers together. i.e. don't
+  #   scapegoat `else/ensure/rescue`, remove them from the block and retry with whats leftover.
   #
   # - :extract_from_multiple - Multiple invalid blocks in isolation are present, but we were able to find one that could be removed
   #   to make a valid set along with outer leaning i.e. `[`, `in)&lid` , `vaild`, `]`. Different from :invalid_inside_split_pair because
@@ -65,7 +65,7 @@ def call
       @next = if @problem == :multiple_invalid_parents
         invalid.map { |b| BlockNode.from_blocks([b]) }
       else
-        [BlockNode.from_blocks(invalid)]
+        invalid
       end
 
       self
@@ -86,32 +86,35 @@ def call
       diagnose_one_or_more_parents
     end
 
+    # Diagnose left/right
+    #
+    # Handles cases where the block is made up of a several nodes and is book ended by
+    # nodes leaning in the correct direction that pair with one another. For example [`{`, `b@&[d`, `}`]
+    #
+    # This is different from above/below which also has matching blocks, but those are outside of the current
+    # block array (they are above and below it respectively)
+    #
     # ## (:invalid_inside_split_pair) Handle case where keyword/end (or any pair) is falsely reported as invalid in isolation but
     # holds a syntax error inside of it.
     #
     # Example:
     #
     # ```
-    # def cow # left, invalid in isolation, valid when paired with end
-    # ```
-    #
-    # ```
+    # def cow        # left, invalid in isolation, valid when paired with end
     #   inv&li) code # Actual problem to be isolated
-    # ```
-    #
-    # ```
-    # end # right, invalid in isolation, valid when paired with def
+    # end            # right, invalid in isolation, valid when paired with def
     # ```
     private def diagnose_left_right
       invalid = block.parents.select(&:invalid?)
+      return false if invalid.length < 3
 
       left = invalid.detect { |block| block.leaning == :left }
       right = invalid.reverse_each.detect { |block| block.leaning == :right }
 
-      if left && right && invalid.length >= 3 && BlockNode.from_blocks([left, right]).valid?
+      if left && right && BlockNode.from_blocks([left, right]).valid?
         @problem = :invalid_inside_split_pair
 
-        invalid.reject! { |x| x == left || x == right }
+        invalid.reject! { |b| b == left || b == right }
 
         # If the left/right was not mapped properly or we've accidentally got a :multiple_invalid_parents
         # we can get a false positive, double check the invalid lines fully capture the problem
@@ -130,16 +133,10 @@ def call
     # Example:
     #
     # ```
-    # def cow # above
-    # ```
-    #
-    # ```
+    # def cow         # above
     #   print inv&li) # Actual problem
     # rescue => e     # Invalid in isolation, valid when paired with above/below
-    # ```
-    #
-    # ```
-    # end # below
+    # end             # below
     # ```
     #
     # ## (:extract_from_multiple) Handle syntax seems fine in isolation, but not when combined with above/below leaning blocks
@@ -148,22 +145,16 @@ def call
     #
     # ```
     # [ # above
-    # ```
-    #
-    # ```
     #    missing_comma_not_okay
     #    missing_comma_okay
-    # ```
-    #
-    # ```
     # ] # below
     # ```
+    #
     private def diagnose_above_below
       invalid = block.parents.select(&:invalid?)
 
       above = block.above if block.above&.leaning == :left
       below = block.below if block.below&.leaning == :right
-
       return false if above.nil? || below.nil?
 
       if invalid.reject! { |block|
@@ -172,11 +163,17 @@ def call
          }
 
         if invalid.any?
+          # At this point invalid array was reduced and represents only
+          # nodes that are invalid when paired with it's above/below
+          # however, we may need to split the node apart again
           @problem = :remove_pseudo_pair
-          invalid
-        else
 
+          [BlockNode.from_blocks(invalid, above: above, below: below)]
+        else
           invalid = block.parents.select(&:invalid?)
+
+          # If we can remove one node from many blocks to make the other blocks valid then, that
+          # block must be the problem
           if (b = invalid.detect { |b| BlockNode.from_blocks([above, invalid - [b], below].flatten).valid? })
             @problem = :extract_from_multiple
             [b]
@@ -189,6 +186,7 @@ def call
     private def diagnose_one_or_more_parents
       invalid = block.parents.select(&:invalid?)
       @problem = if invalid.length > 1
+
         :multiple_invalid_parents
       else
         :one_invalid_parent

lib/dead_end/journey.rb

@@ -8,7 +8,7 @@ module DeadEnd
   # valid code from it's parent
   #
   #  node = tree.root
-  #  journey = Journe.new(node)
+  #  journey = Journey.new(node)
   #  journey << Step.new(node.parents[0])
   #  expect(journey.node).to eq(node.parents[0])
   #

spec/unit/indent_tree_spec.rb

@@ -625,7 +625,7 @@ def compile
             setup_language_pack_environment(
               ruby_layer_path: File.expand_path("."),
               gem_layer_path: File.expand_path("."),
-              bundle_path: "vendor/bundle", }
+              bundle_path: "vendor/bundle", } # problem
               bundle_default_without: "development:test"
             )
             allow_git do
@@ -672,21 +672,49 @@ def compile
       expect(diagnose.problem).to eq(:one_invalid_parent)
       node = diagnose.next[0]
 
+      expect(node.to_s).to eq(<<~'EOM'.indent(4))
+        setup_language_pack_environment(
+          ruby_layer_path: File.expand_path("."),
+          gem_layer_path: File.expand_path("."),
+          bundle_path: "vendor/bundle", } # problem
+          bundle_default_without: "development:test"
+        )
+      EOM
+
       diagnose = DiagnoseNode.new(node).call
       expect(diagnose.problem).to eq(:invalid_inside_split_pair)
       node = diagnose.next[0]
 
+      expect(node.to_s).to eq(<<~'EOM'.indent(6))
+        ruby_layer_path: File.expand_path("."),
+        gem_layer_path: File.expand_path("."),
+        bundle_path: "vendor/bundle", } # problem
+        bundle_default_without: "development:test"
+      EOM
+
       diagnose = DiagnoseNode.new(node).call
+      node = diagnose.next[0]
       expect(diagnose.problem).to eq(:remove_pseudo_pair)
-      expect(node.parents.length).to eq(4)
+
+      expect(node.to_s).to eq(<<~'EOM'.indent(6))
+        ruby_layer_path: File.expand_path("."),
+        gem_layer_path: File.expand_path("."),
+        bundle_path: "vendor/bundle", } # problem
+      EOM
 
       diagnose = DiagnoseNode.new(node).call
       node = diagnose.next[0]
+      expect(diagnose.problem).to eq(:remove_pseudo_pair)
+
+      expect(node.to_s).to eq(<<~'EOM'.indent(6))
+          bundle_path: "vendor/bundle", } # problem
+      EOM
 
       diagnose = DiagnoseNode.new(node).call
       expect(diagnose.problem).to eq(:self)
+
       expect(node.to_s).to eq(<<~'EOM'.indent(6))
-        bundle_path: "vendor/bundle", }
+          bundle_path: "vendor/bundle", } # problem
       EOM
     end