bug#32795: 26.1; provided-mode-derived-p does not support parent modes set with defalias

bug#32795: 26.1; provided-mode-derived-p does not support parent modes set with defalias

[Andrew Schwartzmeyer]

The actual bug was that I expected was for `TODO` to be highlighted in a
`CMakeLists.txt` file. Normally this is done by `global-hl-todo-mode`,
which is activated for all modes derived from `prog-mode`. However,
`cmake-mode` is derived from `cmake--parent-mode` which is set like
this:

    ;; For compatibility with Emacs < 24
    (defalias 'cmake--parent-mode
      (if (fboundp 'prog-mode) 'prog-mode 'fundamental-mode))

After researching, I found multiple packages that set the parent mode
like this, using `prog-mode` when it's available. So with Emacs 26.1,
`cmake-mode` definitely derives from `prog-mode`, but the function that
`hl-todo-mode` uses to check this, `derived-mode-p`, does not handle
`defalias` correctly. The `hl-todo-mode` logic is:

    (defcustom hl-todo-activate-in-modes '(prog-mode) ...)
    (defun hl-todo--turn-on-mode-if-desired ()
      (when (apply #'derived-mode-p hl-todo-activate-in-modes)
        (hl-todo-mode 1)))

If this was working as expected, it would see `cmake-mode` as derived
from `prog-mode`, and therefore highlight the `TODO`, but it does not.
I was able to make a minimal repro as follows:

    ;; Broken behavior (seen in cmake-mode.el and groovy-mode.el):
    (defalias 'alias-parent-mode
      (if (fboundp 'prog-mode) 'prog-mode 'fundamental-mode))
    
    (define-derived-mode alias-mode alias-parent-mode "Test")

    ;; Should print `prog-mode', but prints `alias-parent-mode'
    (message "Aliased derived: %s"
             (provided-mode-derived-p 'alias-mode 'prog-mode))
    
    ;; Existing working behavior:
    (define-derived-mode test-mode prog-mode "Test")

    ;; Correctly prints `prog-mode'
    (message "Directly derived: %s"
    (provided-mode-derived-p 'test-mode 'prog-mode))

After looking at `derived-mode-p`, I followed it to
`provided-mode-derived-p`, and I would like to propose the following
patch to it (with an appropriate added commit message):

---
 lisp/subr.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 9e880bc88..a35734812 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1922,7 +1922,11 @@ Only affects hooks run in the current buffer."
 Uses the `derived-mode-parent' property of the symbol to trace backwards.
 If you just want to check `major-mode', use `derived-mode-p'."
   (while (and (not (memq mode modes))
-              (setq mode (get mode 'derived-mode-parent))))
+              (let ((parent (get mode 'derived-mode-parent)))
+                (let ((parentfn (symbol-function parent)))
+                  (setq mode (if (and parentfn (symbolp parentfn))
+                                 parentfn
+                               parent))))))
   mode)
 
 (defun derived-mode-p (&rest modes)
--

This is kind of similar to the logic used by
https://github.com/Fanael/parent-mode/blob/master/parent-mode.el, except
(a) I wrote it originally, and (b) their implementation is recursive,
while I've kept `provided-mode-derived-p` iterative.

After testing, this fixes my original bug, and has not appeared to cause
unexpected behavior. It possibly fixes other bugs, anything that
expected to accurately deduce the parent of a derived mode.

Thanks,

Andy

P.S. Thanks molloy on #emacs for your help when looking at this.

Emacs Version(s):

In GNU Emacs 26.1 (build 1, x86_64-apple-darwin14.5.0, NS appkit-1348.17 Version 10.10.5 (Build 14F2511))
 of 2018-05-30 built on builder10-10.porkrind.org

Also exhibited on Emacs 26.1 on Linux

bug#32795: Acknowledgement (26.1; provided-mode-derived-p does not support parent modes set with defalias)

[Andrew Schwartzmeyer]

Hi all,

I actually just found _another_ scenario that breaks due to this. I was 
editing a Jenkinsfile (groovy-mode) and expecting dtrt-indent to 
automatically adjust the indent, but it was not. Turns out it's the same 
reason again!

https://github.com/jscheid/dtrt-indent/blob/e860db7235147ed5ac1fd8f12b51dbb7cf2e75f1/dtrt-indent.el#L207

     (define-globalized-minor-mode dtrt-indent-global-mode 
dtrt-indent-mode
       (lambda ()
         (when (derived-mode-p 'prog-mode 'text-mode)
     (dtrt-indent-mode))))

The check `(derived-mode-p 'prog-mode 'text-mode)` returns nil, because 
the mode that groovy-mode derived from is set up like this:

     (defalias 'groovy-parent-mode
       (if (fboundp 'prog-mode) 'prog-mode 'fundamental-mode))

Using this for `provided-mode-derived-p` fixes it:

     (defun provided-mode-derived-p (mode &rest modes)
       "Non-nil if MODE is derived from one of MODES.
     Uses the `derived-mode-parent' property of the symbol to trace 
backwards.
     If you just want to check `major-mode', use `derived-mode-p'."
       (while
           (and
            (not (memq mode modes))
            (let* ((parent (get mode 'derived-mode-parent))
                   (parentfn (symbol-function parent)))
              (setq mode (if (and parentfn (symbolp parentfn)) parentfn 
parent)))))
       mode)

Thanks Robert Pluim for the `let*` trick, I was wondering how you do 
that without nesting `let` expressions.

Does anyone have other suggestions before I send a patch file?

Thanks,

Andy

On 09/21/2018 8:22 am, help-debbugs@gnu.org wrote:
> Thank you for filing a new bug report with debbugs.gnu.org.
> 
> This is an automatically generated reply to let you know your message
> has been received.
> 
> Your message is being forwarded to the package maintainers and other
> interested parties for their attention; they will reply in due course.
> 
> Your message has been sent to the package maintainer(s):
>  bug-gnu-emacs@gnu.org
> 
> If you wish to submit further information on this problem, please
> send it to 32795@debbugs.gnu.org.
> 
> Please do not send mail to help-debbugs@gnu.org unless you wish
> to report a problem with the Bug-tracking system.

bug#32795: Acknowledgement (26.1; provided-mode-derived-p does not support parent modes set with defalias)

[Andrew Schwartzmeyer]

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

Patch with tests attached. Please let me know if I missed anything, or 
if the code could be improved. I read through CONTRIBUTE and may have 
accidentally sent this patch in as a new bug report, sorry!

This was generated from 'emacs-26' where the bug occurs, and will merge 
just fine into 'master'. The bug also exists on 'emacs-25' but the code 
is slightly different ('provided-mode-derived-p' had not been extracted 
yet from 'derived-mode-p'). I can supply a back-ported patch as well.

Thanks,

Andy

On 09/21/2018 8:22 am, help-debbugs@gnu.org wrote:
> Thank you for filing a new bug report with debbugs.gnu.org.
> 
> This is an automatically generated reply to let you know your message
> has been received.
> 
> Your message is being forwarded to the package maintainers and other
> interested parties for their attention; they will reply in due course.
> 
> Your message has been sent to the package maintainer(s):
>  bug-gnu-emacs@gnu.org
> 
> If you wish to submit further information on this problem, please
> send it to 32795@debbugs.gnu.org.
> 
> Please do not send mail to help-debbugs@gnu.org unless you wish
> to report a problem with the Bug-tracking system.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-provided-mode-derived-p-when-the-parent-is-an-al.patch --]
[-- Type: text/x-diff; name=0001-Fix-provided-mode-derived-p-when-the-parent-is-an-al.patch, Size: 2833 bytes --]

From f39d7f2b5e92e085927918cf4f9da4ba8df8f366 Mon Sep 17 00:00:00 2001
From: Andrew Schwartzmeyer <andrew@schwartzmeyer.com>
Date: Fri, 21 Sep 2018 00:10:47 -0700
Subject: [PATCH] Fix 'provided-mode-derived-p' when the parent is an alias

Authors of Emacs packages often derive from an alias of a mode instead
of the mode directly.  This is especially the case when deriving from
'prog-mode' as it is relatively new.  Unfortunately, using
'derived-mode-p' to check if some mode is derived from an alias of
'prog-mode' does not work as expected.  The bug is that
'provided-mode-derived-p' should not only return non-nil when MODE is
one of MODES, but also when MODE is an alias of one of MODES.
* lisp/subr.el (provided-mode-derived-p):
Return non-nil when MODE is an alias of any of MODES (Bug#32795).
* test/lisp/subr-tests.el: Add tests for the above.
---
 lisp/subr.el            |  9 +++++++--
 test/lisp/subr-tests.el | 12 ++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 7582b6cdb..6c625b40d 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1919,9 +1919,14 @@ delay-mode-hooks
 (defun provided-mode-derived-p (mode &rest modes)
   "Non-nil if MODE is derived from one of MODES.
 Uses the `derived-mode-parent' property of the symbol to trace backwards.
+Non-nil even if MODE is derived from an alias of any of MODES.
 If you just want to check `major-mode', use `derived-mode-p'."
-  (while (and (not (memq mode modes))
-              (setq mode (get mode 'derived-mode-parent))))
+  (while
+      (and
+       (not (memq mode modes))
+       (let* ((parent (get mode 'derived-mode-parent))
+              (parentfn (symbol-function parent)))
+         (setq mode (if (and parentfn (symbolp parentfn)) parentfn parent)))))
   mode)
 
 (defun derived-mode-p (&rest modes)
diff --git a/test/lisp/subr-tests.el b/test/lisp/subr-tests.el
index 430d71903..a9f72c26d 100644
--- a/test/lisp/subr-tests.el
+++ b/test/lisp/subr-tests.el
@@ -62,6 +62,18 @@
                      (quote
                       (0 font-lock-keyword-face))))))))
 
+(ert-deftest provided-mode-derived-p ()
+  ;; base case: `derived-mode' directly derives `prog-mode'
+  (should (progn
+            (define-derived-mode derived-mode prog-mode "test")
+            (provided-mode-derived-p 'derived-mode 'prog-mode)))
+  ;; edge case: `derived-mode' derives an alias of `prog-mode'
+  (should (progn
+            (defalias 'parent-mode
+              (if (fboundp 'prog-mode) 'prog-mode 'fundamental-mode))
+            (define-derived-mode derived-mode parent-mode "test")
+            (provided-mode-derived-p 'derived-mode 'prog-mode))))
+
 (ert-deftest number-sequence-test ()
   (should (= (length
               (number-sequence (1- most-positive-fixnum) most-positive-fixnum))
-- 
2.19.0

bug#32795: Acknowledgement (26.1; provided-mode-derived-p does not support parent modes set with defalias)

[Eli Zaretskii]

> Date: Mon, 24 Sep 2018 21:15:28 -0700
> From: Andrew Schwartzmeyer <andrew@schwartzmeyer.com>
> 
> The bug also exists on 'emacs-25' but the code is slightly different
> ('provided-mode-derived-p' had not been extracted yet from
> 'derived-mode-p'). I can supply a back-ported patch as well.

No need for that, as we don't expect to make any more releases from
the emacs-25 branch.

Thanks.