[BUG] comint-strip-ctrl-m doesn't function as documentation states

[Jonathan <public@jds.work>]

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))=.

#+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 brefity
      )))
#+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 you're 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.