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

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

[Brian Leung]

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

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'

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Handle-local-variable-major-mode-remaps-specifying-n.patch --]
[-- Type: text/patch, Size: 1765 bytes --]

From 46435cac589506ecb131f480171a2ec1f0f03c55 Mon Sep 17 00:00:00 2001
From: Brian Leung <leungbk@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.

* lisp/files.el (set-auto-mode): Check for any remapping specified in
major-mode-remap-alist.
(hack-local-variables--find-variables): Same.
---
 lisp/files.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index f87e7807301..8e92da6d49d 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3445,7 +3445,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))
+	 (if (not (functionp (alist-get mode major-mode-remap-alist mode)))
 	     (message "Ignoring unknown mode `%s'" mode)
 	   (setq done t)
 	   (set-auto-mode-0 mode keep-mode-if-same)))
@@ -4182,7 +4182,9 @@ 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))
+        (seq-find (lambda (mode)
+                    (fboundp (alist-get mode major-mode-remap-alist mode)))
+                  result)
       result)))
 
 (defun hack-local-variables-apply ()
-- 
2.42.0

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

[Eli Zaretskii]

> From: Brian Leung <leungbk@posteo.net>
> Date: Tue, 12 Dec 2023 13:52:53 +0000
> 
> >From 46435cac589506ecb131f480171a2ec1f0f03c55 Mon Sep 17 00:00:00 2001
> From: Brian Leung <leungbk@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?

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> 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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mode-remap.patch --]
[-- Type: text/x-diff, Size: 3425 bytes --]

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.

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Brian Leung <leungbk@posteo.net>,  67795@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.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

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.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: files.patch --]
[-- Type: text/x-diff, Size: 0 bytes --]

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 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