Allow unloading Eshell

* lisp/eshell/em-extpipe.el (eshell-extpipe):
* lisp/eshell/esh-opt.el (eshell-opt): New groups.  Eshell uses these
to identify modules to unload.

* lisp/eshell/em-hist.el (eshell-hist-unload-hook):
* lisp/eshell/em-ls.el (eshell-ls-unload-hook):
* lisp/eshell/em-smart.el (eshell-smart-unload-hook):
* lisp/eshell/eshell.el (eshell-unload-hook): Make obsolete and move
to...

* lisp/eshell/em-smart.el (em-smart-unload-function):
* lisp/eshell/em-hist.el (em-hist-unload-function):
* lisp/eshell/em-ls.el (em-ls-unload-function):
* lisp/eshell/eshell.el (eshell-unload-function): ... these.

* lisp/eshell/esh-mode.el (eshell-mode-unload-hook):
* lisp/eshell/esh-module.el (eshell-module-unload-hook): Make
obsolete.

* lisp/eshell/em-ls (eshell-ls-enable-in-dired,
eshell-ls-disable-in-dired): New functions...
(eshell-ls-use-in-dired): ... use them.

* lisp/eshell/esh-module.el (eshell-module--feature-name,
eshell-unload-modules): New functions.
(eshell-unload-extension-modules): Use 'eshell-unload-modules'.

* lisp/eshell/eshell.el (eshell-unload-all-modules): Remove.

* test/lisp/eshell/eshell-tests-unload.el: New file.

* doc/misc/eshell.texi (Bugs and ideas): Remove item about unloading
Eshell not working.

* etc/NEWS: Announce this change (bug#61501).

doc/misc/eshell.texi

@@ -2189,8 +2189,6 @@ Hitting space during a process invocation, such as @command{make}, will
 cause it to track the bottom of the output; but backspace no longer
 scrolls back.
 
-@item It's not possible to fully @code{unload-feature} Eshell
-
 @item Menu support was removed, but never put back
 
 @item If an interactive process is currently running, @kbd{M-!} doesn't work

etc/NEWS

@@ -145,6 +145,11 @@ this to your configuration:
 
     (keymap-set eshell-mode-map "<home>" #'eshell-bol-ignoring-prompt)
 
+---
+*** You can now properly unload Eshell.
+Calling "(unload-feature 'eshell)" no longer signals an error, and now
+correctly unloads Eshell and all of its modules.
+
 +++
 *** 'eshell-read-aliases-list' is now an interactive command.
 After manually editing 'eshell-aliases-file', you can use this command
@@ -210,6 +215,12 @@ their customization options.
 This user option has been obsoleted in Emacs 27, use
 'remote-file-name-inhibit-cache' instead.
 
+---
+** User options 'eshell-NAME-unload-hook' are now obsolete.
+These hooks were named incorrectly, and so they never actually ran
+when unloading the correspending feature.  Instead, you should use
+hooks named after the feature name, like 'esh-mode-unload-hook'.
+
 
 * Lisp Changes in Emacs 30.1
 

lisp/eshell/em-extpipe.el

@@ -36,6 +36,21 @@
 
 (eval-when-compile (require 'files-x))
 
+;;;###autoload
+(progn
+(defgroup eshell-extpipe nil
+  "Native shell pipelines.
+
+This module lets you construct pipelines that use your operating
+system's shell instead of Eshell's own pipelining support.  This
+is especially relevant when executing commands on a remote
+machine using Eshell's Tramp integration: using the remote
+shell's pipelining avoids copying the data which will flow
+through the pipeline to local Emacs buffers and then right back
+again."
+  :tag "External pipelines"
+  :group 'eshell-module))
+
 ;;; Functions:
 
 (defun eshell-extpipe-initialize () ;Called from `eshell-mode' via intern-soft!

lisp/eshell/em-hist.el

@@ -80,6 +80,7 @@
      (remove-hook 'kill-emacs-hook 'eshell-save-some-history)))
   "A hook that gets run when `eshell-hist' is unloaded."
   :type 'hook)
+(make-obsolete-variable 'eshell-hist-unload-hook nil "30.1")
 
 (defcustom eshell-history-file-name
   (expand-file-name "history" eshell-directory-name)
@@ -1037,6 +1038,9 @@ If N is negative, search backwards for the -Nth previous match."
   (isearch-done)
   (eshell-send-input))
 
+(defun em-hist-unload-function ()
+  (remove-hook 'kill-emacs-hook 'eshell-save-some-history))
+
 (provide 'em-hist)
 
 ;; Local Variables:

lisp/eshell/em-ls.el

@@ -62,24 +62,27 @@ This is useful for enabling human-readable format (-h), for example."
 This is useful for enabling human-readable format (-h), for example."
   :type '(repeat :tag "Arguments" string))
 
+(defun eshell-ls-enable-in-dired ()
+  "Use `eshell-ls' to read directories in Dired."
+  (require 'dired)
+  (advice-add 'insert-directory :around #'eshell-ls--insert-directory)
+  (advice-add 'dired :around #'eshell-ls--dired))
+
+(defun eshell-ls-disable-in-dired ()
+  "Stop using `eshell-ls' to read directories in Dired."
+  (advice-remove 'insert-directory #'eshell-ls--insert-directory)
+  (advice-remove 'dired #'eshell-ls--dired))
+
 (defcustom eshell-ls-use-in-dired nil
   "If non-nil, use `eshell-ls' to read directories in Dired.
 Changing this without using customize has no effect."
   :set (lambda (symbol value)
-	 (cond (value
-                (require 'dired)
-                (advice-add 'insert-directory :around
-                            #'eshell-ls--insert-directory)
-                (advice-add 'dired :around #'eshell-ls--dired))
-               (t
-                (advice-remove 'insert-directory
-                               #'eshell-ls--insert-directory)
-                (advice-remove 'dired #'eshell-ls--dired)))
+         (if value
+             (eshell-ls-enable-in-dired)
+           (eshell-ls-disable-in-dired))
          (set symbol value))
   :type 'boolean
   :require 'em-ls)
-(add-hook 'eshell-ls-unload-hook #'eshell-ls-unload-function)
-
 
 (defcustom eshell-ls-default-blocksize 1024
   "The default blocksize to use when display file sizes with -s."
@@ -954,10 +957,8 @@ to use, and each member of which is the width of that column
 				 (car file)))))
   (car file))
 
-(defun eshell-ls-unload-function ()
-  (advice-remove 'insert-directory #'eshell-ls--insert-directory)
-  (advice-remove 'dired #'eshell-ls--dired)
-  nil)
+(defun em-ls-unload-function ()
+  (eshell-ls-disable-in-dired))
 
 (provide 'em-ls)
 

lisp/eshell/em-smart.el

@@ -99,6 +99,7 @@ it to get a real sense of how it works."
   "A hook that gets run when `eshell-smart' is unloaded."
   :type 'hook
   :group 'eshell-smart)
+(make-obsolete-variable 'eshell-smart-unload-hook nil "30.1")
 
 (defcustom eshell-review-quick-commands nil
   "If t, always review commands.
@@ -321,6 +322,9 @@ and the end of the buffer are still visible."
     (if clear
 	(remove-hook 'pre-command-hook 'eshell-smart-display-move t))))
 
+(defun em-smart-unload-hook ()
+  (remove-hook 'window-configuration-change-hook #'eshell-refresh-windows))
+
 (provide 'em-smart)
 
 ;; Local Variables:

lisp/eshell/esh-mode.el

@@ -79,6 +79,7 @@
 (defcustom eshell-mode-unload-hook nil
   "A hook that gets run when `eshell-mode' is unloaded."
   :type 'hook)
+(make-obsolete-variable 'eshell-mode-unload-hook nil "30.1")
 
 (defcustom eshell-mode-hook nil
   "A hook that gets run when `eshell-mode' is entered."

lisp/eshell/esh-module.el

@@ -47,6 +47,7 @@ customizing the variable `eshell-modules-list'."
   "A hook run when `eshell-module' is unloaded."
   :type 'hook
   :group 'eshell-module)
+(make-obsolete-variable 'eshell-module-unload-hook nil "30.1")
 
 (defcustom eshell-modules-list
   '(eshell-alias
@@ -85,20 +86,37 @@ Changes will only take effect in future Eshell buffers."
 
 ;;; Code:
 
+(defsubst eshell-module--feature-name (module &optional kind)
+  "Get the feature name for the specified Eshell MODULE."
+  (let ((module-name (symbol-name module))
+        (prefix (cond ((eq kind 'core) "esh-")
+                      ((memq kind '(extension nil)) "em-")
+                      (t (error "unknown module kind %s" kind)))))
+    (if (string-match "^eshell-\\(.*\\)" module-name)
+	(concat prefix (match-string 1 module-name))
+      (error "Invalid Eshell module name: %s" module))))
+
 (defsubst eshell-using-module (module)
   "Return non-nil if a certain Eshell MODULE is in use.
 The MODULE should be a symbol corresponding to that module's
 customization group.  Example: `eshell-cmpl' for that module."
   (memq module eshell-modules-list))
 
+(defun eshell-unload-modules (modules &optional kind)
+  "Try to unload the specified Eshell MODULES."
+  (dolist (module modules)
+    (let ((module-feature (intern (eshell-module--feature-name module kind))))
+      (when (featurep module-feature)
+	(message "Unloading %s..." (symbol-name module))
+        (condition-case-unless-debug _
+            (progn
+              (unload-feature module-feature)
+              (message "Unloading %s...done" (symbol-name module)))
+          (error (message "Unloading %s...failed" (symbol-name module))))))))
+
 (defun eshell-unload-extension-modules ()
-  "Unload any memory resident extension modules."
-  (dolist (module (eshell-subgroups 'eshell-module))
-    (if (featurep module)
-	(ignore-errors
-	  (message "Unloading %s..." (symbol-name module))
-	  (unload-feature module)
-	  (message "Unloading %s...done" (symbol-name module))))))
+  "Try to unload all currently-loaded Eshell extension modules."
+  (eshell-unload-modules (eshell-subgroups 'eshell-module)))
 
 (provide 'esh-module)
 ;;; esh-module.el ends here

lisp/eshell/esh-opt.el

@@ -29,6 +29,11 @@
 ;; defined in esh-util.
 (require 'esh-util)
 
+(defgroup eshell-opt nil
+  "Functions for argument parsing in Eshell commands."
+  :tag "Option parsing"
+  :group 'eshell)
+
 (defmacro eshell-eval-using-options (name macro-args options &rest body-forms)
   "Process NAME's MACRO-ARGS using a set of command line OPTIONS.
 After doing so, stores settings in local symbols as declared by OPTIONS;

lisp/eshell/eshell.el

@@ -199,10 +199,11 @@ shells such as bash, zsh, rc, 4dos."
   :type 'hook
   :group 'eshell)
 
-(defcustom eshell-unload-hook '(eshell-unload-all-modules)
+(defcustom eshell-unload-hook nil
   "A hook run when Eshell is unloaded from memory."
   :type 'hook
   :group 'eshell)
+(make-obsolete-variable 'eshell-unload-hook nil "30.1")
 
 (defcustom eshell-buffer-name "*eshell*"
   "The basename used for Eshell buffers.
@@ -370,28 +371,14 @@ corresponding to a successful execution."
 	      (set status-var eshell-last-command-status))
 	  (cadr result))))))
 
-;;; Code:
-
-(defun eshell-unload-all-modules ()
-  "Unload all modules that were loaded by Eshell, if possible.
-If the user has require'd in any of the modules, or customized a
-variable with a :require tag (such as `eshell-prefer-to-shell'), it
-will be impossible to unload Eshell completely without restarting
-Emacs."
-  ;; if the user set `eshell-prefer-to-shell' to t, but never loaded
-  ;; Eshell, then `eshell-subgroups' will be unbound
-  (when (fboundp 'eshell-subgroups)
-    (dolist (module (eshell-subgroups 'eshell))
-      ;; this really only unloads as many modules as possible,
-      ;; since other `require' references (such as by customizing
-      ;; `eshell-prefer-to-shell' to a non-nil value) might make it
-      ;; impossible to unload Eshell completely
-      (if (featurep module)
-	  (ignore-errors
-	    (message "Unloading %s..." (symbol-name module))
-	    (unload-feature module)
-	    (message "Unloading %s...done" (symbol-name module)))))
-    (message "Unloading eshell...done")))
+(defun eshell-unload-function ()
+  (eshell-unload-extension-modules)
+  ;; Wait to unload core modules until after `eshell' has finished
+  ;; unloading.  `eshell' depends on several of them, so they can't be
+  ;; unloaded immediately.
+  (run-at-time 0 nil #'eshell-unload-modules
+               (reverse (eshell-subgroups 'eshell)) 'core)
+  nil)
 
 (run-hooks 'eshell-load-hook)
 

test/lisp/eshell/eshell-tests-unload.el

@@ -0,0 +1,99 @@
+;;; eshell-tests-unload.el --- test unloading Eshell  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2023 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Tests for unloading Eshell.
+
+;;; Code:
+
+(require 'ert)
+(require 'ert-x)
+
+;; In order to test unloading Eshell, don't require any of its files
+;; at the top level.  This means we need to explicitly declare some of
+;; the variables and functions we'll use.
+(defvar eshell-directory-name)
+(defvar eshell-history-file-name)
+(defvar eshell-last-dir-ring-file-name)
+(defvar eshell-modules-list)
+
+(declare-function eshell-module--feature-name "esh-module"
+                  (module &optional kind))
+(declare-function eshell-subgroups "esh-util" (groupsym))
+
+(defvar max-unload-time 5
+  "The maximum amount of time to wait to unload Eshell modules, in seconds.
+See `unload-eshell'.")
+
+(defun load-eshell ()
+  "Load Eshell by calling the `eshell' function and immediately closing it."
+  (save-current-buffer
+    (ert-with-temp-directory eshell-directory-name
+      (let* (;; We want no history file, so prevent Eshell from falling
+             ;; back on $HISTFILE.
+             (process-environment (cons "HISTFILE" process-environment))
+             (eshell-history-file-name nil)
+             (eshell-last-dir-ring-file-name nil)
+             (eshell-buffer (eshell t)))
+        (let (kill-buffer-query-functions)
+          (kill-buffer eshell-buffer))))))
+
+(defun unload-eshell ()
+  "Unload Eshell, waiting until the core modules are unloaded as well."
+  (let ((debug-on-error t)
+        (inhibit-message t))
+    (unload-feature 'eshell)
+    ;; We unload core modules are unloaded from a timer, since they
+    ;; need to wait until after `eshell' itself is unloaded.  Wait for
+    ;; this to finish.
+    (let ((start (current-time)))
+      (while (featurep 'esh-arg)
+        (when (> (float-time (time-since start))
+                 max-unload-time)
+          (error "timed out waiting to unload Eshell modules"))
+        (sit-for 0.1)))))
+
+;;; Tests:
+
+(ert-deftest eshell-test-unload/default ()
+  "Test unloading Eshell with the default list of extension modules."
+  (load-eshell)
+  (unload-eshell))
+
+(ert-deftest eshell-test-unload/no-modules ()
+  "Test unloading Eshell with no extension modules."
+  (require 'esh-module)
+  (let (eshell-modules-list)
+    (load-eshell))
+  (dolist (module (eshell-subgroups 'eshell-module))
+    (should-not (featurep (intern (eshell-module--feature-name module)))))
+  (unload-eshell))
+
+(ert-deftest eshell-test-unload/all-modules ()
+  "Test unloading Eshell with every extension module."
+  (require 'esh-module)
+  (let ((eshell-modules-list (eshell-subgroups 'eshell-module)))
+    (load-eshell))
+  (dolist (module (eshell-subgroups 'eshell-module))
+    (should (featurep (intern (eshell-module--feature-name module)))))
+  (unload-eshell))
+
+(provide 'eshell-tests-unload)
+;;; eshell-tests-unload.el ends here