bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Pip Cet]

(dotimes (i 10) (insert (propertize "test " 'field (cons nil nil))))

C-p C-a C-n C-SPC C-e M-x count-words

Expected result:

message "Region has 1 line, 10 words, and 50 characters"

Actual result:

message "Region has 1 line, 1 word, and 50 characters"

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Daniel Koning]

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

Pip Cet <pipcet@gmail.com> writes:

> Expected result:
>
> message "Region has 1 line, 10 words, and 50 characters"
>
> Actual result:
>
> message "Region has 1 line, 1 word, and 50 characters"

`count-words' should be disabling field-boundary behavior in
`forward-word-strictly' by setting `inhibit-field-text-motion'. Patch
included.

I grepped for other calls to `count-words' that might be making the same
omission, but everything I saw looked like a use case that should
respect field boundaries.

Daniel


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: count-words patch --]
[-- Type: text/x-patch, Size: 998 bytes --]

From af0d9c48aaf99d8df5fa55a8ba2c0ee42bf695e9 Mon Sep 17 00:00:00 2001
From: Daniel Koning <dk@danielkoning.com>
Date: Wed, 10 Jun 2020 14:42:39 -0500
Subject: [PATCH] Don't stop at field boundaries when counting words
 (Bug#41761)

* lisp/simple.el (count-words): Ensure that `forward-word-strictly'
moves point from one field to the next during the word-counting loop.

Copyright-paperwork-exempt: yes
---
 lisp/simple.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 6d7600e414..9d898eeeec 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1319,7 +1319,9 @@ count-words
 END, without printing any message."
   (interactive (list nil nil))
   (cond ((not (called-interactively-p 'any))
-	 (let ((words 0))
+	 (let ((words 0)
+               ;; Count across field boundaries. (Bug#41761)
+               (inhibit-field-text-motion t))
 	   (save-excursion
 	     (save-restriction
 	       (narrow-to-region start end)
-- 
2.20.1

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Stefan Kangas]

close 41761 28.1
thanks

Daniel Koning <dk@danielkoning.com> writes:

> Pip Cet <pipcet@gmail.com> writes:
>
>> Expected result:
>>
>> message "Region has 1 line, 10 words, and 50 characters"
>>
>> Actual result:
>>
>> message "Region has 1 line, 1 word, and 50 characters"
>
> `count-words' should be disabling field-boundary behavior in
> `forward-word-strictly' by setting `inhibit-field-text-motion'. Patch
> included.

Thanks for the patch.  It looks good to me so I pushed it as commit
c560ba3036d8.  I also pushed a regression test for this bug separately.

Closing this bug now.

Best regards,
Stefan Kangas

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 14 Aug 2020 02:58:19 -0700
> Cc: 41761@debbugs.gnu.org, Pip Cet <pipcet@gmail.com>
> 
> >> Expected result:
> >>
> >> message "Region has 1 line, 10 words, and 50 characters"
> >>
> >> Actual result:
> >>
> >> message "Region has 1 line, 1 word, and 50 characters"
> >
> > `count-words' should be disabling field-boundary behavior in
> > `forward-word-strictly' by setting `inhibit-field-text-motion'. Patch
> > included.
> 
> Thanks for the patch.  It looks good to me so I pushed it as commit
> c560ba3036d8.

Thanks.  This change in behavior should be in NEWS.  And I'm not sure
we shouldn't provide an optional knob to get back the old behavior:
some package or user might rely on it.

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Daniel Koning]

Eli Zaretskii <eliz@gnu.org> writes:

> This change in behavior should be in NEWS.  And I'm not sure
> we shouldn't provide an optional knob to get back the old behavior:
> some package or user might rely on it.

I don't think I agree. The "new" behavior is what the documentation
described to begin with. And it's a complexity burden to solder new
switches onto these simple functions -- more burdensome (I say) than
trusting users to impose the constraint by hand if that's what they
want. Something like:

    (count-words start (min end (field-end start t)))

Daniel

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Eli Zaretskii]

> From: Daniel Koning <dk@danielkoning.com>
> Cc: Stefan Kangas <stefan@marxist.se>,  41761@debbugs.gnu.org,
>   pipcet@gmail.com
> Date: Fri, 14 Aug 2020 13:48:44 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > This change in behavior should be in NEWS.  And I'm not sure
> > we shouldn't provide an optional knob to get back the old behavior:
> > some package or user might rely on it.
> 
> I don't think I agree.

It's okay to disagree, but I still think it should be in NEWS.  It's a
change in a long-standing behavior, so users should be told about it.

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Daniel Koning]

Eli Zaretskii <eliz@gnu.org> writes:

>> > This change in behavior should be in NEWS.  And I'm not sure
>> > we shouldn't provide an optional knob to get back the old behavior:
>> > some package or user might rely on it.
>>
>> I don't think I agree.
>
> It's okay to disagree, but I still think it should be in NEWS.

Excuse my imprecision in quoting you; I don't object to the change going
into NEWS. I just meant to contend that the old behavior shouldn't be
enshrined in a customization item or anything like that. Some deeply
rooted bugs become load-bearing over time, but this doesn't look like
one of those, only (you might say literally) an overlooked edge case.

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Eli Zaretskii]

> From: Daniel Koning <dk@danielkoning.com>
> Cc: stefan@marxist.se,  41761@debbugs.gnu.org,  pipcet@gmail.com
> Date: Fri, 14 Aug 2020 14:44:52 -0500
> 
> this doesn't look like one of those, only (you might say literally)
> an overlooked edge case.

We have no reliable way of determining whether that is the case.
backward-compatibility is important in a veteran package such as
Emacs, so we generally try at least to give users who want previous
behavior a fire escape.

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> It's okay to disagree, but I still think it should be in NEWS.  It's a
> change in a long-standing behavior, so users should be told about it.

I'm not sure how to best formulate the NEWS.

Does this look okay?

diff --git a/etc/NEWS b/etc/NEWS
index 53e60cdb5c..b375fef5b7 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -145,6 +145,11 @@ setting the variable 'auto-save-visited-mode'
buffer-locally to nil.
 description of the properties.  Likewise 'button-describe' does the
 same for a button.

+---
+** 'count-words' now skips field boundaries.
+Before, 'count-words' incorrectly stopped counting the number of words
+in buffer at a field boundary.  This bug has been fixed.
+
 \f
 * Changes in Specialized Modes and Packages in Emacs 28.1

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 14 Aug 2020 16:06:30 -0700
> Cc: 41761@debbugs.gnu.org, pipcet@gmail.com
> 
> +** 'count-words' now skips field boundaries.
> +Before, 'count-words' incorrectly stopped counting the number of words
> +in buffer at a field boundary.  This bug has been fixed.

I don't think it was a bug, so the wording should be different.
Something like

  'count-words' now crosses field boundaries.
  Originally, 'count-words' ignored the text inside field boundaries,
  now it includes such text in the count.

Thanks.

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Daniel Koning]

Eli Zaretskii <eliz@gnu.org> writes:

>> +** 'count-words' now skips field boundaries.
>> +Before, 'count-words' incorrectly stopped counting the number of words
>> +in buffer at a field boundary.  This bug has been fixed.
>
> I don't think it was a bug, so the wording should be different.

If the old behavior didn't qualify as a bug, then wouldn't you say the
docstring was buggy? It said that the function returned or printed
something that it really didn't ("the number of words between START and
END"). And there's nothing in the docs to suggest that we should read
that as "between START and END, or between START and the next field
boundary if that boundary comes before END."

(Nor does this rule apply to the other tallies that the function makes.
Put an extra line of text beneath the line containing the several
fields, and the interactive `count-words' message will reflect that line
in its line count and character count, but not in its word count. So
between "(defun count-words" and its matching ")", something is amiss,
whether it's code or documentation.)

If you don't like the word "bug" for this, the NEWS item might at least
mention that the prior behavior was undocumented and inconsistent, just
to clarify why the manual entry isn't changing along with the code.

Daniel

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Eli Zaretskii]

> From: Daniel Koning <dk@danielkoning.com>
> Cc: Stefan Kangas <stefan@marxist.se>,  41761@debbugs.gnu.org,
>   pipcet@gmail.com
> Date: Sat, 15 Aug 2020 12:44:10 -0500
> 
> If you don't like the word "bug" for this, the NEWS item might at least
> mention that the prior behavior was undocumented and inconsistent, just
> to clarify why the manual entry isn't changing along with the code.

Sorry, I don't understand the point you are trying to make.  Is
something wrong or inaccurate with the text I proposed for NEWS?

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Daniel Koning]

Eli Zaretskii <eliz@gnu.org> writes:

> Is something wrong or inaccurate with the text I proposed for NEWS?

Based on other NEWS entries, and the discussion earlier in this thread
of Stefan's original phrasing, I assumed that it was customary to
mention in NEWS that a change fixes a defect (which I believe this one
is, for the reasons I outlined in my last message). If that's not
expected, then it doesn't much matter.

I do think the prior behavior differs somewhat from the way you phrased
it. I would write it as:

    Originally, when 'inhibit-field-text-motion' was nil, 'count-words'
    stopped counting words at the end of the field containing START.

(It didn't ignore text inside fields, but rather text that was not
inside one specific field.)

Daniel

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Eli Zaretskii]

> From: Daniel Koning <dk@danielkoning.com>
> Cc: stefan@marxist.se,  41761@debbugs.gnu.org,  pipcet@gmail.com
> Date: Sat, 15 Aug 2020 14:44:18 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Is something wrong or inaccurate with the text I proposed for NEWS?
> 
> Based on other NEWS entries, and the discussion earlier in this thread
> of Stefan's original phrasing, I assumed that it was customary to
> mention in NEWS that a change fixes a defect (which I believe this one
> is, for the reasons I outlined in my last message). If that's not
> expected, then it doesn't much matter.

No, we don't mention bug fixes in NEWS.

> I do think the prior behavior differs somewhat from the way you phrased
> it. I would write it as:
> 
>     Originally, when 'inhibit-field-text-motion' was nil, 'count-words'
>     stopped counting words at the end of the field containing START.
> 
> (It didn't ignore text inside fields, but rather text that was not
> inside one specific field.)

I'm not sure I understand the "end of field containing START" part.
Supposed the region includes several fields, what would the previous
code do?

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Daniel Koning]

Eli Zaretskii <eliz@gnu.org> writes:

>> I do think the prior behavior differs somewhat from the way you phrased
>> it. I would write it as:
>>
>>     Originally, when 'inhibit-field-text-motion' was nil, 'count-words'
>>     stopped counting words at the end of the field containing START.
>>
>> (It didn't ignore text inside fields, but rather text that was not
>> inside one specific field.)
>
> I'm not sure I understand the "end of field containing START" part.
> Supposed the region includes several fields, what would the previous
> code do?

It would only count the words between START and the next field boundary,
no matter how many fields came after that. (For START, substitute "the
beginning of the region" or "the beginning of the buffer" if it was
called interactively.)

Try reinstalling the old function definition and evaluating:

ELISP> (with-temp-buffer
         (dotimes (i 9) (insert (propertize "4 words 22 characters " 'field i)))
         (insert "\n4 words 22 characters")
         (call-interactively #'count-words)) ; operates on whole buffer

=> "Buffer has 2 lines, 4 words, and 220 characters."

That result arises because of how `count-words' generates its return
value: it moves point to the beginning of the buffer (after narrowing
the buffer to START and END if they were supplied), then repeatedly
calls `forward-word-strictly' until that function returns nil,
incrementing the running total each time. Before, if it ran into a field
boundary, it would stop short, because `forward-word-strictly' doesn't
jump field boundaries by default.

I think the value of that form demonstrates why the old behavior was
wrong. There does not exist any part of the temp buffer that "has 2
lines, 4 words, and 220 characters." It gives inconsistent results
because the line count and character count use techniques which aren't
affected by fields.

Daniel

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Eli Zaretskii]

> From: Daniel Koning <dk@danielkoning.com>
> Cc: stefan@marxist.se,  41761@debbugs.gnu.org,  pipcet@gmail.com
> Date: Sun, 16 Aug 2020 14:04:14 -0500
> 
> > I'm not sure I understand the "end of field containing START" part.
> > Supposed the region includes several fields, what would the previous
> > code do?
> 
> It would only count the words between START and the next field boundary,
> no matter how many fields came after that. (For START, substitute "the
> beginning of the region" or "the beginning of the buffer" if it was
> called interactively.)

Thanks, then how about this NEWS entry:

  'count-words' now crosses field boundaries.
  Originally, 'count-words' would stop counting at the first field
  boundary it encountered; now it keeps counting all the way to the
  region's (or buffer's) end.

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Daniel Koning]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, then how about this NEWS entry:
>
>   'count-words' now crosses field boundaries.
>   Originally, 'count-words' would stop counting at the first field
>   boundary it encountered; now it keeps counting all the way to the
>   region's (or buffer's) end.

Looks accurate to me. Thanks!

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, then how about this NEWS entry:
>
>   'count-words' now crosses field boundaries.
>   Originally, 'count-words' would stop counting at the first field
>   boundary it encountered; now it keeps counting all the way to the
>   region's (or buffer's) end.

LGTM as well.  Please push to master.

(Or, if it's more convenient for you, I could push it in your name.)

Best regards,
Stefan Kangas

bug#41761: [PATCH] bug#41761: 28.0.50; M-x count-words counts words only up to a field boundary

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Mon, 17 Aug 2020 13:51:02 +0000
> Cc: 41761@debbugs.gnu.org, pipcet@gmail.com
> 
> >   'count-words' now crosses field boundaries.
> >   Originally, 'count-words' would stop counting at the first field
> >   boundary it encountered; now it keeps counting all the way to the
> >   region's (or buffer's) end.
> 
> LGTM as well.  Please push to master.

Done.