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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 32795 in the body.
You can then email your comments to 32795 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Andrew Schwartzmeyer <andrew <at> schwartzmeyer.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 26.1; provided-mode-derived-p does not support parent modes set with
 defalias
Date: Fri, 21 Sep 2018 00:20:48 -0700

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

Message #8 received at 32795 <at> debbugs.gnu.org (full text, mbox):

From: Andrew Schwartzmeyer <andrew <at> schwartzmeyer.com>
To: 32795 <at> debbugs.gnu.org
Subject: Re: bug#32795: Acknowledgement (26.1; provided-mode-derived-p does
 not support parent modes set with defalias)
Date: Mon, 24 Sep 2018 19:36:11 -0700

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 <at> 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 <at> gnu.org
> 
> If you wish to submit further information on this problem, please
> send it to 32795 <at> debbugs.gnu.org.
> 
> Please do not send mail to help-debbugs <at> gnu.org unless you wish
> to report a problem with the Bug-tracking system.

Message #11 received at 32795 <at> debbugs.gnu.org (full text, mbox):

From: Andrew Schwartzmeyer <andrew <at> schwartzmeyer.com>
To: 32795 <at> debbugs.gnu.org
Subject: Re: bug#32795: Acknowledgement (26.1; provided-mode-derived-p does
 not support parent modes set with defalias)
Date: Mon, 24 Sep 2018 21:15:28 -0700

[Message part 1 (text/plain, inline)]

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 <at> 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 <at> gnu.org
> 
> If you wish to submit further information on this problem, please
> send it to 32795 <at> debbugs.gnu.org.
> 
> Please do not send mail to help-debbugs <at> gnu.org unless you wish
> to report a problem with the Bug-tracking system.

[0001-Fix-provided-mode-derived-p-when-the-parent-is-an-al.patch (text/x-diff, attachment)]

Message #14 received at 32795 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andrew Schwartzmeyer <andrew <at> schwartzmeyer.com>
Cc: 32795 <at> debbugs.gnu.org
Subject: Re: bug#32795: Acknowledgement (26.1;
 provided-mode-derived-p does not support parent modes set with
 defalias)
Date: Tue, 25 Sep 2018 08:22:53 +0300

> Date: Mon, 24 Sep 2018 21:15:28 -0700
> From: Andrew Schwartzmeyer <andrew <at> 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.

Message #21 received at 32795-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: andrew <at> schwartzmeyer.com
Cc: 32795-done <at> debbugs.gnu.org
Subject: Re: bug#32832: [PATCH] Fix 'provided-mode-derived-p' when the parent
 is an alias
Date: Sat, 29 Sep 2018 10:01:01 +0300

> From: andrew <at> schwartzmeyer.com
> Date: Mon, 24 Sep 2018 21:09:39 -0700
> 
> From: Andrew Schwartzmeyer <andrew <at> schwartzmeyer.com>
> 
> 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.

Thanks, pushed.

In the future please either format your patches with "git
format-patch" or make sure the first line of the commit log message is
a single sentence followed by an empty line (this is explained in
CONTRIBUTE), to make the patch application easier.

Also, you don't seem to have a copyright assignment on file, and this
contribution comes close to exhausting the amount of changes we can
accept without such an assignment.  So I urge you to start your legal
paperwork rolling, to allow us to accept more contributions from you.
If you are interested, I will send you the assignment form off-list.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.