search

emacs-lsp / lsp-metals

lsp-mode :heart: metals
https://emacs-lsp.github.io/lsp-metals
GNU General Public License v3.0
58 stars 33 forks source link

Fix wrong-number-of-arguments error from lsp-metals--model-refresh. #12

Closed dangerousben closed 4 years ago

dangerousben commented 4 years ago

This was causing the following error:

Error processing message (wrong-number-of-arguments ((t) (workspace) "Handle `metals-model-refresh' notification refreshing lenses.
WORKSPACE is the workspace the notification was received from." (mapc #'(lambda (buffer) (save-current-buffer (set-buffer buffer) (if (and (boundp 'lsp-lens-mode) lsp-lens-mode) (progn (lsp--lens-schedule-refresh t))))) (progn (or (and (memq (type-of workspace) cl-struct-lsp--workspace-tags) t) (signal 'wrong-type-argument (list 'lsp--workspace workspace))) (aref workspace 9)))) 2).
kurnevsky commented 4 years ago

@dangerousben in which cases do you have this error? metals-model-refresh doesn't have any parameters: https://github.com/scalameta/metals/blob/4920adc4e870b957cbea2fa999f723ccdc3622fe/metals/src/main/scala/scala/meta/internal/metals/ClientCommands.scala#L100

dangerousben commented 4 years ago

@kurnevsky with lsp-lens-auto-enable set to t, it happens whenever I open a file with code lenses, for example NewProjectLspSuite from metals.

Looking at the code, lsp-metals--execute-client-command does:

(apply command (append (list workspace) arguments? nil))))

so it will include the extra arg even if it's nil (I'm not quite sure what the extra nil on the end is for).

kurnevsky commented 4 years ago

(append (list workspace) arguments? nil) will return list of one element in case if arguments? is nil. And it should be nil for lsp-metals--model-refresh. Also lsp-metals--execute-client-command is executed only on user action. So the problem should be different. Which emacs and metals versions do you use? Also did you test your fix - does it work for you?

yyoncho commented 4 years ago

In this code, the arity of command depends on whether there are arguments or no.

e. g.

(apply 'list (append (list 1) [1] nil)) -> (list 1 1)
(apply 'list (append (list 1) [] nil)) -> (list 1)
dangerousben commented 4 years ago

Which emacs and metals versions do you use?

emacs 28.0.50 / ffb89ed5f0 metals 0.9.1-SNAPSHOT / db4b5f88 (both built from git just now)

Also did you test your fix - does it work for you?

Yes, it works for me.

Investigating further:

    (message "got %S, argument? is: %S" command arguments?))

gives me:

got lsp-metals--model-refresh, argument? is: [nil]

ie, it looks like arguments? is [nil], not nil, which would explain an extra nil in the list after append is applied.

kurnevsky commented 4 years ago

They added an arg recently: https://github.com/scalameta/metals/commit/a821715e949ca87e48cc5b41fe70815789eae405

dangerousben commented 4 years ago

They added an arg recently: scalameta/metals@a821715

I could be off the mark here due to lack of knowledge of LSP or metals, but if this is something defined by the metals implementation rather than fixed in the standard, wouldn't it be safer just to assume that any client command could come with arguments?

kurnevsky commented 4 years ago

I'd say that this is a breaking change of protocol and it's good we caught it. Let's wait for answer in https://github.com/scalameta/metals/issues/1863 and see if and how we should handle it.

kurnevsky commented 4 years ago

This arg was reverted and won't get into upcoming release: https://github.com/scalameta/metals/pull/1869 I believe it can be closed now. Thanks for reporting this problem!