Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 07/09/2023 17:39, João Távora wrote:
> On Thu, Sep 7, 2023 at 2:00 AM Dmitry Gutov <dmitry@gutov.dev> wrote:
> > On 02/09/2023 12:55, João Távora wrote:
> > > Anyway I invite everyone to have a look and try to improve it, perhaps
> > > moving it out of Eglot into the shiny new "refactoring interface" if
> > > those ever become a thing.
> >
> > Regarding the diff approach vs. "shiny new", I've run a little
> > comparison with VS Code: doing a rename of 'glyph_row' across the Emacs
> > codebase (a moderately sized project) takes about ~1.44s to produce the
> > diff.
>
> FWIW, I got 0.85s on that same test on a GNU/Linux VM running on a 6
> yo laptop with 4 cores assigned.  IMO this is more than acceptable for
> this contrived example which renames a symbol across many files.

Not that many: these are 260 edits just across 3 files.

And I see nothing contrived about such an example. There exist much 
bigger projects anyway, which we should ideally support well too.

> A much more typical example of renaming a symbol in a single function
> resolved in 0.02s.  Most examples of applying quick fixes and moving
> .hpp inlined member functions to out-of-line in .cpp resolve in similar
> quasi-instantaneous fashion.

That's great, but for such small examples one could almost do without 
advanced interfaces too, couldn't they?

> Now, let me clarify what I meant by "shiny new refactoring interface".
> It's not what you think I meant, I believe. What I meant is a framework
> for:
>
> 1. collecting 'potential sets of changes' -- for clarity let's call
>     "actions" after LSP nomenclature -- from a number of different
>     backends.  Examples of actions: "rename thing at point", "organize
>     imports", "write as while loops as cl-loop"
>
> 2. presenting actions names to the user -- either via the minibuffer
>     or by annotating buffer position where these tweaks apply.

These could be split into two:

- A hook which would return the available actions at point. This seems 
to be the primary mode of operations of LSP editors: first you choose 
the location, and then find out which which operations apply to it.

- Some way (possibly also a hook) to find all the "points of interest" 
in a buffer -- these could also be annotated with a lightbulb or etc in 
the margin or fringe. Not sure which place this has when using LSP 
(perhaps some diagnostics with edits attached?), but I vaguely remember 
being suggested lists of refactorings by IDEs I tried in the past (<10 
years ago).

Aside from "renames", which would probably apply almost everywhere 
(though e.g. clangd doesn't know how to rename macros; I wonder at what 
step that we would indicate that to the user).

> 3. after the user chooses an action, develop it into a set of changes
>     -- again, for clarity and consonance with LSP nomenclature, we
>     may call these "edits".

Sure. Possibly carrying more-or-less same information that LSP's edits 
do. Though we could see whether it can be useful to annotate them with 
additional info, e.g. name/type/etc (where we'd deduce that a function 
called X is being renamed, or a file is being moved, or both; which 
could be reflects in the changeset's display more intelligently).

> 4. presenting these edits to the user via a number of different
>     user-confirmation strategies.
>
> For very common "rename" or "organize imports" actions, steps 1
> and 2 may be coalesced and a special command may be provided that
> goes directly to step 3.  LSP supports these "shortcuts".

Sure. I wouldn't know what position "organize imports" should apply to 
anyway. But to do with backends which don't support it?

> Regarding 4 (which seems to be the current focal point of the
> discussion) Eglot has three main types of confirmation strategies
> available.  See eglot-confirm-server-edits for more.
>
> * 'diff', which just presents the edits
> * 'summary', which summarizes the edits in the minibuffer and prompts
>    to apply.
> * nil, which means no confirmation at all
>
> There is also 'maybe-diff' and 'maybe-summary' which behave like
> nil if all the files in the change list are already visited buffers.

Not sure why the cutoff is there: and not for example "if all the 
changes are in the current file", or "are all visible on the screen".

Anyway, the more the merrier, I suppose.

> That said, it's the whole sequence 1 to 4, not just 4, that needs
> to be generalized out of Eglot and into Emacs.  Note that some parts
> of 1..4, like the annotation of actions in buffers or the support for
> file creation and deletion, aren't yet in Eglot, so there's good work
> to be done.  Some of the "annotation" logic already exists in Flymake,
> and Flymake must be taken into the equation too, and early.

The reason for talking about 4 first is twofold:

- I understand 4 better than the rest at this point of time, and it 
could be plugged into some existing bits of functionality already (e.g. 
Eglot's refactorings and Xref/Project names).

- There will likely be two entry points to the new library, just like 
with Xref we have xref-backend-functions and xref-show-xrefs. The former 
depends on the latter. By choosing a good enough data model for step 4 
(e.g. how changes are represented), we can better build requirements for 
step 1, given that whatever hook(s) we add, they would already need to 
be documented with the details about the data in/out flow.

> Also, it would be very good if we could have an early backend which is
> *not* LSP.  An obvious candidate is Elisp.  I'd like to know what is
> the minimal effort to write a lisp function based on the new
> macroexpanding tricks that Stefan Monnier developed that says
> precisely where a given symbol is used as a variable in a function.
> As far as I understand,  these techniques are currently being used for
> accurate completion, but they could maybe be used for accurate
> refactorings.

Xref could easily be an early backend which supports renaming only. 
Eglisp's xref-backend-references could make use of the macroexpanding 
thing to include local variables too.

> > Not to belittle the new addition, though - it's a good step for Eglot.
>
> As someone who actually uses Eglot daily and is an avid consumer
> of refactoring functionality, my opinion is that the "diff"
> confirmation strategy that Philip proposed and implemented is freaking
> great, even if the implementation is a bit contorted (though mostly
> isolated).
>
> IMO diff is the lingua franca for communicating changes to source
> code around the world, even in those pretty web interfaces that
> you and many others like.  So it makes full sense to support it
> and to support it well.

I'm okay with diffs myself obviously, and we should keep this UI option 
if feasible, but I'd also prefer to have something more compact 
available, for the "web interface lovers" of the world. Diffs do have 
some barrier to understanding, even we are very much used to them. And 
diff-with-context can be pretty long too.

> > - I would probably want to bind the originally proposed
> > 'diff-apply-everything' to 'C-c C-c' (e.g. "commit the change to disk"),
> > but that's already taken in diff-mode.
>
> > - 'git diff' has a less-frequently used option called '--word-diff'
> > which could reduce the length of the diff in the case I am testing (but
> > I guess diff-mode would have to be updated to support that output). And
> > another way in that direction would be to reduce the size of the diff
> > context (e.g. to 0).
>
> I'm not at all opposed to implementing other confirmation strategies
> that look more like what VSCode or other editors show users.  Or
> augmenting the existing 'diff' strategy with new shortcuts and stuff.
> But I wouldn't overthink UI details at this point in the game either.

The first suggestion in particular looked like a modest enough tweak 
that does not depend on the rest of this discussion (and it would likely 
have a diff-specific implementation). But if we try to make it atomic, 
that's an increase in complexity, of course.