Re: Adding refactoring capabilities to Emacs

[Dmitry Gutov]

On 08/09/2023 14:10, João Távora wrote:
> On Thu, Sep 7, 2023 at 11:39 PM Dmitry Gutov <dmitry@gutov.dev> wrote:
>
> > Not that many: these are 260 edits just across 3 files.
>
> glyph_row? As in 'struct glyph_row' of src/dispextern.h?  My clangd
> suggested changes to 7 files.   Maybe not contrived but certainly
> not common either, and well worth the subsecond latency considering
> the time you'd want to spend reviewing such a mammoth change.

Not the type but the identifier with the same name (struct glyph_row 
*glyph_row). Anyway, 3 or 7, it might as well be 70 files in another case.

Although the impact somewhat depends on whether the current performance 
is proportional to the number of files, or the number of edits.

> > > A much more typical example of renaming ab 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?
>
> For local renames, yes, which is precisely why the reason why eglot-rename
> has a special 'maybe-summary' confirmation method (I use maybe-diff)
> whose point is to skips the confirmation if it detects the change to be "local
> enough"

But a rename can just as well be global. Which is simple in theory, but 
it can be a good idea to double-check whether all identifiers are found 
anyway (or check for false positives). Though perhaps it's my habit due 
to working with Grep-based renames a lot.

> For others,  like extracting methods, etc, not at all.  They may be
> simple, but you generally still want to have a look at what's being
> especially if it targets on or

Sure.

> > > 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.
> > - 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
>
> That's what I meant yes "annotate".  But I'd leave the annotation
> interface to a second phase.  As you note, it's not obvious when to collect
> these actions with special affinity to buffer positions .  In LSP, some of
> them are already being collected because of diagnostics.

Yes, when to collect and how to display. The collection method could 
also be "pull" or "push": with hooks we usually do the "pull" thing, but 
since flymake (and often other diagnostics) work asynchronously a "push" 
mechanism might make more sense.

> Flymake is the main existing Emacs interface that periodically annotates
> the buffer with pieces of information about your code gotten from a server.
> It knows how to display things in the fringe, and is fairly customizable.
> You can have multiple backends, etc, it works asynchronously and generally
> has a lot of common logic for knowing how to update only parts of the
> buffer etc.
>
> It could be argued we should use it for this purpose, but then again, it
> could be argued we shouldn't, especially if we suspect equating "localized
> actions" to diagnostics is simply wrong.  This feature request came up in the
> past in the Eglot tracker and I gave it a go once or twice with Flymake.
> But maybe I just didn't try hard enough.

We should probably abstract over Flymake either way. But I agree that 
all this can be left until later.

> > 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).
>
> That's not a bad idea, but is an implementation detail.  We
> should probably not expose this representation (at least not too early)
> to conserve the freedom to change it.

You skipped over the remainder of my email where I explained why I liked 
to start with step 4.

We can still want to be able to tweak it during Emacs 30's development, 
of course, and maybe even later, but it's impossible not to discuss and 
document if we want customizable/swappable UIs for confirmation anyway.

> > Sure. I wouldn't know what position "organize imports" should apply to
> > anyway. But to do with backends which don't support it?
>
> Maybe at least one of the backends will support it.  Else issue an
> error, that's not much more I can see.  Some backends may
> not even support rename.  It's not like in xref where "find definitions"
> and "find references" which are things that almost all xref backends
> would be able to do.

Backends which don't support rename could fall back to xref which worst 
case falls back to grep. Or something like that.

Anyway, I guess we'll implicitly decide that both "rename" and "organize 
imports" are always available and create commands with dedicated 
bindings for them?

Why the latter, BTW? Or are there more commands like that? From what I'm 
reading, "organize imports" is not a part of the official LSP protocol, 
though there are extensions.

> > > 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.
>
>
>
> --
> João Távora