Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version 2.0.247 regression with structural editing, hangs at unbalance + structural delete #1573

Closed
PEZ opened this issue Mar 3, 2022 · 7 comments · Fixed by #1575
Closed
Labels
bug Something isn't working critical editing paredit Paredit and structural editing regression Regression error

Comments

@PEZ
Copy link
Collaborator

PEZ commented Mar 3, 2022

Calva's structural editing hangs in some situations when there is unbalance in the document. Since structural editing was updated in 247, this is very probably a regression there.

This is my repro so far (the vertical bar is the cursor):

([{|)

backspace

Two things happen. 1, the new text is:

([|

Should be:

([|)

And, much, much worse: BOOM. Now Calva is essentially dead.

If you run with auto-closing parens on, this won't happen often. But of course might. Using Calva 2.0.246 is safer.

Update: Calva v2.0.249 reverts back to before the changes to the structural editing were merged.

@PEZ PEZ added bug Something isn't working paredit Paredit and structural editing regression Regression error editing critical labels Mar 3, 2022
@PEZ PEZ changed the title Version 2.0.247 regression with structural editing Version 2.0.247 regression with structural editing, hangs at unbalance + structural delete Mar 3, 2022
@PEZ
Copy link
Collaborator Author

PEZ commented Mar 3, 2022

@corasaurus-hex While my changes to the structural editing are most likely to have caused this. In the same release we also did the no-floating-promises update. Is there a possibility that could cause this? There are some promises involved in the Paredit backspace function:

export function backspace(

@PEZ
Copy link
Collaborator Author

PEZ commented Mar 3, 2022

@corasaurus-hex I now tried with fd3be49 which has the no-floating-promise changes and then things worked, so this regression is not caused by that.

PEZ added a commit that referenced this issue Mar 4, 2022
PEZ added a commit that referenced this issue Mar 4, 2022
PEZ added a commit that referenced this issue Mar 4, 2022
Making the test runner skip it t not croak CI.
@PEZ
Copy link
Collaborator Author

PEZ commented Mar 4, 2022

Some expert digging by @corasaurus-hex shifted our attention to the formatter and formatPosition, which happens after paredit backspace. This function in particular.

calva-cursor-hang-1573-formatpositioninfo

I first suspected the cursor.rangeForCurrentForm(index) call, but e069624 did not agree.

Then I saw cursor.getFunctionName(), and had better luck with 3d4c381 Lookie:

calva-cursor-hang-1573-getfunctionname.mp4

@PEZ
Copy link
Collaborator Author

PEZ commented Mar 5, 2022

Closing in a bit more. This cursor.backwardListOfType test never finishes:

image

PEZ added a commit that referenced this issue Mar 5, 2022
One of them exposes #1573 (and is skipped for now)
@PEZ
Copy link
Collaborator Author

PEZ commented Mar 5, 2022

Hmmm... backwardList returns true for this unbalance case, even though it doesn't move. That is just waiting to cause a hang!

image

PEZ added a commit that referenced this issue Mar 5, 2022
Even closer to the source of #1573 now
@PEZ
Copy link
Collaborator Author

PEZ commented Mar 5, 2022

This fixes the issue.

    /**
     * Moves this cursor backwards to the open paren of the containing sexpr, or until the start of the document.
     */
    backwardList(): boolean {
        const cursor = this.clone();
        while (cursor.backwardSexp()) {
            // move backward until the cursor cannot move backward anymore
        }
-       if (cursor.getPrevToken().type === 'open') {
+       if (cursor.getPrevToken().type === 'open' && cursor.offsetStart !== this.offsetStart) {
            this.set(cursor);
            return true;
        }
        return false;
    }

However, it's very strange.... why did this not cause trouble until #1566?

@PEZ
Copy link
Collaborator Author

PEZ commented Mar 5, 2022

No, that broke backwardList totally. This is a much better fix:

    /**
     * Moves this cursor backwards to the open paren of the containing sexpr, or until the start of the document.
     */
    backwardList(): boolean {
        const cursor = this.clone();
        while (cursor.backwardSexp()) {
            // move backward until the cursor cannot move backward anymore
        }
        if (cursor.getPrevToken().type === 'open') {
            const checkCursor = cursor.clone();
            if (checkCursor.backwardUpList() && checkCursor.forwardSexp()) {
                this.set(cursor);
                return true;
            }
        }
        return false;
    }

PEZ added a commit that referenced this issue Mar 5, 2022
Fixes #1573 w/o breaking backwardList like 18732d4 did
@PEZ PEZ mentioned this issue Mar 5, 2022
15 tasks
@PEZ PEZ linked a pull request Mar 5, 2022 that will close this issue
15 tasks
PEZ added a commit that referenced this issue Mar 5, 2022
@PEZ PEZ closed this as completed in 18732d4 Mar 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working critical editing paredit Paredit and structural editing regression Regression error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant