bug#39546: 28.0.50; Do not require subr-x at run time

bug#39546: 28.0.50; Do not require subr-x at run time

[Tino Calancha]

X-Debbugs-Cc: Tassilo Horn <tsdh@gnu.org>,Michael Albinus <michael.albinus@gmx.de>,Lars Magne Ingebrigtsen <larsi@gnus.org>
Severity: wishlist
tags: patch

This patch replaces
(require subr-x)
with
(eval-when-compile (require 'subr-x))

on several files using just defsubst/defmacro from subr-x.el.

In addition, it moves back replace-region-contents into subr.el: been a defun,
and documented in the manual, then it fits better inside subr.el.

--8<-----------------------------cut here---------------start------------->8---
commit 87abdb434ad892256c4a432eb86ac5628d253891
Author: Constantino Calancha <tino.calancha@gmail.com>
Date:   Mon Feb 10 14:19:10 2020 +0100

    Do not require subr-x at run time
    
    Move back `replace-region-contents' to subr.el according
    with the convention for subr-x.
    * calendar/time-date.el
    * net/net-utils.el
    * net/nsm.el
    * net/tramp-compat.el:
    Require subr-x at compile time.
    
    * lisp/subr.el (replace-region-contents): Move it from subr-x.el.
    
    * lisp/emacs-lisp/json.el:  Do not require subr-x since now
    `replace-region-contents' is loaded at start up.

diff --git a/lisp/calendar/time-date.el b/lisp/calendar/time-date.el
index e2402de801..b4b13037bf 100644
--- a/lisp/calendar/time-date.el
+++ b/lisp/calendar/time-date.el
@@ -37,7 +37,7 @@
 ;;; Code:
 
 (require 'cl-lib)
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 
 (defmacro with-decoded-time-value (varlist &rest body)
   "Decode a time value and bind it according to VARLIST, then eval BODY.
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 044c9aada0..883607a549 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -254,34 +254,6 @@ string-remove-suffix
       (substring string 0 (- (length string) (length suffix)))
     string))
 
-(defun replace-region-contents (beg end replace-fn
-                                    &optional max-secs max-costs)
-  "Replace the region between BEG and END using REPLACE-FN.
-REPLACE-FN runs on the current buffer narrowed to the region.  It
-should return either a string or a buffer replacing the region.
-
-The replacement is performed using `replace-buffer-contents'
-which also describes the MAX-SECS and MAX-COSTS arguments and the
-return value.
-
-Note: If the replacement is a string, it'll be placed in a
-temporary buffer so that `replace-buffer-contents' can operate on
-it.  Therefore, if you already have the replacement in a buffer,
-it makes no sense to convert it to a string using
-`buffer-substring' or similar."
-  (save-excursion
-    (save-restriction
-      (narrow-to-region beg end)
-      (goto-char (point-min))
-      (let ((repl (funcall replace-fn)))
-	(if (bufferp repl)
-	    (replace-buffer-contents repl max-secs max-costs)
-	  (let ((source-buffer (current-buffer)))
-	    (with-temp-buffer
-	      (insert repl)
-	      (let ((tmp-buffer (current-buffer)))
-		(set-buffer source-buffer)
-		(replace-buffer-contents tmp-buffer max-secs max-costs)))))))))
 
 (provide 'subr-x)
 
diff --git a/lisp/json.el b/lisp/json.el
index 18d7fda882..e31928e825 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -55,7 +55,6 @@
 ;;; Code:
 
 (require 'map)
-(require 'subr-x)
 
 ;; Parameters
 
diff --git a/lisp/net/net-utils.el b/lisp/net/net-utils.el
index ef3651b033..780f34b028 100644
--- a/lisp/net/net-utils.el
+++ b/lisp/net/net-utils.el
@@ -44,7 +44,7 @@
 ;; days is for /sbin to be a symlink to /usr/sbin, but we still need to
 ;; search both for older systems.
 
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 (require 'cl-lib)
 
 (defun net-utils--executable-find-sbin (command)
diff --git a/lisp/net/nsm.el b/lisp/net/nsm.el
index 2d36c5e257..f85529f726 100644
--- a/lisp/net/nsm.el
+++ b/lisp/net/nsm.el
@@ -26,7 +26,7 @@
 
 (require 'cl-lib)
 (require 'rmc)                       ; read-multiple-choice
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 (require 'seq)
 (require 'map)
 
diff --git a/lisp/net/tramp-compat.el b/lisp/net/tramp-compat.el
index 87bcd08b85..8f74683dee 100644
--- a/lisp/net/tramp-compat.el
+++ b/lisp/net/tramp-compat.el
@@ -39,7 +39,7 @@ tramp-unload-file-name-handlers
 (require 'ls-lisp)  ;; Due to `tramp-handle-insert-directory'.
 (require 'parse-time)
 (require 'shell)
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 
 ;; `temporary-file-directory' as function is introduced with Emacs 26.1.
 (declare-function tramp-handle-temporary-file-directory "tramp")
diff --git a/lisp/subr.el b/lisp/subr.el
index b5ec0de156..406cc50611 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -5768,6 +5768,35 @@ flatten-tree
 ;; for discoverability:
 (defalias 'flatten-list 'flatten-tree)
 
+(defun replace-region-contents (beg end replace-fn
+                                    &optional max-secs max-costs)
+  "Replace the region between BEG and END using REPLACE-FN.
+REPLACE-FN runs on the current buffer narrowed to the region.  It
+should return either a string or a buffer replacing the region.
+
+The replacement is performed using `replace-buffer-contents'
+which also describes the MAX-SECS and MAX-COSTS arguments and the
+return value.
+
+Note: If the replacement is a string, it'll be placed in a
+temporary buffer so that `replace-buffer-contents' can operate on
+it.  Therefore, if you already have the replacement in a buffer,
+it makes no sense to convert it to a string using
+`buffer-substring' or similar."
+  (save-excursion
+    (save-restriction
+      (narrow-to-region beg end)
+      (goto-char (point-min))
+      (let ((repl (funcall replace-fn)))
+	(if (bufferp repl)
+	    (replace-buffer-contents repl max-secs max-costs)
+	  (let ((source-buffer (current-buffer)))
+	    (with-temp-buffer
+	      (insert repl)
+	      (let ((tmp-buffer (current-buffer)))
+		(set-buffer source-buffer)
+		(replace-buffer-contents tmp-buffer max-secs max-costs)))))))))
+
 ;; The initial anchoring is for better performance in searching matches.
 (defconst regexp-unmatchable "\\`a\\`"
   "Standard regexp guaranteed not to match any string at all.")

--8<-----------------------------cut here---------------end--------------->8---


In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, Xaw scroll bars)
 of 2020-02-10 built on localhost.example.com
Repository revision: ac9acc1864b02b92de4eb2e98db7b5b0cd03e019
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12007000
System Description: openSUSE Tumbleweed

bug#39546: 28.0.50; Do not require subr-x at run time

[Michael Albinus]

Tino Calancha <tino.calancha@gmail.com> writes:

Hi Tino,

> diff --git a/lisp/net/tramp-compat.el b/lisp/net/tramp-compat.el
> index 87bcd08b85..8f74683dee 100644
> --- a/lisp/net/tramp-compat.el
> +++ b/lisp/net/tramp-compat.el
> @@ -39,7 +39,7 @@ tramp-unload-file-name-handlers
>  (require 'ls-lisp)  ;; Due to `tramp-handle-insert-directory'.
>  (require 'parse-time)
>  (require 'shell)
> -(require 'subr-x)
> +(eval-when-compile (require 'subr-x))
>
>  ;; `temporary-file-directory' as function is introduced with Emacs 26.1.
>  (declare-function tramp-handle-temporary-file-directory "tramp")

Tramp needs `string-join' and `string-empty-p' from subr-x.el. If we
don't load it here, we must eval-when-compile it in almost all Tramp
files, I believe.

Hmm, what's so bad requiring it? (I know the general answer because both
are defsubst'es, but I don't see the point here).

bug#39546: 28.0.50; Do not require subr-x at run time

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Mon, 10 Feb 2020 14:59:42 +0100
> Cc: lars magne ingebrigtsen <larsi@gnus.org>,
>  michael albinus <michael.albinus@gmx.de>, tassilo horn <tsdh@gnu.org>
> 
> In addition, it moves back replace-region-contents into subr.el: been a defun,
> and documented in the manual, then it fits better inside subr.el.

subr.el is preloaded, so doing that will enlarge the Emacs memory
footprint.  Is that function important enough to have it preloaded?

Thanks.

bug#39546: 28.0.50; Do not require subr-x at run time

[Tino Calancha]

On Mon, 10 Feb 2020, Eli Zaretskii wrote:

>> In addition, it moves back replace-region-contents into subr.el: been a defun,
>> and documented in the manual, then it fits better inside subr.el.
>
> subr.el is preloaded, so doing that will enlarge the Emacs memory
> footprint.  Is that function important enough to have it preloaded?
According with its usage at Emacs source (only at json.el) I'd say
the answer to your question is: not worth to be at subr.el.

bug#39546: 28.0.50; Do not require subr-x at run time

[Tino Calancha]

On Mon, 10 Feb 2020, Michael Albinus wrote:

> Tramp needs `string-join' and `string-empty-p' from subr-x.el. If we
> don't load it here, we must eval-when-compile it in almost all Tramp
> files, I believe.

> Hmm, what's so bad requiring it? (I know the general answer because both
> are defsubst'es, but I don't see the point here).

My point is just to follow our convention for subr-x; of course, not a 
strong point, just a aesthetic one.

bug#39546: 28.0.50; Do not require subr-x at run time

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Date: Mon, 10 Feb 2020 18:26:13 +0100 (CET)
> cc: Tino Calancha <tino.calancha@gmail.com>, 39546@debbugs.gnu.org, 
>     larsi@gnus.org, michael.albinus@gmx.de, tsdh@gnu.org
> 
> On Mon, 10 Feb 2020, Eli Zaretskii wrote:
> 
> > subr.el is preloaded, so doing that will enlarge the Emacs memory
> > footprint.  Is that function important enough to have it preloaded?
> According with its usage at Emacs source (only at json.el) I'd say
> the answer to your question is: not worth to be at subr.el.

That's what I thought.  So I suggest to find another file to host this
function, if we want subr-x.el to have only macros and defsubst's.

Thanks.

bug#39546: 28.0.50; Do not require subr-x at run time

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> > subr.el is preloaded, so doing that will enlarge the Emacs memory
>> > footprint.  Is that function important enough to have it preloaded?
>> According with its usage at Emacs source (only at json.el) I'd say
>> the answer to your question is: not worth to be at subr.el.
>
> That's what I thought.  So I suggest to find another file to host this
> function, if we want subr-x.el to have only macros and defsubst's.

Given that it's only a wrapper around replace-buffer-contents which is
always available, I think it shouldn't be hidden too much.  AFAIK, I've
put it in subr.el first and requests were made to move it to subr-x.el.

I've already submitted patches to hindent (and I think to some other 3rd
party package providing code formatting by replacing with the output of
some formatting tool) using that function.

Anyway, if it is going to be moved, I'd to ask you to move it in
emacs-27 so that I can check for and fix external usages I know of
without having to distinguish emacs versions.

Bye,
Tassilo

bug#39546: 28.0.50; Do not require subr-x at run time

[Tino Calancha]

On Mon, 10 Feb 2020, Eli Zaretskii wrote:

>> From: Tino Calancha <tino.calancha@gmail.com>
>> Date: Mon, 10 Feb 2020 18:26:13 +0100 (CET)
>> cc: Tino Calancha <tino.calancha@gmail.com>, 39546@debbugs.gnu.org,
>>     larsi@gnus.org, michael.albinus@gmx.de, tsdh@gnu.org
>>
>> On Mon, 10 Feb 2020, Eli Zaretskii wrote:
>>
>>> subr.el is preloaded, so doing that will enlarge the Emacs memory
>>> footprint.  Is that function important enough to have it preloaded?
>> According with its usage at Emacs source (only at json.el) I'd say
>> the answer to your question is: not worth to be at subr.el.
>
> That's what I thought.  So I suggest to find another file to host this
> function, if we want subr-x.el to have only macros and defsubst's.

replace.el is a good agreement between subr-x.el (it's a defun, so 
it doesn't fit well there) and subr.el.

As subr.el, replace.el is also loaded at start time, but it's ~ half size,
and more important than that, it's all about replacing in Emacs.

bug#39546: 28.0.50; Do not require subr-x at run time

[Tassilo Horn]

Am Mo, 10. Feb 2020, um 20:30, schrieb Tino Calancha:
> replace.el is a good agreement between subr-x.el (it's a defun, so 
> it doesn't fit well there) and subr.el.
> 
> As subr.el, replace.el is also loaded at start time, but it's ~ half size,
> and more important than that, it's all about replacing in Emacs.

Sounds good to me!

Thanks,
  Tassilo

bug#39546: 28.0.50; Do not require subr-x at run time

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: Tino Calancha <tino.calancha@gmail.com>,  39546@debbugs.gnu.org,
>   larsi@gnus.org,  michael.albinus@gmx.de
> Date: Mon, 10 Feb 2020 20:19:05 +0100
> 
> I've already submitted patches to hindent (and I think to some other 3rd
> party package providing code formatting by replacing with the output of
> some formatting tool) using that function.
> 
> Anyway, if it is going to be moved, I'd to ask you to move it in
> emacs-27 so that I can check for and fix external usages I know of
> without having to distinguish emacs versions.

Then maybe we shouldn't move it, just for these reasons.

bug#39546: 28.0.50; Do not require subr-x at run time

[Tassilo Horn]

Am Mo, 10. Feb 2020, um 20:59, schrieb Eli Zaretskii:
> > From: Tassilo Horn <tsdh@gnu.org>
> > Cc: Tino Calancha <tino.calancha@gmail.com>,  39546@debbugs.gnu.org,
> >   larsi@gnus.org,  michael.albinus@gmx.de
> > Date: Mon, 10 Feb 2020 20:19:05 +0100
> > 
> > I've already submitted patches to hindent (and I think to some other 3rd
> > party package providing code formatting by replacing with the output of
> > some formatting tool) using that function.
> > 
> > Anyway, if it is going to be moved, I'd to ask you to move it in
> > emacs-27 so that I can check for and fix external usages I know of
> > without having to distinguish emacs versions.
> 
> Then maybe we shouldn't move it, just for these reasons.

If we all think that replace.el is the right place and it has the benefit of being loaded at startup, then I'm all for it. The hindent PR is not yet merged anyway, and the other one was quickly accepted, so an addendum patch will be, too.

I'd like to see more usages of that function. Basically every package providing support for external formatters should use it, e.g., I'm going to submit a patch for rust-mode providing formatting using rustfmt anytime soon. So better change it now than regretting that multiple not so uncommon packages require subr-x at runtime.

Bye,
Tassilo

bug#39546: 28.0.50; Do not require subr-x at run time

[Tino Calancha]

"Tassilo Horn" <tsdh@gnu.org> writes:

> Am Mo, 10. Feb 2020, um 20:30, schrieb Tino Calancha:
>> replace.el is a good agreement between subr-x.el (it's a defun, so 
>> it doesn't fit well there) and subr.el.
>> 
>> As subr.el, replace.el is also loaded at start time, but it's ~ half size,
>> and more important than that, it's all about replacing in Emacs.
>
> Sounds good to me!

Here is the updated patch:
- It deletes the cookie from subr-x (yes, we have one for when-let)
- Move replace-region-contents inside replace.el (loaded at startup)
- Add all required (eval-when...(req.. 'subr-x)) at tramp files
- Replaced loading subr-x at run time with compile time for some other files

I have run the testsuite without surprises.

--8<-----------------------------cut here---------------start------------->8---
commit f6a7a4fc673f77724ccf0d7ae1fdb164328afc68
Author: Constantino Calancha <tino.calancha@gmail.com>
Date:   Mon Feb 10 21:19:29 2020 +0100

    Do not require subr-x at run time
    
    Move back `replace-region-contents' to subr.el according
    with the convention for subr-x (Bug#39546).
    
    * lisp/subr-x.el (replace-region-contents): Delete it.
      (when-let): Delete autoload cookie.
    
    * calendar/time-date.el
    * net/net-utils.el
    * net/nsm.el
    * net/tramp-compat.el
    * lisp/net/mailcap.el
    * lisp/net/tramp-adb.el
    * lisp/net/tramp-cmds.el
    * lisp/net/tramp-gvfs.el
    * lisp/net/tramp-rclone.el
    * lisp/net/tramp-sh.el
    * lisp/net/tramp-smb.el
    * lisp/net/tramp-sudoedit.el
    * lisp/net/tramp.el:
    Require subr-x at compile time.
    
    * lisp/replace.el (replace-region-contents): Move it from subr-x.el.
    
    * lisp/emacs-lisp/json.el:  Do not require subr-x since now
    `replace-region-contents' is loaded at start up.

diff --git a/lisp/calendar/time-date.el b/lisp/calendar/time-date.el
index e2402de801..b4b13037bf 100644
--- a/lisp/calendar/time-date.el
+++ b/lisp/calendar/time-date.el
@@ -37,7 +37,7 @@
 ;;; Code:
 
 (require 'cl-lib)
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 
 (defmacro with-decoded-time-value (varlist &rest body)
   "Decode a time value and bind it according to VARLIST, then eval BODY.
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 044c9aada0..2c1415ab2a 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -182,7 +182,6 @@ if-let
     (setq spec (list spec)))
   (list 'if-let* spec then (macroexp-progn else)))
 
-;;;###autoload
 (defmacro when-let (spec &rest body)
   "Bind variables according to SPEC and conditionally evaluate BODY.
 Evaluate each binding in turn, stopping if a binding value is nil.
@@ -254,34 +253,6 @@ string-remove-suffix
       (substring string 0 (- (length string) (length suffix)))
     string))
 
-(defun replace-region-contents (beg end replace-fn
-                                    &optional max-secs max-costs)
-  "Replace the region between BEG and END using REPLACE-FN.
-REPLACE-FN runs on the current buffer narrowed to the region.  It
-should return either a string or a buffer replacing the region.
-
-The replacement is performed using `replace-buffer-contents'
-which also describes the MAX-SECS and MAX-COSTS arguments and the
-return value.
-
-Note: If the replacement is a string, it'll be placed in a
-temporary buffer so that `replace-buffer-contents' can operate on
-it.  Therefore, if you already have the replacement in a buffer,
-it makes no sense to convert it to a string using
-`buffer-substring' or similar."
-  (save-excursion
-    (save-restriction
-      (narrow-to-region beg end)
-      (goto-char (point-min))
-      (let ((repl (funcall replace-fn)))
-	(if (bufferp repl)
-	    (replace-buffer-contents repl max-secs max-costs)
-	  (let ((source-buffer (current-buffer)))
-	    (with-temp-buffer
-	      (insert repl)
-	      (let ((tmp-buffer (current-buffer)))
-		(set-buffer source-buffer)
-		(replace-buffer-contents tmp-buffer max-secs max-costs)))))))))
 
 (provide 'subr-x)
 
diff --git a/lisp/json.el b/lisp/json.el
index 18d7fda882..e31928e825 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -55,7 +55,6 @@
 ;;; Code:
 
 (require 'map)
-(require 'subr-x)
 
 ;; Parameters
 
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index 5fe5b4d3a5..75c91946e4 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -29,6 +29,7 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'subr-x))
 (autoload 'mail-header-parse-content-type "mail-parse")
 
 (defgroup mailcap nil
diff --git a/lisp/net/net-utils.el b/lisp/net/net-utils.el
index ef3651b033..780f34b028 100644
--- a/lisp/net/net-utils.el
+++ b/lisp/net/net-utils.el
@@ -44,7 +44,7 @@
 ;; days is for /sbin to be a symlink to /usr/sbin, but we still need to
 ;; search both for older systems.
 
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 (require 'cl-lib)
 
 (defun net-utils--executable-find-sbin (command)
diff --git a/lisp/net/nsm.el b/lisp/net/nsm.el
index 2d36c5e257..f85529f726 100644
--- a/lisp/net/nsm.el
+++ b/lisp/net/nsm.el
@@ -26,7 +26,7 @@
 
 (require 'cl-lib)
 (require 'rmc)                       ; read-multiple-choice
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 (require 'seq)
 (require 'map)
 
diff --git a/lisp/net/tramp-adb.el b/lisp/net/tramp-adb.el
index aa7fe147c2..60c46f1224 100644
--- a/lisp/net/tramp-adb.el
+++ b/lisp/net/tramp-adb.el
@@ -33,6 +33,7 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 
 ;;;###tramp-autoload
diff --git a/lisp/net/tramp-cmds.el b/lisp/net/tramp-cmds.el
index b4dca2321c..871c215e87 100644
--- a/lisp/net/tramp-cmds.el
+++ b/lisp/net/tramp-cmds.el
@@ -29,6 +29,7 @@
 ;;; Code:
 
 (require 'tramp)
+(eval-when-compile (require 'subr-x))
 
 ;; Pacify byte-compiler.
 (declare-function mml-mode "mml")
diff --git a/lisp/net/tramp-compat.el b/lisp/net/tramp-compat.el
index 87bcd08b85..8f74683dee 100644
--- a/lisp/net/tramp-compat.el
+++ b/lisp/net/tramp-compat.el
@@ -39,7 +39,7 @@ tramp-unload-file-name-handlers
 (require 'ls-lisp)  ;; Due to `tramp-handle-insert-directory'.
 (require 'parse-time)
 (require 'shell)
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 
 ;; `temporary-file-directory' as function is introduced with Emacs 26.1.
 (declare-function tramp-handle-temporary-file-directory "tramp")
diff --git a/lisp/net/tramp-gvfs.el b/lisp/net/tramp-gvfs.el
index 0d800cb42b..5b037fb94b 100644
--- a/lisp/net/tramp-gvfs.el
+++ b/lisp/net/tramp-gvfs.el
@@ -102,6 +102,7 @@
 (declare-function dbus-get-unique-name "dbusbind.c")
 
 (eval-when-compile (require 'cl-lib))
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 (require 'dbus)
 (require 'url-parse)
diff --git a/lisp/net/tramp-rclone.el b/lisp/net/tramp-rclone.el
index 445098a5bc..b7c99829ed 100644
--- a/lisp/net/tramp-rclone.el
+++ b/lisp/net/tramp-rclone.el
@@ -36,6 +36,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 
 ;;;###tramp-autoload
diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
index 5a3abc31ea..0c5bc72deb 100644
--- a/lisp/net/tramp-sh.el
+++ b/lisp/net/tramp-sh.el
@@ -32,6 +32,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 
 (declare-function dired-remove-file "dired-aux")
diff --git a/lisp/net/tramp-smb.el b/lisp/net/tramp-smb.el
index f02be394a7..43e775b921 100644
--- a/lisp/net/tramp-smb.el
+++ b/lisp/net/tramp-smb.el
@@ -28,6 +28,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 
 ;; Define SMB method ...
diff --git a/lisp/net/tramp-sudoedit.el b/lisp/net/tramp-sudoedit.el
index f258ad6b93..6ea52bedf8 100644
--- a/lisp/net/tramp-sudoedit.el
+++ b/lisp/net/tramp-sudoedit.el
@@ -34,6 +34,7 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 
 ;;;###tramp-autoload
diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index 19d36c3a97..0783eda09e 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -60,6 +60,7 @@
 (require 'tramp-compat)
 (require 'tramp-integration)
 (require 'trampver)
+(eval-when-compile (require 'subr-x))
 
 ;; Pacify byte-compiler.
 (require 'cl-lib)
diff --git a/lisp/replace.el b/lisp/replace.el
index a0b050637e..74c54a0b80 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -743,6 +743,35 @@ replace-regexp
 	   (if (use-region-p) (region-noncontiguous-p)))))
   (perform-replace regexp to-string nil t delimited nil nil start end backward region-noncontiguous-p))
 
+(defun replace-region-contents (beg end replace-fn
+                                    &optional max-secs max-costs)
+  "Replace the region between BEG and END using REPLACE-FN.
+REPLACE-FN runs on the current buffer narrowed to the region.  It
+should return either a string or a buffer replacing the region.
+
+The replacement is performed using `replace-buffer-contents'
+which also describes the MAX-SECS and MAX-COSTS arguments and the
+return value.
+
+Note: If the replacement is a string, it'll be placed in a
+temporary buffer so that `replace-buffer-contents' can operate on
+it.  Therefore, if you already have the replacement in a buffer,
+it makes no sense to convert it to a string using
+`buffer-substring' or similar."
+  (save-excursion
+    (save-restriction
+      (narrow-to-region beg end)
+      (goto-char (point-min))
+      (let ((repl (funcall replace-fn)))
+	(if (bufferp repl)
+	    (replace-buffer-contents repl max-secs max-costs)
+	  (let ((source-buffer (current-buffer)))
+	    (with-temp-buffer
+	      (insert repl)
+	      (let ((tmp-buffer (current-buffer)))
+		(set-buffer source-buffer)
+		(replace-buffer-contents tmp-buffer max-secs max-costs)))))))))
+
 \f
 (defvar regexp-history nil
   "History list for some commands that read regular expressions.

--8<-----------------------------cut here---------------end--------------->8---

bug#39546: 28.0.50; Do not require subr-x at run time

[Tino Calancha]

Tino Calancha <tino.calancha@gmail.com> writes:

> "Tassilo Horn" <tsdh@gnu.org> writes:
>
>> Am Mo, 10. Feb 2020, um 20:30, schrieb Tino Calancha:
>>> replace.el is a good agreement between subr-x.el (it's a defun, so 
>>> it doesn't fit well there) and subr.el.
>>> 
>>> As subr.el, replace.el is also loaded at start time, but it's ~ half size,
>>> and more important than that, it's all about replacing in Emacs.
>>
>> Sounds good to me!
>
> Here is the updated patch:
> - It deletes the cookie from subr-x (yes, we have one for when-let)
> - Move replace-region-contents inside replace.el (loaded at startup)
> - Add all required (eval-when...(req.. 'subr-x)) at tramp files
> - Replaced loading subr-x at run time with compile time for some other files
>
> I have run the testsuite without surprises.
Well, actually there was a surprise, but who don't like surprises after
all!
I missed this one (it was relying on the whe-let cookie):

diff --git a/lisp/image/exif.el b/lisp/image/exif.el
index 642bc58321..27ad616146 100644
--- a/lisp/image/exif.el
+++ b/lisp/image/exif.el
@@ -62,6 +62,7 @@
 ;;; Code:
 
 (require 'cl-lib)
+(eval-when-compile (require 'subr-x))
 
 (defvar exif-tag-alist
   '((11 processing-software)

bug#39546: 28.0.50; Do not require subr-x at run time

[Tino Calancha]

Tino Calancha <tino.calancha@gmail.com> writes:

>> I have run the testsuite without surprises.
> Well, actually there was a surprise, but who don't like surprises after
> all!
Well, I must recognize I am one of those guys who doesn't like
surprises.  It seems `when-let' became very popular these days.

Here is the updated patch.  It takes in account all usages of `when-let'
once dropped the cookie.

--8<-----------------------------cut here---------------start------------->8---
commit 0c8d7c5312ce167d6123a768c6fe00a2b07288b4
Author: Constantino Calancha <tino.calancha@gmail.com>
Date:   Mon Feb 10 21:56:53 2020 +0100

    Do not require subr-x at run time
    
    Move back `replace-region-contents' to subr.el according
    with the convention for subr-x (Bug#39546).
    
    * lisp/subr-x.el (replace-region-contents): Delete it.
      (when-let): Delete autoload cookie.
    
    * calendar/time-date.el
    * net/net-utils.el
    * net/nsm.el
    * lisp/dired-x.el
    * lisp/emacs-lisp/edebug.el
    
    * lisp/gnus/gnus-sum.el
    * lisp/gnus/mml.el
    * lisp/image/image-converter.el
    * lisp/mail/emacsbug.el
    * lisp/image/exif.el
    
    * net/tramp-compat.el
    * lisp/net/mailcap.el
    * lisp/net/tramp-adb.el
    * lisp/net/tramp-cmds.el
    * lisp/net/tramp-gvfs.el
    
    * lisp/net/tramp-rclone.el
    * lisp/net/tramp-sh.el
    * lisp/net/tramp-smb.el
    * lisp/net/tramp-sudoedit.el
    * lisp/net/tramp.el:
    Require subr-x at compile time.
    
    * lisp/replace.el (replace-region-contents): Move it from subr-x.el.
    
    * lisp/emacs-lisp/json.el:  Do not require subr-x since now
    `replace-region-contents' is loaded at start up.

diff --git a/lisp/calendar/time-date.el b/lisp/calendar/time-date.el
index e2402de801..b4b13037bf 100644
--- a/lisp/calendar/time-date.el
+++ b/lisp/calendar/time-date.el
@@ -37,7 +37,7 @@
 ;;; Code:
 
 (require 'cl-lib)
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 
 (defmacro with-decoded-time-value (varlist &rest body)
   "Decode a time value and bind it according to VARLIST, then eval BODY.
diff --git a/lisp/dired-x.el b/lisp/dired-x.el
index 623a1dd325..73029a0095 100644
--- a/lisp/dired-x.el
+++ b/lisp/dired-x.el
@@ -43,6 +43,7 @@
 ;; This is a no-op if dired-x is being loaded via `dired-load-hook',
 ;; but maybe not if a dired-x function is being autoloaded.
 (require 'dired)
+(eval-when-compile (require 'subr-x))
 
 ;;; User-defined variables.
 
diff --git a/lisp/emacs-lisp/edebug.el b/lisp/emacs-lisp/edebug.el
index 85cc8c8e7a..5a5a79488f 100644
--- a/lisp/emacs-lisp/edebug.el
+++ b/lisp/emacs-lisp/edebug.el
@@ -55,7 +55,9 @@
 (require 'backtrace)
 (require 'macroexp)
 (require 'cl-lib)
-(eval-when-compile (require 'pcase))
+(eval-when-compile
+  (require 'pcase)
+  (require 'subr-x)
 
 ;;; Options
 
diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 044c9aada0..2c1415ab2a 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -182,7 +182,6 @@ if-let
     (setq spec (list spec)))
   (list 'if-let* spec then (macroexp-progn else)))
 
-;;;###autoload
 (defmacro when-let (spec &rest body)
   "Bind variables according to SPEC and conditionally evaluate BODY.
 Evaluate each binding in turn, stopping if a binding value is nil.
@@ -254,34 +253,6 @@ string-remove-suffix
       (substring string 0 (- (length string) (length suffix)))
     string))
 
-(defun replace-region-contents (beg end replace-fn
-                                    &optional max-secs max-costs)
-  "Replace the region between BEG and END using REPLACE-FN.
-REPLACE-FN runs on the current buffer narrowed to the region.  It
-should return either a string or a buffer replacing the region.
-
-The replacement is performed using `replace-buffer-contents'
-which also describes the MAX-SECS and MAX-COSTS arguments and the
-return value.
-
-Note: If the replacement is a string, it'll be placed in a
-temporary buffer so that `replace-buffer-contents' can operate on
-it.  Therefore, if you already have the replacement in a buffer,
-it makes no sense to convert it to a string using
-`buffer-substring' or similar."
-  (save-excursion
-    (save-restriction
-      (narrow-to-region beg end)
-      (goto-char (point-min))
-      (let ((repl (funcall replace-fn)))
-	(if (bufferp repl)
-	    (replace-buffer-contents repl max-secs max-costs)
-	  (let ((source-buffer (current-buffer)))
-	    (with-temp-buffer
-	      (insert repl)
-	      (let ((tmp-buffer (current-buffer)))
-		(set-buffer source-buffer)
-		(replace-buffer-contents tmp-buffer max-secs max-costs)))))))))
 
 (provide 'subr-x)
 
diff --git a/lisp/gnus/gnus-sum.el b/lisp/gnus/gnus-sum.el
index a47e657623..bd865ca9b0 100644
--- a/lisp/gnus/gnus-sum.el
+++ b/lisp/gnus/gnus-sum.el
@@ -25,6 +25,7 @@
 ;;; Code:
 
 (require 'cl-lib)
+(eval-when-compile (require 'subr-x))
 
 (defvar tool-bar-mode)
 (defvar gnus-category-predicate-alist)
diff --git a/lisp/gnus/mml.el b/lisp/gnus/mml.el
index cdd8f3d3a5..903c3ff7ec 100644
--- a/lisp/gnus/mml.el
+++ b/lisp/gnus/mml.el
@@ -27,9 +27,11 @@
 (require 'mm-encode)
 (require 'mm-decode)
 (require 'mml-sec)
-(eval-when-compile (require 'cl-lib))
-(eval-when-compile (require 'url))
-(eval-when-compile (require 'gnus-util))
+(eval-when-compile
+  (require 'cl-lib)
+  (require 'url)
+  (require 'gnus-util)
+  (require 'subr-x))
 
 (autoload 'message-make-message-id "message")
 (declare-function gnus-setup-posting-charset "gnus-msg" (group))
diff --git a/lisp/image/exif.el b/lisp/image/exif.el
index 642bc58321..27ad616146 100644
--- a/lisp/image/exif.el
+++ b/lisp/image/exif.el
@@ -62,6 +62,7 @@
 ;;; Code:
 
 (require 'cl-lib)
+(eval-when-compile (require 'subr-x))
 
 (defvar exif-tag-alist
   '((11 processing-software)
diff --git a/lisp/image/image-converter.el b/lisp/image/image-converter.el
index 0488a13d41..33a8903e94 100644
--- a/lisp/image/image-converter.el
+++ b/lisp/image/image-converter.el
@@ -27,7 +27,9 @@
 ;;; Code:
 
 (require 'cl-generic)
-(eval-when-compile (require 'cl-lib))
+(eval-when-compile
+  (require 'cl-lib)
+  (require 'subr-x))
 
 (defcustom image-converter nil
   "Type of the external image converter to use.
diff --git a/lisp/json.el b/lisp/json.el
index 18d7fda882..e31928e825 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -55,7 +55,6 @@
 ;;; Code:
 
 (require 'map)
-(require 'subr-x)
 
 ;; Parameters
 
diff --git a/lisp/mail/emacsbug.el b/lisp/mail/emacsbug.el
index 7f3dc4454a..3aea5aae8a 100644
--- a/lisp/mail/emacsbug.el
+++ b/lisp/mail/emacsbug.el
@@ -34,6 +34,7 @@
 
 (require 'sendmail)
 (require 'message)
+(eval-when-compile (require 'subr-x))
 
 (defgroup emacsbug nil
   "Sending Emacs bug reports."
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index 5fe5b4d3a5..75c91946e4 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -29,6 +29,7 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'subr-x))
 (autoload 'mail-header-parse-content-type "mail-parse")
 
 (defgroup mailcap nil
diff --git a/lisp/net/net-utils.el b/lisp/net/net-utils.el
index ef3651b033..780f34b028 100644
--- a/lisp/net/net-utils.el
+++ b/lisp/net/net-utils.el
@@ -44,7 +44,7 @@
 ;; days is for /sbin to be a symlink to /usr/sbin, but we still need to
 ;; search both for older systems.
 
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 (require 'cl-lib)
 
 (defun net-utils--executable-find-sbin (command)
diff --git a/lisp/net/nsm.el b/lisp/net/nsm.el
index 2d36c5e257..f85529f726 100644
--- a/lisp/net/nsm.el
+++ b/lisp/net/nsm.el
@@ -26,7 +26,7 @@
 
 (require 'cl-lib)
 (require 'rmc)                       ; read-multiple-choice
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 (require 'seq)
 (require 'map)
 
diff --git a/lisp/net/tramp-adb.el b/lisp/net/tramp-adb.el
index aa7fe147c2..60c46f1224 100644
--- a/lisp/net/tramp-adb.el
+++ b/lisp/net/tramp-adb.el
@@ -33,6 +33,7 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 
 ;;;###tramp-autoload
diff --git a/lisp/net/tramp-cmds.el b/lisp/net/tramp-cmds.el
index b4dca2321c..871c215e87 100644
--- a/lisp/net/tramp-cmds.el
+++ b/lisp/net/tramp-cmds.el
@@ -29,6 +29,7 @@
 ;;; Code:
 
 (require 'tramp)
+(eval-when-compile (require 'subr-x))
 
 ;; Pacify byte-compiler.
 (declare-function mml-mode "mml")
diff --git a/lisp/net/tramp-compat.el b/lisp/net/tramp-compat.el
index 87bcd08b85..8f74683dee 100644
--- a/lisp/net/tramp-compat.el
+++ b/lisp/net/tramp-compat.el
@@ -39,7 +39,7 @@ tramp-unload-file-name-handlers
 (require 'ls-lisp)  ;; Due to `tramp-handle-insert-directory'.
 (require 'parse-time)
 (require 'shell)
-(require 'subr-x)
+(eval-when-compile (require 'subr-x))
 
 ;; `temporary-file-directory' as function is introduced with Emacs 26.1.
 (declare-function tramp-handle-temporary-file-directory "tramp")
diff --git a/lisp/net/tramp-gvfs.el b/lisp/net/tramp-gvfs.el
index 0d800cb42b..5b037fb94b 100644
--- a/lisp/net/tramp-gvfs.el
+++ b/lisp/net/tramp-gvfs.el
@@ -102,6 +102,7 @@
 (declare-function dbus-get-unique-name "dbusbind.c")
 
 (eval-when-compile (require 'cl-lib))
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 (require 'dbus)
 (require 'url-parse)
diff --git a/lisp/net/tramp-rclone.el b/lisp/net/tramp-rclone.el
index 445098a5bc..b7c99829ed 100644
--- a/lisp/net/tramp-rclone.el
+++ b/lisp/net/tramp-rclone.el
@@ -36,6 +36,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 
 ;;;###tramp-autoload
diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
index 5a3abc31ea..0c5bc72deb 100644
--- a/lisp/net/tramp-sh.el
+++ b/lisp/net/tramp-sh.el
@@ -32,6 +32,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 
 (declare-function dired-remove-file "dired-aux")
diff --git a/lisp/net/tramp-smb.el b/lisp/net/tramp-smb.el
index f02be394a7..43e775b921 100644
--- a/lisp/net/tramp-smb.el
+++ b/lisp/net/tramp-smb.el
@@ -28,6 +28,7 @@
 ;;; Code:
 
 (eval-when-compile (require 'cl-lib))
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 
 ;; Define SMB method ...
diff --git a/lisp/net/tramp-sudoedit.el b/lisp/net/tramp-sudoedit.el
index f258ad6b93..6ea52bedf8 100644
--- a/lisp/net/tramp-sudoedit.el
+++ b/lisp/net/tramp-sudoedit.el
@@ -34,6 +34,7 @@
 
 ;;; Code:
 
+(eval-when-compile (require 'subr-x))
 (require 'tramp)
 
 ;;;###tramp-autoload
diff --git a/lisp/net/tramp.el b/lisp/net/tramp.el
index 19d36c3a97..0783eda09e 100644
--- a/lisp/net/tramp.el
+++ b/lisp/net/tramp.el
@@ -60,6 +60,7 @@
 (require 'tramp-compat)
 (require 'tramp-integration)
 (require 'trampver)
+(eval-when-compile (require 'subr-x))
 
 ;; Pacify byte-compiler.
 (require 'cl-lib)
diff --git a/lisp/replace.el b/lisp/replace.el
index a0b050637e..74c54a0b80 100644
--- a/lisp/replace.el
+++ b/lisp/replace.el
@@ -743,6 +743,35 @@ replace-regexp
 	   (if (use-region-p) (region-noncontiguous-p)))))
   (perform-replace regexp to-string nil t delimited nil nil start end backward region-noncontiguous-p))
 
+(defun replace-region-contents (beg end replace-fn
+                                    &optional max-secs max-costs)
+  "Replace the region between BEG and END using REPLACE-FN.
+REPLACE-FN runs on the current buffer narrowed to the region.  It
+should return either a string or a buffer replacing the region.
+
+The replacement is performed using `replace-buffer-contents'
+which also describes the MAX-SECS and MAX-COSTS arguments and the
+return value.
+
+Note: If the replacement is a string, it'll be placed in a
+temporary buffer so that `replace-buffer-contents' can operate on
+it.  Therefore, if you already have the replacement in a buffer,
+it makes no sense to convert it to a string using
+`buffer-substring' or similar."
+  (save-excursion
+    (save-restriction
+      (narrow-to-region beg end)
+      (goto-char (point-min))
+      (let ((repl (funcall replace-fn)))
+	(if (bufferp repl)
+	    (replace-buffer-contents repl max-secs max-costs)
+	  (let ((source-buffer (current-buffer)))
+	    (with-temp-buffer
+	      (insert repl)
+	      (let ((tmp-buffer (current-buffer)))
+		(set-buffer source-buffer)
+		(replace-buffer-contents tmp-buffer max-secs max-costs)))))))))
+
 \f
 (defvar regexp-history nil
   "History list for some commands that read regular expressions.

--8<-----------------------------cut here---------------end--------------->8---

bug#39546: 28.0.50; Do not require subr-x at run time

[Eli Zaretskii]

> From: Tino Calancha <tino.calancha@gmail.com>
> Cc: 39546@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>,
>   michael.albinus@gmx.de,  larsi@gnus.org
> Date: Mon, 10 Feb 2020 22:00:21 +0100
> 
> Tino Calancha <tino.calancha@gmail.com> writes:
> 
> >> I have run the testsuite without surprises.
> > Well, actually there was a surprise, but who don't like surprises after
> > all!
> Well, I must recognize I am one of those guys who doesn't like
> surprises.  It seems `when-let' became very popular these days.
> 
> Here is the updated patch.  It takes in account all usages of `when-let'
> once dropped the cookie.

Thanks.

Are we talking about doing these changes on master?  Because I think
such wide-sweeping changes that aren't bugfixes are not really
appropriate for the release branch.  If you want to move
replace-region-contents to replace.el on emacs-27, I agree, but all
the rest should go to master, I think.

bug#39546: 28.0.50; Do not require subr-x at run time

[Lars Ingebrigtsen]

Tino Calancha <tino.calancha@gmail.com> writes:

> -;;;###autoload
>  (defmacro when-let (spec &rest body)
>    "Bind variables according to SPEC and conditionally evaluate BODY.

Sorry; I haven't been following this thread closely, but I don't think
this is a good idea.  It means putting

+(eval-when-compile (require 'subr-x))

in more and more files, for no particularly good reason that I can see?

What's the reason for removing this autoload?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#39546: 28.0.50; Do not require subr-x at run time

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Tino Calancha <tino.calancha@gmail.com> writes:
>
>> -;;;###autoload
>>  (defmacro when-let (spec &rest body)
>>    "Bind variables according to SPEC and conditionally evaluate BODY.
>
> Sorry; I haven't been following this thread closely, but I don't think
> this is a good idea.  It means putting
>
> +(eval-when-compile (require 'subr-x))
>
> in more and more files, for no particularly good reason that I can see?
>
> What's the reason for removing this autoload?

I think the conclusion was (in a discussion in a different bug report,
if I remember correctly) that it's fine to ;;;###autoload when-let, and
it's also fine to require subr-x at run time (i.e., it's not just macros
and defsubsts).

So I'm closing this bug report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no