Point is at the incorrect location after onTypeFormatting

[jakejx]

Hello there, I encountered two issues when trying to get on type formatting working with the Scala Metal server. Fixing the first issue, led me to discover the second issue:

1. When the firstTriggerCharacter for on type formatting is \n, on type formatting does not trigger at all.
2. When on type formatting does get triggered, after the text edits are applied, the point ends up at the wrong location.

Both issues can be reproduced with this sample Scala project and the the Scala Metals server. In src/main/scala/commons/Main.scala, insert: (point will be represented with ^)

val x = """|"""
            ^

(point is after the pipe)
and hit enter.

What should happen is that Eglot should request for onTypeFormatting from the Metals and produce this:

val x = """|
           |""".stripMargin
            ^           

The point should be after the pipe. But Eglot does not send the onTypeFormatting request and just inserts a newline as per usual. This is caused by issue 1.

I debugged the issue and identified that this is happening because in eglot--post-self-insert-hook, last-input-event is used to determine if the onTypeFormatting request should be made. After hitting enter, the last-input-event is 'return, but the firstTriggerCharacter sent by Metals is \n (See transcript below). So the (eglot-format) in eglot--post-self-insert-hook is not triggered. I fixed this locally by doing this:

(defun eglot--post-self-insert-hook ()
  "Set `eglot--last-inserted-char', maybe call on-type-formatting."
  (setq eglot--last-inserted-char last-input-event)
  (let ((ot-provider (eglot--server-capable :documentOnTypeFormattingProvider))
        (real-input (if (eq last-input-event 'return) 10 last-input-event))) ;; convert 'return to "\n"
    (when (and ot-provider
               (ignore-errors ; github#906, some LS's send empty strings
                 (or (eq real-input
                         (seq-first (plist-get ot-provider :firstTriggerCharacter)))
                     (cl-find real-input
                              (plist-get ot-provider :moreTriggerCharacter)
                              :key #'seq-first))))
      (eglot-format (point) nil real-input))))

Basically checking if the last-input-event is a 'return and changing it to a \n if so. I am pretty sure there are better ways to do this, but I am not familiar enough with elisp. With this, Eglot now sends the onTypeFormatting request (see the transcript below for the sample request and response).

This leads to the second issue. With the same example as above, hitting enter now results in this:

val x = """|
           |""".stripMargin
          ^           

The point is behind the pipe instead of after the pipe. This seems to be caused by the save-excursion in eglot--apply-text-edits. I suspect this is happening because the new line is inserted first, causing the point to be at the start of the new line, then the text edit is applied. Then the point is restored (because of save-excursion) to the start of the line. I don't really know why save-excursion is necessary, but removing the save-excursion fixes the issue and point is at the correct location after the text edit.

Please let me know if additional information is required. Thanks!

Additional information

• Server used: Scala metals
• Emacs version: GNU Emacs 28.1 (build 1, aarch64-apple-darwin21.4.0, Carbon Version 165 AppKit 2113.4) of 2022-04-26
• Operating system: MacOS
• Eglot version: 1.8
• Eglot installation method: Straight with use-package
• Using Doom: No

LSP transcript - M-x eglot-events-buffer (mandatory unless Emacs inoperable)

I have only displayed the relevant parts here

[server-reply] (id:1) Fri May 20 21:40:12 2022:
(:jsonrpc "2.0" :id 1 :result
          (:capabilities
           (:textDocumentSync
            (:openClose t :change 1 :save
                        (:includeText t))
            :hoverProvider t :completionProvider
            (:resolveProvider t :triggerCharacters
                              ["." "*"])
            :signatureHelpProvider
            (:triggerCharacters
             ["(" "[" ","])
            :definitionProvider t :implementationProvider t :referencesProvider t :documentHighlightProvider t :documentSymbolProvider t :workspaceSymbolProvider t :codeActionProvider
            (:codeActionKinds
             ["quickfix" "refactor" "source.organizeImports"])
            :codeLensProvider
            (:resolveProvider :json-false)
            :documentFormattingProvider t :documentRangeFormattingProvider t :documentOnTypeFormattingProvider
            (:firstTriggerCharacter "\n" :moreTriggerCharacter
                                    ["\""])
            :renameProvider
            (:prepareProvider t)
            :foldingRangeProvider t :executeCommandProvider
            (:commands
             ["analyze-stacktrace" "bsp-switch" "build-connect" "compile-cancel" "compile-cascade" "compile-clean" "copy-worksheet-output" "discover-tests" "extract-member-definition" "generate-bsp-config" "goto-position" "goto-super-method" "goto" "build-import" "insert-inferred-type" "new-scala-file" "new-java-file" "new-scala-project" "presentation-compiler-restart" "reset-choice" "build-restart" "doctor-run" "file-decode" "build-disconnect" "list-build-targets" "sources-scan" "ammonite-start" "debug-adapter-start" "ammonite-stop" "super-method-hierarchy"])
            :selectionRangeProvider t :experimental
            (:rangeHoverProvider t))
           :serverInfo
           (:name "Metals" :version "0.11.2")))

[client-request] (id:168) Fri May 20 21:50:39 2022:
(:jsonrpc "2.0" :id 168 :method "textDocument/onTypeFormatting" :params
          (:textDocument
           (:uri "file:///XXXXXX.scala")
           :options
           (:tabSize 4 :insertSpaces t :insertFinalNewline t :trimFinalNewlines t)
           :position
           (:line 19 :character 0)
           :ch "\n"))

[server-reply] (id:168) Fri May 20 21:50:39 2022:
(:jsonrpc "2.0" :id 168 :result
          [(:range
            (:start
             (:line 19 :character 0)
             :end
             (:line 19 :character 0))
            :newText "           |")
           (:range
            (:start
             (:line 19 :character 3)
             :end
             (:line 19 :character 3))
            :newText ".stripMargin")])

[joaotavora]

Thanks for the report.

I am converting this to a discussion as you didn't follow the issue reporting template (why? It has such bold letters...)

Nevertheless, rest assured that I have understood a good part of your clear description, and you are correct that there are two separate behavior here here. The first one is clearly an error in Eglot, the second one, I'm not so sure. I don't use "on type formatting", though I helped implement it (with @nemethf ).

[jakejx]

My apologies, I thought I provided most of the pertinent information required. It was abit tricky to follow the template because there were 2 problems, and the second issue only appear after fixing the first one.

[nemethf]

Without digging deep into the second problem, my understanding of the specification is that the TextEdits shouldn't change the position of the point, but the specification is vague. For example, it doesn't seem to be specified what should happen when the point is in the middle of text that a TextEdit replaces.

The server-reply of id:168 consists of two TextEdits. Maybe to carefully position the point? Just because this works in VSCode, doesn't mean it is in line with the specification. But as I said, I haven't looked into this yet.

[joaotavora]

Yes, the second one is a specification problem. Indeed, for the second problem (not the newline problem, that is clearly a bug) -- I can think of many a good situation where it makes sense to not mess with the caret's position. One of them is automatic brace pairing, where the "on insert" formatting detects an opening delimiter, and inserts a closing one, keeping point in the middle. That is, as far as I understand, what Eglot would do today if it encountered such a server.

This is similar to microsoft/pyright#3197 (comment) by the way. There, the standard eventually covered the legal void in newer versions. Maybe they will do the same here?

[nemethf]

In a slightly different situation, the authors of rust-analyzer reached this conclusion:

    // We have an assist that inserts ` ` after typing `->` in `fn foo() ->{`,
    // but it requires precise cursor positioning to work, and one can't
    // position the cursor with on_type formatting. So, let's just toggle this
    // feature off here, hoping that we'll enable it one day, 😿.

[joaotavora]

My apologies, I thought I provided most of the pertinent information required.

You did provide pertinent information, just not what constitutes this maintainer's relatively narrow definition of an issue report. But the information is very pertinent, relevant and clear.

[nemethf]

It is a bit off-topic, but people often don't follow the issue template. Maybe the Eglot project should change the bug reporting process to use an issue form instead of an issue template. We can ask to community to help to create the form if you (just like me) don't want to understand how an issue form works. If there are more issues than what a project can handle in a timely manner, then I think is OK to make the bug reporting process a bit more complicated so that it results in higher quality reports. Even if the number of reports decreases.

[joaotavora]

Maybe the Eglot project should change the bug reporting process to use an issue form instead of an issue template

Yes, that is a great idea. It's not complicated, I just couldn't muster the effort. I was thinking something like https://github.com/vitejs/vite/blob/main/.github/ISSUE_TEMPLATE/bug_report.yml, but adapted to Eglot's LSP reality.