bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Simen Heggestøyl]

Hi.

Commit 460d5fd971489ba7b573d71a94cdaac2f9f1a767 introduced a new
function `count-sentences', which is nice, but it also silently changed
the return value of `forward-sentence' (and thus also
`backward-sentence') from returning the new value of point to returning
t/nil based on whether point moved or not.

I just discovered this because it caused one of my packages to break,
since it relies on the previous style return values from
`forward-sentence'/`backward-sentence'. It would be easy enough fix on
my part, but I'm wondering whether this change was intended, and if so,
whether it should be documented somewhere so package authors can adapt
more easily?

Or could perhaps the change in return values from
`forward-sentence'/`backward-sentence' be reverted to old behavior, and
the counting logic be left to `count-sentences' instead?

-- Simen

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

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

Simen Heggestøyl <simenheg@runbox.com> writes:

[...]

> Or could perhaps the change in return values from
> `forward-sentence'/`backward-sentence' be reverted to old behavior, and
> the counting logic be left to `count-sentences' instead?

Hi Simen,

Yes maybe I could change `count-sentences' a bit to do this.  What
others think of such a change?
-- 
Manuel Giraud

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Daniel Martín via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> Simen Heggestøyl <simenheg@runbox.com> writes:
>
> [...]
>
>> Or could perhaps the change in return values from
>> `forward-sentence'/`backward-sentence' be reverted to old behavior, and
>> the counting logic be left to `count-sentences' instead?
>
> Hi Simen,
>
> Yes maybe I could change `count-sentences' a bit to do this.  What
> others think of such a change?

I'm all for reverting to the traditional return values of the sentence
commands, specially if they don't complicate the new command much.  The
return value was not documented, but Hyrum's Law
(https://www.hyrumslaw.com/) tells us that there is an unknown amount of
code out there that depends on the traditional return value, anyway.

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Eli Zaretskii]

> Cc: simenheg@runbox.com, manuel@ledu-giraud.fr
> Date: Tue, 07 Mar 2023 12:03:45 +0100
> From:  Daniel Martín via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Manuel Giraud via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs@gnu.org> writes:
> 
> > Simen Heggestøyl <simenheg@runbox.com> writes:
> >
> > [...]
> >
> >> Or could perhaps the change in return values from
> >> `forward-sentence'/`backward-sentence' be reverted to old behavior, and
> >> the counting logic be left to `count-sentences' instead?
> >
> > Hi Simen,
> >
> > Yes maybe I could change `count-sentences' a bit to do this.  What
> > others think of such a change?
> 
> I'm all for reverting to the traditional return values of the sentence
> commands, specially if they don't complicate the new command much.  The
> return value was not documented, but Hyrum's Law
> (https://www.hyrumslaw.com/) tells us that there is an unknown amount of
> code out there that depends on the traditional return value, anyway.

Agreed.

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

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

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

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> I'm all for reverting to the traditional return values of the sentence
>> commands, specially if they don't complicate the new command much.  The
>> return value was not documented, but Hyrum's Law
>> (https://www.hyrumslaw.com/) tells us that there is an unknown amount of
>> code out there that depends on the traditional return value, anyway.
>
> Agreed.

Ok.  I've tried to fix this.  I've done some testing, it works for me™.
If it is ok, I think it should go in emacs-29 (if this is still
possible).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Revert-forward-sentence-default-function-to-return-t.patch --]
[-- Type: text/x-patch, Size: 1724 bytes --]

From 965a803d571ab78f2ca37b4ac01b51a6a0bc7df7 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 7 Mar 2023 16:46:02 +0100
Subject: [PATCH] Revert 'forward-sentence-default-function' to return the
 point

* lisp/textmodes/paragraphs.el
(forward-sentence-default-function): Revert to return the point
position.
(count-sentences): Adapt to this change.
---
 lisp/textmodes/paragraphs.el | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lisp/textmodes/paragraphs.el b/lisp/textmodes/paragraphs.el
index bf249fdcdfb..70ae0f7daeb 100644
--- a/lisp/textmodes/paragraphs.el
+++ b/lisp/textmodes/paragraphs.el
@@ -476,8 +476,7 @@ forward-sentence-default-function
 	    (skip-chars-backward " \t\n")
 	  (goto-char par-end)))
       (setq arg (1- arg)))
-    (let ((npoint (constrain-to-field nil opoint t)))
-      (not (= npoint opoint)))))
+    (constrain-to-field nil opoint t)))
 
 (defvar forward-sentence-function #'forward-sentence-default-function
   "Function to be used to calculate sentence movements.
@@ -499,8 +498,13 @@ count-sentences
       (save-restriction
         (narrow-to-region start end)
         (goto-char (point-min))
-	(while (ignore-errors (forward-sentence))
-	  (setq sentences (1+ sentences)))
+        (let* ((prev (point))
+               (next (forward-sentence)))
+	  (while (and (not (null next))
+                      (not (= prev next)))
+            (setq prev next
+                  next (ignore-errors (forward-sentence))
+                  sentences (1+ sentences))))
         ;; Remove last possibly empty sentence
         (when (/= (skip-chars-backward " \t\n") 0)
           (setq sentences (1- sentences)))
-- 
2.39.1


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

-- 
Manuel Giraud

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Daniel Martín <mardani29@yahoo.es>,
>   62027@debbugs.gnu.org,
>   simenheg@runbox.com
> Date: Tue, 07 Mar 2023 17:07:14 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> >> I'm all for reverting to the traditional return values of the sentence
> >> commands, specially if they don't complicate the new command much.  The
> >> return value was not documented, but Hyrum's Law
> >> (https://www.hyrumslaw.com/) tells us that there is an unknown amount of
> >> code out there that depends on the traditional return value, anyway.
> >
> > Agreed.
> 
> Ok.  I've tried to fix this.  I've done some testing, it works for me™.
> If it is ok, I think it should go in emacs-29 (if this is still
> possible).

Yes, please.  This command is new in Emacs 29, so it needs to be
corrected there.

> Subject: [PATCH] Revert 'forward-sentence-default-function' to return the
>  point
> 
> * lisp/textmodes/paragraphs.el
> (forward-sentence-default-function): Revert to return the point
> position.
> (count-sentences): Adapt to this change.

Don't forget to mention the bug number in the commit log message when
you install this.

Thanks.

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

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

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> Don't forget to mention the bug number in the commit log message when
> you install this.

I can add the bug number if you want but I cannot install on Emacs
repository.  Could you do that for me?
-- 
Manuel Giraud

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: mardani29@yahoo.es,  62027@debbugs.gnu.org,  simenheg@runbox.com
> Date: Tue, 07 Mar 2023 17:42:46 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> > Don't forget to mention the bug number in the commit log message when
> > you install this.
> 
> I can add the bug number if you want but I cannot install on Emacs
> repository.  Could you do that for me?

I tried, but it doesn't apply.  Did you produce the patch from the
emacs-29 branch or from some other branch?

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

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

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

Eli Zaretskii <eliz@gnu.org> writes:

[...]

> I tried, but it doesn't apply.  Did you produce the patch from the
> emacs-29 branch or from some other branch?

Sorry, I produced it on master.  Here is the one for emacs-29.  Will
this appear on master at a later time?
-- 
Manuel Giraud

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Revert-forward-sentence-default-function-to-return-t.patch --]
[-- Type: text/x-patch, Size: 1692 bytes --]

From c1cfd5e99c5227e9c75fc6b3dbb50e60e44c12d0 Mon Sep 17 00:00:00 2001
From: Manuel Giraud <manuel@ledu-giraud.fr>
Date: Tue, 7 Mar 2023 20:03:53 +0100
Subject: [PATCH] Revert 'forward-sentence-default-function' to return the
 point (bug#62027)

* lisp/textmodes/paragraphs.el
(forward-sentence-default-function): Revert to return the point
position.
(count-sentences): Adapt to this change.
---
 lisp/textmodes/paragraphs.el | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lisp/textmodes/paragraphs.el b/lisp/textmodes/paragraphs.el
index 73abb155aaa..a9e28a3275b 100644
--- a/lisp/textmodes/paragraphs.el
+++ b/lisp/textmodes/paragraphs.el
@@ -477,8 +477,7 @@ forward-sentence
 	    (skip-chars-backward " \t\n")
 	  (goto-char par-end)))
       (setq arg (1- arg)))
-    (let ((npoint (constrain-to-field nil opoint t)))
-      (not (= npoint opoint)))))
+    (constrain-to-field nil opoint t)))
 
 (defun count-sentences (start end)
   "Count sentences in current buffer from START to END."
@@ -488,8 +487,13 @@ count-sentences
       (save-restriction
         (narrow-to-region start end)
         (goto-char (point-min))
-	(while (ignore-errors (forward-sentence))
-	  (setq sentences (1+ sentences)))
+        (let* ((prev (point))
+               (next (forward-sentence)))
+          (while (and (not (null next))
+                      (not (= prev next)))
+            (setq prev next
+                  next (ignore-errors (forward-sentence))
+                  sentences (1+ sentences))))
         ;; Remove last possibly empty sentence
         (when (/= (skip-chars-backward " \t\n") 0)
           (setq sentences (1- sentences)))
-- 
2.39.1

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: mardani29@yahoo.es,  62027@debbugs.gnu.org,  simenheg@runbox.com
> Date: Tue, 07 Mar 2023 20:07:44 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> [...]
> 
> > I tried, but it doesn't apply.  Did you produce the patch from the
> > emacs-29 branch or from some other branch?
> 
> Sorry, I produced it on master.  Here is the one for emacs-29.

Thanks, installed.

> Will this appear on master at a later time?

Yes, in a day or two.

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Simen Heggestøyl]

Thanks for the quick fix!

Should perhaps the return values of
`forward-sentence'/`backward-sentence' be documented to prevent this
from happening again?

-- Simen

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

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

Simen Heggestøyl <simenheg@runbox.com> writes:

> Thanks for the quick fix!
>
> Should perhaps the return values of
> `forward-sentence'/`backward-sentence' be documented to prevent this
> from happening again?

Why not: Hyrum's law might have won here.  But OTOH in master,
'forward-sentence' is a now call to 'forward-sentence-function' that
could be changed by the user.  Maybe should we document
'forward-sentence-default-function'?
-- 
Manuel Giraud

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Drew Adams]

> > Should perhaps the return values of
> > `forward-sentence'/`backward-sentence' be documented
> > to prevent this from happening again?
> 
> Why not: Hyrum's law might have won here.  But OTOH in master,
> 'forward-sentence' is a now call to 'forward-sentence-function' that
> could be changed by the user.  Maybe should we document
> 'forward-sentence-default-function'?

Yes, but _both_ should mention the return value.
And not just "to prevent this from happening again."
Documenting this helps users use the functions.

For `forward-sentence' the doc should say that the
value of var `forward-sentence-default-function'
is called, and it returns what that function returns,
and _by default_ that is the new position (point).

Functions that have useful return values should be
documented to mention what the return value is.

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: Eli Zaretskii <eliz@gnu.org>,
>         "62027-done@debbugs.gnu.org"
> 	<62027-done@debbugs.gnu.org>,
>         "mardani29@yahoo.es" <mardani29@yahoo.es>
> Date: Wed, 8 Mar 2023 15:52:51 +0000
> 
> > > Should perhaps the return values of
> > > `forward-sentence'/`backward-sentence' be documented
> > > to prevent this from happening again?
> > 
> > Why not: Hyrum's law might have won here.  But OTOH in master,
> > 'forward-sentence' is a now call to 'forward-sentence-function' that
> > could be changed by the user.  Maybe should we document
> > 'forward-sentence-default-function'?
> 
> Yes, but _both_ should mention the return value.
> And not just "to prevent this from happening again."
> Documenting this helps users use the functions.

Not when the value is a simple consequence of the implementation, no.

forward-sentence does its job by side effect, not by the value it
returns.

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Drew Adams]

> > > > Should perhaps the return values of
> > > > `forward-sentence'/`backward-sentence' be documented
> > > > to prevent this from happening again?
> > >
> > > Why not: Hyrum's law might have won here.  But OTOH in master,
> > > 'forward-sentence' is a now call to 'forward-sentence-function' that
> > > could be changed by the user.  Maybe should we document
> > > 'forward-sentence-default-function'?
> >
> > Yes, but _both_ should mention the return value.
> > And not just "to prevent this from happening again."
> > Documenting this helps users use the functions.
> 
> Not when the value is a simple consequence
> of the implementation, no.

Are you suggesting that to discover that
the position is returned users need to check
the implementation to see what's returned?

There are plenty of movement functions that
don't return point, even though it's not
obvious that their implementation wouldn't
return point.

How would you guess that `next-line' doesn't
return point, without checking its
implementation?  It seems to go out of its
way to return nil (not point), but it's not
obvious (to me) why that is.

More typically we return nil when a non-nil
value indicates something particular - but
not here.  Or we do so when we specifically,
i.e., for some reason, don't want users to
depend on any particular return value.

Not to mention that to know whether returning
point is "a simple consequence of the
implementation" isn't possible for a command
implemented in C, unless you happen to have
the C source code.

> forward-sentence does its job by side effect,
> not by the value it returns.

If you define "its job" that narrowly, yes.

But then why does it return (point)?  A (happy)
accident/coincidence or design/intended?

In any case, even if you define its job as only
moving, and not also as returning point, that
doesn't prevent it being useful that it returns
the position value.  It's useful to allow/use
code such as this:

(act-on-region some-position
               (forward-sentence N))

rather than

(act-on-region some-position
               (progn (forward-sentence N)
                      (point)))

For setq you could define "its job" as just
changing variable values.  But we return the
value.  Why do we?  Because that's useful.

I don't know why Emacs wouldn't want to let
users know about the useful return value of
a function such as `forward-sentence'.

(By "let users know" I mean help commands,
not just by providing the source code.)

And yes, it wouldn't hurt if _more_ motion
functions (including commands) returned
point. Some do (e.g. `move-beginning-of-line').
Some don't (e.g. `end-of-line', `forward-char').
Probably most don't.

And yes, of course there are motion functions
(e.g. `forward-line') that return a value
other than point - a value that's otherwise
useful.

The point isn't that motion functions should
always return point.  The point is that unless
there's a good reason not to return a useful
value, it's helpful to return a useful value -
and for movement, point can be useful to return.

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "manuel@ledu-giraud.fr" <manuel@ledu-giraud.fr>,
>         "simenheg@runbox.com"
> 	<simenheg@runbox.com>,
>         "62027-done@debbugs.gnu.org"
> 	<62027-done@debbugs.gnu.org>,
>         "mardani29@yahoo.es" <mardani29@yahoo.es>
> Date: Wed, 8 Mar 2023 17:52:54 +0000
> 
> > > Yes, but _both_ should mention the return value.
> > > And not just "to prevent this from happening again."
> > > Documenting this helps users use the functions.
> > 
> > Not when the value is a simple consequence
> > of the implementation, no.
> 
> Are you suggesting that to discover that
> the position is returned users need to check
> the implementation to see what's returned?

No.

And sorry, I cannot afford reading all that you wrote in response to
my 2 sentences.  It's too much.

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

[Eli Zaretskii]

> From: Manuel Giraud <manuel@ledu-giraud.fr>
> Cc: Eli Zaretskii <eliz@gnu.org>,  mardani29@yahoo.es,
>   62027-done@debbugs.gnu.org
> Date: Wed, 08 Mar 2023 09:38:03 +0100
> 
> Simen Heggestøyl <simenheg@runbox.com> writes:
> 
> > Thanks for the quick fix!
> >
> > Should perhaps the return values of
> > `forward-sentence'/`backward-sentence' be documented to prevent this
> > from happening again?
> 
> Why not: Hyrum's law might have won here.  But OTOH in master,
> 'forward-sentence' is a now call to 'forward-sentence-function' that
> could be changed by the user.  Maybe should we document
> 'forward-sentence-default-function'?

I see no reason to document that.

bug#62027: Subject: 29.0.60; Breaking change in forward-sentence/backward-sentence

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

Eli Zaretskii <eliz@gnu.org> writes:

[...]

>> Why not: Hyrum's law might have won here.  But OTOH in master,
>> 'forward-sentence' is a now call to 'forward-sentence-function' that
>> could be changed by the user.  Maybe should we document
>> 'forward-sentence-default-function'?
>
> I see no reason to document that.

Fine by me.
-- 
Manuel Giraud