New implementation of finding the top level form range

[brdloush]

What has Changed?

Additional improvement for #942 (slow REPL window output).

I've traced down the main source of slowness being in tlCursor.rangeForDefun. According to PEZ:

The way rangeForDefun works is that it starts at the token-cursor position (0, in this case and most often) and moves forward sexp until it has passed the editor cursor position. In my test I have 60693 words in the window. Which translates to as many tokens. token-cursor.forwardSexp() has to do its work 61K times so it will take some time.

I started with following slack suggestion by PEZ and also tried to get some inspiration from ctrl+alt+down (Paredit Forward Up Sexp):

- If upList():
	- Continue upList() until we can’t anymore
	- note the current position as end of current top-level form
	- go backwardSexp()
	- note the current position as start of current top-level form
- Else:
	- Current form is the current top level form
	
...first upList() needs to be preceded by a forwardList()...

This simple way looked fine first, but once I was trying different evaluations in editor (and in REPL window), it resulted in different/non-functioning behavior, compared to original "slow" version. Particularly I had issues with partial thread macro evals (ctrl+alt+enter), many alt-enter evals on different palces resulted in incorrect block being evaluated, evals inside repl windows acted a bit strangely etc.

After many attempts of trial-and-error fixing, I now have a first draft that I'd like to testusing a vsix, thus creating a draft PR.

It's very likely that the whole function may (should) be written in different way. It's a first attempt, I still don't have enough knowledge about all those different ways of navigation in documents/sexps etc. Consider this PR a POC for performance and regression testing.

My Calva PR Checklist

I have:

• Read How to Contribute.
• Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
• Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
• Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
• [ ] Added to or updated docs in this branch, if appropriate
• Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. NB: You need to sign in/up at Circle CI to find the Artifacts tab.
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• Referenced the issue I am fixing/addressing in a commit message for the pull request.
  • [ ] If I am fixing the issue, I have used [GitHub's fixes/closes syntax](https://help.github.com/en/articles/closing-issues-using-keywords)
  • If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
• [ ] Created the issue I am fixing/addressing, if it was not present.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

• Made sure the PR is directed at the dev branch (unless reasons).
• Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and tested it there if so.
• Read the source changes.
• Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
• Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
  • Tested the particular change
  • Figured if the change might have some side effects and tested those as well.
  • Smoke tested the extension as such.
• If need be, had a chat within the team about particular changes.

Ping @PEZ, @bpringe

[PEZ]

Thanks!

It looks a bit complicated. Maybe that is needed, but it seldom is, is my experience.

For the uses where your first attempts falled I think it would be great to add some tests that demonstrate how it is working with the current implementation. I'm thinking that should probably be done on a branch of its own, straight of dev.

I also will try to add a bit better test coverage around the use cases involving the depth and comment.

So, that is maybe easiest if add this coverage on the same branch. I can give you commit access to the repo, if you are comfortable with that?

[brdloush]

It looks a bit complicated. Maybe that is needed, but it seldom is, is my experience.

Agreed. I also believe that it could (hopefully) be written in simpler way. I was just simply trying (perhaps too hard 😄) to end up with more-or-less same behavior as current dev branch's evaluation commands.

I've been testing the VSIX for a while and I'm quite happy with the behavior. It's way faster and evaluation mostly seems to behave same as before. So far I found one issue and one thing I'm not sure about.

First the issue: when scrolling in the editor, sometimes the comment form's closing bracket gets weird color/background. Probably related to that TODO I have in current code.

Then there's a small difference in what should get evaluated, when your cursor is between two top level forms. For example consider these situations (| used as symbol for current curosor position).

(println "foo")
|

(println "bar")

(println "foo")

|
(println "bar")

Now the dev branch's behavior

• executes foo printout form in 1)
• executes bar printout for in 1)

My branch

• executes bar printout form in 1)
• executes bar printout for in 1)

I'm not sure what's the expected behavior. Strictly speaking, when the cursor is not inside any list, nor is it "directly touching" any symbol, I'd expect that "it has no top-level form (or blank string is its top level form)" - and therefore nothing should get evaluated.

On the other hand I can imagine that people already trained their muscle memory in that way, that they rely on current behavior.

I also will try to add a bit better test coverage around the use cases involving the depth and comment.

That would be great. When you'll be adding those, can you cover the scenario I mentioned ^^^ ? And perhaps also some "most critical" scenarios when evaluating code in REPL window. Without some of the conditions in my code, REPL window worked in that way for me, that it kept evaluating last expression, when I pressed enter on last blank line.

So, that is maybe easiest if add this coverage on the same branch. I can give you commit access to the repo, if you are comfortable with that?

I'm not sure I follow. commit access to which repo?

Btw would you have any advice regarding some efficient workflow for running tests? So far I only executed them from command line using npm run unit-test (or with something like `npx mocha --watch --require ts-node/register 'src/extension-test/unit/**/token-test.ts'). That's fine for just running those tests, but not great for debugging, as I can't use breakpoints etc.

[brdloush]

Commited a yet another still-experimental version of rangeForDefun. This one handles most of the cases I know about correctly, relies more on complex behavior of rangeForCurrentForm and is even able to skip nested comment forms.

Still, it will be interesting to see if it passes those tests we're going to create in dev branch first.

[PEZ]

I created a branch for adding tests for the current implementation. https://github.com/BetterThanTomorrow/calva/tree/1230-range-for-defun-tests

Added some tests there, then realized I want them in dev so cherry picked them. But It's good to have a separate branch anyway, since we might add some more experimental tests.

[bpringe]

Btw would you have any advice regarding some efficient workflow for running tests?

@brdloush You can use the Mocha Test Explorer extension, which will give you a side panel for seeing and running tests, and will also add code lenses above tests for debugging them, so you can place a breakpoint and then click the code lens to debug a test.

[PEZ]

I've created yet another branch now: https://github.com/BetterThanTomorrow/calva/tree/defun-range-experiments 😄

The reason being that I wanted to see what holes there could be in the strategy you and I cooked up in that pair ”programming” session @brdloush. So far I've found no holes. (Not tested the performance, but I see no reason why it should be slow, even though, to keep it simple, we call rangeForCurrent form at every level, even when we don't need to check for comment (such as when submitting in the REPL window).

In this branch there is increased test coverage which is also duplicated to the rangeForDefun2() method. Thinking we can keep it like that until we start swapping out the old method for the new one.

Maybe you should consider filing a new PR based on this branch, @brdloush. Clone the Calva repo if so and work from there, as you have push access now. (I've forgotten to inform you about that, @bpringe, but now that's fixed. 😄 ) Then we can coop on finishing this work. rangeForDefun() is not used from terribly many places, so it shouldn't be too much work updating the call sites.

[bpringe]

I've forgotten to inform you about that, @bpringe, but now that's fixed.

I have no problem with that. 😃

[brdloush]

Closing this PR in favor of #1237 (ie. the one created from defun-range-experiments branch directly in calva repo)