GNU bug report logs - #67795
[PATCH] Handle local-variable major-mode remaps specifying non-existent mode

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 67795 in the body.
You can then email your comments to 67795 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: Brian Leung <leungbk <at> posteo.net>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Handle local-variable major-mode remaps specifying
 non-existent mode
Date: Tue, 12 Dec 2023 13:52:53 +0000

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

Tags: patch





In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, 
cairo
version 1.18.0, Xaw3d scroll bars)
Repository revision: 9434ad25ce2747864e0bcf5665f65eb65a079178
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 
11.0.12101009
System Description: NixOS 24.05 (Uakari)

Configured using:
'configure
--prefix=/nix/store/q131p42vldq964fr9rpdz1qmsqrywa00-emacs-git-20231211.0
--disable-build-details --with-modules --with-x-toolkit=lucid
--with-xft --with-cairo --with-compress-install
--with-toolkit-scroll-bars --with-native-compilation
--without-imagemagick --without-small-ja-dic --with-tree-sitter
--without-xinput2 --without-xwidgets'

[0001-Handle-local-variable-major-mode-remaps-specifying-n.patch (text/patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Brian Leung <leungbk <at> posteo.net>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 67795 <at> debbugs.gnu.org
Subject: Re: bug#67795: [PATCH] Handle local-variable major-mode remaps
 specifying non-existent mode
Date: Tue, 12 Dec 2023 16:04:55 +0200

> From: Brian Leung <leungbk <at> posteo.net>
> Date: Tue, 12 Dec 2023 13:52:53 +0000
> 
> >From 46435cac589506ecb131f480171a2ec1f0f03c55 Mon Sep 17 00:00:00 2001
> From: Brian Leung <leungbk <at> posteo.net>
> Date: Tue, 12 Dec 2023 05:42:56 -0800
> Subject: [PATCH] Handle local-variable major-mode remaps specifying
>  non-existent mode
> 
> In the .clang-format file of current Emacs HEAD, the major mode is
> specified as yaml-mode via a local variable.  However, a user who has
> loaded yaml-ts-mode and executed
> 
> (add-to-list 'major-mode-remap-alist '(yaml-mode . yaml-ts-mode)
> 
> but does not have yaml-mode defined will find that opening the
> .clang-format file does not use yaml-ts-mode.
> 
> This patch fixes that.

Thanks, but if we want to integrate major-mode-remap-alist better, I'd
rather we did that in some lower-level place, so that we wouldn't need
to sprinkle these alist-get calls all over Emacs.

Also, if we do this, there will be no way for specifying a particular
mode in file-local variables.  Do we really want that?

Stefan, WDYT?

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Brian Leung <leungbk <at> posteo.net>, 67795 <at> debbugs.gnu.org
Subject: Re: bug#67795: [PATCH] Handle local-variable major-mode remaps
 specifying non-existent mode
Date: Tue, 12 Dec 2023 11:02:43 -0500

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

> Thanks, but if we want to integrate major-mode-remap-alist better, I'd
> rather we did that in some lower-level place, so that we wouldn't need
> to sprinkle these alist-get calls all over Emacs.
>
> Also, if we do this, there will be no way for specifying a particular
> mode in file-local variables.  Do we really want that?
>
> Stefan, WDYT?

I agree that we should try to keep it in "one place", but I don't think
it can be done right now without some code reorganization :-(

I can't wrap my head around what `hack-local-variables--find-variables`
is supposed to do, but for the other part of the change, maybe the patch
below is a good start?


        Stefan

[mode-remap.patch (text/x-diff, inline)]

diff --git a/lisp/files.el b/lisp/files.el
index f87e7807301..1935e4dbb4b 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3428,12 +3451,9 @@ set-auto-mode
     (if modes
 	(catch 'nop
 	  (dolist (mode (nreverse modes))
-	    (if (not (functionp mode))
-		(message "Ignoring unknown mode `%s'" mode)
-	      (setq done t)
-	      (or (set-auto-mode-0 mode keep-mode-if-same)
-		  ;; continuing would call minor modes again, toggling them off
-		  (throw 'nop nil))))))
+	    (or (setq done (set-auto-mode-0 mode keep-mode-if-same))
+		;; continuing would call minor modes again, toggling them off
+		(throw 'nop nil)))))
     ;; Check for auto-mode-alist entry in dir-locals.
     (unless done
       (with-demoted-errors "Directory-local variables error: %s"
@@ -3445,10 +3465,7 @@ set-auto-mode
     (and (not done)
 	 (setq mode (hack-local-variables t (not try-locals)))
 	 (not (memq mode modes))	; already tried and failed
-	 (if (not (functionp mode))
-	     (message "Ignoring unknown mode `%s'" mode)
-	   (setq done t)
-	   (set-auto-mode-0 mode keep-mode-if-same)))
+	 (setq done (set-auto-mode-0 mode keep-mode-if-same)))
     ;; If we didn't, look for an interpreter specified in the first line.
     ;; As a special case, allow for things like "#!/bin/env perl", which
     ;; finds the interpreter anywhere in $PATH.
@@ -3490,7 +3507,7 @@ set-auto-mode
                               (error
                                "Problem in magic-mode-alist with element %s"
                                re))))))))
-	  (set-auto-mode-0 done keep-mode-if-same)))
+	  (setq done (set-auto-mode-0 done keep-mode-if-same))))
     ;; Next compare the filename against the entries in auto-mode-alist.
     (unless done
       (setq done (set-auto-mode--apply-alist auto-mode-alist
@@ -3515,7 +3532,7 @@ set-auto-mode
                                             (error
                                              "Problem with magic-fallback-mode-alist element: %s"
                                              re))))))))
-	  (set-auto-mode-0 done keep-mode-if-same)))
+	  (setq done (set-auto-mode-0 done keep-mode-if-same))))
     (unless done
       (set-buffer-major-mode (current-buffer)))))
 
@@ -3539,17 +3556,22 @@ set-auto-mode-0
 If optional arg KEEP-MODE-IF-SAME is non-nil, MODE is chased of
 any aliases and compared to current major mode.  If they are the
 same, do nothing and return nil."
-  (unless (and keep-mode-if-same
-	       (or (eq (indirect-function mode)
+  (let ((modefun (alist-get mode major-mode-remap-alist mode)))
+    (unless (and keep-mode-if-same
+		 (or (eq (indirect-function mode)
 		       (indirect-function major-mode))
 		   (and set-auto-mode--last
 		        (eq mode (car set-auto-mode--last))
 		        (eq major-mode (cdr set-auto-mode--last)))))
-    (when mode
-      (funcall (alist-get mode major-mode-remap-alist mode))
-      (unless (eq mode major-mode)
-        (setq set-auto-mode--last (cons mode major-mode)))
-      mode)))
+      (when mode
+        (if (not (functionp modefun))
+            (progn
+              (message "Ignoring unknown mode `%s'" mode)
+              nil)
+          (funcall modefun)
+          (unless (eq mode major-mode)
+            (setq set-auto-mode--last (cons mode major-mode)))
+          mode)))))
 
 (defvar file-auto-mode-skip "^\\(#!\\|'\\\\\"\\)"
   "Regexp of lines to skip when looking for file-local settings.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: leungbk <at> posteo.net, 67795 <at> debbugs.gnu.org
Subject: Re: bug#67795: [PATCH] Handle local-variable major-mode remaps
 specifying non-existent mode
Date: Tue, 12 Dec 2023 19:17:25 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: Brian Leung <leungbk <at> posteo.net>,  67795 <at> debbugs.gnu.org
> Date: Tue, 12 Dec 2023 11:02:43 -0500
> 
> I can't wrap my head around what `hack-local-variables--find-variables`
> is supposed to do, but for the other part of the change, maybe the patch
> below is a good start?

Maybe.  Someone should try running with this for a while and come back
with feedback.  In particular, what happens if MODEFUN is from some
unbundled mode, and we expect Emacs to fall back on something
sensible, instead of signaling an error.

Thanks.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Brian Leung <leungbk <at> posteo.net>
Cc: 67795 <at> debbugs.gnu.org
Subject: Re: bug#67795: [PATCH] Handle local-variable major-mode remaps
 specifying non-existent mode
Date: Tue, 05 Mar 2024 01:28:31 -0500

OK, I had another look at the problem.

For `hack-local-variables--find-variables` I still don't really
understand why we need this (eq handle-mode t) functionality
of the function, so maybe I missed a better way to fix the problem, but
the proposed hunk looks OK, tho it need to be updated to account for
recent changes in `master`.  The new hunk would look like:

    @@ -4235,7 +4241,7 @@ hack-local-variables--find-variables
     	        (forward-line 1)))))))
         (if (eq handle-mode t)
             ;; Return the final mode: setting that's defined.
    -        (car (seq-filter #'fboundp result))
    +        (car (seq-filter (lambda (mode) (fboundp (major-mode-remap mode))) result))
           result)))
     
     (defun hack-local-variables-apply ()

Tho the code can be streamlined a bit so as not to create a list only to
select its first element:

    @@ -4201,8 +4207,9 @@ hack-local-variables--find-variables
                               (not (string-match
                                     "-minor\\'"
                                     (setq val2 (downcase (symbol-name val)))))
    -                           ;; Allow several mode: elements.
    -                           (push (intern (concat val2 "-mode")) result))
    +                          (let ((mode (intern (concat val2 "-mode"))))
    +                             (when (fboundp (major-mode-remap mode))
    +                               (setq result mode))))
                        (cond ((eq var 'coding))
                              ((eq var 'lexical-binding)
                               (unless hack-local-variables--warned-lexical
    @@ -4233,10 +4240,7 @@ hack-local-variables--find-variables
                                             val)
                                        result))))))
                    (forward-line 1)))))))
    -    (if (eq handle-mode t)
    -        ;; Return the final mode: setting that's defined.
    -        (car (seq-filter #'fboundp result))
    -      result)))
    +    result))
     
     (defun hack-local-variables-apply ()
       "Apply the elements of `file-local-variables-alist'.

Any comment/objection on this part?


        Stefan

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Brian Leung <leungbk <at> posteo.net>
Cc: 67795 <at> debbugs.gnu.org
Subject: Re: bug#67795: [PATCH] Handle local-variable major-mode remaps
 specifying non-existent mode
Date: Tue, 05 Mar 2024 02:01:25 -0500

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

I also had time to look at the first hunk, and the "good start"
I proposed wasn't right.

The patch below should be much more than just a "good start", because
I think I got to understand the code this time around :-)
The previous code worked OK but was inconsistent in its handling of
modes that we don't have (i.e. is non-existent).

When `set-auto-mode` does is go through a list of potential candidates
and uses the first one that can be used.  For each candidates, there are
several possibilities:

A. This is the major mode already activated and `keep-mode-if-same` is
   set, so we should do nothing *AND* we should stop right here.
B. The candidate is nil (absent) or is a function we don't have.
   We should skip it and try further candidates.
   This was done for some candidates but not all.
C. The candidate exists: activate it.

So I changed `set-auto-mode-0` to handle B (and return nil in that case)
so that B work consistently for all the candidates and so that
the `functionp` test is applied after remapping rather than before.

But nil was the value returned for A, so I changed that to `:keep`, so it
can be distinguished from B and C.

Then I massaged the `set-auto-mode` code so as to call `set-auto-mode-0`
according to these new rules, which arguably makes the code a bit
simpler (instead of using a `done` variable that we constantly set and
then test, it's just one big `or` where each arm returns the equivalent
of `done`).


        Stefan


* lisp/files.el (set-auto-mode-0): Return `:keep` rather than nil if
the mode is already set and we decided to keep it.
Skip the mode (and return nil) if its function (after remapping) is missing.
(set-auto-mode): Don't test `functionp` any more since
`set-auto-mode-0` does it for us now.
Restructure the code to account for the new behavior of `set-auto-mode-0`,
mostly by replacing the `done` variable with a big `or`.
(hack-local-variables--find-variables): Simplify the (eq handle-mode t)
code so we don't bother building a list, and make it test the
remapped function rather than the mode name instead.

[files.patch (text/x-diff, inline)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Brian Leung <leungbk <at> posteo.net>
Cc: 67795-done <at> debbugs.gnu.org
Subject: Re: bug#67795: [PATCH] Handle local-variable major-mode remaps
 specifying non-existent mode
Date: Thu, 14 Mar 2024 22:17:37 -0400

> The patch below should be much more than just a "good start", because
> I think I got to understand the code this time around :-)
> The previous code worked OK but was inconsistent in its handling of
> modes that we don't have (i.e. is non-existent).

Pushed to `master`.  I think we can close this.
Thanks Brian for the initial patch.  It was very helpful.


        Stefan

Previous Next

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