bug#62761: ruby-add-log-current-method drops some segments when singleton definition references outer module

bug#62761: ruby-add-log-current-method drops some segments when singleton definition references outer module

[Dmitry Gutov]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

Example:

   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?

It seems pretty safe (with decent test coverage, including the new 
test), but probably low urgency. Not a regression either.

[-- Attachment #2: ruby-add-log-reference-containing.diff --]
[-- Type: text/x-patch, Size: 1837 bytes --]

diff --git a/lisp/progmodes/ruby-mode.el b/lisp/progmodes/ruby-mode.el
index beccb8182a7..1199af64821 100644
--- 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))))
diff --git a/test/lisp/progmodes/ruby-mode-tests.el b/test/lisp/progmodes/ruby-mode-tests.el
index 8a75c83d2c3..117385ea3e8 100644
--- a/test/lisp/progmodes/ruby-mode-tests.el
+++ b/test/lisp/progmodes/ruby-mode-tests.el
@@ -567,6 +567,22 @@ ruby-add-log-current-method-namespace-shorthand
     (search-backward "_")
     (should (string= (ruby-add-log-current-method) "C::D#foo"))))
 
+(ert-deftest ruby-add-log-current-method-singleton-referencing-outer ()
+  (ruby-with-temp-buffer (ruby-test-string
+                          "module M
+                          |  module N
+                          |    module C
+                          |      class D
+                          |        def C.foo
+                          |          _
+                          |        end
+                          |      end
+                          |    end
+                          |  end
+                          |end")
+    (search-backward "_")
+    (should (string= (ruby-add-log-current-method) "M::N::C.foo"))))
+
 (ert-deftest ruby-add-log-current-method-after-inner-class ()
   (ruby-with-temp-buffer (ruby-test-string
                           "module M

bug#62761: ruby-add-log-current-method drops some segments when singleton definition references outer module

[Eli Zaretskii]

> 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, 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?

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?

Thanks.

bug#62761: ruby-add-log-current-method drops some segments when singleton definition references outer module

[Dmitry Gutov]

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

bug#62761: ruby-add-log-current-method drops some segments when singleton definition references outer module

[Eli Zaretskii]

> Date: Wed, 12 Apr 2023 03:17:55 +0300
> Cc: 62761@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> 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):

LGTM, thanks.

bug#62761: ruby-add-log-current-method drops some segments when singleton definition references outer module

[Dmitry Gutov]

Version: 29.1

On 12/04/2023 09:17, Eli Zaretskii wrote:
>> Date: Wed, 12 Apr 2023 03:17:55 +0300
>> Cc:62761@debbugs.gnu.org
>> From: Dmitry Gutov<dmitry@gutov.dev>
>>
>> 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):
> LGTM, thanks.

Installed, marking as done.