Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ERL-1474: emacs mode still requires cl which was deprecated in emacs 27.1 #4451

Closed
OTP-Maintainer opened this issue Jan 31, 2021 · 12 comments
Closed
Labels
bug Issue is reported as a bug priority:medium team:PS Assigned to OTP team PS waiting waiting for changes/input from author

Comments

@OTP-Maintainer
Copy link

Original reporter: JIRAUSER13400
Affected version: Not Specified
Component: Not Specified
Migrated from: https://bugs.erlang.org/browse/ERL-1474


erlang.el and erlang-test.el still contain {code}
(require 'cl)
{code}
which emits the {{Package cl is deprecated}} warning.
@OTP-Maintainer OTP-Maintainer added bug Issue is reported as a bug team:PS Assigned to OTP team PS priority:medium labels Feb 10, 2021
@ranjanified
Copy link
Contributor

ranjanified commented Feb 11, 2021

On startup, I also see this warning in my emacs(GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.5, cairo version 1.16.0) of 2020-09-26).

As per 1015, and as per @kjellwinblad 's comment.

However, a quick rgrep of emacs folder yields on branch master, erlang.el and erlang-test.el entries as such
(eval-when-compile (require 'cl))

Any tips please? Can these entries be removed, as they already have been in few other files, as per the comment.

@ranjanified
Copy link
Contributor

auto-highlight-symbol-mode has a dependency on (require 'cl). At least this is the one I am loading from source.

In my emacs config I am doing (require 'edts-start)

  1. which loads edts-mode.
  2. edts-mode has a dependency on auto-highlight-symbol-mode
  3. auto-highlight-symbol-mode has dependency on 'cl

Hence the warning, even though the 2 entries mentioned in previous comment are taken away.

@ranjanified
Copy link
Contributor

I removed (require 'edts-start) and added (require 'erlang) in my emacs config, and the warning is back. That means, with the current erlang.el and erlang-test.el, this warning still shows up.

@ranjanified
Copy link
Contributor

Changing the entries from (require 'cl) to (require 'cl-lib) takes away the warning on launch of Emacs, but leaves with 2 entries in Messages buffer:-

  1. Error during redisplay: (jit-lock-function 1) signaled (void-function block).
    The fix it seems is to replace usage of block with cl-block. At least that is what is taking away the warning.

  2. Error during redisplay: (jit-lock-function 1) signaled (void-function return)
    Similarly, fix is to replace return with cl-return.

@leoliu
Copy link
Contributor

leoliu commented Feb 12, 2021

The following fixes deprecation warnings.

diff --git a/lib/tools/emacs/erlang-test.el b/lib/tools/emacs/erlang-test.el
index c1c0cd72..3dab5e55 100644
--- a/lib/tools/emacs/erlang-test.el
+++ b/lib/tools/emacs/erlang-test.el
@@ -62,8 +62,6 @@
 
 ;;; Code:
 
-(eval-when-compile
-  (require 'cl))
 (require 'ert)
 (require 'erlang)
 
@@ -127,8 +125,8 @@ (ert-deftest erlang-test-tags ()
 
 (defun erlang-test-create-erlang-file (erlang-file)
   (with-temp-file erlang-file
-    (loop for (_ . code) in erlang-test-code
-          do (insert code "\n"))))
+    (cl-loop for (_ . code) in erlang-test-code
+             do (insert code "\n"))))
 
 (defun erlang-test-compile-tags (erlang-file tags-file)
   (should (zerop (call-process "etags" nil nil nil
@@ -143,20 +141,20 @@ (defun erlang-test-completion-table ()
                  (sort (erlang-expected-completion-table) #'string-lessp))))
 
 (defun erlang-expected-completion-table ()
-  (append (loop for (symbol . _) in erlang-test-code
-                when (stringp symbol)
-                append (list symbol (concat "erlang_test:" symbol)))
+  (append (cl-loop for (symbol . _) in erlang-test-code
+                   when (stringp symbol)
+                   append (list symbol (concat "erlang_test:" symbol)))
           (list "erlang_test:" "erlang_test:module_info")))
 
 (defun erlang-test-xref-find-definitions (erlang-file erlang-buffer)
-  (loop for (tagname . code) in erlang-test-code
-        for line = 1 then (1+ line)
-        do (when tagname
-             (switch-to-buffer erlang-buffer)
-             (erlang-test-xref-jump tagname erlang-file line)
-             (when (string-equal tagname "function")
-               (erlang-test-xref-jump (concat "erlang_test:" tagname)
-                                      erlang-file line))))
+  (cl-loop for (tagname . code) in erlang-test-code
+           for line = 1 then (1+ line)
+           do (when tagname
+                (switch-to-buffer erlang-buffer)
+                (erlang-test-xref-jump tagname erlang-file line)
+                (when (string-equal tagname "function")
+                  (erlang-test-xref-jump (concat "erlang_test:" tagname)
+                                         erlang-file line))))
   (erlang-test-xref-jump "erlang_test:" erlang-file 1))
 
 (defun erlang-test-xref-jump (id expected-file expected-line)
@@ -225,27 +223,27 @@ (defun erlang-test-format-opt (elisp &optional expected-erlang)
 
 
 (ert-deftest erlang-test-parse-id ()
-  (loop for id-string in '("fun/10"
-                           "qualified-function module:fun/10"
-                           "record reko"
-                           "macro _SYMBOL"
-                           "macro MACRO/10"
-                           "module modula"
-                           "macro"
-                           nil)
-        for id-list in '((nil nil "fun" 10)
-                         (qualified-function "module" "fun" 10)
-                         (record nil "reko" nil)
-                         (macro nil "_SYMBOL" nil)
-                         (macro nil "MACRO" 10)
-                         (module nil "modula" nil)
-                         (nil nil "macro" nil)
-                         nil)
-        for id-list2 = (erlang-id-to-list id-string)
-        do (should (equal id-list id-list2))
-        for id-string2 = (erlang-id-to-string id-list)
-        do (should (equal id-string id-string2))
-        collect id-list2))
+  (cl-loop for id-string in '("fun/10"
+                              "qualified-function module:fun/10"
+                              "record reko"
+                              "macro _SYMBOL"
+                              "macro MACRO/10"
+                              "module modula"
+                              "macro"
+                              nil)
+           for id-list in '((nil nil "fun" 10)
+                            (qualified-function "module" "fun" 10)
+                            (record nil "reko" nil)
+                            (macro nil "_SYMBOL" nil)
+                            (macro nil "MACRO" 10)
+                            (module nil "modula" nil)
+                            (nil nil "macro" nil)
+                            nil)
+           for id-list2 = (erlang-id-to-list id-string)
+           do (should (equal id-list id-list2))
+           for id-string2 = (erlang-id-to-string id-list)
+           do (should (equal id-string id-string2))
+           collect id-list2))
 
 
 (provide 'erlang-test)
diff --git a/lib/tools/emacs/erlang.el b/lib/tools/emacs/erlang.el
index 2ac7ba63..ad2db1ed 100644
--- a/lib/tools/emacs/erlang.el
+++ b/lib/tools/emacs/erlang.el
@@ -76,7 +76,6 @@
 ;;     M-x toggle-debug-on-error RET
 ;;; Code:
 
-(eval-when-compile (require 'cl))
 (require 'align)
 (require 'comint)
 (require 'tempo)
@@ -1420,7 +1419,7 @@ (define-derived-mode erlang-mode prog-mode "Erlang"
   (erlang-electric-init)
   (erlang-menu-init)
   (erlang-mode-variables)
-  (erlang-check-module-name-init)
+  (add-hook 'before-save-hook 'erlang-check-module-name nil t)
   (erlang-man-init)
   (erlang-tags-init)
   (erlang-font-lock-init)
@@ -1431,7 +1430,6 @@ (define-derived-mode erlang-mode prog-mode "Erlang"
         (setq-local eldoc-documentation-function #'ignore))
     (add-function :before-until (local 'eldoc-documentation-function)
                   #'erldoc-eldoc-function))
-  (run-hooks 'erlang-mode-hook)
 
   ;; Align maps.
   (add-to-list 'align-rules-list
@@ -4084,11 +4082,11 @@ (defun erlang-remove-quotes (str)
 (defun erlang-match-next-exported-function (max)
   "Returns non-nil if there is an exported function in the current
 buffer between point and MAX."
-  (block nil
-         (while (and (not erlang-inhibit-exported-function-name-face)
-                     (erlang-match-next-function max))
-           (when (erlang-last-match-exported-p)
-             (return (match-data))))))
+  (catch 'return
+    (while (and (not erlang-inhibit-exported-function-name-face)
+                (erlang-match-next-function max))
+      (when (erlang-last-match-exported-p)
+        (throw 'return (match-data))))))
 
 (defun erlang-match-next-function (max)
   "Searches forward in current buffer for the next erlang function,
@@ -4116,28 +4114,6 @@ (defun erlang-function-exported-p (name arity)
 
 ;;; Check module name
 
-;; The function `write-file', bound to C-x C-w, calls
-;; `set-visited-file-name' which clears the hook.  :-(
-;; To make sure that the hook always is present, we advise
-;; `set-visited-file-name'.
-(defun erlang-check-module-name-init ()
-  "Initialize the functionality to compare file and module names.
-
-Unless we have `before-save-hook', we advice the function
-`set-visited-file-name' since it clears the variable
-`local-write-file-hooks'."
-  (if (boundp 'before-save-hook)
-      (add-hook 'before-save-hook 'erlang-check-module-name nil t)
-    (require 'advice)
-    (when (fboundp 'ad-advised-definition-p)
-      (unless (ad-advised-definition-p 'set-visited-file-name)
-        (defadvice set-visited-file-name (after erlang-set-visited-file-name
-                                                activate)
-          (if (eq major-mode 'erlang-mode)
-              (add-hook 'local-write-file-hooks 'erlang-check-module-name))))
-      (add-hook 'local-write-file-hooks 'erlang-check-module-name))))
-
-
 (defun erlang-check-module-name ()
   "If the module name doesn't match file name, ask for permission to change.
 
@@ -4146,7 +4122,7 @@ (defun erlang-check-module-name ()
 source is silently changed.  If it is set to the atom `ask', the user
 is prompted.
 
-This function is normally placed in the hook `local-write-file-hooks'."
+This function is normally placed in the hook `before-save-hook'."
   (if erlang-check-module-name
       (let ((mn (erlang-add-quotes-if-needed
                  (erlang-get-module)))
@@ -5358,7 +5334,7 @@ (defun erlang-refine-xrefs (xrefs kind tag is-regexp)
         (cl-loop for xref in xrefs
                  for loc = (xref-item-location xref)
                  for file = (xref-location-group loc)
-                 do (pushnew file files :test 'string-equal))
+                 do (cl-pushnew file files :test 'string-equal))
         (or (cl-loop for file in files
                      append (erlang-xrefs-in-file file kind tag is-regexp))
             ;; Failed for some reason.  Pretend like it is raining and

@ranjanified
Copy link
Contributor

Thanks @leoliu . There are many things in the diff, apart from the 2 points that I have noticed so far. I will follow through it.

  1. Once I am done following up, is it Ok if I can raise a PR with these changes, or if you are doing it already then I will not?
  2. Any reason you have preferred (throw ...) and (catch ...) over (cl-block ...) and (cl-return ...)? You seem to be an avid Emacs Lisper, may be you can help me with some tips?

@leoliu
Copy link
Contributor

leoliu commented Feb 12, 2021

For Q2, throw and catch are stock emacs primitives. cl-block and cl-return are built on them I think.

I also do some cleanup by removing the hack for lack of before-save-hook in very ancient emacs and fix a small bug where erlang-mode-hook is run twice.

I will see if I can send a PR in a day or two. Thanks.

@IngelaAndin IngelaAndin added the waiting waiting for changes/input from author label Feb 15, 2021
@leoliu
Copy link
Contributor

leoliu commented Feb 21, 2021

I was going to create a PR just now but discovered that the tools/emacs has diverged between maint and master branches. Which branch should I use to create a PR?

@IngelaAndin
Copy link
Contributor

It depends. Generally, bug fixes are done for the maint branch and feature additions are done for the master branch. If you can not make a solution to maint that can be merged to master because of diverged code and master needs a different solution then it becomes a little more complicated and I think only the OTP team has handled such changes so far. So if that is the case I think it would be easiest to base it on the master branch.

@dgud
Copy link
Contributor

dgud commented Feb 22, 2021

In this case I don't think the merge will cause any problems so you can do as you want if it still works on old emacs
versions, we still have emacs-24 on our test runs.

leoliu added a commit to leoliu/otp that referenced this issue Feb 22, 2021
Do not depend on cl. Assume presence of before-save-hook (Emacs 22)
and remove erlang-check-module-name-init. Remove (run-hooks
'erlang-mode-hook) from erlang-mode definition because
define-derived-mode does it already. Replace erlang-caddr with caddr
from Emacs 26.

erlang.el now compiles cleanly in Emacs 24.5, 25, 26, 27.
@leoliu
Copy link
Contributor

leoliu commented Feb 22, 2021

Sorry my bad. I just discovered I have a stale OTP fork from 2017 that is causing the divergence.

dgud added a commit that referenced this issue Feb 26, 2021
tools: Fix emacs compiler warnings (#4451)
OTP-17225
@dgud
Copy link
Contributor

dgud commented Feb 26, 2021

Fix merged to maint and master.

@dgud dgud closed this as completed Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug priority:medium team:PS Assigned to OTP team PS waiting waiting for changes/input from author
Projects
None yet
Development

No branches or pull requests

5 participants