Subject: Re: bug#76615: 14.0.9; error with amsmath style hooks

From: Stefan Monnier <[email-masked]>
Date: Wed, 02 Apr 2025 18:13:33 -0400

> I agree that the right approach is to make RefTeX work properly in
> non-file buffers.  To explore this, I implemented the approach you
> suggested (see the attached patch).

Looks pretty good.  Matches my expectations fairly well.
Should we push it to `master`?
AFAICT it should be safe in the sense that it should not affect behavior
in file buffers.  See comments below.

> This bug report arose from a simpler issue: C-x v v C-c C-w in a tex
> document (with AUCTeX installed) makes vc activate LaTeX-mode in a
> temporary non-file buffer, where AUCTeX style hooks attempt to load
> RefTeX (catastrophically).  For this narrow issue, the workaround
> provided by Ikumi suffices.

Yes, that was clear, thanks.

        Stefan

> @@ -456,7 +458,7 @@ reftex-isearch-isearch-search
>  ;; beginning/end of the file list, depending of the search direction.
>  (defun reftex-isearch-switch-to-next-file (crt-buf &optional wrapp)
>    (reftex-access-scan-info)
> -  (let ((cb (buffer-file-name crt-buf))
> +  (let ((cb (reftex-get-buffer-identifier crt-buf))
>  	(flist (reftex-all-document-files)))
>      (when flist
>        (if wrapp

I think we had better use the buffer object here instead of using
"buffer:<NAME>".  More generally, I think we should never convert
"buffer:<NAME>" back to a buffer.  I guess that means such a conversion
would only ever be used when displaying info for a human to read, in
which case we could just as well use `prin1`s format `#<buffer ...>`.

> --- a/lisp/textmodes/reftex-ref.el
> +++ b/lisp/textmodes/reftex-ref.el
> @@ -73,8 +73,13 @@ reftex-label-info-update
>           (file    (nth 3 cell))
>           (comment (nth 4 cell))
>           (note    (nth 5 cell))
> -         (buf (reftex-get-file-buffer-force
> -               file (not (eq t reftex-keep-temporary-buffers)))))
> +         (buf (cond
> +               ((and (stringp file)
> +                     (string-match "\\`buffer:\\(.*\\)" file))
> +                (get-buffer (match-string 1 file)))
> +               (file (reftex-get-file-buffer-force
> +                      file (not (eq t reftex-keep-temporary-buffers))))
> +               (t nil))))

So here presumably we'd use

    (cond
     ((bufferp file) file)
     [...]

and I wouldn't test `file` before calling `reftex-get-file-buffer-force`
since we didn't test it before either.

> +;;; =========================================================================
> +;;;
> +;;; Helper functions for handling both file paths and buffer objects
> +;;;
> +
> +(defun reftex-get-buffer-identifier (&optional buffer)
> +  "Return a buffer file name or a buffer identifier.
> +For file buffers, returns `buffer-file-name'.
> +For non-file buffers, returns a string with format
> +\"buffer:BUFFER-NAME\".  When BUFFER is nil, use the current buffer."
> +  (with-current-buffer (or buffer (current-buffer))
> +    (or buffer-file-name (concat "buffer:" (buffer-name)))))

So I think what I said above means this would become:

    (or (buffer-file-name buffer) buffer (current-buffer))

> +(defun reftex-get-buffer-for-master (master)
> +  "Get the buffer associated with MASTER.
> +MASTER can be a file path or a buffer object."
> +  (cond
> +   ((bufferp master) master)
> +   ((stringp master) (find-file-noselect master))
> +   (t nil)))

I wouldn't test `(stringp master)`, so we get a "clean" error if
`master` is neither a buffer nor a string.

> +(defun reftex-get-directory-for-master (master)
> +  "Get the directory associated with MASTER.
> +MASTER can be a file path or a buffer object."
> +  (cond
> +   ((bufferp master) (with-current-buffer master default-directory))
> +   ((stringp master) (file-name-directory master))
> +   (t default-directory)))

Same here and in most other functions.

> +(defun reftex-file-or-buffer-name (master)
> +  "Get a display name for MASTER.
> +Returns the filename for file masters, or the buffer name for buffer
> +masters."
> +  (cond
> +   ((bufferp master) (buffer-name master))
> +   ((stringp master) (file-name-nondirectory master))
> +   (t "")))

This doesn't seem to be used.

> +(defun reftex-abbreviate-or-get-name (master)
> +  "Get a nice display name for MASTER.
> +For files, returns the abbreviated file name.
> +For buffers, returns the buffer name."
> +  (cond
> +   ((bufferp master) (buffer-name master))
> +   ((stringp master) (abbreviate-file-name master))
> +   (t "")))

I think I'd use (prin1-to-string master) in the buffer case, just to
avoid confusing the user in case a buffer name looks like a file name.
And I'd drop the `(stringp master)` test.

> +(defun reftex-get-basename-for-master (master)
> +  "Get the base name (without extension) for MASTER.
> +For file masters, returns the file name without directory and extension.
> +For buffer objects, returns a sanitized version of the buffer name
> +suitable for use in LaTeX labels."
> +  (cond
> +   ((bufferp master)
> +    (let ((name (buffer-name master)))
> +      ;; Remove extension if present
> +      (if (string-match "\\(.*\\)\\.[^.]*\\'" name)
> +          (match-string 1 name)
> +        name)))

Why not (file-name-base (buffer-name master))?

>  ;;; =========================================================================
>  ;;;
>  ;;; Multibuffer Variables
> @@ -282,8 +354,11 @@ reftex-next-multifile-index
>  (defun reftex-tie-multifile-symbols ()
>    "Tie the buffer-local symbols to globals connected with the master file.
>  If the symbols for the current master file do not exist, they are created."
> -  (let* ((master (file-truename (reftex-TeX-master-file)))
> -         (index (assoc master reftex-master-index-list))
> +  (let* ((master (reftex-TeX-master-file))
> +         (master-key (if (bufferp master)
> +                         master
> +                       (file-truename master)))

I think you can use `reftex-get-truename-for-master` here, no?
I'd also keep the name of the second var as `master` (which just hides
the previous var of the same name), so we don't need to change the rest
of the code.

> @@ -293,7 +368,7 @@ reftex-tie-multifile-symbols
>        ;; Get a new index and add info to the alist.
>        (setq index (reftex-next-multifile-index)
>              newflag t)
> -      (push (cons master index) reftex-master-index-list))
> +      (push (cons master-key index) reftex-master-index-list))

BTW, while this works, it's a bit more at risk of creating a memory leak
where `reftex-master-index-list` keeps references to long-dead buffers.
Maybe a `kill-buffer-hook` could be used to remove entries from this list?

> @@ -377,7 +452,7 @@ reftex-TeX-master-file
>            (buffer-file-name)))))
>      (cond
>       ((null master)
> -      (error "Need a filename for this buffer, please save it first"))
> +      (current-buffer))
>       ((or (file-exists-p (concat master ".tex"))
>            (find-buffer-visiting (concat master ".tex")))
>        ;; Ahh, an extra .tex was missing...

IIRC the `master` of one buffer may end up stored in a `TeX-master`
variable of a related buffer (see `reftex-find-duplicate-labels`), so we
need to adjust the code that consults `TeX-master` to account for the
fact that it can be a buffer.  Also, it seems that `tex-main-file`
signals an error if `buffer-file-name` is nil, so we presumably need to
wrap the call to that function in the same kind of `condition-case` as
we do when calling `TeX-master`.

>      (cond
> +     ;; For non-file buffers, skip file operations but allow initialization
> +     ((and is-buffer (memq action '(readable read kill)))
> +      ;; Return appropriate values for buffer objects
> +      (cond ((eq action 'readable) nil)
> +            ((eq action 'read) nil)
> +            ((eq action 'kill) t)))
> +
>       ((eq action 'readable)
> -      (file-readable-p file))
> +      (and file (file-readable-p file)))

Here we know `file` is not a buffer, so I'd skip the `file` test.