bug#69914: comint-strip-ctrl-m doesn't function as documentation states

bug#69914: comint-strip-ctrl-m doesn't function as documentation states

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

Hey folks,

There appears to either be a bug or just inaccurate documentation of =comint-strip-ctrl-m=. At the very bottom, I've included some context about my use case by which I discovered this bug that may or may not be relevant to you. The documentation for that function states:

#+begin_quote
Strip trailing ^M characters from the current output group.

This function could be on comint-output-filter-functions or bound to a key.
#+end_quote

=comint-output-filter-functions= states the following:

#+begin_quote
...These functions get one argument, a string containing the text as originally
inserted.  Note that this might not be the same as the buffer contents between
comint-last-output-start and the buffer's process-mark, if other filter
functions have already modified the buffer.
#+end_quote

Looking at the implementation of =comint-strip-ctrl-m= it appears that it completely ignores the =string= argument and instead uses =(get-buffer-process (current-buffer))= in direct contradiction to the documentation.

#+begin_src emacs-lisp
(defun comint-strip-ctrl-m (&optional _string interactive)
  "Strip trailing `^M' characters from the current output group.
This function could be on `comint-output-filter-functions' or bound to a key."
  (interactive (list nil t))
  (let ((process (get-buffer-process (current-buffer))))
    (if (not process)
        ;; This function may be used in
        ;; `comint-output-filter-functions', and in that case, if
        ;; there's no process, then we should do nothing.  If
        ;; interactive, report an error.
        (when interactive
          (error "No process in the current buffer"))
      ;;; rest omitted for brevity
      )))
#+end_src

This represents unexpected and undocumented behavior, as you anticipate =comint-strip-ctrl-m= to behave like any other comint output filter functions. I'd like to propose 3 different possible solutions for a patch and would like input on which is preferred as this code was originally introduced in 1994. I can submit a patch once a solution has been determined.

1. Update the documentation and leave as is. This is the simplest solution and would just require doc-string updates to indicate that =comint-strip-ctrl-m= is a "unique" filter function among the other filter functions that exist. This does not seem preferable to me.

2. Update the implementation of =comint-strip-ctrl-m= itself to conform it to the documented API. This would mean anything currently depending on it reading the =current-buffer= would break, and since there are plenty of unknowns in that regard, this also does not seem preferable.

3. Add a new version of the function with a different name that conforms to the documented API =comint-strip-ctrl-m-output= or something similar and deprecate the original.

If we do decide to deprecate the original, I'm happy to include a deprecation warning and keep an eye on it popping up in core to ensure that we handle those issues over time.

Any guidance would be useful. Thank you all for your hard work.

- Jonathan

PS: Additional Context as promised:

I was developing a package that runs SQL queries in a "hidden" SQLi buffer and so I needed to strip carriage return characters out of the output. Using this filter I had thought it would perform the task, but it did not. So digging through the documentation I discovered this error. I think it's pretty reasonable that filter functions conform to the documented api or should at least be noted otherwise.

bug#69914: comint-strip-ctrl-m doesn't function as documentation states

[Eli Zaretskii]

> Date: Wed, 20 Mar 2024 14:15:39 +0000
> From:  Jonathan via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> There appears to either be a bug or just inaccurate documentation of =comint-strip-ctrl-m=. At the very bottom, I've included some context about my use case by which I discovered this bug that may or may not be relevant to you. The documentation for that function states:
> 
> #+begin_quote
> Strip trailing ^M characters from the current output group.
> 
> This function could be on comint-output-filter-functions or bound to a key.
> #+end_quote
> 
> =comint-output-filter-functions= states the following:
> 
> #+begin_quote
> ...These functions get one argument, a string containing the text as originally
> inserted.  Note that this might not be the same as the buffer contents between
> comint-last-output-start and the buffer's process-mark, if other filter
> functions have already modified the buffer.
> #+end_quote
> 
> Looking at the implementation of =comint-strip-ctrl-m= it appears that it completely ignores the =string= argument and instead uses =(get-buffer-process (current-buffer))= in direct contradiction to the documentation.

Actually, AFAICT, almost all of the filtering functions intended for
comint-output-filter-functions ignore its string argument.  Isn't that
so?

> #+begin_src emacs-lisp
> (defun comint-strip-ctrl-m (&optional _string interactive)
>   "Strip trailing `^M' characters from the current output group.
> This function could be on `comint-output-filter-functions' or bound to a key."
>   (interactive (list nil t))
>   (let ((process (get-buffer-process (current-buffer))))
>     (if (not process)
>         ;; This function may be used in
>         ;; `comint-output-filter-functions', and in that case, if
>         ;; there's no process, then we should do nothing.  If
>         ;; interactive, report an error.
>         (when interactive
>           (error "No process in the current buffer"))
>       ;;; rest omitted for brevity
>       )))
> #+end_src
> 
> This represents unexpected and undocumented behavior, as you anticipate =comint-strip-ctrl-m= to behave like any other comint output filter functions. I'd like to propose 3 different possible solutions for a patch and would like input on which is preferred as this code was originally introduced in 1994. I can submit a patch once a solution has been determined.

Given that almost all the filter functions behave the same (unless you
disagree), it sounds like ignoring the string is a de-facto standard
behavior.  So we should document that, and I guess adding a new
function, without deprecating the existing one, is the most reasonable
way ahead?

bug#69914: comint-strip-ctrl-m doesn't function as documentation states

[Eli Zaretskii]

Ping! Could you please answer my questions below?

> Cc: 69914@debbugs.gnu.org
> Date: Sat, 23 Mar 2024 09:30:16 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Wed, 20 Mar 2024 14:15:39 +0000
> > From:  Jonathan via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > 
> > There appears to either be a bug or just inaccurate documentation of =comint-strip-ctrl-m=. At the very bottom, I've included some context about my use case by which I discovered this bug that may or may not be relevant to you. The documentation for that function states:
> > 
> > #+begin_quote
> > Strip trailing ^M characters from the current output group.
> > 
> > This function could be on comint-output-filter-functions or bound to a key.
> > #+end_quote
> > 
> > =comint-output-filter-functions= states the following:
> > 
> > #+begin_quote
> > ...These functions get one argument, a string containing the text as originally
> > inserted.  Note that this might not be the same as the buffer contents between
> > comint-last-output-start and the buffer's process-mark, if other filter
> > functions have already modified the buffer.
> > #+end_quote
> > 
> > Looking at the implementation of =comint-strip-ctrl-m= it appears that it completely ignores the =string= argument and instead uses =(get-buffer-process (current-buffer))= in direct contradiction to the documentation.
> 
> Actually, AFAICT, almost all of the filtering functions intended for
> comint-output-filter-functions ignore its string argument.  Isn't that
> so?
> 
> > #+begin_src emacs-lisp
> > (defun comint-strip-ctrl-m (&optional _string interactive)
> >   "Strip trailing `^M' characters from the current output group.
> > This function could be on `comint-output-filter-functions' or bound to a key."
> >   (interactive (list nil t))
> >   (let ((process (get-buffer-process (current-buffer))))
> >     (if (not process)
> >         ;; This function may be used in
> >         ;; `comint-output-filter-functions', and in that case, if
> >         ;; there's no process, then we should do nothing.  If
> >         ;; interactive, report an error.
> >         (when interactive
> >           (error "No process in the current buffer"))
> >       ;;; rest omitted for brevity
> >       )))
> > #+end_src
> > 
> > This represents unexpected and undocumented behavior, as you anticipate =comint-strip-ctrl-m= to behave like any other comint output filter functions. I'd like to propose 3 different possible solutions for a patch and would like input on which is preferred as this code was originally introduced in 1994. I can submit a patch once a solution has been determined.
> 
> Given that almost all the filter functions behave the same (unless you
> disagree), it sounds like ignoring the string is a de-facto standard
> behavior.  So we should document that, and I guess adding a new
> function, without deprecating the existing one, is the most reasonable
> way ahead?
> 
> 
> 
> 

bug#69914: comint-strip-ctrl-m doesn't function as documentation states

[Eli Zaretskii]

Ping! Ping!  Any interest in pursuing this issue further?  If so,
could you please answer my questions below?

> Cc: 69914@debbugs.gnu.org
> Date: Sat, 06 Apr 2024 11:58:51 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Ping! Could you please answer my questions below?
> 
> > Cc: 69914@debbugs.gnu.org
> > Date: Sat, 23 Mar 2024 09:30:16 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > 
> > > Date: Wed, 20 Mar 2024 14:15:39 +0000
> > > From:  Jonathan via "Bug reports for GNU Emacs,
> > >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > > 
> > > There appears to either be a bug or just inaccurate documentation of =comint-strip-ctrl-m=. At the very bottom, I've included some context about my use case by which I discovered this bug that may or may not be relevant to you. The documentation for that function states:
> > > 
> > > #+begin_quote
> > > Strip trailing ^M characters from the current output group.
> > > 
> > > This function could be on comint-output-filter-functions or bound to a key.
> > > #+end_quote
> > > 
> > > =comint-output-filter-functions= states the following:
> > > 
> > > #+begin_quote
> > > ...These functions get one argument, a string containing the text as originally
> > > inserted.  Note that this might not be the same as the buffer contents between
> > > comint-last-output-start and the buffer's process-mark, if other filter
> > > functions have already modified the buffer.
> > > #+end_quote
> > > 
> > > Looking at the implementation of =comint-strip-ctrl-m= it appears that it completely ignores the =string= argument and instead uses =(get-buffer-process (current-buffer))= in direct contradiction to the documentation.
> > 
> > Actually, AFAICT, almost all of the filtering functions intended for
> > comint-output-filter-functions ignore its string argument.  Isn't that
> > so?
> > 
> > > #+begin_src emacs-lisp
> > > (defun comint-strip-ctrl-m (&optional _string interactive)
> > >   "Strip trailing `^M' characters from the current output group.
> > > This function could be on `comint-output-filter-functions' or bound to a key."
> > >   (interactive (list nil t))
> > >   (let ((process (get-buffer-process (current-buffer))))
> > >     (if (not process)
> > >         ;; This function may be used in
> > >         ;; `comint-output-filter-functions', and in that case, if
> > >         ;; there's no process, then we should do nothing.  If
> > >         ;; interactive, report an error.
> > >         (when interactive
> > >           (error "No process in the current buffer"))
> > >       ;;; rest omitted for brevity
> > >       )))
> > > #+end_src
> > > 
> > > This represents unexpected and undocumented behavior, as you anticipate =comint-strip-ctrl-m= to behave like any other comint output filter functions. I'd like to propose 3 different possible solutions for a patch and would like input on which is preferred as this code was originally introduced in 1994. I can submit a patch once a solution has been determined.
> > 
> > Given that almost all the filter functions behave the same (unless you
> > disagree), it sounds like ignoring the string is a de-facto standard
> > behavior.  So we should document that, and I guess adding a new
> > function, without deprecating the existing one, is the most reasonable
> > way ahead?
> > 
> > 
> > 
> > 
> 
> 
> 
>