bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#62761: ruby-add-log-current-method drops some segments when singleto


From: Dmitry Gutov
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
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0

On 11/04/2023 08:51, Eli Zaretskii wrote:
Date: Tue, 11 Apr 2023 00:02:03 +0300
From: Dmitry Gutov <dmitry@gutov.dev>

    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))))






reply via email to

[Prev in Thread] Current Thread [Next in Thread]