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

[Stefan Kangas <stefan@marxist.se>]

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

tags 22847 + patch
thanks

Glenn Morris <rgm@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

[-- Attachment #2: 0001-Make-nil-value-of-fill-column-obsolete.patch --]
[-- Type: text/x-diff, Size: 3066 bytes --]

From 5f41d8df85cd7e16a7a335592a02b3dc38dc9b0b Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Sat, 15 Aug 2020 06:56:05 +0200
Subject: [PATCH] Make nil value of fill-column obsolete

* lisp/textmodes/fill.el (current-fill-column): Make nil value of
'fill-column' obsolete.  (Bug#22847)
(current-fill-column--has-warned): New variable to track warning.
* lisp/simple.el (do-auto-fill): Remove handling of nil return value
from 'current-fill-column'.
* etc/NEWS: Announce obsoletion of this usage.
---
 etc/NEWS               |  9 +++++++++
 lisp/simple.el         |  2 +-
 lisp/textmodes/fill.el | 11 ++++++++++-
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index e51a3630b6..227d231e9d 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -145,6 +145,15 @@ setting the variable 'auto-save-visited-mode' buffer-locally to nil.
 description of the properties.  Likewise 'button-describe' does the
 same for a button.
 
+** 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.
+
+You could add something like the following to your init file:
+
+    (add-hook 'foo-mode-hook (lambda () (auto-fill-mode -1))
+
 \f
 * Changes in Specialized Modes and Packages in Emacs 28.1
 
diff --git a/lisp/simple.el b/lisp/simple.el
index 1cb93c5722..a2b45746e2 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7519,7 +7519,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 15b13af568..06ae9c0ddc 100644
--- a/lisp/textmodes/fill.el
+++ b/lisp/textmodes/fill.el
@@ -139,6 +139,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',
@@ -164,7 +166,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")
+        (setq current-fill-column--has-warned t))
+      most-positive-fixnum)))
 
 (defun canonically-space-region (beg end)
   "Remove extra spaces between words in region.
-- 
2.28.0