Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Nikolai Weibull]

Hi!

Smie-auto-fill doesn’t respect comment-auto-fill-only-comments.  I’m
not quite sure whether it actually should, but the issue I’m having is
that auto-fill is being applied in sh-mode, even though I have
comment-auto-fill-only-comments.

  Nikolai

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Nikolai Weibull]

Hi!

I’d really appreciate some input on this issue.

Thanks!

  Nikolai

On Thu, Apr 6, 2017 at 2:04 PM, Nikolai Weibull <now@disu.se> wrote:
> Hi!
>
> Smie-auto-fill doesn’t respect comment-auto-fill-only-comments.  I’m
> not quite sure whether it actually should, but the issue I’m having is
> that auto-fill is being applied in sh-mode, even though I have
> comment-auto-fill-only-comments.
>
>   Nikolai

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Ian Dunn]

Nikolai Weibull <now@disu.se> writes:

> Hi!
>
> I’d really appreciate some input on this issue.
>
> Thanks!
>
>   Nikolai
>
> On Thu, Apr 6, 2017 at 2:04 PM, Nikolai Weibull <now@disu.se> wrote:
>> Hi!
>>
>> Smie-auto-fill doesn’t respect comment-auto-fill-only-comments.  I’m
>> not quite sure whether it actually should, but the issue I’m having is
>> that auto-fill is being applied in sh-mode, even though I have
>> comment-auto-fill-only-comments.
>>
>>   Nikolai
>
>

I can confirm this.  I've seen this on both sh-mode and octave-mode.

To mitigate the problem, you can add the following to the mode hooks for those that use SMIE (sh, octave, etc.):

(setq-local auto-fill-function 'do-auto-fill)

I don't know if there's something special smie-auto-fill does that this will stop, but at least this will respect comment-auto-fill-only-comments.

-- 
Ian Dunn

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Tom Tromey]

>>>>> "Nikolai" == Nikolai Weibull <now@disu.se> writes:

Nikolai> Smie-auto-fill doesn’t respect comment-auto-fill-only-comments.  I’m
Nikolai> not quite sure whether it actually should, but the issue I’m having is
Nikolai> that auto-fill is being applied in sh-mode, even though I have
Nikolai> comment-auto-fill-only-comments.

I recently noticed this for other programming modes, like js-mode.
I wasn't sure if this is some recent(-ish) change or if I just never
happened to notice it before somehow.

Anyway, I'm trying out the appended, which I'd like to push in.

This won't fix the SMIE issue though.  That would require a similar fix
in smie-auto-fill.

Or maybe this should just be done directly in do-auto-fill itself?

Tom

diff --git a/lisp/progmodes/prog-mode.el b/lisp/progmodes/prog-mode.el
index 8f66f1c..b5bb637 100644
--- a/lisp/progmodes/prog-mode.el
+++ b/lisp/progmodes/prog-mode.el
@@ -129,6 +129,11 @@ prog-widen
         (narrow-to-region (car chunk) (or (cdr chunk) (point-max)))
       (widen))))
 
+(defun prog-mode-do-auto-fill ()
+  "Like `do-auto-fill' but respect `comment-auto-fill-only-comments'."
+  (when (or (not comment-auto-fill-only-comments)
+            (nth 4 (syntax-ppss)))
+    (do-auto-fill)))
 
 (defvar-local prettify-symbols-alist nil
   "Alist of symbol prettifications.
@@ -293,6 +298,7 @@ prog-mode
   "Major mode for editing programming language source code."
   (setq-local require-final-newline mode-require-final-newline)
   (setq-local parse-sexp-ignore-comments t)
+  (setq-local normal-auto-fill-function #'prog-mode-do-auto-fill)
   ;; Any programming language is always written left to right.
   (setq bidi-paragraph-direction 'left-to-right))
 

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Stefan Monnier]

> This won't fix the SMIE issue though.  That would require a similar fix
> in smie-auto-fill.

We should try and find a solution which doesn't require changing every
normal-auto-fill-function.


        Stefan

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Stefan Monnier]

>> This won't fix the SMIE issue though.  That would require a similar fix
>> in smie-auto-fill.
> We should try and find a solution which doesn't require changing every
> normal-auto-fill-function.

I think the patch below "does the trick" but it's rather ugly.


        Stefan


diff --git a/lisp/newcomment.el b/lisp/newcomment.el
index 4b261c34c6..6a2bf35788 100644
--- a/lisp/newcomment.el
+++ b/lisp/newcomment.el
@@ -1316,13 +1316,6 @@ comment-dwim
               (insert (comment-padleft comment-end add)))
             (indent-according-to-mode)))))))
 
-;;;###autoload
-(defcustom comment-auto-fill-only-comments nil
-  "Non-nil means to only auto-fill inside comments.
-This has no effect in modes that do not define a comment syntax."
-  :type 'boolean
-  :group 'comment)
-
 (defun comment-valid-prefix-p (prefix compos)
     "Check that the adaptive fill prefix is consistent with the context.
 PREFIX is the prefix (presumably guessed by `adaptive-fill-mode').
@@ -1384,7 +1377,6 @@ comment-indent-new-line
     ;; If we are not inside a comment and we only auto-fill comments,
     ;; don't do anything (unless no comment syntax is defined).
     (unless (and comment-start
-		 comment-auto-fill-only-comments
 		 (not (called-interactively-p 'interactive))
 		 (not (save-excursion
 			(prog1 (setq compos (comment-beginning))
diff --git a/lisp/simple.el b/lisp/simple.el
index edc822eb51..72f265d544 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7175,6 +7175,12 @@ normal-auto-fill-function
 ;; can be useful to prevent auto-filling.
 (put 'auto-fill-function 'safe-local-variable 'null)
 
+(defcustom comment-auto-fill-only-comments nil
+  "Non-nil means to only auto-fill inside comments.
+This has no effect in modes that do not define a comment syntax."
+  :type 'boolean
+  :group 'comment)
+
 (define-minor-mode auto-fill-mode
   "Toggle automatic line breaking (Auto Fill mode).
 With a prefix argument ARG, enable Auto Fill mode if ARG is
@@ -7191,8 +7197,19 @@ auto-fill-mode
 The value of `normal-auto-fill-function' specifies the function to use
 for `auto-fill-function' when turning Auto Fill mode on."
   :variable (auto-fill-function
-             . (lambda (v) (setq auto-fill-function
-                            (if v normal-auto-fill-function)))))
+             . (lambda (v)
+                 (if (not v)
+                     (setq auto-fill-function nil)
+                   (setq auto-fill-function normal-auto-fill-function)
+                   (add-function :around normal-auto-fill-function
+                                 (lambda (orig-fun &rest args)
+                                   (if (and comment-start
+		                            comment-auto-fill-only-comments
+                                            (not (nth 4 (syntax-ppss))))
+                                       ;; Comments exist and we only want to
+                                       ;; auto-fill them but we're not in one!
+                                       nil
+                                     (apply orig-fun args))))))))
 
 ;; This holds a document string used to document auto-fill-mode.
 (defun auto-fill-function ()

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Tom Tromey]

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

>>> This won't fix the SMIE issue though.  That would require a similar fix
>>> in smie-auto-fill.
>> We should try and find a solution which doesn't require changing every
>> normal-auto-fill-function.

Stefan> I think the patch below "does the trick" but it's rather ugly.

I think the idea of changing auto-fill-mode to handle this case is
better than what I was doing.

However, I also agree that the advice is ugly...

Stefan> +                   (setq auto-fill-function normal-auto-fill-function)
Stefan> +                   (add-function :around normal-auto-fill-function
Stefan> +                                 (lambda (orig-fun &rest args)
Stefan> +                                   (if (and comment-start
Stefan> +		                            comment-auto-fill-only-comments
Stefan> +                                            (not (nth 4 (syntax-ppss))))
Stefan> +                                       ;; Comments exist and we only want to
Stefan> +                                       ;; auto-fill them but we're not in one!
Stefan> +                                       nil
Stefan> +                                     (apply orig-fun args))))))))

Could this just setq auto-fill-function to a function that wraps (the
captured value of) normal-auto-fill-function?  That would avoid using advice.

Tom

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Stefan Monnier]

> Could this just setq auto-fill-function to a function that wraps (the
> captured value of) normal-auto-fill-function?  That would avoid using advice.

Not sure what you mean: "a function that wraps (the captured value of)
normal-auto-fill-function" is exactly what add-function creates.
[ Tho my patch is buggy, it should say

         (add-function :around (local auto-fill-function)
  i.s.o
         (add-function :around normal-auto-fill-function
]

But maybe a cleaner patch would change the C code so it doesn't (funcall
auto-fill-function) directly but instead it calls a fixed Elisp function
which does the comment-auto-fill-only-comments check and then funcalls
auto-fill-function.  This would probably make it desirable to
move auto-fill-function from buffer.c to simple.el or fill.el.


        Stefan

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Ian Dunn]

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

>
> But maybe a cleaner patch would change the C code so it doesn't (funcall
> auto-fill-function) directly but instead it calls a fixed Elisp function
> which does the comment-auto-fill-only-comments check and then funcalls
> auto-fill-function.  This would probably make it desirable to
> move auto-fill-function from buffer.c to simple.el or fill.el.
>

I agree.  It'd be easier for developers if the logic of
comment-auto-fill-only-comments were dealt with outside of auto-fill-function.

-- 
Ian Dunn

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Tom Tromey]

Stefan> Not sure what you mean: "a function that wraps (the captured value of)
Stefan> normal-auto-fill-function" is exactly what add-function creates.

Yeah, not sure what I was thinking either.

Stefan> But maybe a cleaner patch would change the C code so it doesn't (funcall
Stefan> auto-fill-function) directly but instead it calls a fixed Elisp function
Stefan> which does the comment-auto-fill-only-comments check and then funcalls
Stefan> auto-fill-function.  This would probably make it desirable to
Stefan> move auto-fill-function from buffer.c to simple.el or fill.el.

Here's a patch to do this.

I kept auto-fill-function in the C code because internal_self_insert
checks its value, which seems like it could possibly still be a relevant
optimization.  It's easy enough to remove, though, if you think that's
safe.

Another question is whether prog-mode should set
comment-auto-fill-only-comments.  I tend to think so, but of course
I'm already doing that in my .emacs...

Let me know what you think.  I'd like to push this, or one of these
patches, because this is the main bug I run into in ordinary Emacs use.

Tom

diff --git a/lisp/simple.el b/lisp/simple.el
index ea3a495..971be6b 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7191,6 +7191,12 @@ default-indent-new-line
        ;; If we're not inside a comment, just try to indent.
        (t (indent-according-to-mode))))))
 
+(defun internal-auto-fill ()
+  "The function called by `self-insert-command' to perform auto-filling."
+  (when (or (not comment-auto-fill-only-comments)
+            (nth 4 (syntax-ppss)))
+    (do-auto-fill)))
+
 (defvar normal-auto-fill-function 'do-auto-fill
   "The function to use for `auto-fill-function' if Auto Fill mode is turned on.
 Some major modes set this.")
diff --git a/src/cmds.c b/src/cmds.c
index 51652d5..6f2db86 100644
--- a/src/cmds.c
+++ b/src/cmds.c
@@ -268,9 +268,10 @@ Whichever character you type to run this command is inserted.
 The numeric prefix argument N says how many times to repeat the insertion.
 Before insertion, `expand-abbrev' is executed if the inserted character does
 not have word syntax and the previous character in the buffer does.
-After insertion, the value of `auto-fill-function' is called if the
-`auto-fill-chars' table has a non-nil value for the inserted character.
-At the end, it runs `post-self-insert-hook'.  */)
+After insertion, `internal-auto-fill' is called if
+`auto-fill-function' is non-nil and if the `auto-fill-chars' table has
+a non-nil value for the inserted character.  At the end, it runs
+`post-self-insert-hook'.  */)
   (Lisp_Object n)
 {
   CHECK_NUMBER (n);
@@ -475,7 +476,7 @@ internal_self_insert (int c, EMACS_INT n)
 	   that.  Must have the newline in place already so filling and
 	   justification, if any, know where the end is going to be.  */
 	SET_PT_BOTH (PT - 1, PT_BYTE - 1);
-      auto_fill_result = call0 (BVAR (current_buffer, auto_fill_function));
+      auto_fill_result = call0 (Qinternal_auto_fill);
       /* Test PT < ZV in case the auto-fill-function is strange.  */
       if (c == '\n' && PT < ZV)
 	SET_PT_BOTH (PT + 1, PT_BYTE + 1);
@@ -494,6 +495,8 @@ internal_self_insert (int c, EMACS_INT n)
 void
 syms_of_cmds (void)
 {
+  DEFSYM (Qinternal_auto_fill, "internal-auto-fill");
+
   DEFSYM (Qundo_auto_amalgamate, "undo-auto-amalgamate");
   DEFSYM (Qundo_auto__this_command_amalgamating,
           "undo-auto--this-command-amalgamating");

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Stefan Monnier]

> +(defun internal-auto-fill ()
> +  "The function called by `self-insert-command' to perform auto-filling."
> +  (when (or (not comment-auto-fill-only-comments)
> +            (nth 4 (syntax-ppss)))
> +    (do-auto-fill)))

Please check also comment-start, since you don't want to check
(syntax-ppss) if there's no comment-start defined.  Well, technically,
it shouldn't make any difference, but it's the way it's been defined so
far, so it makes sense to preserve it.

The patch looks good, but please also remove the
comment-auto-fill-only-comments handling from newcomment.el [ BTW, yes, not
only I feel silly about this name now, but I already felt silly about
it when I came up with it.  ]


        Stefan

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Tom Tromey]

Stefan> Please check also comment-start, since you don't want to check
Stefan> (syntax-ppss) if there's no comment-start defined.  Well, technically,
Stefan> it shouldn't make any difference, but it's the way it's been defined so
Stefan> far, so it makes sense to preserve it.

Stefan> The patch looks good, but please also remove the
Stefan> comment-auto-fill-only-comments handling from newcomment.el

Like the appended?
I wasn't sure if I did the newcomment.el part correctly.

Also, any comment on the prog-mode suggestion?

thanks,
Tom

diff --git a/lisp/newcomment.el b/lisp/newcomment.el
index 4b261c3..44dd7e1 100644
--- a/lisp/newcomment.el
+++ b/lisp/newcomment.el
@@ -1381,10 +1381,9 @@ comment-indent-new-line
   (interactive)
   (comment-normalize-vars t)
   (let (compos comin)
-    ;; If we are not inside a comment and we only auto-fill comments,
-    ;; don't do anything (unless no comment syntax is defined).
+    ;; If we are not inside a comment don't do anything (unless no
+    ;; comment syntax is defined).
     (unless (and comment-start
-		 comment-auto-fill-only-comments
 		 (not (called-interactively-p 'interactive))
 		 (not (save-excursion
 			(prog1 (setq compos (comment-beginning))
diff --git a/lisp/simple.el b/lisp/simple.el
index ea3a495..33a4082 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7191,6 +7191,13 @@ default-indent-new-line
        ;; If we're not inside a comment, just try to indent.
        (t (indent-according-to-mode))))))
 
+(defun internal-auto-fill ()
+  "The function called by `self-insert-command' to perform auto-filling."
+  (when (or (not comment-start)
+            (not comment-auto-fill-only-comments)
+            (nth 4 (syntax-ppss)))
+    (do-auto-fill)))
+
 (defvar normal-auto-fill-function 'do-auto-fill
   "The function to use for `auto-fill-function' if Auto Fill mode is turned on.
 Some major modes set this.")
diff --git a/src/cmds.c b/src/cmds.c
index 51652d5..6f2db86 100644
--- a/src/cmds.c
+++ b/src/cmds.c
@@ -268,9 +268,10 @@ Whichever character you type to run this command is inserted.
 The numeric prefix argument N says how many times to repeat the insertion.
 Before insertion, `expand-abbrev' is executed if the inserted character does
 not have word syntax and the previous character in the buffer does.
-After insertion, the value of `auto-fill-function' is called if the
-`auto-fill-chars' table has a non-nil value for the inserted character.
-At the end, it runs `post-self-insert-hook'.  */)
+After insertion, `internal-auto-fill' is called if
+`auto-fill-function' is non-nil and if the `auto-fill-chars' table has
+a non-nil value for the inserted character.  At the end, it runs
+`post-self-insert-hook'.  */)
   (Lisp_Object n)
 {
   CHECK_NUMBER (n);
@@ -475,7 +476,7 @@ internal_self_insert (int c, EMACS_INT n)
 	   that.  Must have the newline in place already so filling and
 	   justification, if any, know where the end is going to be.  */
 	SET_PT_BOTH (PT - 1, PT_BYTE - 1);
-      auto_fill_result = call0 (BVAR (current_buffer, auto_fill_function));
+      auto_fill_result = call0 (Qinternal_auto_fill);
       /* Test PT < ZV in case the auto-fill-function is strange.  */
       if (c == '\n' && PT < ZV)
 	SET_PT_BOTH (PT + 1, PT_BYTE + 1);
@@ -494,6 +495,8 @@ internal_self_insert (int c, EMACS_INT n)
 void
 syms_of_cmds (void)
 {
+  DEFSYM (Qinternal_auto_fill, "internal-auto-fill");
+
   DEFSYM (Qundo_auto_amalgamate, "undo-auto-amalgamate");
   DEFSYM (Qundo_auto__this_command_amalgamating,
           "undo-auto--this-command-amalgamating");

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Stefan Monnier]

>    (let (compos comin)
> -    ;; If we are not inside a comment and we only auto-fill comments,
> -    ;; don't do anything (unless no comment syntax is defined).
> +    ;; If we are not inside a comment don't do anything (unless no
> +    ;; comment syntax is defined).
>      (unless (and comment-start
> -		 comment-auto-fill-only-comments
>  		 (not (called-interactively-p 'interactive))
>  		 (not (save-excursion
>  			(prog1 (setq compos (comment-beginning))

I think we want to remove the whole test: this `unless` is there
specifically in order to do nothing if we're outside of comments and
comment-auto-fill-only-comments is non-nil.  But now the
"comment-auto-fill-only-comments is non-nil" case is already handled
outside, so we can just always do the auto-fill as if
comment-auto-fill-only-comments were nil.


        Stefan

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Tom Tromey]

>    (let (compos comin)
> -    ;; If we are not inside a comment and we only auto-fill comments,
> -    ;; don't do anything (unless no comment syntax is defined).
> +    ;; If we are not inside a comment don't do anything (unless no
> +    ;; comment syntax is defined).
>      (unless (and comment-start
> -		 comment-auto-fill-only-comments
>  		 (not (called-interactively-p 'interactive))
>  		 (not (save-excursion
>  			(prog1 (setq compos (comment-beginning))

Stefan> I think we want to remove the whole test: this `unless` is there
Stefan> specifically in order to do nothing if we're outside of comments and
Stefan> comment-auto-fill-only-comments is non-nil.  But now the
Stefan> "comment-auto-fill-only-comments is non-nil" case is already handled
Stefan> outside, so we can just always do the auto-fill as if
Stefan> comment-auto-fill-only-comments were nil.

I looked at this, but I don't really understand this code, and there
were more spots in the function that seemed to be affected by the setq
in the snippet above; so consequently I think someone else ought to do
this, and for my part I'm dropping the patch.

thanks,
Tom

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Stefan Monnier]

> I looked at this, but I don't really understand this code, and there
> were more spots in the function that seemed to be affected by the setq
> in the snippet above; so consequently I think someone else ought to do
> this, and for my part I'm dropping the patch.

OK, we can simplify this code later,


        Stefan

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Tom Tromey]

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

[resurrecting an old thread]

>> I looked at this, but I don't really understand this code, and there
>> were more spots in the function that seemed to be affected by the setq
>> in the snippet above; so consequently I think someone else ought to do
>> this, and for my part I'm dropping the patch.

Stefan> OK, we can simplify this code later,

I keep getting bitten by this problem and I wonder whether here you
meant that I could check in the patch as-is, without fixing up all of
comment-indent-new-line.

Also, I still wonder whether prog-mode should set
comment-auto-fill-only-comments to t.  I tend to think so.

thanks,
Tom

Re: Smie-auto-fill doesn’t respect comment-auto-fill-only-comments

[Stefan Monnier]

> I keep getting bitten by this problem and I wonder whether here you
> meant that I could check in the patch as-is, without fixing up all of
> comment-indent-new-line.

Yes, that's what I meant.

> Also, I still wonder whether prog-mode should set
> comment-auto-fill-only-comments to t.  I tend to think so.

Personally, I'm fine with changing the default like you suggest.
But let's keep it as a separate question.


        Stefan