bug#40576: call-process-region does not accept nil as first argument

bug#40576: call-process-region does not accept nil as first argument

[Pietro Giorgianni]

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

Hi,

According to the documentation of call-process-region,
  If START is nil, that means to use the entire buffer contents; END is
ignored.

But when I run:
(call-process-region nil nil "/bin/cat" t (current-buffer))
I get:
Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p nil)
  call-process-region(nil nil "/bin/cat" t #<buffer *scratch*>)
  eval((call-process-region nil nil "/bin/cat" t (current-buffer)) nil)
  elisp--eval-last-sexp(t)
  eval-last-sexp(t)
  eval-print-last-sexp(nil)
  funcall-interactively(eval-print-last-sexp nil)
  call-interactively(eval-print-last-sexp nil nil)
  command-execute(eval-print-last-sexp)

If, instead, I run:
(call-process-region (point-min) (point-max) "/bin/cat" t (current-buffer))

It works.

Am I interpreting the documentation wrong?

Emacs version:
GNU Emacs 26.3 (build 1, x86_64-apple-darwin18.2.0, NS appkit-1671.20
Version 10.14.3 (Build 18D109)) of 2019-09-02


Thank you

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

bug#40576: call-process-region does not accept nil as first argument

[Philipp Stephani]

Am So., 12. Apr. 2020 um 17:44 Uhr schrieb Pietro Giorgianni
<giorgian@gmail.com>:
>
> Hi,
>
> According to the documentation of call-process-region,
>   If START is nil, that means to use the entire buffer contents; END is
> ignored.
>
> But when I run:
> (call-process-region nil nil "/bin/cat" t (current-buffer))
> I get:
> Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p nil)
>   call-process-region(nil nil "/bin/cat" t #<buffer *scratch*>)
>   eval((call-process-region nil nil "/bin/cat" t (current-buffer)) nil)
>   elisp--eval-last-sexp(t)
>   eval-last-sexp(t)
>   eval-print-last-sexp(nil)
>   funcall-interactively(eval-print-last-sexp nil)
>   call-interactively(eval-print-last-sexp nil nil)
>   command-execute(eval-print-last-sexp)
>
> If, instead, I run:
> (call-process-region (point-min) (point-max) "/bin/cat" t (current-buffer))
>
> It works.
>
> Am I interpreting the documentation wrong?

Nope, looks like a genuine bug (that happens only if DELETE is non-nil).

bug#40576: call-process-region does not accept nil as first argument

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 12 Apr 2020 18:01:40 +0200
> Cc: 40576@debbugs.gnu.org
> 
> > According to the documentation of call-process-region,
> >   If START is nil, that means to use the entire buffer contents; END is
> > ignored.
> >
> > But when I run:
> > (call-process-region nil nil "/bin/cat" t (current-buffer))
> > I get:
> > Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p nil)
> >   call-process-region(nil nil "/bin/cat" t #<buffer *scratch*>)
> >   eval((call-process-region nil nil "/bin/cat" t (current-buffer)) nil)
> >   elisp--eval-last-sexp(t)
> >   eval-last-sexp(t)
> >   eval-print-last-sexp(nil)
> >   funcall-interactively(eval-print-last-sexp nil)
> >   call-interactively(eval-print-last-sexp nil nil)
> >   command-execute(eval-print-last-sexp)
> >
> > If, instead, I run:
> > (call-process-region (point-min) (point-max) "/bin/cat" t (current-buffer))
> >
> > It works.
> >
> > Am I interpreting the documentation wrong?
> 
> Nope, looks like a genuine bug (that happens only if DELETE is non-nil).

Right.  But there's more here than meets the eye, because the change
after which we started advertising the special meaning of nil for
START exposed a problem: write_region, called from create_temp_file,
has special meaning for START = nil: it widens the buffer and writes
the entire buffer contents to the temp file.  Which isn't right when
write_region is called from call-process-region, as it allows access
to inaccessible portion of the buffer, something we shouldn't do.

So I propose the patch below to fix this bug on the master branch.
Any objections?

diff --git a/src/callproc.c b/src/callproc.c
index 8883415..7f495a3 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -1039,8 +1039,8 @@ DEFUN ("call-process-region", Fcall_process_region, Scall_process_region,
 
 START and END are normally buffer positions specifying the part of the
 buffer to send to the process.
-If START is nil, that means to use the entire buffer contents; END is
-ignored.
+If START is nil, that means to use the entire accessible part of the
+buffer; END is ignored.
 If START is a string, then send that string to the process
 instead of any buffer contents; END is ignored.
 The remaining arguments are optional.
@@ -1087,6 +1087,14 @@ t (mix it with ordinary output), or a file name string.
       empty_input = XFIXNUM (start) == XFIXNUM (end);
     }
 
+  if (NILP (start))
+    {
+      XSETFASTINT (start, BEGV);
+      args[0] = start;
+      XSETFASTINT (end, ZV);
+      args[1] = end;
+    }
+
   if (!empty_input)
     fd = create_temp_file (nargs, args, &infile);
   else

bug#40576: call-process-region does not accept nil as first argument

[Philipp Stephani]

Am So., 12. Apr. 2020 um 18:21 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sun, 12 Apr 2020 18:01:40 +0200
> > Cc: 40576@debbugs.gnu.org
> >
> > > According to the documentation of call-process-region,
> > >   If START is nil, that means to use the entire buffer contents; END is
> > > ignored.
> > >
> > > But when I run:
> > > (call-process-region nil nil "/bin/cat" t (current-buffer))
> > > I get:
> > > Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p nil)
> > >   call-process-region(nil nil "/bin/cat" t #<buffer *scratch*>)
> > >   eval((call-process-region nil nil "/bin/cat" t (current-buffer)) nil)
> > >   elisp--eval-last-sexp(t)
> > >   eval-last-sexp(t)
> > >   eval-print-last-sexp(nil)
> > >   funcall-interactively(eval-print-last-sexp nil)
> > >   call-interactively(eval-print-last-sexp nil nil)
> > >   command-execute(eval-print-last-sexp)
> > >
> > > If, instead, I run:
> > > (call-process-region (point-min) (point-max) "/bin/cat" t (current-buffer))
> > >
> > > It works.
> > >
> > > Am I interpreting the documentation wrong?
> >
> > Nope, looks like a genuine bug (that happens only if DELETE is non-nil).
>
> Right.  But there's more here than meets the eye, because the change
> after which we started advertising the special meaning of nil for
> START exposed a problem: write_region, called from create_temp_file,
> has special meaning for START = nil: it widens the buffer and writes
> the entire buffer contents to the temp file.  Which isn't right when
> write_region is called from call-process-region, as it allows access
> to inaccessible portion of the buffer, something we shouldn't do.

I think that's pretty much intentional. The documentation says

"If START is nil, that means to use the entire buffer contents"

It specifically doesn't say to only use the accessible portion of the
buffer. Given that this behavior probably has been in place since
commit 561cb8e159e7eff7a6487a45a1cfab47ba456030 from 1994, it would be
rather unwise to introduce such a breaking change.

bug#40576: call-process-region does not accept nil as first argument

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 12 Apr 2020 18:44:58 +0200
> Cc: Pietro Giorgianni <giorgian@gmail.com>, 40576@debbugs.gnu.org
> 
> > Right.  But there's more here than meets the eye, because the change
> > after which we started advertising the special meaning of nil for
> > START exposed a problem: write_region, called from create_temp_file,
> > has special meaning for START = nil: it widens the buffer and writes
> > the entire buffer contents to the temp file.  Which isn't right when
> > write_region is called from call-process-region, as it allows access
> > to inaccessible portion of the buffer, something we shouldn't do.
> 
> I think that's pretty much intentional. The documentation says
> 
> "If START is nil, that means to use the entire buffer contents"

That sentence is an addition made in 2016, AFAICT.

> It specifically doesn't say to only use the accessible portion of the
> buffer. Given that this behavior probably has been in place since
> commit 561cb8e159e7eff7a6487a45a1cfab47ba456030 from 1994, it would be
> rather unwise to introduce such a breaking change.

But then START = nil would work, whereas START = 2 will signal an
error if the buffer is narrowed.  Does that make sense?

bug#40576: call-process-region does not accept nil as first argument

[Philipp Stephani]

Am So., 12. Apr. 2020 um 18:01 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am So., 12. Apr. 2020 um 17:44 Uhr schrieb Pietro Giorgianni
> <giorgian@gmail.com>:
> >
> > Hi,
> >
> > According to the documentation of call-process-region,
> >   If START is nil, that means to use the entire buffer contents; END is
> > ignored.
> >
> > But when I run:
> > (call-process-region nil nil "/bin/cat" t (current-buffer))
> > I get:
> > Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p nil)
> >   call-process-region(nil nil "/bin/cat" t #<buffer *scratch*>)
> >   eval((call-process-region nil nil "/bin/cat" t (current-buffer)) nil)
> >   elisp--eval-last-sexp(t)
> >   eval-last-sexp(t)
> >   eval-print-last-sexp(nil)
> >   funcall-interactively(eval-print-last-sexp nil)
> >   call-interactively(eval-print-last-sexp nil nil)
> >   command-execute(eval-print-last-sexp)
> >
> > If, instead, I run:
> > (call-process-region (point-min) (point-max) "/bin/cat" t (current-buffer))
> >
> > It works.
> >
> > Am I interpreting the documentation wrong?
>
> Nope, looks like a genuine bug (that happens only if DELETE is non-nil).

I've now fixed this on master (commit 42306747d8).

bug#40576: call-process-region does not accept nil as first argument

[Philipp Stephani]

Am So., 12. Apr. 2020 um 18:44 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am So., 12. Apr. 2020 um 18:21 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > > From: Philipp Stephani <p.stephani2@gmail.com>
> > > Date: Sun, 12 Apr 2020 18:01:40 +0200
> > > Cc: 40576@debbugs.gnu.org
> > >
> > > > According to the documentation of call-process-region,
> > > >   If START is nil, that means to use the entire buffer contents; END is
> > > > ignored.
> > > >
> > > > But when I run:
> > > > (call-process-region nil nil "/bin/cat" t (current-buffer))
> > > > I get:
> > > > Debugger entered--Lisp error: (wrong-type-argument integer-or-marker-p nil)
> > > >   call-process-region(nil nil "/bin/cat" t #<buffer *scratch*>)
> > > >   eval((call-process-region nil nil "/bin/cat" t (current-buffer)) nil)
> > > >   elisp--eval-last-sexp(t)
> > > >   eval-last-sexp(t)
> > > >   eval-print-last-sexp(nil)
> > > >   funcall-interactively(eval-print-last-sexp nil)
> > > >   call-interactively(eval-print-last-sexp nil nil)
> > > >   command-execute(eval-print-last-sexp)
> > > >
> > > > If, instead, I run:
> > > > (call-process-region (point-min) (point-max) "/bin/cat" t (current-buffer))
> > > >
> > > > It works.
> > > >
> > > > Am I interpreting the documentation wrong?
> > >
> > > Nope, looks like a genuine bug (that happens only if DELETE is non-nil).
> >
> > Right.  But there's more here than meets the eye, because the change
> > after which we started advertising the special meaning of nil for
> > START exposed a problem: write_region, called from create_temp_file,
> > has special meaning for START = nil: it widens the buffer and writes
> > the entire buffer contents to the temp file.  Which isn't right when
> > write_region is called from call-process-region, as it allows access
> > to inaccessible portion of the buffer, something we shouldn't do.
>
> I think that's pretty much intentional. The documentation says
>
> "If START is nil, that means to use the entire buffer contents"
>
> It specifically doesn't say to only use the accessible portion of the
> buffer. Given that this behavior probably has been in place since
> commit 561cb8e159e7eff7a6487a45a1cfab47ba456030 from 1994, it would be
> rather unwise to introduce such a breaking change.

While the behavior of call-process-region and write-region is
unfortunate in this respect, I think it's way too late to change it
now. We should rather explicitly call out in the docstrings that
buffer restrictions are ignored.

bug#40576: call-process-region does not accept nil as first argument

[Philipp Stephani]

Am So., 12. Apr. 2020 um 19:07 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sun, 12 Apr 2020 18:44:58 +0200
> > Cc: Pietro Giorgianni <giorgian@gmail.com>, 40576@debbugs.gnu.org
> >
> > > Right.  But there's more here than meets the eye, because the change
> > > after which we started advertising the special meaning of nil for
> > > START exposed a problem: write_region, called from create_temp_file,
> > > has special meaning for START = nil: it widens the buffer and writes
> > > the entire buffer contents to the temp file.  Which isn't right when
> > > write_region is called from call-process-region, as it allows access
> > > to inaccessible portion of the buffer, something we shouldn't do.
> >
> > I think that's pretty much intentional. The documentation says
> >
> > "If START is nil, that means to use the entire buffer contents"
>
> That sentence is an addition made in 2016, AFAICT.

Sure, but the behavior has been around for so long that it's very
likely somebody already relies on it.

>
> > It specifically doesn't say to only use the accessible portion of the
> > buffer. Given that this behavior probably has been in place since
> > commit 561cb8e159e7eff7a6487a45a1cfab47ba456030 from 1994, it would be
> > rather unwise to introduce such a breaking change.
>
> But then START = nil would work, whereas START = 2 will signal an
> error if the buffer is narrowed.  Does that make sense?

No, absolutely not. But I think it's too late to change it.

bug#40576: call-process-region does not accept nil as first argument

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 12 Apr 2020 19:07:59 +0200
> Cc: Pietro Giorgianni <giorgian@gmail.com>, 40576@debbugs.gnu.org
> 
> We should rather explicitly call out in the docstrings that buffer
> restrictions are ignored.

But the restrictions aren't ignored.  They are only ignored if START
is nil.  That makes no sense.

bug#40576: call-process-region does not accept nil as first argument

[Philipp Stephani]

Am So., 12. Apr. 2020 um 19:25 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Sun, 12 Apr 2020 19:07:59 +0200
> > Cc: Pietro Giorgianni <giorgian@gmail.com>, 40576@debbugs.gnu.org
> >
> > We should rather explicitly call out in the docstrings that buffer
> > restrictions are ignored.
>
> But the restrictions aren't ignored.  They are only ignored if START
> is nil.  That makes no sense.

I completely agree that it doesn't make sense. I'm just saying that
the behavior has been there for so long that we realistically can't
change it any more.

bug#40576: call-process-region does not accept nil as first argument

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sun, 12 Apr 2020 19:37:07 +0200
> Cc: Pietro Giorgianni <giorgian@gmail.com>, 40576@debbugs.gnu.org
> 
> > But the restrictions aren't ignored.  They are only ignored if START
> > is nil.  That makes no sense.
> 
> I completely agree that it doesn't make sense. I'm just saying that
> the behavior has been there for so long that we realistically can't
> change it any more.

I think we can change it, but I won't argue further.