bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Alan Mackenzie]

Hello, Emacs.

Emacs-28, emacs started with site-start.el and .emacs.

On calling

    (parse-partial-sexp 19 18 nil nil s)

Emacs surely ought to signal an error, since 18 < 19.  It doesn't,
though.  It leaves point at a random position and returns

    (0 nil nil nil nil nil 0 nil nil nil nil)

This is a bug.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> On calling
>
>     (parse-partial-sexp 19 18 nil nil s)

(What's `s' here?)

> Emacs surely ought to signal an error, since 18 < 19.

It's common for Emacs functions that take a region to not care about
whether to is larger than from or not.  The

  validate_region (&from, &to);

function fixes it up.

> It doesn't,
> though.  It leaves point at a random position and returns
>
>     (0 nil nil nil nil nil 0 nil nil nil nil)

For me it leaves point at 19.

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

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Alan Mackenzie]

Hello, Lars.

On Sun, Aug 08, 2021 at 20:14:25 +0200, Lars Ingebrigtsen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > On calling
> >
> >     (parse-partial-sexp 19 18 nil nil s)

> (What's `s' here?)

Sorry, should have said.  It was a syntax state at position 19,
something like:

    (0 nil 8 34 nil nil 0 nil 13 nil nil)

representing a "normal" string starting at position 13.

> > Emacs surely ought to signal an error, since 18 < 19.

> It's common for Emacs functions that take a region to not care about
> whether to is larger than from or not.

parse-partial-sexp doesn't work on a region.  It works with a starting
position and ending position, which are not interchangeable.  For
example, the status s (above) is the status at 19, not at 18.

> The

>   validate_region (&from, &to);

> function fixes it up.

It doesn't fix it up, it messes it up.

> > It doesn't,
> > though.  It leaves point at a random position and returns
> >
> >     (0 nil nil nil nil nil 0 nil nil nil nil)

> For me it leaves point at 19.

I think it may have done that for me, too, though perhaps not every
time.

Getting back on topic, I think there are no legitimate uses for (> start
limit), and every such call is a bug.  It would seem such a call
discards the input state argument, which would be a bug in itself if the
whole thing weren't a bug.

It's obvious to me that Emacs should throw an error in these
circumstances.  You seem to be disagreeing, or at least to be unsure.
One data point is it took me over an hour's time to track it down, time
that could have been better spent if Emacs had thrown an error.

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

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> parse-partial-sexp doesn't work on a region.  It works with a starting
> position and ending position, which are not interchangeable.

Well...  they are interchangeable now, as they are in many functions in
Emacs.

> It's obvious to me that Emacs should throw an error in these
> circumstances.  You seem to be disagreeing, or at least to be unsure.

Making this signal an error wouldn't be backwards-compatible, so I think
that's a no go.

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

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Alan Mackenzie]

Hello, Lars.

On Mon, Aug 09, 2021 at 14:42:21 +0200, Lars Ingebrigtsen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > parse-partial-sexp doesn't work on a region.  It works with a
> > starting position and ending position, which are not
> > interchangeable.

> Well...  they are interchangeable now, as they are in many functions
> in Emacs.

I can't see any use whatsoever for reversing START and LIMIT.  Can you?
I would be extremely surprised if any hacker had _ever_ intentionally
used this "facility".

We definitely have a bug here.  The documentation in the elisp manual
says that the scanning is done "starting at START".  You're saying it's
perfectly OK to start scanning at "LIMIT"?  This violates the doc.

In other words, you're sort of saying that it's the documentation at
fault, not the function.

> > It's obvious to me that Emacs should throw an error in these
> > circumstances.  You seem to be disagreeing, or at least to be
> > unsure.

> Making this signal an error wouldn't be backwards-compatible, so I
> think that's a no go.

Fixing _any_ bug leads to "backwards-incompatibility", that is, if one
is determined to regard buggy behaviour as something to be preserved.

This whole discussion feels a bit surrealistic.  Is it not obvious that
(parse-partial-sexp 19 18) ought to throw an error?

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

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> We definitely have a bug here.  The documentation in the elisp manual
> says that the scanning is done "starting at START".  You're saying it's
> perfectly OK to start scanning at "LIMIT"?  This violates the doc.

This is what all function like this say.  To take patient zero --
narrow-to-region:

---
When calling from Lisp, pass two arguments START and END:
positions (integers or markers) bounding the text that should
remain visible.
---

Nothing here about allowing END to come before START, but it does allow
that, and so do most (all?) similar commands.

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

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Tue, 10 Aug 2021 15:50:49 +0200
> Cc: 49944@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
> 
> Alan Mackenzie <acm@muc.de> writes:
> 
> > We definitely have a bug here.  The documentation in the elisp manual
> > says that the scanning is done "starting at START".  You're saying it's
> > perfectly OK to start scanning at "LIMIT"?  This violates the doc.
> 
> This is what all function like this say.  To take patient zero --
> narrow-to-region:
> 
> ---
> When calling from Lisp, pass two arguments START and END:
> positions (integers or markers) bounding the text that should
> remain visible.
> ---
> 
> Nothing here about allowing END to come before START, but it does allow
> that, and so do most (all?) similar commands.

Indeed, if the results are predictable, I see no reason not to support
START and END in any order, as we do in many places.

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > We definitely have a bug here.  The documentation in the elisp manual
>> > says that the scanning is done "starting at START".  You're saying it's
>> > perfectly OK to start scanning at "LIMIT"?  This violates the doc.
>> 
>> This is what all function like this say.  To take patient zero --
>> narrow-to-region:
>> 
>> ---
>> When calling from Lisp, pass two arguments START and END:
>> positions (integers or markers) bounding the text that should
>> remain visible.
>> ---
>> 
>> Nothing here about allowing END to come before START, but it does allow
>> that, and so do most (all?) similar commands.
>
> Indeed, if the results are predictable, I see no reason not to support
> START and END in any order, as we do in many places.

I think the case of `parse-partial-sexp` is different because it
receives the argument OLDSTATE which provides the parser's state which
should apply at START.  If START and END are swapped you'll generally
get an incorrect result.

So, I don't care if we signal an error when START > END or if we "do
nothing" (as is customary in some language's `for` loops), but we should
always consider that OLDSTATE is the state that belongs with START and
hence we can't start parsing at END towards START because we don't know
what parsing state to use at END.


        Stefan

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I think the case of `parse-partial-sexp` is different because it
> receives the argument OLDSTATE which provides the parser's state which
> should apply at START.  If START and END are swapped you'll generally
> get an incorrect result.
>
> So, I don't care if we signal an error when START > END or if we "do
> nothing" (as is customary in some language's `for` loops), but we should
> always consider that OLDSTATE is the state that belongs with START and
> hence we can't start parsing at END towards START because we don't know
> what parsing state to use at END.

Ah; thanks for the explanation.

But I guess you'll get incorrect results if you pass in any OLDSTATE
that doesn't belong to START, not just when START and END are swapped?
It's just that if START/END obviously wrong (i.e., END is smaller than
START), then the function here "helpfully" swaps them and things get
even more confusing than they would normally be if you pass in a wrong
OLDSTATE (or wrong START).

So perhaps signalling an error here is the correct thing after all?  (Or
just not doing any swapping.)

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

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2021-08-10 16:54:59] wrote:
> But I guess you'll get incorrect results if you pass in any OLDSTATE
> that doesn't belong to START, not just when START and END are swapped?

If the caller needs to know whether swapping will take place in order to
pass the right OLDSTATE, then it defeats the purpose of swapping (which
is presumably to make life easier for the caller).

> So perhaps signalling an error here is the correct thing after all?
> (Or just not doing any swapping.)

I don't have an opinion on signaling an error or not, but swapping is
a bad idea here IMO.


        Stefan

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  acm@muc.de,  49944@debbugs.gnu.org
> Date: Tue, 10 Aug 2021 16:54:59 +0200
> 
> > So, I don't care if we signal an error when START > END or if we "do
> > nothing" (as is customary in some language's `for` loops), but we should
> > always consider that OLDSTATE is the state that belongs with START and
> > hence we can't start parsing at END towards START because we don't know
> > what parsing state to use at END.
> 
> Ah; thanks for the explanation.
> 
> But I guess you'll get incorrect results if you pass in any OLDSTATE
> that doesn't belong to START, not just when START and END are swapped?
> It's just that if START/END obviously wrong (i.e., END is smaller than
> START), then the function here "helpfully" swaps them and things get
> even more confusing than they would normally be if you pass in a wrong
> OLDSTATE (or wrong START).
> 
> So perhaps signalling an error here is the correct thing after all?  (Or
> just not doing any swapping.)

The reordering is the side effect of calling validate_region, so we'd
need to expend extra effort NOT to reorder START and END.

How about just documenting that OLDSTATE should be the state at START,
and that's it?

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> The reordering is the side effect of calling validate_region, so we'd
> need to expend extra effort NOT to reorder START and END.
>
> How about just documenting that OLDSTATE should be the state at START,
> and that's it?

The current use of `validate_region` means that OLDSTATE will be used as
the state at "either FROM or TO, whichever is smaller".

So I don't understand what it is you're proposing: if we document 
that OLDSTATE should be the state at FROM, then we need to change the
code so that it never swaps FROM and TO.  And if we don't want to change
the code, then we should document that OLDSTATE will be used as the
state at TO when TO is smaller than FROM.


        Stefan

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  acm@muc.de,  49944@debbugs.gnu.org
> Date: Tue, 10 Aug 2021 11:44:55 -0400
> 
> > The reordering is the side effect of calling validate_region, so we'd
> > need to expend extra effort NOT to reorder START and END.
> >
> > How about just documenting that OLDSTATE should be the state at START,
> > and that's it?
> 
> The current use of `validate_region` means that OLDSTATE will be used as
> the state at "either FROM or TO, whichever is smaller".
> 
> So I don't understand what it is you're proposing: if we document 
> that OLDSTATE should be the state at FROM, then we need to change the
> code so that it never swaps FROM and TO.  And if we don't want to change
> the code, then we should document that OLDSTATE will be used as the
> state at TO when TO is smaller than FROM.

The latter.  By documenting the expectations of the function, we leave
it to the callers to make sure their code works as intended.

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> So I don't understand what it is you're proposing: if we document 
>> that OLDSTATE should be the state at FROM, then we need to change the
>> code so that it never swaps FROM and TO.  And if we don't want to change
>> the code, then we should document that OLDSTATE will be used as the
>> state at TO when TO is smaller than FROM.
>
> The latter.  By documenting the expectations of the function, we leave
> it to the callers to make sure their code works as intended.

I think that sounds like a very complicated interface -- it would be
better if we didn't swap.

However, there may well be code out there today that calls the function
with OLDSTATE matching the point passed in by END, which is then swapped
with START (because it's smaller), and things work by accident.

Probably unlikely, but to detect this we'd have to signal an error, like
Alan suggested originally.  So...  perhaps we should just do that?
I.e., signal an error if END is smaller than START?  (And we could fix
up the swapping for clarity, but it wouldn't be necessary.)

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

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Lars Ingebrigtsen]

Alan Mackenzie <acm@muc.de> writes:

> Emacs-28, emacs started with site-start.el and .emacs.
>
> On calling
>
>     (parse-partial-sexp 19 18 nil nil s)
>
> Emacs surely ought to signal an error, since 18 < 19.  It doesn't,
> though.  It leaves point at a random position and returns
>
>     (0 nil nil nil nil nil 0 nil nil nil nil)
>
> This is a bug.

I've now made this change (and the doc string clarification discussed).
There are no test failures after the change, and I'm not getting any
errors when running it normally, either.

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

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Alan Mackenzie]

Hello, Lars.

On Sat, Aug 21, 2021 at 15:24:20 +0200, Lars Ingebrigtsen wrote:
> Alan Mackenzie <acm@muc.de> writes:

> > Emacs-28, emacs started with site-start.el and .emacs.

> > On calling

> >     (parse-partial-sexp 19 18 nil nil s)

> > Emacs surely ought to signal an error, since 18 < 19.  It doesn't,
> > though.  It leaves point at a random position and returns

> >     (0 nil nil nil nil nil 0 nil nil nil nil)

> > This is a bug.

> I've now made this change (and the doc string clarification discussed).
> There are no test failures after the change, and I'm not getting any
> errors when running it normally, either.

Thanks!

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

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Lars Ingebrigtsen [2021-08-21 15:24 +0200] wrote:

> Alan Mackenzie <acm@muc.de> writes:
>
>> Emacs-28, emacs started with site-start.el and .emacs.
>>
>> On calling
>>
>>     (parse-partial-sexp 19 18 nil nil s)
>>
>> Emacs surely ought to signal an error, since 18 < 19.  It doesn't,
>> though.  It leaves point at a random position and returns
>>
>>     (0 nil nil nil nil nil 0 nil nil nil nil)
>>
>> This is a bug.
>
> I've now made this change (and the doc string clarification discussed).
> There are no test failures after the change, and I'm not getting any
> errors when running it normally, either.

Found one :).

0. emacs -Q
1. M-x toggle-debug-on-error RET
2. M-x ielm RET RET

Debugger entered--Lisp error:
    (error "End position should be larger than start position.")
  parse-partial-sexp(#<marker at 64 in *ielm*> 64)
  ielm-return()
  funcall-interactively(ielm-return)
  call-interactively(ielm-return nil nil)
  command-execute(ielm-return)

One option is to replace XFIXNUM with fix_position, which is what the
subsequent validate_region also does.  Another option is to
CHECK_FIXNUM_COERCE_MARKER before the comparison.  Any preference?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-recent-parse-partial-sexp-argument-validation.patch --]
[-- Type: text/x-diff, Size: 1069 bytes --]

From 34c87c83fa86263da863da6c0a920c746b8af02b Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sat, 21 Aug 2021 22:55:58 +0100
Subject: [PATCH] Fix recent parse-partial-sexp argument validation

* src/syntax.c (parse-partial-sexp): Also handle markers as
arguments (bug#49944).  Tweak error message to follow conventions in
"(elisp) Signaling Errors".
---
 src/syntax.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/syntax.c b/src/syntax.c
index adc0da730e..057a4c3b1f 100644
--- a/src/syntax.c
+++ b/src/syntax.c
@@ -3595,8 +3595,8 @@ DEFUN ("parse-partial-sexp", Fparse_partial_sexp, Sparse_partial_sexp, 2, 6, 0,
   else
     target = TYPE_MINIMUM (EMACS_INT);	/* We won't reach this depth.  */
 
-  if (XFIXNUM (to) < XFIXNUM (from))
-    error ("End position should be larger than start position.");
+  if (fix_position (to) < fix_position (from))
+    error ("End position is smaller than start position");
 
   validate_region (&from, &to);
   internalize_parse_state (oldstate, &state);
-- 
2.32.0


[-- Attachment #3: Type: text/plain, Size: 20 bytes --]


Thanks,

-- 
Basil

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Alan Mackenzie]

Hello, Basil.

On Sat, Aug 21, 2021 at 23:11:55 +0100, Basil L. Contovounesios wrote:
> Lars Ingebrigtsen [2021-08-21 15:24 +0200] wrote:

[ .... ]

> > I've now made this change (and the doc string clarification discussed).
> > There are no test failures after the change, and I'm not getting any
> > errors when running it normally, either.

> Found one :).

> 0. emacs -Q
> 1. M-x toggle-debug-on-error RET
> 2. M-x ielm RET RET

> Debugger entered--Lisp error:
>     (error "End position should be larger than start position.")
>   parse-partial-sexp(#<marker at 64 in *ielm*> 64)
>   ielm-return()
>   funcall-interactively(ielm-return)
>   call-interactively(ielm-return nil nil)
>   command-execute(ielm-return)

> One option is to replace XFIXNUM with fix_position, which is what the
> subsequent validate_region also does.  Another option is to
> CHECK_FIXNUM_COERCE_MARKER before the comparison.  Any preference?

I think the use of fix_position is the best here, and your patch is
right.  Importantly, it corrects the error message, such that it does not
give the impression that End position == Start position would signal an
error.

> >From 34c87c83fa86263da863da6c0a920c746b8af02b Mon Sep 17 00:00:00 2001
> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Sat, 21 Aug 2021 22:55:58 +0100
> Subject: [PATCH] Fix recent parse-partial-sexp argument validation

> * src/syntax.c (parse-partial-sexp): Also handle markers as
> arguments (bug#49944).  Tweak error message to follow conventions in
> "(elisp) Signaling Errors".
> ---
>  src/syntax.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

> diff --git a/src/syntax.c b/src/syntax.c
> index adc0da730e..057a4c3b1f 100644
> --- a/src/syntax.c
> +++ b/src/syntax.c
> @@ -3595,8 +3595,8 @@ DEFUN ("parse-partial-sexp", Fparse_partial_sexp, Sparse_partial_sexp, 2, 6, 0,
>    else
>      target = TYPE_MINIMUM (EMACS_INT);	/* We won't reach this depth.  */

> -  if (XFIXNUM (to) < XFIXNUM (from))
> -    error ("End position should be larger than start position.");
> +  if (fix_position (to) < fix_position (from))
> +    error ("End position is smaller than start position");

>    validate_region (&from, &to);
>    internalize_parse_state (oldstate, &state);
> -- 
> 2.32.0



> Thanks,

> -- 
> Basil

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Lars Ingebrigtsen]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> -  if (XFIXNUM (to) < XFIXNUM (from))
> -    error ("End position should be larger than start position.");
> +  if (fix_position (to) < fix_position (from))
> +    error ("End position is smaller than start position");

Looks good to me.

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

bug#49944: parse-partial-sexp fails to signal an error when (> START LIMIT).

[Basil L. Contovounesios via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Lars Ingebrigtsen [2021-08-22 17:02 +0200] wrote:

> Looks good to me.

Thanks Alan and Lars, pushed.

Fix recent parse-partial-sexp argument validation
ff2124d297 2021-08-22 23:52:23 +0100
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=ff2124d2979ee9ba5b8e22147fa8ccaa00e2cc4f

-- 
Basil