GNU bug report logs - #22847
#17062: 24.3 current-fill-column breaks fill-match-adaptive-prefix

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 22847 in the body.
You can then email your comments to 22847 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
To: bug-gnu-emacs <at> gnu.org
Subject: #17062: 24.3 current-fill-column breaks fill-match-adaptive-prefix
Date: Mon, 29 Feb 2016 08:33:23 +0100

[Message part 1 (text/plain, inline)]

|reopen| #17062


Unfortunatly can't deliver a backtrace, as it was some times ago.

The bug is visible by program-logic already.

fill-match-adaptive-prefix counts on current-fill-column having:

(>= (+ (current-left-margin) (length str)) (current-fill-column))

This will be broken if current-fill-column returns nil.

Returning nil is possible, see inside current-fill-column:

    (if fill-column

If fill-column is nil, current-fill-column will return nil which was the 
case coming upon.
A fix might make sure an integer is returned anyway: think at 0 or 
default value.

[Message part 2 (text/html, inline)]

Message #10 received at 22847 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
Cc: 22847 <at> debbugs.gnu.org
Subject: Re: bug#22847: #17062: 24.3 current-fill-column breaks
 fill-match-adaptive-prefix
Date: Mon, 29 Feb 2016 17:56:54 +0200

> From: Andreas Röhler <andreas.roehler <at> easy-emacs.de>
> Date: Mon, 29 Feb 2016 08:33:23 +0100
> 
> fill-match-adaptive-prefix counts on current-fill-column having:
> 
> (>= (+ (current-left-margin) (length str)) (current-fill-column))
> 
> This will be broken if current-fill-column returns nil.
> 
> Returning nil is possible, see inside current-fill-column:
> 
> (if fill-column
> 
> If fill-column is nil, current-fill-column will return nil which was the case coming upon.
> A fix might make sure an integer is returned anyway: think at 0 or default value.

While at that, please also fix the calculation of string length: it
should use 'string-width', not 'length', IMO.

Message #13 received at 22847 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: 22847 <at> debbugs.gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#22847: #17062: 24.3 current-fill-column breaks
 fill-match-adaptive-prefix
Date: Thu, 08 Dec 2016 17:32:38 -0500

So this seems like a mess.

fill-column is documented to be an integer, and that is its custom-type.
Nowhere does it say it can be nil, AFAICS.
Many places in Emacs are not prepared for it (or current-fill-column) to
be nil.

Yet 1e87252 explicitly added a check for a nil fill-column to
current-fill-column. AFAICS, do-auto-fill is the only place in Emacs
that tests for this, and uses it to disable auto-fill.

The only uses I find for "(setq fill-column nil)" are a few people using
it in their .emacs to disable auto-fill (I guess) in some major mode.
The idiomatic way to do this is just to turn off auto-fill in that mode.

TLDR:
Let's remove the test for nil fill-column in current-fill-column.

Message #18 received at 22847 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 22847 <at> debbugs.gnu.org
Subject: Re: bug#22847: #17062: 24.3 current-fill-column breaks
 fill-match-adaptive-prefix
Date: Fri, 09 Dec 2016 10:08:29 +0200

> From: Glenn Morris <rgm <at> gnu.org>
> cc: Eli Zaretskii <eliz <at> gnu.org>
> Date: Thu, 08 Dec 2016 17:32:38 -0500
> 
> So this seems like a mess.

But a very old one.

> fill-column is documented to be an integer, and that is its custom-type.
> Nowhere does it say it can be nil, AFAICS.
> Many places in Emacs are not prepared for it (or current-fill-column) to
> be nil.
> 
> Yet 1e87252 explicitly added a check for a nil fill-column to
> current-fill-column. AFAICS, do-auto-fill is the only place in Emacs
> that tests for this, and uses it to disable auto-fill.
> 
> The only uses I find for "(setq fill-column nil)" are a few people using
> it in their .emacs to disable auto-fill (I guess) in some major mode.
> The idiomatic way to do this is just to turn off auto-fill in that mode.
> 
> TLDR:
> Let's remove the test for nil fill-column in current-fill-column.

I don't understand what you propose to do instead.
current-fill-column does arithmetics on fill-column when it's non-nil,
so we cannot just remove the test, because the function will then
signal an error.

I see 3 possible ways to fix these bugs:

  . Fix the code which is not prepared for fill-column being nil to be
    prepared.  This leaves everyone happy, except, perhaps, the person
    who would need to fix all those places in Emacs.

  . Change current-fill-column to return most-positive-fixnum when
    fill-column is nil.  This is an easy way out, but it might slow
    down do-auto-fill when fill-column is nil.  Not sure if we care
    about that slow down.

  . Disallow fill-column being nil and remove the test from
    current-fill-column without changing anything else, i.e. let it
    signal an error, perhaps with some text that tells this value is
    no longer supported.  This will break setups of those who use that
    value to disable auto-fill, something that was available since
    forever, so I don't think we can do that.

Comments?

Message #21 received at 22847 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 22847 <at> debbugs.gnu.org
Subject: Re: bug#22847: #17062: 24.3 current-fill-column breaks
 fill-match-adaptive-prefix
Date: Sat, 10 Dec 2016 21:18:57 -0500

Eli Zaretskii wrote:

>> TLDR:
>> Let's remove the test for nil fill-column in current-fill-column.
>
> I don't understand what you propose to do instead.
> current-fill-column does arithmetics on fill-column when it's non-nil,
> so we cannot just remove the test, because the function will then
> signal an error.

Yes, I'm fine with the error.

> I see 3 possible ways to fix these bugs:
>
>   . Fix the code which is not prepared for fill-column being nil to be
>     prepared.  This leaves everyone happy, except, perhaps, the person
>     who would need to fix all those places in Emacs.

I think this would be a waste of time for the Emacs, and third party,
maintainers.

>   . Change current-fill-column to return most-positive-fixnum when
>     fill-column is nil.

I suppose this would be ok, so long as it comes with something like a
once-per session display-warning about this being an obsolete usage that
will be removed soon.

>   . Disallow fill-column being nil and remove the test from
>     current-fill-column without changing anything else, i.e. let it
>     signal an error, perhaps with some text that tells this value is
>     no longer supported.  This will break setups of those who use that
>     value to disable auto-fill, something that was available since
>     forever, so I don't think we can do that.

That's what I would do. I don't have a problem breaking an undocumented
feature that already fails in several places, and has a trivial
workaround (don't want auto-fill - don't turn it on). Other times I can
recall similar breakage happening: byte-compile of nil, setq with odd
number of arguments. People gripe for a bit, then get on with life.

Message #24 received at 22847 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 22847 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#22847: #17062: 24.3 current-fill-column breaks
 fill-match-adaptive-prefix
Date: Fri, 14 Aug 2020 22:14:09 -0700

[Message part 1 (text/plain, inline)]

tags 22847 + patch
thanks

Glenn Morris <rgm <at> gnu.org> writes:

> Eli Zaretskii wrote:
>
>>> TLDR:
>>> Let's remove the test for nil fill-column in current-fill-column.
>>
>> I don't understand what you propose to do instead.
>> current-fill-column does arithmetics on fill-column when it's non-nil,
>> so we cannot just remove the test, because the function will then
>> signal an error.
>
> Yes, I'm fine with the error.
>
>> I see 3 possible ways to fix these bugs:
>>
>>   . Fix the code which is not prepared for fill-column being nil to be
>>     prepared.  This leaves everyone happy, except, perhaps, the person
>>     who would need to fix all those places in Emacs.
>
> I think this would be a waste of time for the Emacs, and third party,
> maintainers.

Agreed.

>>   . Change current-fill-column to return most-positive-fixnum when
>>     fill-column is nil.
>
> I suppose this would be ok, so long as it comes with something like a
> once-per session display-warning about this being an obsolete usage that
> will be removed soon.

I've attached a proposed patch which does that here.

>>   . Disallow fill-column being nil and remove the test from
>>     current-fill-column without changing anything else, i.e. let it
>>     signal an error, perhaps with some text that tells this value is
>>     no longer supported.  This will break setups of those who use that
>>     value to disable auto-fill, something that was available since
>>     forever, so I don't think we can do that.
>
> That's what I would do. I don't have a problem breaking an undocumented
> feature that already fails in several places, and has a trivial
> workaround (don't want auto-fill - don't turn it on). Other times I can
> recall similar breakage happening: byte-compile of nil, setq with odd
> number of arguments. People gripe for a bit, then get on with life.

I'm perfectly fine with this solution as well, if we prefer that.

Thoughts?

Best regards,
Stefan Kangas

[0001-Make-nil-value-of-fill-column-obsolete.patch (text/x-diff, attachment)]

Message #29 received at 22847 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 22847 <at> debbugs.gnu.org, Glenn Morris <rgm <at> gnu.org>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#22847: #17062: 24.3 current-fill-column breaks
 fill-match-adaptive-prefix
Date: Mon, 10 May 2021 13:48:09 +0200

Stefan Kangas <stefan <at> marxist.se> writes:

> +** Setting fill-columns to nil is obsolete.
> +This undocumented use of fill-columns is now obsolete.  If you have
> +set this value to nil disable auto filling, stop setting this variable
> +and disable auto-fill-mode in the relevant mode instead.

Skimming this thread, this patch makes sense to me, I think?  But it was
never applied?

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

Message #32 received at 22847 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefan <at> marxist.se>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 22847 <at> debbugs.gnu.org, Glenn Morris <rgm <at> gnu.org>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#22847: #17062: 24.3 current-fill-column breaks
 fill-match-adaptive-prefix
Date: Mon, 10 May 2021 08:29:49 -0500

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Stefan Kangas <stefan <at> marxist.se> writes:
>
>> +** Setting fill-columns to nil is obsolete.
>> +This undocumented use of fill-columns is now obsolete.  If you have
>> +set this value to nil disable auto filling, stop setting this variable
>> +and disable auto-fill-mode in the relevant mode instead.
>
> Skimming this thread, this patch makes sense to me, I think?  But it was
> never applied?

There might be a subtle problem with it after all but I can't remember
the details now.

This code is a little bit more tricky than what first meets the eye, I
think.  So I would suggest someone takes a closer look before
installing.

Message #35 received at 22847 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 22847 <at> debbugs.gnu.org, Glenn Morris <rgm <at> gnu.org>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#22847: #17062: 24.3 current-fill-column breaks
 fill-match-adaptive-prefix
Date: Wed, 12 May 2021 15:25:51 +0200

Stefan Kangas <stefan <at> marxist.se> writes:

> There might be a subtle problem with it after all but I can't remember
> the details now.
>
> This code is a little bit more tricky than what first meets the eye, I
> think.  So I would suggest someone takes a closer look before
> installing.

I've given the patch a try, and it seems to work as advertised.  Does
anybody else have a comment?  I've respun the patch against the current
tree and done some minor edits:

diff --git a/etc/NEWS b/etc/NEWS
index de3779cd73..3069b4d498 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -108,6 +108,16 @@ avoid security issues when executing untrusted code.  See the manual
 page for 'seccomp' system call, for details about Secure Computing
 filters.
 
+** Setting 'fill-column' to nil is obsolete.
+This undocumented use of 'fill-column' is now obsolete.  If you have
+set this value to nil disable auto filling, instead disable
+'auto-fill-mode' in the relevant mode instead.
+
+For instance, you could add something like the following to your init
+file:
+
+    (add-hook 'foo-mode-hook (lambda () (auto-fill-mode -1))
+
 
 * Changes in Emacs 28.1
 
diff --git a/lisp/simple.el b/lisp/simple.el
index b4e34f1e4c..d21daf9e19 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7931,7 +7931,7 @@ do-auto-fill
   (let (fc justify give-up
 	   (fill-prefix fill-prefix))
     (if (or (not (setq justify (current-justification)))
-	    (null (setq fc (current-fill-column)))
+	    (setq fc (current-fill-column))
 	    (and (eq justify 'left)
 		 (<= (current-column) fc))
 	    (and auto-fill-inhibit-regexp
diff --git a/lisp/textmodes/fill.el b/lisp/textmodes/fill.el
index 3914bdeb83..f394171fb6 100644
--- a/lisp/textmodes/fill.el
+++ b/lisp/textmodes/fill.el
@@ -133,6 +133,8 @@ adaptive-fill-function
 (defvar fill-indent-according-to-mode nil ;Screws up CC-mode's filling tricks.
   "Whether or not filling should try to use the major mode's indentation.")
 
+(defvar current-fill-column--has-warned nil)
+
 (defun current-fill-column ()
   "Return the fill-column to use for this line.
 The fill-column to use for a buffer is stored in the variable `fill-column',
@@ -158,7 +160,14 @@ current-fill-column
 			     (< col fill-col)))
 	    (setq here change
 		  here-col col))
-	  (max here-col fill-col)))))
+	  (max here-col fill-col))
+      ;; This warning was added in 28.1.  It should be removed later,
+      ;; and this function changed to never return nil.
+      (unless current-fill-column--has-warned
+        (lwarn '(fill-column) :warning
+               "Setting this variable to nil is obsolete; use `(auto-fill-mode -1)' instead")
+        (setq current-fill-column--has-warned t))
+      most-positive-fixnum)))
 
 (defun canonically-space-region (beg end)
   "Remove extra spaces between words in region.


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

Message #38 received at 22847 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Stefan Kangas <stefan <at> marxist.se>
Cc: 22847 <at> debbugs.gnu.org, Glenn Morris <rgm <at> gnu.org>,
 Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#22847: #17062: 24.3 current-fill-column breaks
 fill-match-adaptive-prefix
Date: Fri, 23 Jul 2021 14:58:22 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> I've given the patch a try, and it seems to work as advertised.  Does
> anybody else have a comment? 

There were no comments, so I've now pushed Stefan K's patch to the
trunk.

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.