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.bugs Subject: bug#62761: ruby-add-log-current-method drops some segments when singleton definition references outer module Date: Wed, 12 Apr 2023 03:17:55 +0300 Message-ID: <0e094cf2-70e0-29c8-30b4-a4a21d78eb62@gutov.dev> References: <9d81bdc8-355d-7d32-8d3c-361ea0ff585c@gutov.dev> <83bkjvarvt.fsf@gnu.org> 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="7685"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Cc: 62761@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Apr 12 02:19:22 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@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 1pmOCm-0001ni-S2 for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 12 Apr 2023 02:19:22 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pmOCX-0008Td-HS; Tue, 11 Apr 2023 20:19:05 -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 1pmOCV-0008TR-Id for bug-gnu-emacs@gnu.org; Tue, 11 Apr 2023 20:19:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1pmOCV-0006Dg-8r for bug-gnu-emacs@gnu.org; Tue, 11 Apr 2023 20:19:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pmOCU-0006kV-Ea for bug-gnu-emacs@gnu.org; Tue, 11 Apr 2023 20:19:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Dmitry Gutov Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 12 Apr 2023 00:19:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 62761 X-GNU-PR-Package: emacs Original-Received: via spool by 62761-submit@debbugs.gnu.org id=B62761.168125868925883 (code B ref 62761); Wed, 12 Apr 2023 00:19:02 +0000 Original-Received: (at 62761) by debbugs.gnu.org; 12 Apr 2023 00:18:09 +0000 Original-Received: from localhost ([127.0.0.1]:38541 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pmOBc-0006jP-UR for submit@debbugs.gnu.org; Tue, 11 Apr 2023 20:18:09 -0400 Original-Received: from wnew4-smtp.messagingengine.com ([64.147.123.18]:40921) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pmOBa-0006it-26 for 62761@debbugs.gnu.org; Tue, 11 Apr 2023 20:18:07 -0400 Original-Received: from compute6.internal (compute6.nyi.internal [10.202.2.47]) by mailnew.west.internal (Postfix) with ESMTP id 2CE292B06DCE; Tue, 11 Apr 2023 20:17:59 -0400 (EDT) Original-Received: from mailfrontend2 ([10.202.2.163]) by compute6.internal (MEProxy); Tue, 11 Apr 2023 20:17:59 -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=fm1; t= 1681258678; x=1681262278; bh=i2mYzfrAtLieRm3gXq92lZV6niiuZIs311a aO/ndgUI=; b=pYSYHyH0t+QLXqezLPFsDgpzfung1kT0P85HZ1/pNw9IKXHYxTx 0FxgdCSlMS9jE7FRaINwwGxFUllqhT3I2mVTopy7sl510wvMiLRzz9deD3f8SClk aFI0S0UDLGtwnUtBU2r6I2aIa0TdHHozv63Y0Q/amGOqNnYUrf08oh7AkjE+lO/G fmXK5yCFOSl73CUdbtyrJ207SVn/N/psoT8Xid3wgcMBGAAdSwF0BzXTxtJFnNb/ 99P1Ou1bxzwxnriYu1dAFrptL7ySFHbMFDrPEeLvy+N7vtfm1PqXSBo0MabWC8Xb x5vIpRWO1/3cZRwWZys/iWN3Il+UwDdRMoA== 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=fm3; t= 1681258678; x=1681262278; bh=i2mYzfrAtLieRm3gXq92lZV6niiuZIs311a aO/ndgUI=; b=VpG5uCPnFZmzO1ZZxq8GoG0czvyAKgkLopq+AlhUmqO2hbSc31x eTbsfAohFVZxRlEaQjtftb4wid5XVcd9U1t9g7oaoGZQXwQjyzLakv2qgxevmt9j o+xhGWfPYDQUqELTkQaG4BGrmplV4MXoNEJHLLgocgfZzXdCa7bsogw4d+b0RrUh QnVdKQLeyjVYvrvCsErUo+dftVoqeR6w7ivJXFgYkG8Tl0dZTvv1UN35ky0RYtFt Jy971rQo2DARWuHtEwn1BCYr5aI9QN81yFnwqt6/4RstbYPQfh2u3fFCeVskgDDe PSQv+pMsb4dW3nvod1caeZOrnptQGqPoEEQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrvdekhedgfeefucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepkfffgggfuffvvehfhfgjtgfgsehtkeertddtfeejnecuhfhrohhmpeffmhhi thhrhicuifhuthhovhcuoegumhhithhrhiesghhuthhovhdruggvvheqnecuggftrfgrth htvghrnhephfffheeljeffgeffueeghfekkedtfffgheejvdegjeettdduheeufffggfef jeehnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepug hmihhtrhihsehguhhtohhvrdguvghv X-ME-Proxy: Feedback-ID: i0e71465a:Fastmail Original-Received: by mail.messagingengine.com (Postfix) with ESMTPA; Tue, 11 Apr 2023 20:17:56 -0400 (EDT) Content-Language: en-US In-Reply-To: <83bkjvarvt.fsf@gnu.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:259687 Archived-At: On 11/04/2023 08:51, Eli Zaretskii wrote: >> Date: Tue, 11 Apr 2023 00:02:03 +0300 >> From: Dmitry Gutov >> >> module M >> module N >> module C >> class D >> def C.foo >> _ >> end >> end >> end >> end >> end >> >> (ruby-add-log-current-method) currently returns "M::C.foo" >> >> While it should return "M::N::C.foo". Patch attached. >> >> This discovery stems from Mattias Engdegård's report (in private) about >> an ignored return value from `nreverse`. >> >> Is this good for emacs-29? > > I guess so. But I wonder: this code was there since ruby-mode.el was > added to Emacs in 2008, Right. But the condition (if (string-equal (car ml) (car mn)) succeeds in a very rare case to begin with: when the definition uses a baroque, redundant kind of syntax (repeating the name of the constant instead of just using "def self.xxx" inside the appropriate module). And 'nreverse' is only harmful when there are >1 element in the list, meaning the bug also needs there to be at least 2 levels of nesting above said constant (both M and N, in the above example). And ruby-add-log-current-method isn't used often these days anyway, I guess (except from a little package of mine called robe). All this is to say, it's not surprising that this bug was never reported in the recent years. > so are you saying that (cdr ml) can never be > nil at this place, or if it is, it's okay to leave ml at the nil > value? It definitely can be nil (that's when the loop terminates -- as soon as ml reaches that value). > IOW, the return value of nreverse has been ignored, but what about the > nreverse call before that: > >> --- a/lisp/progmodes/ruby-mode.el >> +++ b/lisp/progmodes/ruby-mode.el >> @@ -1911,7 +1911,7 @@ ruby-add-log-current-method >> (while ml >> (if (string-equal (car ml) (car mn)) >> (setq mlist (nreverse (cdr ml)) ml nil)) >> - (or (setq ml (cdr ml)) (nreverse mlist)))) >> + (setq ml (cdr ml)))) >> (if mlist >> (setcdr (last mlist) (butlast mn)) >> (setq mlist (butlast mn)))) > > Isn't the second nreverse there to undo the first? > > I guess I'm asking for slightly more detailed rationale for the change > you are proposing, to include the analysis of the code involved and > where that code is mistaken. For example, why not say > > (setq mlist (nreverse mlist)) > or > (setq ml (nreverse mlist)) > > instead of just removing the 2nd nreverse call? There are, in total, 3 'nreverse' calls inside the surrounding 'let'. In the normal case (when the constant name is "resolved", that is, the search has a hit), the second 'nreverse', visible in the patch context above, is executed. The first one is not visible, and it's unconditional. Your point about "counteracting" is not without merit: one case I didn't test is when the scope matching the name is not found (meaning the constant on which the method is defined fails to "resolve"). Then the middle 'nreverse' is never called, and the list structure stays reversed. But that's basically undecidable code, and even one could write something like this using runtime constant redefinition, our parser cannot decide the full name of the constant the method is being defined at. Here's a slight modification of the patch which leaves just one (optional) 'nreverse' call, which acts on a fully temporary structure, and thus is unable to alter the values held inside mlist unless really intended. It keeps the behavior the same in the previous cases and slightly improves the one above (the "undecidable" one): diff --git a/lisp/progmodes/ruby-mode.el b/lisp/progmodes/ruby-mode.el index beccb8182a7..b252826680c 100644 --- a/lisp/progmodes/ruby-mode.el +++ b/lisp/progmodes/ruby-mode.el @@ -1905,13 +1905,13 @@ ruby-add-log-current-method (progn (unless (string-equal "self" (car mn)) ; def self.foo ;; def C.foo - (let ((ml (nreverse mlist))) + (let ((ml (reverse mlist))) ;; If the method name references one of the ;; containing modules, drop the more nested ones. (while ml (if (string-equal (car ml) (car mn)) (setq mlist (nreverse (cdr ml)) ml nil)) - (or (setq ml (cdr ml)) (nreverse mlist)))) + (setq ml (cdr ml)))) (if mlist (setcdr (last mlist) (butlast mn)) (setq mlist (butlast mn))))