From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Dmitry Gutov Newsgroups: gmane.emacs.devel Subject: Re: Adding refactoring capabilities to Emacs Date: Sat, 9 Sep 2023 01:35:35 +0300 Message-ID: <858c929d-61c3-633e-ae9a-de8c5e5c40d1@gutov.dev> References: <83fs4f36wi.fsf@gnu.org> <5ddf5fe1-e5c9-2105-5cb8-a96bc8cecc86@gutov.dev> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="13440"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Cc: Eli Zaretskii , "Philip K." , Stefan Monnier , emacs-devel@gnu.org To: =?UTF-8?B?Sm/Do28gVMOhdm9yYQ==?= Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sat Sep 09 00:36:38 2023 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qek5d-0003A7-2I for ged-emacs-devel@m.gmane-mx.org; Sat, 09 Sep 2023 00:36:38 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qek4o-0003C0-OL; Fri, 08 Sep 2023 18:35:46 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qek4m-0003BN-NB for emacs-devel@gnu.org; Fri, 08 Sep 2023 18:35:44 -0400 Original-Received: from out5-smtp.messagingengine.com ([66.111.4.29]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qek4j-0004qK-IH; Fri, 08 Sep 2023 18:35:44 -0400 Original-Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id 564865C0121; Fri, 8 Sep 2023 18:35:39 -0400 (EDT) Original-Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Fri, 08 Sep 2023 18:35:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gutov.dev; h=cc :cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to; s=fm3; t= 1694212539; x=1694298939; bh=QwBhSkcRbBrmhtHz7VrPQ45hjTqIyASIYWj brt7Sx90=; b=fIaPc5OAPJsaqSkiriFnDOXNeJM1+tSseZ8kuelwX4RaxGjh4oQ eHuSgdqgsOiJkQW3ZCee/DtiblmuBJTtEs6WwyAQ5IASdqUOZW1I/rsxqezy57sZ +cwIOSoyjhy6HqU2UAJziGneEHUeU6YqfVgpeHoHcWVF1YCJLRftkMgtOugqVjQN +91EWOdriUve9tZRAoTZ2HbPB+A0oKftpd3fVjh0L/E3lAgRKex2sKvga4U2SPe+ ycCB8YWeDi458s9yeQu1ilD9eJG5XPIxpf3kevrd/ZrD9d2Dc2GPWSKVE14ThFNl isTXCWcvSllg2018Zf/Hz8hEz8i+OGCkg6Q== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t= 1694212539; x=1694298939; bh=QwBhSkcRbBrmhtHz7VrPQ45hjTqIyASIYWj brt7Sx90=; b=yYyLUZhCbADlTFBJ+6oji3AfWFe5+bKWfK4cMOam0NsDkvAqM6h RODwLjyHTXIY8FgflyyyfZtXbT+KNUDAsKGNTgMhtNil6nLPOHGVXTkSW5PYtcJm ck5uENoF2OzWJh7kRfKgGGlI7gP2N4G4p0ESmbqFH1fW+nP9Z7OBvX3n+cnwfTdw vNvQYaaXY294qgzfMXuHuK2wWfkR2OkIQjCn9Jg8f35bnW0pPV3wxlL8kOiN3wMU XucYkiF9YOjNfy+K9RvCvnDA99biaOmqLJMMYW+UJeKmyrq9RP4KD8NoDKybhToK c8coFzNHteBVaiS5smVkcLn9Uf4YKSn3L1g== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedviedrudehkedgudduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepkfffgggfuffvvehfhfgjtgfgsehtkeertddtfeejnecuhfhrohhmpeffmhhi thhrhicuifhuthhovhcuoegumhhithhrhiesghhuthhovhdruggvvheqnecuggftrfgrth htvghrnhephfffheeljeffgeffueeghfekkedtfffgheejvdegjeettdduheeufffggfef jeehnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepug hmihhtrhihsehguhhtohhvrdguvghv X-ME-Proxy: Feedback-ID: i0e71465a:Fastmail Original-Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 8 Sep 2023 18:35:37 -0400 (EDT) Content-Language: en-US In-Reply-To: Received-SPF: pass client-ip=66.111.4.29; envelope-from=dmitry@gutov.dev; helo=out5-smtp.messagingengine.com X-Spam_score_int: -42 X-Spam_score: -4.3 X-Spam_bar: ---- X-Spam_report: (-4.3 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-1.473, RCVD_IN_DNSWL_LOW=-0.7, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:310371 Archived-At: On 08/09/2023 14:10, João Távora wrote: > On Thu, Sep 7, 2023 at 11:39 PM Dmitry Gutov 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