From 2216173eb6c83c093bc66c385012563fa50aa841 Mon Sep 17 00:00:00 2001 From: Chris Bouchard Date: Sun, 22 Sep 2024 22:51:50 -0400 Subject: [PATCH 1/6] Provide a unified (lsp-interface INTERFACE ...) pcase form This commit provides a new unified pcase form (lsp-interface INTERFACE ...) to replace the old per-interface (INTERFACE ...) forms -- the latter are now deprecated. (Unfortunately, I don't think there's a way to mark a pcase form as obsolete.) I've turned the existing pcase-defmacro definition into a helper function. The new pcase form delegates to that helper function, and the old pcase forms now delegate to the new form. This change addresses a few issues, which are detailed in #4430. In short: * The existing forms aren't namespaced. * The lsp-mode package adds hundreds of forms, which each add several lines to pcase's generated docstring, adding up to over 1000 lines. * Starting in Emacs 31, the number of forms added by lsp-mode causes a noticeable slowdown when loading the interactive help for pcase. --- lsp-protocol.el | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/lsp-protocol.el b/lsp-protocol.el index d5b98a00e5..fcfbff0c79 100644 --- a/lsp-protocol.el +++ b/lsp-protocol.el @@ -129,7 +129,7 @@ Allowed params: %s" interface (reverse (-map #'cl-first params))) $$result)) (-partition 2 plist)) $$result))) - `(pcase-defmacro ,interface (&rest property-bindings) + `(cl-defun ,(intern (format "lsp--pcase-macroexpander-%s" interface)) (&rest property-bindings) ,(if lsp-use-plists ``(and (pred listp) @@ -225,6 +225,8 @@ Allowed params: %s" interface (reverse (-map #'cl-first params))) output-bindings) (setf current-list (cddr current-list)))))) output-bindings)))) + `(pcase-defmacro ,interface (&rest property-bindings) + `(lsp-interface ,',interface ,@property-bindings)) (-mapcat (-lambda ((label . name)) (list `(defun ,(intern (format "lsp:%s-%s" @@ -246,6 +248,21 @@ Allowed params: %s" interface (reverse (-map #'cl-first params))) (apply #'append) (cl-list* 'progn)))) +(pcase-defmacro lsp-interface (interface &rest property-bindings) + "If EXPVAL is an instance of the LSP interface INTERFACE, destructure its +properties. + +Each :PROPERTY key may be followed by an optional PATTERN, which is a `pcase' +pattern to apply to the property value. Otherwise, PROPERTY is bound to the +property value. + +\(fn INTERFACE [:PROPERTY [PATTERN]]...)" + (cl-check-type interface symbol) + (let ((lsp-pcase-macroexpander + (intern (format "lsp--pcase-macroexpander-%s" interface)))) + (cl-assert (fboundp lsp-pcase-macroexpander) "not a known LSP interface: %s" interface) + (apply lsp-pcase-macroexpander property-bindings))) + (if lsp-use-plists (progn (defun lsp-get (from key) From 4f1e7ff4bf062631f73e25f18a170eeccb3f03dd Mon Sep 17 00:00:00 2001 From: Chris Bouchard Date: Mon, 23 Sep 2024 09:36:45 -0400 Subject: [PATCH 2/6] Add a comment and TODO about deprecating per-interface pcase forms --- lsp-protocol.el | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lsp-protocol.el b/lsp-protocol.el index fcfbff0c79..8d21eab13f 100644 --- a/lsp-protocol.el +++ b/lsp-protocol.el @@ -225,6 +225,12 @@ Allowed params: %s" interface (reverse (-map #'cl-first params))) output-bindings) (setf current-list (cddr current-list)))))) output-bindings)))) + ;; These per-interface `pcase' forms are deprecated. Prefer + ;; the new (lsp-interface INTERFACE ...) form defined below. + ;; (See emacs-lsp/lsp-mode#4430.) + ;; + ;; TODO: Remove this `pcase-defmacro' in the next major + ;; version. `(pcase-defmacro ,interface (&rest property-bindings) `(lsp-interface ,',interface ,@property-bindings)) (-mapcat (-lambda ((label . name)) From 33ef39b389a07cada9820cb6791430552d604b0a Mon Sep 17 00:00:00 2001 From: Chris Bouchard Date: Mon, 23 Sep 2024 09:44:42 -0400 Subject: [PATCH 3/6] Improve docstring for (lsp-interface ...) pcase form I've tried to summarize the behavior of the existing implementation. --- lsp-protocol.el | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lsp-protocol.el b/lsp-protocol.el index 8d21eab13f..64d46e7cc6 100644 --- a/lsp-protocol.el +++ b/lsp-protocol.el @@ -255,12 +255,16 @@ Allowed params: %s" interface (reverse (-map #'cl-first params))) (cl-list* 'progn)))) (pcase-defmacro lsp-interface (interface &rest property-bindings) - "If EXPVAL is an instance of the LSP interface INTERFACE, destructure its -properties. + "If EXPVAL is an instance of INTERFACE, destructure it by matching its +properties. EXPVAL should be a plist or hash table depending on the variable +`lsp-use-plists'. -Each :PROPERTY key may be followed by an optional PATTERN, which is a `pcase' -pattern to apply to the property value. Otherwise, PROPERTY is bound to the -property value. +INTERFACE should be an LSP interface defined with `lsp-interface'. This form +will not match if any of INTERFACE's required fields are missing in EXPVAL. + +Each :PROPERTY keyword matches a field in EXPVAL. The keyword may be followed by +an optional PATTERN, which is a `pcase' pattern to apply to the field's value. +Otherwise, PROPERTY is let-bound to the field's value. \(fn INTERFACE [:PROPERTY [PATTERN]]...)" (cl-check-type interface symbol) From a8af3c5ea43491046c2a533bea9b07325f5b5d8c Mon Sep 17 00:00:00 2001 From: Chris Bouchard Date: Mon, 23 Sep 2024 20:09:27 -0400 Subject: [PATCH 4/6] Replace per-interface pcase forms with the new unified form I used ripgrep to search for all occurrences of `pcase`, and then checked each to see if it was using a pattern that looked like an LSP interface name. It's very possible I missed something, but hopefully not. --- lsp-completion.el | 4 ++-- lsp-mode.el | 16 +++++++-------- test/lsp-protocol-test.el | 41 ++++++++++++++++++++++++--------------- 3 files changed, 35 insertions(+), 26 deletions(-) diff --git a/lsp-completion.el b/lsp-completion.el index 0f2be89544..dab10513fd 100644 --- a/lsp-completion.el +++ b/lsp-completion.el @@ -575,8 +575,8 @@ Others: CANDIDATES" (apply #'delete-region markers) (insert prefix) (pcase text-edit? - ((TextEdit) (lsp--apply-text-edit text-edit?)) - ((InsertReplaceEdit :insert :replace :new-text) + ((lsp-interface TextEdit) (lsp--apply-text-edit text-edit?)) + ((lsp-interface InsertReplaceEdit :insert :replace :new-text) (lsp--apply-text-edit (lsp-make-text-edit :new-text new-text diff --git a/lsp-mode.el b/lsp-mode.el index 19600a78a3..c397b70c82 100644 --- a/lsp-mode.el +++ b/lsp-mode.el @@ -5245,11 +5245,11 @@ identifier and the position respectively." type Location, LocationLink, Location[] or LocationLink[]." (setq locations (pcase locations - ((seq (or (Location) - (LocationLink))) + ((seq (or (lsp-interface Location) + (lsp-interface LocationLink))) (append locations nil)) - ((or (Location) - (LocationLink)) + ((or (lsp-interface Location) + (lsp-interface LocationLink)) (list locations)))) (cl-labels ((get-xrefs-in-file @@ -5616,9 +5616,9 @@ When language is nil render as markup if `markdown-mode' is loaded." (let ((inhibit-message t)) (or (pcase content - ((MarkedString :value :language) + ((lsp-interface MarkedString :value :language) (lsp--render-string value language)) - ((MarkupContent :value :kind) + ((lsp-interface MarkupContent :value :kind) (lsp--render-string value kind)) ;; plain string ((pred stringp) (lsp--render-string content "markdown")) @@ -6408,11 +6408,11 @@ perform the request synchronously." (-mapcat (-lambda (sym) (pcase-exhaustive sym - ((DocumentSymbol :name :children? :selection-range (Range :start)) + ((lsp-interface DocumentSymbol :name :children? :selection-range (lsp-interface Range :start)) (cons (cons (concat path name) (lsp--position-to-point start)) (lsp--xref-elements-index children? (concat path name " / ")))) - ((SymbolInformation :name :location (Location :range (Range :start))) + ((lsp-interface SymbolInformation :name :location (lsp-interface Location :range (lsp-interface Range :start))) (list (cons (concat path name) (lsp--position-to-point start)))))) symbols)) diff --git a/test/lsp-protocol-test.el b/test/lsp-protocol-test.el index 9a5adc2c65..7b5a422cf3 100644 --- a/test/lsp-protocol-test.el +++ b/test/lsp-protocol-test.el @@ -81,28 +81,34 @@ (lsp-make-my-position :line 30 :character 40 :camelCase nil) :specialProperty 42))) (should (pcase particular-range - ((MyRange :start (MyPosition :line start-line :character start-char :camel-case start-camelcase) - :end (MyPosition :line end-line :character end-char :camel-case end-camelCase)) + ((lsp-interface MyRange + :start (lsp-interface MyPosition + :line start-line :character start-char :camel-case start-camelcase) + :end (lsp-interface MyPosition + :line end-line :character end-char :camel-case end-camelCase)) t) (_ nil))) (should (pcase particular-extended-range - ((MyExtendedRange) + ((lsp-interface MyExtendedRange) t) (_ nil))) ;; a subclass can be matched by a pattern for a parent class (should (pcase particular-extended-range - ((MyRange :start (MyPosition :line start-line :character start-char :camel-case start-camelcase) - :end (MyPosition :line end-line :character end-char :camel-case end-camelCase)) + ((lsp-interface MyRange + :start (lsp-interface MyPosition + :line start-line :character start-char :camel-case start-camelcase) + :end (lsp-interface MyPosition + :line end-line :character end-char :camel-case end-camelCase)) t) (_ nil))) ;; the new patterns should be able to be used with existing ones (should (pcase (list particular-range particular-extended-range) - ((seq (MyRange) - (MyExtendedRange)) + ((seq (lsp-interface MyRange) + (lsp-interface MyExtendedRange)) t) (_ nil))) @@ -110,8 +116,8 @@ ;; not in the order specified by the inner patterns (should-not (pcase (list particular-range particular-extended-range) - ((seq (MyExtendedRange) - (MyRange)) + ((seq (lsp-interface MyExtendedRange) + (lsp-interface MyRange)) t) (_ nil))) @@ -122,8 +128,11 @@ ;; and the second instance is an equality check against the other ;; :character value, which is different. (should-not (pcase particular-range - ((MyRange :start (MyPosition :line start-line :character :camel-case start-camelcase) - :end (MyPosition :line end-line :character :camel-case end-camelCase)) + ((lsp-interface MyRange + :start (lsp-interface MyPosition + :line start-line :character :camel-case start-camelcase) + :end (lsp-interface MyPosition + :line end-line :character :camel-case end-camelCase)) t) (_ nil))) @@ -131,7 +140,7 @@ ;; should still match if the required stuff matches. Missing ;; optional properties are bound to nil. (should (pcase particular-range - ((MyRange :start (MyPosition :optional?)) + ((lsp-interface MyRange :start (lsp-interface MyPosition :optional?)) (null optional?)) (_ nil))) @@ -139,23 +148,23 @@ ;; the interface, even if the expr-val has all the types specified ;; by the interface. This is a programmer error. (should-error (pcase particular-range - ((MyRange :something-unrelated) + ((lsp-interface MyRange :something-unrelated) t) (_ nil))) ;; we do not use camelCase at this stage. This is a programmer error. (should-error (pcase particular-range - ((MyRange :start (MyPosition :camelCase)) + ((lsp-interface MyRange :start (lsp-interface MyPosition :camelCase)) t) (_ nil))) (should (pcase particular-range - ((MyRange :start (MyPosition :camel-case)) + ((lsp-interface MyRange :start (lsp-interface MyPosition :camel-case)) t) (_ nil))) ;; :end is missing, so we should fail to match the interface. (should-not (pcase (lsp-make-my-range :start (lsp-make-my-position :line 10 :character 20 :camelCase nil)) - ((MyRange) + ((lsp-interface MyRange) t) (_ nil))))) From cd3c963b5195b8bdabe614de931def6b99856082 Mon Sep 17 00:00:00 2001 From: Chris Bouchard Date: Mon, 23 Sep 2024 20:28:07 -0400 Subject: [PATCH 5/6] Remove the per-interface pcase forms The consensus in #4430 is that we're ok with making this breaking change and communicating it in the CHANGELOG. I've replaced all uses in lsp-mode itself. --- lsp-protocol.el | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lsp-protocol.el b/lsp-protocol.el index 64d46e7cc6..f3b8b6cd82 100644 --- a/lsp-protocol.el +++ b/lsp-protocol.el @@ -225,14 +225,6 @@ Allowed params: %s" interface (reverse (-map #'cl-first params))) output-bindings) (setf current-list (cddr current-list)))))) output-bindings)))) - ;; These per-interface `pcase' forms are deprecated. Prefer - ;; the new (lsp-interface INTERFACE ...) form defined below. - ;; (See emacs-lsp/lsp-mode#4430.) - ;; - ;; TODO: Remove this `pcase-defmacro' in the next major - ;; version. - `(pcase-defmacro ,interface (&rest property-bindings) - `(lsp-interface ,',interface ,@property-bindings)) (-mapcat (-lambda ((label . name)) (list `(defun ,(intern (format "lsp:%s-%s" From 2ff5cddb9220eaba84c78807adc1c3c6c172af39 Mon Sep 17 00:00:00 2001 From: Chris Bouchard Date: Mon, 23 Sep 2024 20:56:55 -0400 Subject: [PATCH 6/6] Add CHANGELOG entry for pcase changes I'm an Org newb, so hopefully this is well-formatted. --- CHANGELOG.org | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.org b/CHANGELOG.org index 143718f23b..853f05f2cd 100644 --- a/CHANGELOG.org +++ b/CHANGELOG.org @@ -14,7 +14,9 @@ * Change ~ruff-lsp~ to ~ruff~ for python lsp client. All ~ruff-lsp~ customizable variable change to ~ruff~. Lsp server command now is ~["ruff" "server"]~ instead of ~["ruff-lsp"]~. * Add futhark support * Optimize overlay creation by checking window visibility first - + * Replace the per-interface ~(INTERFACE ...)~ pcase forms with a single, + unified ~(lsp-interface INTERFACE ...)~ form. The per-interface forms are no + longer generated. *This is a breaking change.* (See #4430.) ** 9.0.0 * Add language server config for QML (Qt Modeling Language) using qmlls.