WIP Tree and heuristic based re-write #152
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I used this as a starting point for work on building a tree as it has the LeftRightPairDiff internals I wanted. Basically everything below this is true in isolation but false in its entirety. I ended up not using the BalanceHeuristicExpand as a concept #129. TLDR; I've been trying to optimize a "worst-case" perf scenario (9,000 line file) to get under my 1-second threshold. When I started, it was at ~2 seconds. After this last optimization, it's well under the threshold! ``` Before [main]: 1.22 seconds After [this commit]: 0.67386 seconds ``` > Cumulatively, this is 2.6x faster than 3.1.1 and 47x faster than 1.2.0. ## Profiling before/after Expansion before (called 2,291 times, 42.04% of overall time) Parse before (called 3,315 for 31.16% of overall time)   Expansion after (called 654 times, 29.42% of overall time) Parse after (called 1,507 times for 29.35% of overall time)  > Note that parsing happens within the expansion method call, so while it seems "cheaper" to parse than expand based on this profiling data, it's the parsing that is making expansion slow. ## Goal Make the algorithm faster, so it doesn't timeout even when given a file with 9,000 lines. ## Strategy (background) There are two general strategies for improving dead_end perf: - Reduce/remove calls to Ripper (syntax error detection), as it is slow. For example, not calling Ripper if all code lines have been previously "hidden" (indicating valid code). - Improve computational complexity for non-ripper operations. For example, using a priority heap over sorting an array We call Ripper for 2 cases. Both for individual code blocks to see if it contains a syntax error. We also call Ripper later on the whole document to see if that particular syntax error was making the document unparsable. Ripper is slower to parse more lines, so we only re-parse the entire document when we detect a new invalid block as a prior optimization. If we can build "better" valid blocks, then we call Ripper fewer times on the overall document. If we can build larger blocks, we reduce the overall number of iterations for the algorithm. This property reduces Ripper calls and speeds up general "computational complexity" as there are N fewer operations to perform overall. ## Approach This approach builds on the concept that dead_end is a uniform cost search by adding a "heuristic" (think a-star search) when building blocks. At a high level, a heuristic is a quality measure that can also be incomplete. For a great explanation, see https://www.redblobgames.com/pathfinding/a-star/introduction.html. What heuristic can we add? We know that if the code has an unbalanced pair of special characters, it cannot be valid. For example, this code `{ hello: "world"` is unbalanced. It is missing a `}`. It contains a syntax error due to this imbalance. In the dead_end code, we can count known special pairs using `Ripper.lex` output (which we already have). Here are the currently tracked pairs: - `{}` - `()` - `[]` - keyword/end This information can be used as a heuristic. Code with unbalanced pairs always contains a syntax error, but balanced code may have a different (harder to detect) syntax error. The code's balance hints at how we should expand an existing code block. For example, the code `{ hello: "world"` must expand towards the right/down to be valid. This code would be known as "left-leaning" as it is heavier on the left side. Rather than searching in an arbitrary direction, the heuristic determines the most sensible direction to expand. Previously, we expanded blocks by scanning up and down, counting keyword/end pairs (outside of the block), and looking at indentation. This process was adequate but required that we take small steps and produce small blocks. It also has no concept of if the code it holds is syntactically valid or not until a full Ripper parse is performed. That combination means it can produce invalid code blocks (which can be desirable when hunting for invalid code). But when we want blocks to be valid, we can make more efficient moves. ## Implementation: LexDiffPair helper class I introduced a new class, `LexDiffPair`, to accommodate a heuristic expansion. A `LexDiffPair` can be in one of several states: - Balanced (:equal) - Leaning left (:left) - Leaning right (:right) - Leaning both (:both) > An example of code line leaning both ways would be `) do |x|`. Here the `)` is leaning right (needs to expand up) while the `do` is leaning left (needs to expand down). Each code line generates its own `LexDiffPair`. Internally the information is stored as a diff of a count of each of the above pairs. A positive number indicates left-leaning, a negative number indicates right-leaning, a zero is balanced. This format allows for fast concatenation and comparison. ## BalanceHeuristicExpand When a block is passed to `BalanceHeuristicExpand`, a `LexPairDiff` is created from all of its lines. The code's balance is then used to determine which direction to expand. If the code is leaning left or right, the main goal is to get it back into balance. Continue to expand it until balance is achieved. If the code is already balanced, try to expand it while maintaining balance. For example, add already balanced code and any above/below pair of lines that cancel out. Finally, intentionally knock it off-balance by expanding up and then recurse the process (to ideally re-balance it and then exit). If the code is leaning both ways, try to grab any extra "free" (already balanced) lines. Then expand up and down since it must expand both ways to become valid. As this process is repeated, it should eventually find a match in one direction and then be leaning left or right, which will expand faster on the next iteration (after it is put back on the frontier and popped again later). Example: An invalid code block might come in like this: ``` start_line: keyword.location.start_line, start_char: keyword.location.end_char + 1, end_line: last_node.location.end_line, end_char: last_node.location.end_char ``` And it could expand it to: ``` if exceptions || variable RescueEx.new( exceptions: exceptions, variable: variable, location: Location.new( start_line: keyword.location.start_line, start_char: keyword.location.end_char + 1, end_line: last_node.location.end_line, end_char: last_node.location.end_char ) ) end ``` > Note it would expand it quite a bit more, but I'm abbreviating here. Here's the complete process across several expansions https://gist.github.com/schneems/e62216b610a36a81a98d9d8146b0611a ## When to use heuristic expand After experimentation, the best place to use this new expansion technique was when an existing invalid block comes off of the frontier. This algorithm tends to correct such blocks and balance them out. When a block is "corrected" in this way, it reduces the overall number of times the document must be passed to Ripper (extremely slow). Also, since larger blocks reduce the overall iteration, we try to expand several times (while preserving balance) and take the largest valid expansion. We use the original indentation-based expansion for code blocks that are already valid. The downside of using a heuristic that preserves balance is that ultimately we want the algorithm to generate invalid blocks. The original expansion can produce invalid expansions, which is useful. There is a separate process for adding unvisited lines to the frontier (rather than expanding existing blocks). Unvisited lines are not a good candidate for heuristic expansion as it works a little too well. If we only add "unbalanced" code in an added block, we lose some of the context we desire (see comments for more details in `parse_blocks_from_indent_line`). ## Concerns One concern with this implementation is that calling the heuristic expansion three times was the only way to produce valid results. I'm not sure why. It might be an undiscovered property of the system, or perhaps all of my code examples to date are somehow biased in a specific way. The way to get more information is to put it out into the world and get feedback. Another concern is that there are now two expansion classes. Or three if you count `parse_blocks_from_indent_line`. It's not always clear why you would choose one over another except that "it provides the best outcome". It might be possible to simplify some of this logic or remove or combine competing expansion methods. Hopefully, patterns will emerge pointing to opportunities to guide that usage.
The "left/right" balanced heuristic proved to be to unstable to be used as a block expansion by itself. I found that while I could produce better blocks (valid source code) the old algorithm was optimized to work with itself and not with a better input. Instead of experimenting more with a different expansion algorithm I want to take a new direction and re-write search from the ground up. The idea is this: We need a tree we can search. The more time spent building a higher quality tree the better and more accurate the actual exploration of the tree will be. I've experimented with tree construction before. I know that I want nodes to be able to handle intersection/capture with many other nodes. I know that I want all valid source code to be built into logically separate "chunks" previously I've described these as "pyramids" as they stick out when viewed from the side #118 (comment). This work was done in an exploratory fashion so I'm going back and annotating some of my prior commits to flesh out the notes. Most of these were intended to be refactored out later and are perhaps less "professional" than they would be otherwise. I've decided to leave them in here where their inclusion helps to highlight what guided my decision making process as I was writing the code. I believe that will be more valuable in the long run than deleting all the notes and refactoring into one single large commit (which I usually prefer for open source feature work). All that's to say: I know some of these commits and their messages aren't great, just roll with it, the end result is worth it. ## Goal: Indentation focused blocks - A block's inner are comprised of sub-blocks Ideally I want these to be two separate blocks (certainly not 5 separate lines, that's useless) ``` class Zoo def foo print def bar print end end ``` If we can't do that, then it may be okay to build the (incorrect) match as long as we can determine at what indent level the problem was created, then we can work backwards if "class zoo" is implicated, we know it's missing an end and reverse scan up ## Thinking here Desired properties: - I think we can somehow leverage data about indentation to inform the blocks, We've got two cases we need to handle at the same time: ``` print "one" print "one" print "one" ``` And ``` def one print "one" end def two print "one" end def three Print "three end ``` Both should produce 3 inner "blocks". Since we're operating only at the level of one block at a time, how do we figure out how to generate the "inner". - One option: If we scan ahead to find all blocks we want to join together then we can make one block with ALL of those other sub blocks. - I like that - I want large blocks that can be "zoomed" into to expose the "inner" code. Next task is to look through all existing examples and build demo blocks, develop a feel for what's useful and what isn't then figure out if we can find patterns
I'm sure there are edge cases but I'm liking the properties of the tree.
It lets me view blocks logically the way I construct them in my head (based on indentation).
I may want to make some convenience methods, for instance given the if/else/end cadence I want to quickly view
```
Args.new(parts: [argument], location: argument.location)
Args.new(
parts: arguments.parts << argument,
location: arguments.location.to(argument.location)
)
```
And:
```
if arguments.parts.empty?
else
end
```
If the problem is in the block, this will let me figure out if I need to "zoom" in another level or if I've already isolated the issue. In the current indentation.
So far all my tests are mostly with valid code and mostly regular indentation. I don't know how well these properties will hold when we face some "real" world code cases
Really loving these BlockNode properties I don't want an invalid node to "capture" a valid one. We can prevent this by ensuring that the blocks are always built from the inside out. For example if we're going up and see `)` It's a sign that above us is building a valid block. If we grab it now, it means the the above code couldn't just be ``` ( # inner ) ``` It would have to be ``` ( # inner ) # plus extra stuff here ``` Which we don't want. If all the code is completely valid then we'll eventually build correct blocks. Otherwise (I'm hoping) that we'll preserve relatively logical blocks that isolate their own syntax errors. One unknown is how a :both leaning line would do here. I think if we are expanding up a line leaning "both" would need to expand down so we should capture it. Same follows for expanding down. So I think the current logic will hold, but perhaps there will be edge cases to find. Either way, this is quite an exciting development. I am hopeful that this is the right path. I want to keep working on the tree logic, adding some more tests. Then I want to take a stab at building an algorithm that searches the tree. We can't purely rely on leaning when doing a search, as the syntax error could be due to missing `|` or an extra comma etc. However it might inform us as to which nodes to look at first as long as we fall back to checking all nodes (if those prove to not hold the full set of syntax errors. Also worth noting that I think we'll have to completely re-visit the "capture context" logic. As it was mostly based on the blocks that the prior search produced. This new search will make different shaped blocks. We should optimize for speed and quality. The "bad" blocks returned should contain the error, and we should do it fast. Later on we might want to split the blocks up, say by chunking into logical blocks at the same indentation based off of newline/whitespace as the old algorithm did. This new algorithm throws away empty/double-newline lines currently. That allows us to completely get rid of any "is this line empty" checks which littered the prior algorithm. It's better to normalize and build a general algorithm that works and doesn't have to handle edge cases. Then later go back and clean up the results instead of making the search/tree-building overly complicated just to simplify showing results.
Currently spending 40% of time building the tree in popping off nodes
```
40.12% 0.35% 3.16 0.03 0.00 3.13 23373 DeadEnd::PriorityQueue#pop
```
Spending 9% of time enqueueing nodes. It might be worth it to revisit using an insertion sort (with bsearch_index) to see if it's faster than a heap implementation as we're guaranteeing to search through all elements in the queue.
Currently perf for tree building our target case is:
```
$ be rspec spec/unit/indent_tree_spec.rb:7
Run options: include {:locations=>{"./spec/unit/indent_tree_spec.rb"=>[7]}}
DeadEnd::IndentTree
WIP syntax_tree.rb.txt for performance validation
Finished in 0.75664 seconds (files took 0.14354 seconds to load)
1 example, 0 failures
```
Main has the full search around 1.2~1.1 seconds with the same time to load). So our budget to hit equal perf is 0.45 seconds. I'm hoping we can beat that by a bunch, but we've not even touched ripper yet and it's *supposed* to be the expensive part.
Previously I wasn't using `bsearch_index` which seems like a MASSIVE oversight. However we still see speed gains whenever we don't pop the whole queue.
InsertionSortQueue is much slower on insert, however on insert + pop for all elements it's roughly 2~3x faster than a priority queue (benchmark ips included in spec).
```
$ DEBUG_PERF=1 be rspec spec/unit/priority_queue_spec.rb
DeadEnd::CodeFrontier
Warming up --------------------------------------
Bsearch insertion 18.000 i/100ms
Priority queue 7.000 i/100ms
Calculating -------------------------------------
Bsearch insertion 193.177 (± 3.1%) i/s - 972.000 in 5.037030s
Priority queue 74.128 (± 2.7%) i/s - 371.000 in 5.009844s
Comparison:
Bsearch insertion: 193.2 i/s
Priority queue : 74.1 i/s - 2.61x (± 0.00) slower
```
This reduces the prior stated metric from ~0.75 seconds down to ~0.57 seconds
```
$ rspec ./spec/unit/indent_tree_spec.rb:8
Run options: include {:locations=>{"./spec/unit/indent_tree_spec.rb"=>[8]}}
DeadEnd::IndentTree
WIP syntax_tree.rb.txt for performance validation
Finished in 0.57633 seconds (files took 0.30931 seconds to load)
1 example, 0 failures
```
(Though I'm not sure why "files took" time went up by so much. That seems strange
The prior results are actually MUCH better than initially thought. I forgot to trigger the "deleted" logic that cleans up/removes blocks from the frontier/queue.
That roughly dropped time to build the tree in half which gives us much more time to work with:
```
$ rspec ./spec/unit/indent_tree_spec.rb:8
Run options: include {:locations=>{"./spec/unit/indent_tree_spec.rb"=>[8]}}
DeadEnd::IndentTree
WIP syntax_tree.rb.txt for performance validation
Finished in 0.25907 seconds (files took 0.31338 seconds to load)
1 example, 0 failures
```
Naturally it makes a test fail
rspec ./spec/unit/indent_tree_spec.rb:20 # DeadEnd::IndentTree invalid if/else end with surrounding code
UGHHHH, It might be a good time to implement tree creation tracing.
Here's the problem:
```
=============
Expanding block
location: arguments.location.to(argument.location)
Result:
parts: arguments.parts << argument,
location: arguments.location.to(argument.location)
=============
Expanding block
parts: arguments.parts << argument,
location: arguments.location.to(argument.location)
Result:
Args.new(
parts: arguments.parts << argument,
location: arguments.location.to(argument.location)
)
=============
Expanding block
@location = location
Result:
@arguments = arguments
@block = block
@location = location
=============
Expanding block
@arguments = arguments
@block = block
@location = location
Result:
def initialize(arguments:, block:, location:)
@arguments = arguments
@block = block
@location = location
end
=============
Expanding block
Args.new(
parts: arguments.parts << argument,
location: arguments.location.to(argument.location)
)
Result:
if arguments.parts.empty?
Args.new(parts: [argument], location: argument.location)
else
Args.new(
parts: arguments.parts << argument,
location: arguments.location.to(argument.location)
)
end
```
The block
```
Args.new(parts: [argument], location: argument.location)
```
Needs to be expanded first. It should be based on it's indent/next indent
I can work with that
I need the ability to introspect the tree building process as it's recursive and complex. This commit adds an initial recording mechnism.
Before: ``` 18.11% 0.00% 0.31 0.00 0.00 0.31 1 DeadEnd::BlockDocument#call 18.86% 0.30% 0.32 0.01 0.00 0.32 7711 DeadEnd::InsertionSortQueue#<< ``` After: ``` 17.22% 0.00% 0.29 0.00 0.00 0.29 1 DeadEnd::BlockDocument#call 6.34% 0.10% 0.11 0.00 0.00 0.11 2238 DeadEnd::InsertionSortQueue#<< ```
We're already looping through all lex values, we can capture the left/right count at the same time Before ``` 26.14% 1.45% 0.44 0.02 0.00 0.42 9252 DeadEnd::CodeLine#initialize ``` After ``` 25.32% 1.26% 0.41 0.02 0.00 0.39 9252 DeadEnd::CodeLine#initialize ```
TODO: - Enable separate search/tree reporting - Follow question and answer until we have a reasonable algorithm
I wanted to start with this test case as it has a property that causes it to fail even when most/all other test cases pass. It's the lone holdover that resisted my initial attempts to add a "heuristic expansion" in #129.
When dealing with a non KW problem at the wrong indentation (too low). The next idea is to make the inner/outer logic smarter. ## Primary cases - Problem is within an kw/else/end - Problem is missing kw/end -
This spec causes a massive problem When we have a mis-indentation in the "wrong" direction it causes the blocks to no longer capture their inner contents.
This allows us to match generating next_indent priority with changes to the building of the indentation tree
I am very happy with this direction.
When looking at a node, we can determine if it holds a syntax error or not. If it's invalid it means that either one of its parents are the problem or it is.
Through test based exploration I found several properties that seem to indicate a specific case happened. For instance if only one parent is invalid in isolation, then we should explore just that parent.
I wanted to decouple inspection from modification so that we can look at a node to guess what's wrong with it, without having to take the next step of actually taking an action
to return the invalid inner/parent node. This produces some possible performance problems as some of the checks for diagnose may not be cheap. I'm mitigating that where possible via memoizing expensive values. It's not ideal, but it's holding up okay.
This strategy handles all of our problems like a champ, except for the full `|` example which needs more investigation.
Note to future self we also need to handle:
- actually investigating multiple syntax errors, like if there are two syntax errors in a document
- Handling the `else` case. This is where kw/end are matched but there's one or more dangling pseudo keywords ("ensure", "rescue", "else", "elsif", "when", "retry") left in the middle.
When making block nodes from an array, if the array only contains a single node, then don't duplicate it, use its parents instead. This fixes having to diagnose essentially the same block twice which was happening due to generating blocks.
Works surprisingly well The only thing we're not covering (that I know of) is the case of if an issue exists in more than one block. We need to figure out how to either detect and deal with this case
Still need: Actual multi failure test
When a node has two diverging branches that each contain a syntax error we need to follow both branches where the search forks. This commit adds the ability to fork the search.
Previously I implemented recording/tracing on building the tree, we also need to introspect how the tree is searched. This commit adds that tracing and DRYs up the code a bit. I don't love that there's this DEFAULT_VALUE that needs to be understood by so many abstraction layers. However if we want a false value like `nil` to represent "default state" then it seems to be needed. We also want to be as lazy as possible about tracing because each individual level/layer might need to be invoked with the debug env vars. This is good enough for now. The only other major downside is that the performance of this is quite expensive. For a search that takes half a second with tracing off it takes 1 min 47 seconds with it on.
Previously the BlockNode knew how to both "diagnose" it's own problem as well as how to return its parent that likely holds the core issue. That caused a lot of extra, somewhat confusing code to go into BlockNode that wasn't really the responsibility of that object. This commit introduces a new class `Diagnose` it is responsible for naming the problem with the current node and extracting the likely issue. This makes BlockNode smaller and more focused, it also isolates all the extraction logic to one convienent location. This was done as a pure refactor. After fully extracting the existing logic, I was able to clean up IndentSearch a lot thanks to the new API. It is now cleaner and more focused. The IndentSearch class is now mainly responsible for maintaining the guarantees that each journey only contains steps that would produce a valid document. I also found a problem where we were claiming that a block node was not a leaf even though it was. This was due to passing in a non-empty parents array to the constructor when we shouldn't have. That's now been fixed
When there's a node inside of leaning elements it might look valid in isolation, for example: ``` cat ``` Is valid code but: ``` [ cat dog, bird ] ``` Is not (missing a comma). We are already enumerating each internal element to see if it's valid in isolation i.e.: ``` [ cat ] ``` and ``` [ dog, ] ``` and ``` [ bird ] ``` However due to ruby's permissive rules all of these will be true and no single problem will be reported ## The fix Rather than checking each element in isolation, we can do the opposite. Check if removing that element from the inner block array would produce a valid outcome: ``` [ cat dog, # bird ] # Still invalid ``` ``` [ cat # dog, bird ] # Still invalid ``` ``` [ # cat dog, bird ] # VALID! ``` If one and only one block can be removed to produce a valid outcome then it must hold the problem. Worth noting that it feels like there's a more generic case to handle several of the different diagnose states but I'm unable to find it right now. I would love to clean up the Diagnose class to make some of these checks cleaner/simpler
Renamed the problem symbols to be more indicative of their root causes rather than some obscure thing that only makes sense to me.
This case fails because all "next indent" heuristics for the 3 blocks are exactly the same
```
Block lines: 8..9 (pop)
indent: 2 next_indent: 2
1 describe "things" do
2 it "blerg" do
3 end # one
4
5 it "flerg"
6 end # two
7
❯ 8 it "zlerg" do
❯ 9 end # three
10 end
```
Is expanded to
```
Block lines: 2..9 (expand)
indent: 2 next_indent: 0
1 describe "things" do
❯ 2 it "blerg" do
❯ 3 end # one
4
❯ 5 it "flerg"
❯ 6 end # two
7
❯ 8 it "zlerg" do
❯ 9 end # three
10 end
```
Not great. Also for the case where inner is indented:
```
Block lines: 7..7 (pop)
indent: 4 next_indent: 4
1 describe "things" do
2 it "blerg" do
3 print foo
4 end # one
5
6 it "flerg"
❯ 7 print foo
8 end # two
9
10 it "zlerg" do
11 print foo
12 end # three
13 end
```
Is expanded to:
```
Block lines: 3..7 (expand)
indent: 2 next_indent: 2
1 describe "things" do
2 it "blerg" do
❯ 3 print foo
❯ 4 end # one
5
❯ 6 it "flerg"
❯ 7 print foo
8 end # two
9
10 it "zlerg" do
11 print foo
12 end # three
13 end
```
Which is a valid way to remove the syntax error as it removes the end, but it doesn't do it entirely as expected.
That's the "wrong" end to remove.
The current block node implementation is optimized for the "hard" cases from the old algorithm, but turns out it doesn't do some of the "basic" stuff the old one did.
We've got a few paths to remediate these cases:
- We can solve them "in post" in the same way that Capture Context "fixed" otherwise ambiguous or meh results from the old search
- We can update the way we're building blocks
- We can update the way we're searching blocks
I think each of these cases will need something a little different. For the case where I want to return the line missing a "do", where we're grabbing the correct "end", I think we can/should fix that in post.
There's a lot of other cases where I'm not quite sure.
Also worth mentioning that some of these failure modes may change if any values are placed "inside" them for example:
```
def foo
def blerg
end
```
Returns a match saying the problem is `def foo` but if you put something "in" the `def blerg` it says the problem is `def blerg`
```
def foo
def blerg
print lol
end
```
Let's take it one step at a time, focus on one failure case, see if it breaks other cases when it's fixed...then iterate.
Fixes 6 out of 8 of the prior failures. The concept is quite simple as well. If a node leans one way, capture in the direction needed to theoretically fix it until we find a node that is leaning. It fails though when we look at a larger example because blocks tend to self balance for valid code and this produces very large outputs for large inputs. It's not suitable for a final answer, but it's a good first step. I introduced a failing test with "handles heredocs" to capture that need.
We were making some questionable decisions when building blocks. I refactored out the IndentTree building so that it can be done step by step so we can observe it. This allows for tests that are quite verbose, but should allow for use of intuition "that shouldn't be the next step" as blocks are being built.
I found that some nodes at the same indent were being expanded to cover the entire "inner" before other blocks could capture them. For example in:
```
describe "things" do
it "blerg" do
print foo1
end # one
it "flerg"
print foo2
end # two
it "zlerg" do
print foo3
end # three
end
```
This block:
```
it "zlerg" do
print foo3
end # three
```
Would expand to capture the above `end # two` even though logically it should be captured by the `print foo2` block.
To accommodate this I added an extra condition to block node expansion saying to not capture a node leaning in the opposite direction as expansion if that node is also a leaf. Essentially if a node is leaning right, it needs to expand up. I want either it to expand itself, or for it's "inner" equal block to expand out to capture it.
The other case I added was to enforce the indentation check for nodes leaning opposite of expansion direction, but only on first expansion (other wise we end up with "split" blocks where one block will not engulf all inner blocks.
Those changes exposed a problem with the `next_indent` calculation where sometimes it would come back at a higher value than the current indent which is not correct. Fixing this by adding a final check/guarantee when deriving that value.
This change seems good in isolation but is causing a lot of test failures due to tight coupling between tests and implementation. I need to go back and re-work the tests to see if there's any fundamental "disagreements" or if they just need to be updated to new/better values.
There's one problem fundamental to the :both case that seems not well handled here
Follow the search logic, does it make sense? If not, time to rename and update
Welp the last commit simply is labeled "WIPPPPPPPPPPPPPPPPPPPPPPPPPP" and it was made in February
From a prior commit
```
The other case I added was to enforce the indentation check for nodes leaning opposite of expansion direction, but only on first expansion (other wise
we end up with "split" blocks where one block will not engulf all inner blocks.
Those changes exposed a problem with the `next_indent` calculation where sometimes it would come back at a higher value than the current indent which
is not correct. Fixing this by adding a final check/guarantee when deriving that value.
This change seems good in isolation but is causing a lot of test failures due to tight coupling between tests and implementation. I need to go back an
d re-work the tests to see if there's any fundamental "disagreements" or if they just need to be updated to new/better values.
There's one problem fundamental to the :both case that seems not well handled here
```
192 examples, 4 failures, 1 pending
```
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_tree_spec.rb:614 # DeadEnd::IndentTree doesn't scapegoat rescue
rspec ./spec/unit/indent_tree_spec.rb:693 # DeadEnd::IndentTree finds random pipe (|) wildly misindented
rspec ./spec/unit/indent_tree_spec.rb:1024 # DeadEnd::IndentTree syntax_tree.rb.txt for performance validation
```
schneems
force-pushed
the
schneems/indent_tree
branch
2 times, most recently
from
4123499
to
97698e6
Compare
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.
schneems
force-pushed
the
schneems/indent_tree
branch
from
97698e6
to
02285d2
Compare
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_tree_spec.rb:1048 # DeadEnd::IndentTree syntax_tree.rb.txt for performance validation
When we think that multiple nodes are invalid, check to see if removing one in isolation makes the others valid, if so report that node to be the problem.
```
Finished in 3.58 seconds (files took 0.23647 seconds to load)
192 examples, 1 failure, 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
```
This last failure:
```
1) Requires with ruby cli detects require error and adds a message with auto mode
Failure/Error: expect(out).to include('❯ 5 it "flerg"').once
expected "--> /var/folders/l9/w5ggmcjd28d57p4rwb1m7kph0000gp/T/d20220607-97216-rq4ujg/script.rb\n\nUnmatched `... /var/folders/l9/w5ggmcjd28d57p4rwb1m7kph0000gp/T/d20220607-97216-rq4ujg/require.rb:1:in `<main>'\n" to include "❯ 5 it \"flerg\"" once but it is included 0 times
Diff:
@@ -1,14 +1,27 @@
-❯ 5 it "flerg"
+--> /var/folders/l9/w5ggmcjd28d57p4rwb1m7kph0000gp/T/d20220607-97216-rq4ujg/script.rb
+
+Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ?
+
+ 1 describe "things" do
+ 2 it "blerg" do
+ 3 end
+❯ 7 end
+ 9 it "zlerg" do
+ 10 end
+ 11 end
+/Users/rschneeman/Documents/projects/dead_end/lib/dead_end/core_ext.rb:13:in `load': /var/folders/l9/w5ggmcjd28d57p4rwb1m7kph0000gp/T/d20220607-97216-rq4ujg/script.rb:11: syntax error, unexpected `end', expecting end-of-input (SyntaxError)
+ from /Users/rschneeman/Documents/projects/dead_end/lib/dead_end/core_ext.rb:13:in `load'
+ from /var/folders/l9/w5ggmcjd28d57p4rwb1m7kph0000gp/T/d20220607-97216-rq4ujg/require.rb:1:in `<main>'
# ./spec/integration/ruby_command_line_spec.rb:72:in `block (3 levels) in <module:DeadEnd>'
# ./spec/integration/ruby_command_line_spec.rb:47:in `block (2 levels) in <module:DeadEnd>'
Finished in 0.40714 seconds (files took 0.20038 seconds to load)
1 example, 1 failure
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
```
Happens due to display. We found the correct `end` but are not showing the line missing the `do`. Previously we would have corrected for this in "capture context" which doesn't quite work because we don't just want extra context displayed, we want to point at that being a problem line.
i.e.
```
+ 1 describe "things" do
+ 2 it "blerg" do
+ 3 end
+ 5 it "flerg"
+❯ 7 end
+ 9 it "zlerg" do
+ 10 end
+ 11 end
```
Isn't quite good enough.
Overall performance is great
## Misindent pipe
Before:
```
Unmatched `|', missing `|' ?
❯ 1067 def add_yarn_binary
❯ 1068 return [] if yarn_preinstalled?
❯ 1069 |
❯ 1075 end
```
After
```
16 class LanguagePack::Ruby < LanguagePack::Base
❯ 1069 |
1344 end
```
Unmatched keyword, missing `end' ?
1 Rails.application.routes.draw do
107 constraints -> { Rails.application.config.non_production } do
111 end
❯ 113 namespace :admin do
121 end
## Webmock, missing comma
Before:
```
syntax error, unexpected ':', expecting end-of-input
1 describe "webmock tests" do
22 it "body" do
27 query = Cutlass::FunctionQuery.new(
❯ 28 port: port
❯ 29 body: body
30 ).call
34 end
35 end
```
After:
```
syntax error, unexpected ':', expecting end-of-input
1 describe "webmock tests" do
22 it "body" do
27 query = Cutlass::FunctionQuery.new(
❯ 28 port: port
30 ).call
34 end
35 end
```
## Missing end require tree
Before:
```
Unmatched keyword, missing `end' ?
5 module DerailedBenchmarks
6 class RequireTree
7 REQUIRED_BY = {}
9 attr_reader :name
10 attr_writer :cost
❯ 13 def initialize(name)
❯ 18 def self.reset!
❯ 25 end
73 end
74 end
```
After:
```
Unmatched keyword, missing `end' ?
5 module DerailedBenchmarks
6 class RequireTree
❯ 13 def initialize(name)
18 def self.reset!
25 end
73 end
74 end
```
## Rexe missing end
Before:
```
Unmatched keyword, missing `end' ?
16 class Rexe
❯ 77 class Lookups
❯ 78 def input_modes
❯ 148 end
551 end
```
After:
```
Unmatched keyword, missing `end' ?
16 class Rexe
77 class Lookups
❯ 78 def input_modes
87 def input_formats
94 end
148 end
551 end
```
## Display invalid blocks missing end
Before:
```
Unmatched keyword, missing `end' ?
1 module SyntaxErrorSearch
3 class DisplayInvalidBlocks
❯ 36 def filename
❯ 38 def code_with_filename
❯ 45 end
63 end
64 end
```
After:
```
Unmatched keyword, missing `end' ?
1 module SyntaxErrorSearch
3 class DisplayInvalidBlocks
17 def call
34 end
❯ 36 def filename
38 def code_with_filename
45 end
63 end
64 end
```
## Next steps
Since output performance is great and it looks like this is the only edge case, let's go through the old code base to make sure we're testing all known cases. Once we are satisfied there then the next step is to add some context back into this specific match. It needs to be done outside of capture code context because we need it highlighted.
The purpose of the Ruby CLI tests isn't to check formatting it's to ensure that Dead end integrates with Ruby in the real world. To that end I'm putting the failing test into `integration/dead_end_spec.rb` where it better fits. I've also added a more advanced case that includes internal elements inside of the method that I think should be omitted. The main problem with the existing test cases is that they came from real world scenarios where the prior algorithm performed poorly. Since this is a different algorithm it has different characteristics that cause it to perform poorly in different scenarios. ``` Finished in 3.44 seconds (files took 0.20514 seconds to load) 194 examples, 2 failures, 1 pending Failed examples: rspec ./spec/integration/dead_end_spec.rb:207 # Integration tests that don't spawn a process (like using the cli) missing `do` highlights more than `end` simple rspec ./spec/integration/dead_end_spec.rb:238 # Integration tests that don't spawn a process (like using the cli) missing `do` highlights more than `end`, with internal contents ```
schneems
changed the title
- https://github.com/zombocom/dead_end/blob/622dfddda92e27235425dd5a370618c5257731e8/spec/unit/code_search_spec.rb ``` 22 examples, 9 failures Failed examples: rspec ./spec/integration/dead_end_spec.rb:208 # Integration tests that don't spawn a process (like using the cli) missing `do` highlights more than `end` simple rspec ./spec/integration/dead_end_spec.rb:239 # Integration tests that don't spawn a process (like using the cli) missing `do` highlights more than `end`, with internal contents rspec ./spec/integration/dead_end_spec.rb:302 # Integration tests that don't spawn a process (like using the cli) squished do regression rspec ./spec/integration/dead_end_spec.rb:400 # Integration tests that don't spawn a process (like using the cli) handles no spaces between blocks rspec ./spec/integration/dead_end_spec.rb:447 # Integration tests that don't spawn a process (like using the cli) Format Code blocks real world example rspec ./spec/integration/dead_end_spec.rb:492 # Integration tests that don't spawn a process (like using the cli) returns syntax error in outer block without inner block rspec ./spec/integration/dead_end_spec.rb:514 # Integration tests that don't spawn a process (like using the cli) finds multiple syntax errors rspec ./spec/integration/dead_end_spec.rb:547 # Integration tests that don't spawn a process (like using the cli) finds a naked end rspec ./spec/integration/dead_end_spec.rb:565 # Integration tests that don't spawn a process (like using the cli) handles mismatched | ``` Failing tests need to be investigated. Most are variations on a missing keyword but present end. Next I want to group related failures together in the file so I can understand which might have competing requirements/properties. The other thing that seems difficult is that this sometimes identifies one syntax error as several which the old algorithm never did. Looking into it some can be fixed in "post" while others need investigation for why they're failing. ## Unknown ### Needs investigation 0 ``` 1 class Blerg ❯ 2 Foo.call do |a 5 class Foo 6 end # two 7 end # three Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? 1 class Blerg ❯ 3 end # one 5 class Foo 6 end # two 7 end # three handles mismatched | (FAILED - 1) Failures: 1) Integration tests that don't spawn a process (like using the cli) handles mismatched | Failure/Error: raise("this should be one failure, not two") RuntimeError: this should be one failure, not two # ./spec/integration/dead_end_spec.rb:603:in `block (2 levels) in <module:DeadEnd>' ``` ### Needs investigation 1 Expected: ``` 7 context "test" do ❯ 8 it "should" do 9 end ``` Actual ``` +Unmatched keyword, missing `end' ? + + 1 context "foo bar" do +❯ 7 context "test" do + 9 end # ./spec/integration/dead_end_spec.rb:418:in `block (2 levels) in <module:DeadEnd>' ``` ### Needs investigation 2 Expected: ``` it "finds a naked end" do source = <<~'EOM' def foo end # one end # two EOM io = StringIO.new DeadEnd.call( io: io, source: source ) expect(io.string).to include(<<~'EOM') ❯ end # one EOM end ``` Actual: ``` +Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? + + 1 def foo +❯ 3 end # two ``` ## Fix it in post 1 ``` +Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? + + 1 describe "things" do + 2 it "blerg" do + 3 end +❯ 6 end + 8 it "zlerg" do + 9 end + 10 end # ./spec/integration/dead_end_spec.rb:227:in `block (2 levels) in <module:DeadEnd>' ``` Singular end, can fix it in post ``` - 1 describe "things" do\n 2 it "blerg" do\n 3 end\n❯ 5 it "flerg"\n❯ 14 end\n 16 it "zlerg" do\n 18 end\n 19 end\n +Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? + + 1 describe "things" do + 2 it "blerg" do + 3 end +❯ 14 end + 16 it "zlerg" do + 18 end + 19 end # ./spec/integration/dead_end_spec.rb:268:in `block (2 levels) in <module:DeadEnd>' ``` Same, can fix it in post ``` +Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? + + 1 def call +❯ 15 end # one + 16 end # two # ./spec/integration/dead_end_spec.rb:329:in `block (2 levels) in <module:DeadEnd>' ``` Same, can fix in post ``` +Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? + +❯ 6 end # two # ./spec/integration/dead_end_spec.rb:508:in `block (2 levels) in <module:DeadEnd>' ``` Same ``` +Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? + + 1 describe "hi" do +❯ 3 end + 4 end +Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? + + 5 it "blerg" do +❯ 7 end + 8 end # ./spec/integration/dead_end_spec.rb:532:in `block (2 levels) in <module:DeadEnd>' ``` Same ## Fix it in post 2 Subtly different: ``` +Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ? + + 2 RSpec.describe AclassNameHere, type: :worker do + 3 describe "thing" do + 13 end # line 16 accidental end, but valid block +❯ 23 end # mismatched due to 16 + 24 end # ./spec/integration/dead_end_spec.rb:481:in `block (2 levels) in <module:DeadEnd>' ```
The algorithm builds a pretty good looking tree but then it sees this:
```
1 class Blerg
❯ 2 Foo.call do |lol
❯ 3 print lol
❯ 4 end # one
5 print lol
6 class Foo
7 end # two
8 end # three
```
It thinks that there are two invalid nodes here
```
❯ 2 Foo.call do |lol
```
and
```
❯ 4 end # one
```
While it's obvious to you and me that they belong together the search algorithm calls a `multiple_invalid_parents` and continues to explore both which gives us the weird output:
```
+Unmatched `|', missing `|' ?
+Unmatched keyword, missing `end' ?
+
+ 1 class Blerg
+❯ 2 Foo.call do |lol
+ 4 end # one
+ 6 class Foo
+ 8 end # three
+Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ?
+
+ 1 class Blerg
+ 2 Foo.call do |lol
+❯ 4 end # one
+ 6 class Foo
+ 7 end # two
+ 8 end # three
```
Interesting enough if we combined these two it would be the perfect output, which makes me think it's not a different problem than the "fix it in post" ones I mentioned on the last commit, but rather might be a variation on them.
Sometimes we only match the `end` but not the source line. Other times we match the end AND the source line if they both have a syntax error on them.
A harder case is replacing the mismatched pipe with something mismatched up like
```
class Blerg
Foo.call }
print haha
print lol
end # one
print lol
class Foo
end # two
end # three
```
Gives us:
```
❯ 1 class Blerg
9 end # three
Unmatched `}', missing `{' ?
1 class Blerg
❯ 2 Foo.call }
5 end # one
7 class Foo
9 end # three
Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ?
1 class Blerg
❯ 5 end # one
7 class Foo
8 end # two
9 end # three
Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ?
1 class Blerg
❯ 9 end # three
```
So it's like double the problem from before. `class Blerg/end # three` belong together and `Foo.call }/end #one` belong together. Technically one of these is not a good match `class Blerg/end # three`.
We could do something naive that might work long term or try to get more clever.
It seems like we could add another grouping round after the search round. We could try to intelligently pair nodes together and do fancy stuff like re-check parsability of the daocument. That would be the advanced path.
The "easy" path would be to shove all these groups together at the very end so that the output might look like this:
```
❯ 1 class Blerg
❯ 2 Foo.call }
❯ 5 end # one
❯ 9 end # three
```
Which is honestly pretty good. Not ideal, but good. The main downside is we would eventually need a way to split off ACTUAL multiple syntax errors and report on them. I don't remember at this point if that was a feature of the old algorithm or not. If not, then it's no regression so maybe it's fine to start there.
So that's good. Maybe the search algorithm is good enough and it's just up to touching up the post operations. Here's the list from last time of the failure:
### Needs investigation 0
```
1 class Blerg
❯ 2 Foo.call do |a
5 class Foo
6 end # two
7 end # three
Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ?
1 class Blerg
❯ 3 end # one
5 class Foo
6 end # two
7 end # three
handles mismatched | (FAILED - 1)
Failures:
1) Integration tests that don't spawn a process (like using the cli) handles mismatched |
Failure/Error: raise("this should be one failure, not two")
RuntimeError:
this should be one failure, not two
# ./spec/integration/dead_end_spec.rb:603:in `block (2 levels) in <module:DeadEnd>'
```
### Needs investigation 1
Expected:
```
7 context "test" do
❯ 8 it "should" do
9 end
```
Actual
```
+Unmatched keyword, missing `end' ?
+
+ 1 context "foo bar" do
+❯ 7 context "test" do
+ 9 end
# ./spec/integration/dead_end_spec.rb:418:in `block (2 levels) in <module:DeadEnd>'
```
### Needs investigation 2
Expected:
```
it "finds a naked end" do
source = <<~'EOM'
def foo
end # one
end # two
EOM
io = StringIO.new
DeadEnd.call(
io: io,
source: source
)
expect(io.string).to include(<<~'EOM')
❯ end # one
EOM
end
```
Actual:
```
+Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ?
+
+ 1 def foo
+❯ 3 end # two
```
## Fix it in post 1
```
+Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ?
+
+ 1 describe "things" do
+ 2 it "blerg" do
+ 3 end
+❯ 6 end
+ 8 it "zlerg" do
+ 9 end
+ 10 end
# ./spec/integration/dead_end_spec.rb:227:in `block (2 levels) in <module:DeadEnd>'
```
Several of these repeated
## Fix it in post 2
Subtly different:
```
+Unmatched `end', missing keyword (`do', `def`, `if`, etc.) ?
+
+ 2 RSpec.describe AclassNameHere, type: :worker do
+ 3 describe "thing" do
+ 13 end # line 16 accidental end, but valid block
+❯ 23 end # mismatched due to 16
+ 24 end
# ./spec/integration/dead_end_spec.rb:481:in `block (2 levels) in <module:DeadEnd>'
```
Next time I want to look at "### Needs investigation 2" to see how it compares/contrasts to the other ones.
schneems
force-pushed
the
schneems/indent_tree
branch
from
fcb8e01
to
4dfa1b2
Compare
schneems
added a commit
that referenced
this pull request
Mar 9, 2023
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 #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 ```
matzbot
pushed a commit
to ruby/ruby
that referenced
this pull request
Apr 6, 2023
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 ruby/syntax_suggest#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 ``` ruby/syntax_suggest@13739c6946
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This effort was originally started in response to #118 which I kept a log on experiments there for some time. The core goal is more performance. With a lot of benchmarking and optimizing it seems that the best way to get more performance is to make an algorithm that builds better guesses.
I then split out several promising branches with experiments and I've finally converged to this branch.
Beyond performance, one issue with the existing code is that searching for syntax errors is heavily coupled to the process of transforming the document. It searches as it transforms. There are two main abstractions CodeLine and CodeBlock. Both are mostly data values and don't give much semantic meaning to their contents. This is especially true when trying to optimize this codehttps://github.com/zombocom/dead_end/blob/622dfddda92e27235425dd5a370618c5257731e8/lib/dead_end/parse_blocks_from_indent_line.rb#L37. Basically we can try to make intelligent guesses, but sometimes we're wrong and when that happens we have already mutated our source so there's not an easy way to "go back".
Intelligent code structure: graph of nodes
This re-write splits out the searching from structure generating. It first builds a graph of nodes using indentation and the heuristic of which way code is "leaning" (i.e.
def talkis incomplete and is leaning left, it needs to expand right/down to find a match). Each node is an attempt at building a valid code block. It also contains references to its "parents" one or more code blocks that it may hold. When generating nodes, the parent blocks are created first and then recursively joined to create children. Nodes also hold a reference to nodes both above and below them. This is an especially useful property as some code is not valid without a surrounding context. For examplehello: "world"is not syntactically valid ruby code, butspeak(hello: "world")is valid.Diagnose nodes
Once the graph is created, then it can be searched. The entire document's worth of nodes is sent into a Diagnose class where it suggests how we could move forward to isolate the problem. This logic is heavily coupled to the shape of graph that's generated. For example, if we are diagnosing a node with 3 parents, we could systematically try validating the syntax while removing 1 parent at a time to see if it's removal makes the other parents valid. That might indicate (but not prove) that the removed node holds the problem.
Search the diagnosis
Once nodes are generated and a robust set of common problems can be diagnosed then searching the graph becomes somewhat straightforward. The path to eliminating all syntax errors is not linear so must be represented by a branching structure. Each walk of the graph is encoded into a
Journey, and only when all possible journeys have ended successfully can the original document become parsable (indicating that all syntax errors are found/eliminated). The steps in this process are preserved versus in the current algorithm the searching process is destructive. If a bad guess is made, we could in theory detect it somehow and roll back.Next
The performance of this approach is above and beyond my expectations. It is much faster. It's also possibly more accurate. However, it also produces different problems than the original. Right now I'm able to get most test cases passing with the same or better-than-original output. The edge cases where that's not true need to be investigated.
Some old properties like this problem code:
Being reduced to:
Were properties of the search algorithm itself as the v1 algorithm transforms/hides code as it searches. Since this algorithm is different it will tell you correctly that the problem is the
endbut we must figure out how to teach it that thedeffoois related, but that the interiorprint 'lol'is not.Essentially it's a process of picking a failing case, trying to modify one of the algorithms mentioned above, or possibly adding some pre or post-processing step not yet present.
This progress is still very much considered experimental.