lsp-interface generates a lot of noise in pcase documentation

[chrisbouchard]

Is your feature related or already mentioned on the wishlist? — No, I don't believe so.

TL;DR

I'd like to consider if there's a way lsp-mode could be (or could evolve to be) more conscientious with its use of pcase-defmacro.

Context

I recently opened the function documentation for pcase in my Emacs (to read about the map pattern). I noticed that, interspersed with the standard documentation, there was a lot of “noise” like:

-- (WatchKind &rest PROPERTY-BINDINGS)

Not documented.

-- (CodeAction &rest PROPERTY-BINDINGS)

Not documented.

-- (JSONError &rest PROPERTY-BINDINGS)

Not documented.

…

To quantify, the function documentation for `pcase' in my Emacs is just under 1300 lines long, of which about 1200 are these auto-generated patterns.

I didn't know where these patterns came from at first, but a quick Google for “emacs WatchKind” lead me to lsp-protocol.el, and from there I found the pcase-macro in lsp-interface (added in #2481). pcase-defmacro includes each new pattern into pcase's function documentation for discoverability.

As a user, this is not a great experience. It's important to me that the pcase function documentation be readable, because it's the primary in-editor way that I can discover what patterns are available (including via extensions). I do appreciate that these are technically patterns that are available, but I get the sense they weren't really intended for general consumption, and anyway their sheer number overwhelms any usefulness in documenting them like this.

Also, since these patterns aren't namespaced, there's (a) no indication that they belong to this project, and (b) no guarantee they don't collide with patterns defined by a different package.

My Ideal Solution

Forgetting for a moment that there is already code using the existing patterns, I would love if lsp-interface followed the approach used by cl-struct and defined a single pcase pattern like (lsp-interface INTERFACE &rest PROPERTY-BINDINGS). This pcase-defmacro could include a proper docstring—e.g., describing its use and the forms allowed as property bindings. It would also address the namespacing concern. And most importantly (for me), it would be a single entry in the pcase function documentation rather than hundreds.

I imagine that the current generated pcase-defmacro in lsp-interface could be repurposed as a generated lsp-interface--pcase-pattern-INTERFACE function, to which the lsp-interface pattern could delegate based on its interface parameter. (This would happen during macro compilation, so there wouldn't be a runtime cost for the indirection.)

So for example, code like

(pcase expr
  ((JSONError :message :code) ...))

would instead be written

(pcase expr
  ((lsp-interface JSONError :message :code) ...))

and expand to the exact same base pcase patterns.

Something Practical?

Returning to the real world, I appreciate that my suggestion would be a breaking change. I suppose the first question is: Does my suggestion seem palatable? My primary goal is to keep the pcase function documentation readable, so I'm not married to it in particular—I'm just not sure there's a solution other than to reduce the number of patterns defined. (For example, I think it would be a non-starter to simply include a docstring in the existing pcase-defmacro, since that would add hundreds more lines of repeated documentation to pcase's documentation.)

Also, as a newcomer to lsp-mode's development, are these interface patterns intended to be used outside of lsp-mode itself? I did a quick GitHub code search and only found a couple uses in lsp-mode itself. I also couldn't find the feature mentioned in the changelog or documentation. If this isn't a supported feature, maybe a breaking change wouldn't be too painful?

Alternatively, I imagine a new lsp-interface pattern could be added along side the existing patterns, which could then be deprecated. For example, the existing pcase-defmacro body could be turned into the generated lsp-interface--pcase-pattern-INTERFACE function, and the pcase-defmacro repurposed to return a new lsp-interface pattern.

Thanks for your attention and feedback. Whatever the path forward, I'll be happy to contribute—I don't mean this to be a rant or demand! I'll probably start poking at the deprecation approach when I get some free time.

[iNecas]

I'm recently experiencing performance issues in Emacs every time generating a documentation for pcase is needed. I've narrowed the issue down to when calling (documentation 'pcase), which can easily take tens of seconds, and eventually causes issues in code completion or searching documentation via helpful functions. I don't have a final proof, but I highly suspect lsp-mode adding so many pcases there is contributing a lot to it.

Running emacs master branch on 2024-07-15 eae1104f97ef944127eb5c977129b55f137e0830 and I see couple of changes in pcase recently, so it might be contributing to this.

[kiennq]

I like your proposal especially when it has the precedent of cl-struct. Would you be willing to provide a PR for that? This will be a breaking change, but it would gear toward a better functionality as well documentation readability for pcase. @leungbk as well.

[chrisbouchard]

@kiennq Yeah! I started looking into it a bit last week. I can say that there doesn't seem to be any way to deprecate a pcase form defined with pcase-defmacro, like one would a normal function or macro, so that's something that would need to be communicated separately.

(I tried using (declare (obsolete …)), and also make-obsolete on the NAME--pcase-macroexpander function it generates. Unfortunately the byte compiler never actually sees a (NAME--pcase-macroexpander …) form when compiling (pcase … ((NAME …))—the NAME--pcase-macroexpander function is called directly while expanding the pcase macro—so there's no deprecation warning. Also, the way that pcase builds its docstring means it won't generate a deprecation warning there either.)

[dgutov]

Maybe deprecating is not much as issue, if the macros are only used in the same package.

[chrisbouchard]

That's actually something I'm unclear on: are these pcase forms part of lsp-mode's public interface? Or are they only an internal implementation detail? I'm only an lsp-mode end user, so I'm not familiar with the conventions.

I was planning to keep the old pcase forms around in this PR, delegating to the new one—then they could be dropped in the next major version. If they are only meant to be used internally, then maybe we can just drop them now. (Though, of course, Hyrum's Law might apply…)

[chrisbouchard]

I just opened #4559 as a draft. It currently adds the new (lsp-interface INTERFACE ...) pcase form and preserves the old per-interface forms as well. I'll start replacing uses of the old pcase forms in lsp-mode next.

[kiennq]

@yyoncho @leungbk, is it okay to drop support for the old pcase form? It makes Emacs slower (might be due to a bug in the new version) and also pollute the pcase docs so more relevant forms are hidden. We can announce this as a breaking change in the CHANGELOG so broken packages that depends on the old behaviors can fix it if they want.

[leungbk]

kiennq @.***> writes:

@yyoncho @leungbk, is it okay to drop support for the old pcase form? It makes Emacs slower (might be due to a bug in the new version) and also pollute the pcase docs so more relevant forms are hidden. We can announce this as a breaking change in the CHANGELOG so broken packages that depends on the old behaviors can fix it if they want.

I'm OK with dropping the old form.

[chrisbouchard]

I'm OK with dropping the old form.

That does simplify things. :tada:

[chrisbouchard]

Sorry for all the commit noise. I think that PR is ready for review. :+1: