copilot-emacs / copilot.el

An unofficial Copilot plugin for Emacs.
MIT License
1.79k stars 126 forks source link

indentation issue with overlays and completions #179

Closed ultronozm closed 12 months ago

ultronozm commented 1 year ago

Put the following in an elisp buffer:

(defun print-sum (a b)
  (let ((sum (+ a b)))

Position the cursor at the end of the second line and do M-x copilot-complete. An overlay with (print sum) appears on the next next with the correct indentation. If I accept the completion, I get:

(defun print-sum (a b)
  (let ((sum (+ a b)))
    (print sum)))

So far, so good. Now, go back to the initial buffer contents and insert a newline. The buffer should then look as follows, with "|" indicating cursor position:

(defun print-sum (a b)
  (let ((sum (+ a b)))
    |

Now do M-x copilot-complete (if necessary). Observe that the suggested completion has its overlay indented too much (the same amount as the current column). If we do M-x copilot-accept-completion, then we get:

(defun print-sum (a b)
  (let ((sum (+ a b)))
        (print sum)))

It's easy to patch things up afterwards via M-q, etc., but that seems suboptimal. I suspect that the copilot module "knows" the correct indentation, but that copilot.el somehow fails to "subtract" the indentation of point? Haven't tried to debug this yet.

raymond-w-ko commented 1 year ago

Does merged PR #178 fix this?

ultronozm commented 1 year ago

Does merged PR #178 fix this?

No. I'm on the latest version, and the issue is anyway unrelated to narrowing.

raymond-w-ko commented 1 year ago

Sorry, I got confused with issue #173 for some reason. 😞

ultronozm commented 1 year ago

I think the following example rules out some other possibilities:

(defun print-hello-then-print-sum (a b)
  (let ((sum (+ a b)))
    (print "hello")

Position the cursor at the beginning of the next line, all the way to the left, like where "|" is in the following:

(defun print-hello-then-print-sum (a b)
  (let ((sum (+ a b)))
    (print "hello")
|

Then do M-x copilot-complete. Get similar issue as above:

(defun print-hello-then-print-sum (a b)
  (let ((sum (+ a b)))
    (print "hello")
        (print sum)))

Using edebug on copilot--show-completion and inspecting the argument completion-data shows that the error has already occurred:

(:uuid "27fc2ffc-6e2d-41c2-9b23-e116eb3008bf"
       :text "        (print sum)))"
       :range (:start (:line 3 :character 0)
                      :end (:line 3 :character 0))
       :displayText "        (print sum)))"
       :position (:line 3 :character 0)
       :docVersion 315)

I wasn't quickly able to understand what happens in the jsonrpc-lambda block inside copilot-complete. Does this result in some preprocessing of whatever is returned by the underlying copilot plugin?

ultronozm commented 1 year ago

I don't know exactly how the copilot parameters tabSize and indentSize are used, but they seem to have the effect of requiring that completions on an empty line must begin with a multiple of that many spaces. For emacs-lisp-mode, the function copilot--infer-indentation-offset returns 8; this passes through editorconfig-indentation-alist and lisp-indent-offset (which is nil) and finally tab-width.

If I'm not mistaken, tab-width is not relevant for elisp buffers, while lisp-indent-offset has a different meaning than how it is interpreted here. So I can think of a couple fixes:

  1. Modify the definition of copilot--indentation-alist to contain (emacs-lisp-mode some-variable-with-value-one) in its initial "exceptions" block (and possibly modify copilot--infer-indentation-offset so that we can simply use (emacs-lisp-mode 1) in the alist). See https://github.com/zerolfx/copilot.el/commit/38bd7a8b8bac45af2105e847f01be17820a7f501.

  2. Modify the default behavior of copilot--infer-indentation-offset to return 1 rather than tab-width.

I don't have a clear enough global picture to know which is better.

Please correct me if you happen to know that I've misinterpreted the role of tabSize and indentSize. A quick search turned up https://github.com/orgs/community/discussions/10789, which suggested at least that tabSize didn't have any actual effect a year and a half ago.

zerolfx commented 1 year ago

I believe a good solution is to notify the user when Copilot uses tab-width instead of a mode-specific variable.

I think assigning a default value like one is not ideal, as users may have different preferences (I personally prefer two).

ultronozm commented 1 year ago

I think assigning a default value like one is not ideal, as users may have different preferences (I personally prefer two).

How have you made this preference known to copilot with elisp? Via tab-width?

zerolfx commented 1 year ago

I think assigning a default value like one is not ideal, as users may have different preferences (I personally prefer two).

How have you made this preference known to copilot with elisp? Via tab-width?

Yes, I explicitly set tab-width to 2 for emacs-lisp-mode.

github-actions[bot] commented 12 months ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

zerolfx commented 12 months ago

closed by #191

itsrobel commented 9 months ago

I believe a good solution is to notify the user when Copilot uses tab-width instead of a mode-specific variable.

I think assigning a default value like one is not ideal, as users may have different preferences (I personally prefer two).

Is there any way I can turn off this warning, I keep receiving this when I open my .el files

Best, <3

blahgeek commented 9 months ago

@amarshavir please see https://github.com/zerolfx/copilot.el/pull/212#issuecomment-1862487382