Close paren on type formatting reformats whole document instead of current expression

[jryans]

Here's an example demonstrating the issue:

#lang racket/base

(struct document (author
  title
  content))

(struct book
  document
  (publisher)); <- Remove and re-add this close paren

If you remove and re-add the last closing paren in the file (for the book struct), you end up seeing that document struct earlier in the file gets reformatted, even though we are not working on that that expression. This behaviour is surprising and gets in the way when working on shared codebases that may use all sorts of different formatting styles. Ideally, only the actively edited expression would be reformatted when format on type is enabled.

After typing the close paren, VS Code shows the following LSP traffic:

[Trace - 17:55:29] Sending request 'textDocument/onTypeFormatting - (60)'.
Params: {
    "textDocument": {
        "uri": "..."
    },
    "position": {
        "line": 8,
        "character": 14
    },
    "ch": ")",
    "options": {
        "tabSize": 2,
        "insertSpaces": true,
        "insertFinalNewline": true
    }
}

[Trace - 17:55:29] Received response 'textDocument/onTypeFormatting - (60)' in 14ms.
Result: [
    {
        "newText": "",
        "range": {
            "end": {
                "character": 17,
                "line": 0
            },
            "start": {
                "character": 17,
                "line": 0
            }
        }
    },
    {
        "newText": "",
        "range": {
            "end": {
                "character": 0,
                "line": 1
            },
            "start": {
                "character": 0,
                "line": 1
            }
        }
    },
    {
        "newText": "",
        "range": {
            "end": {
                "character": 24,
                "line": 2
            },
            "start": {
                "character": 24,
                "line": 2
            }
        }
    },
    {
        "newText": "",
        "range": {
            "end": {
                "character": 7,
                "line": 3
            },
            "start": {
                "character": 7,
                "line": 3
            }
        }
    },
    {
        "newText": "                ",
        "range": {
            "end": {
                "character": 0,
                "line": 3
            },
            "start": {
                "character": 0,
                "line": 3
            }
        }
    },
    {
        "newText": "",
        "range": {
            "end": {
                "character": 11,
                "line": 4
            },
            "start": {
                "character": 11,
                "line": 4
            }
        }
    },
    {
        "newText": "                ",
        "range": {
            "end": {
                "character": 0,
                "line": 4
            },
            "start": {
                "character": 0,
                "line": 4
            }
        }
    },
    {
        "newText": "",
        "range": {
            "end": {
                "character": 0,
                "line": 5
            },
            "start": {
                "character": 0,
                "line": 5
            }
        }
    },
    {
        "newText": "",
        "range": {
            "end": {
                "character": 12,
                "line": 6
            },
            "start": {
                "character": 12,
                "line": 6
            }
        }
    },
    {
        "newText": "",
        "range": {
            "end": {
                "character": 10,
                "line": 7
            },
            "start": {
                "character": 10,
                "line": 7
            }
        }
    },
    {
        "newText": "",
        "range": {
            "end": {
                "character": 14,
                "line": 8
            },
            "start": {
                "character": 14,
                "line": 8
            }
        }
    }
]

[Trace - 17:55:29] Sending notification 'textDocument/didChange'.
Params: {
    "textDocument": {
        "uri": "...",
        "version": 11
    },
    "contentChanges": [
        {
            "range": {
                "start": {
                    "line": 4,
                    "character": 0
                },
                "end": {
                    "line": 4,
                    "character": 0
                }
            },
            "rangeLength": 0,
            "text": "                "
        },
        {
            "range": {
                "start": {
                    "line": 3,
                    "character": 0
                },
                "end": {
                    "line": 3,
                    "character": 0
                }
            },
            "rangeLength": 0,
            "text": "                "
        }
    ]
}

The confirms that onTypeFormatting support in the lang server reformatted the whole file.

I suppose what's needed here is the lang server's onTypeFomatting support should specially handle the close paren character, and then look for the current expression, and only reformat that...?

[dannypsnl]

Interesting, in doc.rkt:190-208 should already make a filter

[dannypsnl]

If there can have a bug, it should be doc-find-containing-paren

[6cdh]

Oh, it's a bug. I thought it's a feature that it reformat the whole document.

[6cdh]

I think I know where the bug come from. From specification - DocumentOnTypeFormattingParams:

/**
 * The position around which the on type formatting should happen.
 * This is not necessarily the exact position where the character denoted
 * by the property `ch` got typed.
 */
position: Position;

The position could not be the position of the inserted character. It's an actually useless parameter. But in code

https://github.com/jeapostrophe/racket-langserver/blob/45f57674d8dbe936cfa5ac73f91b8261c4d5fe49/text-document.rkt#L524

It uses pos to find the innermost pair of parentheses that contains the position pos. It can be wrong as the reason above.

In the example code, the position that the client passed is this:

(publisher))
            ^

Our code should not depend on the precise definition of position.

[dannypsnl]

Not quite? It says the position “not necessary” be the same, but it still is the same in most cases, where you point out the position is after the inserted, I think that’s correct.

Maybe we can use pos-1 instead of pos?

Another solution should be we stop using find parentheses, but the find prev sexp and find next sexp to detect the scope?

[6cdh]

No. The current code expects position is here:

(publisher))
           ^

This is the position of ch in the request.

Suppose position can be position-of(ch) or position-of(ch) + 1, then we can't process this case:

)))
 ^
 ch

It would be ambiguous. This is a problem.

[dannypsnl]

Yes, but I think a finite scope is still better than the whole file.

[6cdh]

I have an evil idea. We can save the position information in did-change!, and use it in on-type-formatting. So we can do precise formatting. Another normal idea is, because we only format from line x to line y currently instead of character level, we can find the last closed parenthese at this line, and find where its matching open parenthese at.

[6cdh]

The second idea can have a condition, that if text[position] has an closed parenthese, then use doc-find-containing-paren.

[6cdh]

I prefer the evil solution. We just need to add a last-closed-paren field in struct Doc, and maintain it.

[jryans]

I was curious what other lang servers do for this case...

Looking at Rust Analyzer, it seems like they assume that for on-type formatting, you can find a precise position by subtracting one character from the position in the protocol message. It seems to work for this one example here... but of course, I'm not sure what other clients might do.

If we really can't depend on the position field, then perhaps one of @6cdh's ideas is they way to go.

[dannypsnl]

Rust analyzer assume there has no other plugin will affect the position

[6cdh]

After some search, I found only a little editor supports onTypeFormatting:

• vscode - cursor position, code here and getPosition
• neovim - no support
• helix - no support
• emacs lsp mode - cursor position, code here

I think they all looked into vscode's implementation. So a possible solution is to assume it's cursor position.

[6cdh]

It's been 5 days since our last discussion. What are your thoughts? I think the solution that assuming it's the cursor position looks like a safe option. If you agree, I can do this today. It would be a small fix.

[dannypsnl]

Agree

[jryans]

Sounds like a good approach to me, thanks for working on this. 😄