compile.el legacy compatibility

compile.el legacy compatibility

[Troy Hinckley]

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

I recently was adding a compilation error to
compilation-error-regexp-alist, and needed to use a custom function to
determine the line number. By inspection I was able to see that all the
primary fields (file, line, line-end, col, col-end) can take either a
subexpression number (for the regexp) or a function. However `line` is
handled differently then all the rest. If you define a function for line,
it ignores all your other fields and calls a legacy function handler
in compilation-parse-errors. It looks like this is to support "old
compile.el" from 2003. I see a lot of code marked obsolete since 24.1.

I was wondering what is your policy on removing obsolete content. I think
it would be a much better for current users if "line" was handled like all
the other fields instead of reverting the entire error to legacy mode in an
undocumented way.

[-- Attachment #2: Type: text/html, Size: 985 bytes --]

Re: compile.el legacy compatibility

[Stefan Monnier]

> I was wondering what is your policy on removing obsolete content. I think
> it would be a much better for current users if "line" was handled like all
> the other fields instead of reverting the entire error to legacy mode in an
> undocumented way.

This current case can be handled as follows (by order of my own preferences):
- Drop this old compatibility.
- Refine the compatibility check: the old code only allowed (FILE LINE COLUMN)
  and not the more general (BEG-LINE . END-LINE) case, so we could still support
  the old (FILE LINE COLUMN) case where LINE is a function while also
  supporting the case where LINE is (BEG-LINE . END-LINE) and BEG-LINE
  and END-LINE can be functions.
- Refine the compatibility by catching a `wrong-number-of-arguments`
  error when calling the LINE function with 2 args and considering it
  a tell-tale sign that it's not an old-style LINE function but
  a new-style function instead.
- Refine the compatibility by catching a `wrong-number-of-arguments`
  error when calling the LINE function with 0 args and considering it
  a sign that it's probably an old-style LINE function.


        Stefan

Re: compile.el legacy compatibility

[Troy Hinckley]

> Refine the compatibility by catching a `wrong-number-of-arguments` error when calling the LINE function with 2 args and considering it a tell-tale sign that it's not an old-style LINE function but a new-style function instead

Of the changes that keep the old style, This would be my preference. The second option presented would not work because the new style does not require (LINE . OLD-LINE) and still works with (FILE LINE COL). 

However since we changed to the new style at least 15 years ago, it seems it would be best to drop the old compatibility (which is your first preference as well). Who has the power to make that call? 

- Troy Hinckley

> On Mar 24, 2019, at 11:51 AM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
> 
> Refine the compatibility by catching a `wrong-number-of-arguments`
>  error when calling the LINE function with 2 args and considering it
>  a tell-tale sign that it's not an old-style LINE function but

Re: compile.el legacy compatibility

[Stefan Monnier]

> However since we changed to the new style at least 15 years ago, it seems it
> would be best to drop the old compatibility (which is your first preference
> as well). Who has the power to make that call? 

That would be Eli,


        Stefan

Re: compile.el legacy compatibility

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Tue, 26 Mar 2019 10:05:12 -0400
> 
> > However since we changed to the new style at least 15 years ago, it seems it
> > would be best to drop the old compatibility (which is your first preference
> > as well). Who has the power to make that call? 
> 
> That would be Eli,

I have no opinion on this, provided that the incompatible change is
clearly documented in NEWS and in relevant places in compile.el (if
there are such places).

Thanks.

Re: compile.el legacy compatibility

[Stefan Monnier]

>> > However since we changed to the new style at least 15 years ago, it
>> > seems it would be best to drop the old compatibility (which is your
>> > first preference as well). Who has the power to make that call? 
>> That would be Eli,
> I have no opinion on this, provided that the incompatible change is
> clearly documented in NEWS and in relevant places in compile.el (if
> there are such places).

Thanks, done.


        Stefan

Re: compile.el legacy compatibility

[Troy Hinckley]

[-- Attachment #1.1: Type: text/plain, Size: 653 bytes --]

I checked out the latest commit, and it looks like there is some more code
we need to remove to complete this.

On Wed, Apr 3, 2019 at 9:21 AM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> >> > However since we changed to the new style at least 15 years ago, it
> >> > seems it would be best to drop the old compatibility (which is your
> >> > first preference as well). Who has the power to make that call?
> >> That would be Eli,
> > I have no opinion on this, provided that the incompatible change is
> > clearly documented in NEWS and in relevant places in compile.el (if
> > there are such places).
>
> Thanks, done.
>
>
>         Stefan
>
>

[-- Attachment #1.2: Type: text/html, Size: 1027 bytes --]

[-- Attachment #2: 0001-Remove-obselete-compile.el-code.patch --]
[-- Type: application/octet-stream, Size: 7508 bytes --]

From fad195eba10ced934d9bae3a81780ff45349fcd1 Mon Sep 17 00:00:00 2001
From: CeleritasCelery <t.macman@gmail.com>
Date: Sat, 23 Mar 2019 06:04:57 -0700
Subject: [PATCH] Remove obselete compile.el code

---
 lisp/progmodes/compile.el  | 67 +++++++++++++++++-----------------------------
 lisp/progmodes/prolog.el   | 15 +----------
 lisp/textmodes/tex-mode.el |  4 +--
 3 files changed, 26 insertions(+), 60 deletions(-)

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index 1a0d9bdbb7..cbe36a30f7 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -820,11 +820,6 @@ Faces `compilation-error-face', `compilation-warning-face',
 (defvar compilation-leave-directory-face 'font-lock-builtin-face
   "Face name to use for leaving directory messages.")
 
-;; Used for compatibility with the old compile.el.
-(defvar compilation-parse-errors-function nil)
-(make-obsolete-variable 'compilation-parse-errors-function
-			'compilation-error-regexp-alist "24.1")
-
 (defcustom compilation-auto-jump-to-first-error nil
   "If non-nil, automatically jump to the first error during compilation."
   :type 'boolean
@@ -1081,7 +1076,7 @@ POS and RES.")
                (setq file (if (functionp file) (funcall file)
                             (match-string-no-properties file))))
 	  (let ((dir
-	    (unless (file-name-absolute-p file)
+	         (unless (file-name-absolute-p file)
                    (let ((pos (compilation--previous-directory
                                (match-beginning 0))))
                      (when pos
@@ -1310,34 +1305,29 @@ FMTS is a list of format specs for transforming the file name.
             (and proc (memq (process-status proc) '(run open))))
       (setq end (line-beginning-position))))
   (compilation--remove-properties start end)
-  (if compilation-parse-errors-function
-      ;; An old package!  Try the compatibility code.
-      (progn
-        (goto-char start)
-        (compilation--compat-parse-errors end))
-
-    ;; compilation-directory-matcher is the only part that really needs to be
-    ;; parsed sequentially.  So we could split it out, handle directories
-    ;; like syntax-propertize, and the rest as font-lock-keywords.  But since
-    ;; we want to have it work even when font-lock is off, we'd then need to
-    ;; use our own compilation-parsed text-property to keep track of the parts
-    ;; that have already been parsed.
-    (goto-char start)
-    (while (re-search-forward (car compilation-directory-matcher)
-                              end t)
-      (compilation--flush-directory-cache (match-beginning 0) (match-end 0))
-      (when compilation-debug
-        (font-lock-append-text-property
-         (match-beginning 0) (match-end 0)
-         'compilation-debug
-         (vector 'directory compilation-directory-matcher)))
-      (dolist (elt (cdr compilation-directory-matcher))
-        (add-text-properties (match-beginning (car elt))
-                             (match-end (car elt))
-                             (compilation-directory-properties
-                              (car elt) (cdr elt)))))
-
-    (compilation-parse-errors start end)))
+
+  ;; compilation-directory-matcher is the only part that really needs to be
+  ;; parsed sequentially.  So we could split it out, handle directories
+  ;; like syntax-propertize, and the rest as font-lock-keywords.  But since
+  ;; we want to have it work even when font-lock is off, we'd then need to
+  ;; use our own compilation-parsed text-property to keep track of the parts
+  ;; that have already been parsed.
+  (goto-char start)
+  (while (re-search-forward (car compilation-directory-matcher)
+                            end t)
+    (compilation--flush-directory-cache (match-beginning 0) (match-end 0))
+    (when compilation-debug
+      (font-lock-append-text-property
+       (match-beginning 0) (match-end 0)
+       'compilation-debug
+       (vector 'directory compilation-directory-matcher)))
+    (dolist (elt (cdr compilation-directory-matcher))
+      (add-text-properties (match-beginning (car elt))
+                           (match-end (car elt))
+                           (compilation-directory-properties
+                            (car elt) (cdr elt)))))
+
+  (compilation-parse-errors start end))
 
 (defun compilation--note-type (type)
   "Note that a new message with severity TYPE was seen.
@@ -2878,15 +2868,6 @@ TRUE-DIRNAME is the `file-truename' of DIRNAME, if given."
   (setq compilation-gcpro nil)
   ;; FIXME: the old code reset the directory-stack, so maybe we should
   ;; put a `directory change' marker of some sort, but where?  -stef
-  ;;
-  ;; FIXME: The old code moved compilation-current-error (which was
-  ;; virtually represented by a mix of compilation-parsing-end and
-  ;; compilation-error-list) to point-min, but that was only meaningful for
-  ;; the internal uses of compilation-forget-errors: all calls from external
-  ;; packages seem to be followed by a move of compilation-parsing-end to
-  ;; something equivalent to point-max.  So we heuristically move
-  ;; compilation-current-error to point-max (since the external package
-  ;; won't know that it should do it).  --Stef
   (setq compilation-current-error nil)
   (let* ((proc (get-buffer-process (current-buffer)))
 	 (mark (if proc (process-mark proc)))
diff --git a/lisp/progmodes/prolog.el b/lisp/progmodes/prolog.el
index 296a7ac3c9..cd5a1a4ecc 100644
--- a/lisp/progmodes/prolog.el
+++ b/lisp/progmodes/prolog.el
@@ -1711,13 +1711,10 @@ This function must be called from the source code buffer."
       (remove-function (process-filter process)
                        #'prolog-consult-compile-filter))))
 
-(defvar compilation-error-list)
-
 (defun prolog-parse-sicstus-compilation-errors (limit)
   "Parse the prolog compilation buffer for errors.
 Argument LIMIT is a buffer position limiting searching.
 For use with the `compilation-parse-errors-function' variable."
-  (setq compilation-error-list nil)
   (message "Parsing SICStus error messages...")
   (let (filepath dir file errorline)
     (while
@@ -1736,17 +1733,7 @@ For use with the `compilation-parse-errors-function' variable."
       (if (string-match "\\(.*/\\)\\([^/]*\\)$" filepath)
           (progn
             (setq dir (match-string 1 filepath))
-            (setq file (match-string 2 filepath))))
-
-      (setq compilation-error-list
-            (cons
-             (cons (save-excursion
-                     (beginning-of-line)
-                     (point-marker))
-                   (list (list file dir) errorline))
-             compilation-error-list)
-            ))
-    ))
+            (setq file (match-string 2 filepath)))))))
 
 (defun prolog-consult-compile-filter (process output)
   "Filter function for Prolog compilation PROCESS.
diff --git a/lisp/textmodes/tex-mode.el b/lisp/textmodes/tex-mode.el
index 9c91d27b94..2fdbba84cf 100644
--- a/lisp/textmodes/tex-mode.el
+++ b/lisp/textmodes/tex-mode.el
@@ -2489,9 +2489,7 @@ Only applies the FSPEC to the args part of FORMAT."
       (tex-send-command tex-shell-cd-command dir)))
   (with-current-buffer (process-buffer (tex-send-command cmd))
     (setq compilation-last-buffer (current-buffer))
-    (compilation-forget-errors)
-    ;; Don't parse previous compilations.
-    (set-marker compilation-parsing-end (1- (point-max))))
+    (compilation-forget-errors))
   (tex-display-shell)
   (setq tex-last-buffer-texed (current-buffer)))
 \f
-- 
2.12.0

Re: compile.el legacy compatibility

[Stefan Monnier]

> I checked out the latest commit, and it looks like there is some more code
> we need to remove to complete this.

AFAICT what you're suggesting removes another backward compatibility
feature, made obsolete around the same time but otherwise independent.
Furthermore, this one is better documented in the code (e.g. with
a make-obsolete-variable), so I think we can remove it when we decide to
drop the other "obsolete since 24.1" features.


        Stefan


> On Wed, Apr 3, 2019 at 9:21 AM Stefan Monnier <monnier@iro.umontreal.ca>
> wrote:
>
>> >> > However since we changed to the new style at least 15 years ago, it
>> >> > seems it would be best to drop the old compatibility (which is your
>> >> > first preference as well). Who has the power to make that call?
>> >> That would be Eli,
>> > I have no opinion on this, provided that the incompatible change is
>> > clearly documented in NEWS and in relevant places in compile.el (if
>> > there are such places).
>>
>> Thanks, done.
>>
>>
>>         Stefan
>>
>>
> From fad195eba10ced934d9bae3a81780ff45349fcd1 Mon Sep 17 00:00:00 2001
> From: CeleritasCelery <t.macman@gmail.com>
> Date: Sat, 23 Mar 2019 06:04:57 -0700
> Subject: [PATCH] Remove obselete compile.el code
>
> ---
>  lisp/progmodes/compile.el  | 67 +++++++++++++++++-----------------------------
>  lisp/progmodes/prolog.el   | 15 +----------
>  lisp/textmodes/tex-mode.el |  4 +--
>  3 files changed, 26 insertions(+), 60 deletions(-)
>
> diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
> index 1a0d9bdbb7..cbe36a30f7 100644
> --- a/lisp/progmodes/compile.el
> +++ b/lisp/progmodes/compile.el
> @@ -820,11 +820,6 @@ Faces `compilation-error-face', `compilation-warning-face',
>  (defvar compilation-leave-directory-face 'font-lock-builtin-face
>    "Face name to use for leaving directory messages.")
>  
> -;; Used for compatibility with the old compile.el.
> -(defvar compilation-parse-errors-function nil)
> -(make-obsolete-variable 'compilation-parse-errors-function
> -			'compilation-error-regexp-alist "24.1")
> -
>  (defcustom compilation-auto-jump-to-first-error nil
>    "If non-nil, automatically jump to the first error during compilation."
>    :type 'boolean
> @@ -1081,7 +1076,7 @@ POS and RES.")
>                 (setq file (if (functionp file) (funcall file)
>                              (match-string-no-properties file))))
>  	  (let ((dir
> -	    (unless (file-name-absolute-p file)
> +	         (unless (file-name-absolute-p file)
>                     (let ((pos (compilation--previous-directory
>                                 (match-beginning 0))))
>                       (when pos
> @@ -1310,34 +1305,29 @@ FMTS is a list of format specs for transforming the file name.
>              (and proc (memq (process-status proc) '(run open))))
>        (setq end (line-beginning-position))))
>    (compilation--remove-properties start end)
> -  (if compilation-parse-errors-function
> -      ;; An old package!  Try the compatibility code.
> -      (progn
> -        (goto-char start)
> -        (compilation--compat-parse-errors end))
> -
> -    ;; compilation-directory-matcher is the only part that really needs to be
> -    ;; parsed sequentially.  So we could split it out, handle directories
> -    ;; like syntax-propertize, and the rest as font-lock-keywords.  But since
> -    ;; we want to have it work even when font-lock is off, we'd then need to
> -    ;; use our own compilation-parsed text-property to keep track of the parts
> -    ;; that have already been parsed.
> -    (goto-char start)
> -    (while (re-search-forward (car compilation-directory-matcher)
> -                              end t)
> -      (compilation--flush-directory-cache (match-beginning 0) (match-end 0))
> -      (when compilation-debug
> -        (font-lock-append-text-property
> -         (match-beginning 0) (match-end 0)
> -         'compilation-debug
> -         (vector 'directory compilation-directory-matcher)))
> -      (dolist (elt (cdr compilation-directory-matcher))
> -        (add-text-properties (match-beginning (car elt))
> -                             (match-end (car elt))
> -                             (compilation-directory-properties
> -                              (car elt) (cdr elt)))))
> -
> -    (compilation-parse-errors start end)))
> +
> +  ;; compilation-directory-matcher is the only part that really needs to be
> +  ;; parsed sequentially.  So we could split it out, handle directories
> +  ;; like syntax-propertize, and the rest as font-lock-keywords.  But since
> +  ;; we want to have it work even when font-lock is off, we'd then need to
> +  ;; use our own compilation-parsed text-property to keep track of the parts
> +  ;; that have already been parsed.
> +  (goto-char start)
> +  (while (re-search-forward (car compilation-directory-matcher)
> +                            end t)
> +    (compilation--flush-directory-cache (match-beginning 0) (match-end 0))
> +    (when compilation-debug
> +      (font-lock-append-text-property
> +       (match-beginning 0) (match-end 0)
> +       'compilation-debug
> +       (vector 'directory compilation-directory-matcher)))
> +    (dolist (elt (cdr compilation-directory-matcher))
> +      (add-text-properties (match-beginning (car elt))
> +                           (match-end (car elt))
> +                           (compilation-directory-properties
> +                            (car elt) (cdr elt)))))
> +
> +  (compilation-parse-errors start end))
>  
>  (defun compilation--note-type (type)
>    "Note that a new message with severity TYPE was seen.
> @@ -2878,15 +2868,6 @@ TRUE-DIRNAME is the `file-truename' of DIRNAME, if given."
>    (setq compilation-gcpro nil)
>    ;; FIXME: the old code reset the directory-stack, so maybe we should
>    ;; put a `directory change' marker of some sort, but where?  -stef
> -  ;;
> -  ;; FIXME: The old code moved compilation-current-error (which was
> -  ;; virtually represented by a mix of compilation-parsing-end and
> -  ;; compilation-error-list) to point-min, but that was only meaningful for
> -  ;; the internal uses of compilation-forget-errors: all calls from external
> -  ;; packages seem to be followed by a move of compilation-parsing-end to
> -  ;; something equivalent to point-max.  So we heuristically move
> -  ;; compilation-current-error to point-max (since the external package
> -  ;; won't know that it should do it).  --Stef
>    (setq compilation-current-error nil)
>    (let* ((proc (get-buffer-process (current-buffer)))
>  	 (mark (if proc (process-mark proc)))
> diff --git a/lisp/progmodes/prolog.el b/lisp/progmodes/prolog.el
> index 296a7ac3c9..cd5a1a4ecc 100644
> --- a/lisp/progmodes/prolog.el
> +++ b/lisp/progmodes/prolog.el
> @@ -1711,13 +1711,10 @@ This function must be called from the source code buffer."
>        (remove-function (process-filter process)
>                         #'prolog-consult-compile-filter))))
>  
> -(defvar compilation-error-list)
> -
>  (defun prolog-parse-sicstus-compilation-errors (limit)
>    "Parse the prolog compilation buffer for errors.
>  Argument LIMIT is a buffer position limiting searching.
>  For use with the `compilation-parse-errors-function' variable."
> -  (setq compilation-error-list nil)
>    (message "Parsing SICStus error messages...")
>    (let (filepath dir file errorline)
>      (while
> @@ -1736,17 +1733,7 @@ For use with the `compilation-parse-errors-function' variable."
>        (if (string-match "\\(.*/\\)\\([^/]*\\)$" filepath)
>            (progn
>              (setq dir (match-string 1 filepath))
> -            (setq file (match-string 2 filepath))))
> -
> -      (setq compilation-error-list
> -            (cons
> -             (cons (save-excursion
> -                     (beginning-of-line)
> -                     (point-marker))
> -                   (list (list file dir) errorline))
> -             compilation-error-list)
> -            ))
> -    ))
> +            (setq file (match-string 2 filepath)))))))
>  
>  (defun prolog-consult-compile-filter (process output)
>    "Filter function for Prolog compilation PROCESS.
> diff --git a/lisp/textmodes/tex-mode.el b/lisp/textmodes/tex-mode.el
> index 9c91d27b94..2fdbba84cf 100644
> --- a/lisp/textmodes/tex-mode.el
> +++ b/lisp/textmodes/tex-mode.el
> @@ -2489,9 +2489,7 @@ Only applies the FSPEC to the args part of FORMAT."
>        (tex-send-command tex-shell-cd-command dir)))
>    (with-current-buffer (process-buffer (tex-send-command cmd))
>      (setq compilation-last-buffer (current-buffer))
> -    (compilation-forget-errors)
> -    ;; Don't parse previous compilations.
> -    (set-marker compilation-parsing-end (1- (point-max))))
> +    (compilation-forget-errors))
>    (tex-display-shell)
>    (setq tex-last-buffer-texed (current-buffer)))
>  \f