Bugs related to buffer-local tags-file-name

Bugs related to buffer-local tags-file-name

[Josiah Schwab]

Hello,

There is a collection of issues related to setting buffer-local values
of tags-file-name (either directly with setq-local or via
visit-tags-table).  Many of these have been around for a long time.

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=158
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=17326
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=23164

These bugs (and bugs merged into them) have clear steps to reproduce the
issues, as well as decent analysis (and in some cases proposed patches).

This morning, I reproduced a number of these with current release
version of (emacs 25.1) and the development version (git
c542fab20f53374dd0c71dce949ef370fc76fa48).

Could someone who understands etags.el take a look at these bugs?  It
seems possible that some minor refactoring might kill several of these
at once.

Josiah

P.S.  I posted to debbugs, but my simple set-up and some uniform
instructions are also available at

  https://github.com/jschwab/emacs-tags-issues

Re: Bugs related to buffer-local tags-file-name

[Dmitry Gutov]

Hi!

On 22.11.2016 20:53, Josiah Schwab wrote:

> There is a collection of issues related to setting buffer-local values
> of tags-file-name (either directly with setq-local or via
> visit-tags-table).  Many of these have been around for a long time.
>
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=158
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=17326
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=23164
>
> These bugs (and bugs merged into them) have clear steps to reproduce the
> issues, as well as decent analysis (and in some cases proposed patches).

If you could try the one-line patch from 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=23164#23 and verify which 
of the bugs it fixes (and whether it makes anything worse), it would 
very helpful.

> This morning, I reproduced a number of these with current release
> version of (emacs 25.1) and the development version (git
> c542fab20f53374dd0c71dce949ef370fc76fa48).
>
> Could someone who understands etags.el take a look at these bugs?  It
> seems possible that some minor refactoring might kill several of these
> at once.

I'm not sure such someone exists anymore. Also see my comment in 
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21443#28.

Dmitry.

Re: Bugs related to buffer-local tags-file-name

[Josiah Schwab]

Hi Dmitry,

Thanks for your message.

> If you could try the one-line patch from
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=23164#23 and verify
> which of the bugs it fixes (and whether it makes anything worse), it
> would very helpful.

I applied your patch against git master and tested again.

It solves bug #23164, but has no effect on the other bugs.

> I'm not sure such someone exists anymore. Also see my comment in
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21443#28.

Thanks for pointing me to this.  Sounds like it may be a lost cause.

That also makes me feel a little better for being horribly confused as I
tried to understand etags.el :)

Josiah

Re: Bugs related to buffer-local tags-file-name

[Eli Zaretskii]

> From: Josiah Schwab <jschwab@gmail.com>
> Date: Tue, 22 Nov 2016 17:54:05 -0800
> Cc: emacs-devel@gnu.org
> 
> > If you could try the one-line patch from
> > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=23164#23 and verify
> > which of the bugs it fixes (and whether it makes anything worse), it
> > would very helpful.
> 
> I applied your patch against git master and tested again.
> 
> It solves bug #23164, but has no effect on the other bugs.
> 
> > I'm not sure such someone exists anymore. Also see my comment in
> > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=21443#28.
> 
> Thanks for pointing me to this.  Sounds like it may be a lost cause.

I'm working on these bugs.  I hope to be ready with a patch in a day
or two.  It's just a matter of getting one or two key issues right in
etags.el.

Re: Bugs related to buffer-local tags-file-name

[Dmitry Gutov]

On 24.11.2016 19:34, Eli Zaretskii wrote:

> I'm working on these bugs.  I hope to be ready with a patch in a day
> or two.  It's just a matter of getting one or two key issues right in
> etags.el.

Thanks, Eli. Looking forward to testing your patch.

Re: Bugs related to buffer-local tags-file-name

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 25 Nov 2016 00:10:18 +0200
> Cc: emacs-devel@gnu.org
> 
> On 24.11.2016 19:34, Eli Zaretskii wrote:
> 
> > I'm working on these bugs.  I hope to be ready with a patch in a day
> > or two.  It's just a matter of getting one or two key issues right in
> > etags.el.
> 
> Thanks, Eli. Looking forward to testing your patch.

Glad to help, stay tuned.

Btw, this doesn't invalidate your assessment that we lack a regular
maintainer for etags.el.  I just felt uneasy about so many unsolved
bugs, and upon closer examination, the problems look rather minor.
Volunteers are welcome to come on board and take better care of
etags.el in the future.  TIA!

Re: Bugs related to buffer-local tags-file-name

[Eli Zaretskii]

> Date: Fri, 25 Nov 2016 09:38:03 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: jschwab@gmail.com, emacs-devel@gnu.org
> 
> > From: Dmitry Gutov <dgutov@yandex.ru>
> > Date: Fri, 25 Nov 2016 00:10:18 +0200
> > Cc: emacs-devel@gnu.org
> > 
> > On 24.11.2016 19:34, Eli Zaretskii wrote:
> > 
> > > I'm working on these bugs.  I hope to be ready with a patch in a day
> > > or two.  It's just a matter of getting one or two key issues right in
> > > etags.el.
> > 
> > Thanks, Eli. Looking forward to testing your patch.
> 
> Glad to help, stay tuned.

The patch is below.  I will try to come up with a few tests later.

For the record, I found 2 issues with local tags tables:

  . Several functions called visit-tags-table-buffer in a loop, but
    since that function returns with current buffer set to the tags
    table buffer it visits, the next call in the loop starts from a
    different buffer, which doesn't DTRT when the tags table is local
    to the buffer where the user invoked the calling command.  The
    patch below teaches visit-tags-table-buffer to start from a
    certain buffer specified by the caller.

  . Somewhat orthogonal, but related problem: tags-completion-table
    was set up as a buffer-local variable in the buffer visiting the
    tags table itself, which makes no sense at all, especially when
    there's more than one tags table, and it gets in the way when the
    tags table is buffer-local.  (It somehow succeeded to work with
    global tags tables, but that's by sheer luck.)

After applying the patch below, all the test cases posted or discussed
in the referenced bugs work as I'd expect.  There's a gray area in
setting a global tags table after you set a local tags table in some
buffer, but the semantics of this is unclear to begin with, and the
results with the patch below make sense to me, as at least one
possible interpretation of what that means.

Please test.

2016-11-26  Eli Zaretskii  <eliz@gnu.org>

	* lisp/progmodes/etags.el (visit-tags-table): After
	'visit-tags-table-buffer' returns, retrieve the value of
	'tags-file-name' from the buffer we started in.  Force
	recomputation of 'tags-completion-table' next time it is used,
	since the list of tags table has changed.
	(visit-tags-table-buffer): Accept an additional optional argument
	CBUF, the buffer in which to start processing, and switch to that
	buffer if CBUF is non-nil.  All callers changed to supply a
	non-nil CBUF when they call 'visit-tags-table-buffer' in a loop.
	(tags-completion-table): Accept an argument, the buffer for which
	to build 'tags-completion-table', and that buffer's completion
	table.
	(tags-lazy-completion-table): Pass the current buffer to
	'tags-completion-table'.

diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el
index 7d4521c..0bd3695 100644
--- a/lisp/progmodes/etags.el
+++ b/lisp/progmodes/etags.el
@@ -304,19 +304,28 @@ visit-tags-table
   ;; Calling visit-tags-table-buffer with tags-file-name set to FILE will
   ;; initialize a buffer for FILE and set tags-file-name to the
   ;; fully-expanded name.
-  (let ((tags-file-name file))
+  (let ((tags-file-name file)
+        (cbuf (current-buffer)))
     (save-excursion
       (or (visit-tags-table-buffer file)
 	  (signal 'file-missing (list "Visiting tags table"
 				      "No such file or directory"
 				      file)))
-      ;; Set FILE to the expanded name.
-      (setq file tags-file-name)))
+      ;; Set FILE to the expanded name.  Do that in the buffer we
+      ;; started from, because visit-tags-table-buffer switches
+      ;; buffers after updating tags-file-name, so if tags-file-name
+      ;; is local in the buffer we started, that value is only visible
+      ;; in that buffer.
+      (setq file (with-current-buffer cbuf tags-file-name))))
   (if local
-      ;; Set the local value of tags-file-name.
-      (set (make-local-variable 'tags-file-name) file)
+      (progn
+        ;; Force recomputation of tags-completion-table.
+        (setq-local tags-completion-table nil)
+        ;; Set the local value of tags-file-name.
+        (setq-local tags-file-name file))
     ;; Set the global value of tags-file-name.
-    (setq-default tags-file-name file)))
+    (setq-default tags-file-name file)
+    (setq tags-completion-table nil)))
 
 (defun tags-table-check-computed-list ()
   "Compute `tags-table-computed-list' from `tags-table-list' if necessary."
@@ -540,17 +549,20 @@ tags-next-table
     (setq tags-file-name (car tags-table-list-pointer))))
 
 ;;;###autoload
-(defun visit-tags-table-buffer (&optional cont)
+(defun visit-tags-table-buffer (&optional cont cbuf)
   "Select the buffer containing the current tags table.
-If optional arg is a string, visit that file as a tags table.
-If optional arg is t, visit the next table in `tags-table-list'.
-If optional arg is the atom `same', don't look for a new table;
+If optional arg CONT is a string, visit that file as a tags table.
+If optional arg CONT is t, visit the next table in `tags-table-list'.
+If optional arg CONT is the atom `same', don't look for a new table;
  just select the buffer visiting `tags-file-name'.
-If arg is nil or absent, choose a first buffer from information in
+If CONT is nil or absent, choose a first buffer from information in
  `tags-file-name', `tags-table-list', `tags-table-list-pointer'.
+Optional second arg CBUF, if non-nil, specifies the initial buffer,
+which is important if that buffer has a local value of `tags-file-name'.
 Returns t if it visits a tags table, or nil if there are no more in the list."
 
   ;; Set tags-file-name to the tags table file we want to visit.
+  (if cbuf (set-buffer cbuf))
   (cond ((eq cont 'same)
 	 ;; Use the ambient value of tags-file-name.
 	 (or tags-file-name
@@ -752,28 +764,30 @@ tags-included-tables
   (or tags-included-tables
       (setq tags-included-tables (funcall tags-included-tables-function))))
 \f
-(defun tags-completion-table ()
-  "Build `tags-completion-table' on demand.
+(defun tags-completion-table (buf)
+  "Build `tags-completion-table' on demand for BUF's tags tables.
 The tags included in the completion table are those in the current
-tags table and its (recursively) included tags tables."
-  (or tags-completion-table
-      ;; No cached value for this buffer.
-      (condition-case ()
-	  (let (tables cont)
-	    (message "Making tags completion table for %s..." buffer-file-name)
-	    (save-excursion
-	      ;; Iterate over the current list of tags tables.
-	      (while (visit-tags-table-buffer cont)
-		;; Find possible completions in this table.
-                (push (funcall tags-completion-table-function) tables)
-                (setq cont t)))
-	    (message "Making tags completion table for %s...done"
-		     buffer-file-name)
-	    ;; Cache the result in a buffer-local variable.
-	    (setq tags-completion-table
-                  (nreverse (delete-dups (apply #'nconc tables)))))
-	(quit (message "Tags completion table construction aborted.")
-	      (setq tags-completion-table nil)))))
+tags table for BUF and its (recursively) included tags tables."
+  (with-current-buffer buf
+    (or tags-completion-table
+        ;; No cached value for this buffer.
+        (condition-case ()
+            (let (tables cont)
+              (message "Making tags completion table for %s..."
+                       buffer-file-name)
+              (save-excursion
+                ;; Iterate over the current list of tags tables.
+                (while (visit-tags-table-buffer cont buf)
+                  ;; Find possible completions in this table.
+                  (push (funcall tags-completion-table-function) tables)
+                  (setq cont t)))
+              (message "Making tags completion table for %s...done"
+                       buffer-file-name)
+              ;; Cache the result in a variable.
+              (setq tags-completion-table
+                    (nreverse (delete-dups (apply #'nconc tables)))))
+          (quit (message "Tags completion table construction aborted.")
+                (setq tags-completion-table nil))))))
 
 ;;;###autoload
 (defun tags-lazy-completion-table ()
@@ -784,7 +798,9 @@ tags-lazy-completion-table
           ;; If we need to ask for the tag table, allow that.
           (let ((enable-recursive-minibuffers t))
             (visit-tags-table-buffer))
-          (complete-with-action action (tags-completion-table) string pred))))))
+          (complete-with-action action
+                                (tags-completion-table buf)
+                                string pred))))))
 
 ;;;###autoload (defun tags-completion-at-point-function ()
 ;;;###autoload   (if (or tags-table-list tags-file-name)
@@ -1084,6 +1100,7 @@ find-tag-in-order
 	(case-fold-search (if (memq tags-case-fold-search '(nil t))
 			      tags-case-fold-search
 			    case-fold-search))
+        (cbuf (current-buffer))
 	)
     (save-excursion
 
@@ -1104,8 +1121,7 @@ find-tag-in-order
       (catch 'qualified-match-found
 
 	;; Iterate over the list of tags tables.
-	(while (or first-table
-		   (visit-tags-table-buffer t))
+	(while (or first-table (visit-tags-table-buffer t cbuf))
 
 	  (and first-search first-table
 	       ;; Start at beginning of tags file.
@@ -1707,25 +1723,26 @@ next-file
 	((eq initialize t)
 	 ;; Initialize the list from the tags table.
 	 (save-excursion
-	   ;; Visit the tags table buffer to get its list of files.
-	   (visit-tags-table-buffer)
-	   ;; Copy the list so we can setcdr below, and expand the file
-	   ;; names while we are at it, in this buffer's default directory.
-	   (setq next-file-list (mapcar 'expand-file-name (tags-table-files)))
-	   ;; Iterate over all the tags table files, collecting
-	   ;; a complete list of referenced file names.
-	   (while (visit-tags-table-buffer t)
-	     ;; Find the tail of the working list and chain on the new
-	     ;; sublist for this tags table.
-	     (let ((tail next-file-list))
-	       (while (cdr tail)
-		 (setq tail (cdr tail)))
-	       ;; Use a copy so the next loop iteration will not modify the
-	       ;; list later returned by (tags-table-files).
-	       (if tail
-		   (setcdr tail (mapcar 'expand-file-name (tags-table-files)))
-		 (setq next-file-list (mapcar 'expand-file-name
-					      (tags-table-files))))))))
+           (let ((cbuf (current-buffer)))
+             ;; Visit the tags table buffer to get its list of files.
+             (visit-tags-table-buffer)
+             ;; Copy the list so we can setcdr below, and expand the file
+             ;; names while we are at it, in this buffer's default directory.
+             (setq next-file-list (mapcar 'expand-file-name (tags-table-files)))
+             ;; Iterate over all the tags table files, collecting
+             ;; a complete list of referenced file names.
+             (while (visit-tags-table-buffer t cbuf)
+               ;; Find the tail of the working list and chain on the new
+               ;; sublist for this tags table.
+               (let ((tail next-file-list))
+                 (while (cdr tail)
+                   (setq tail (cdr tail)))
+                 ;; Use a copy so the next loop iteration will not modify the
+                 ;; list later returned by (tags-table-files).
+                 (if tail
+                     (setcdr tail (mapcar 'expand-file-name (tags-table-files)))
+                   (setq next-file-list (mapcar 'expand-file-name
+                                                (tags-table-files)))))))))
 	(t
 	 ;; Initialize the list by evalling the argument.
 	 (setq next-file-list (eval initialize))))
@@ -1921,8 +1938,9 @@ list-tags
     (princ (substitute-command-keys "':\n\n"))
     (save-excursion
       (let ((first-time t)
-	    (gotany nil))
-	(while (visit-tags-table-buffer (not first-time))
+	    (gotany nil)
+            (cbuf (current-buffer)))
+	(while (visit-tags-table-buffer (not first-time) cbuf)
 	  (setq first-time nil)
 	  (if (funcall list-tags-function file)
 	      (setq gotany t)))
@@ -1945,8 +1963,9 @@ tags-apropos
     (tags-with-face 'highlight (princ regexp))
     (princ (substitute-command-keys "':\n\n"))
     (save-excursion
-      (let ((first-time t))
-	(while (visit-tags-table-buffer (not first-time))
+      (let ((first-time t)
+            (cbuf (current-buffer)))
+	(while (visit-tags-table-buffer (not first-time) cbuf)
 	  (setq first-time nil)
 	  (funcall tags-apropos-function regexp))))
     (etags-tags-apropos-additional regexp))
@@ -2107,9 +2126,10 @@ etags--xref-find-definitions
          (marks (make-hash-table :test 'equal))
          (case-fold-search (if (memq tags-case-fold-search '(nil t))
                                tags-case-fold-search
-                             case-fold-search)))
+                             case-fold-search))
+         (cbuf (current-buffer)))
     (save-excursion
-      (while (visit-tags-table-buffer (not first-time))
+      (while (visit-tags-table-buffer (not first-time) cbuf)
         (setq first-time nil)
         (dolist (order-fun (cond (regexp? find-tag-regexp-tag-order)
                                  (t etags-xref-find-definitions-tag-order)))

Re: Bugs related to buffer-local tags-file-name

[Josiah Schwab]

Hello Eli,

> After applying the patch below, all the test cases posted or discussed
> in the referenced bugs work as I'd expect.

I tested this patch against git master (on top of b7f3a30) and this
resolved all three bugs mentioned in my email.

Thank you!

Josiah

Re: Bugs related to buffer-local tags-file-name

[Eli Zaretskii]

> From: Josiah Schwab <jschwab@gmail.com>
> Cc: dgutov@yandex.ru, emacs-devel@gnu.org
> Date: Sat, 26 Nov 2016 09:39:52 -0800
> 
> Hello Eli,
> 
> > After applying the patch below, all the test cases posted or discussed
> > in the referenced bugs work as I'd expect.
> 
> I tested this patch against git master (on top of b7f3a30) and this
> resolved all three bugs mentioned in my email.
> 
> Thank you!

Thanks for testing (and for persevering).  I will push this, together
with the tests and some documentation changes, in a few days, if no
problems come up.

Re: Bugs related to buffer-local tags-file-name

[Eli Zaretskii]

> From: Josiah Schwab <jschwab@gmail.com>
> Date: Sat, 26 Nov 2016 09:39:52 -0800
> Cc: emacs-devel@gnu.org, dgutov@yandex.ru
> 
> Hello Eli,
> 
> > After applying the patch below, all the test cases posted or discussed
> > in the referenced bugs work as I'd expect.
> 
> I tested this patch against git master (on top of b7f3a30) and this
> resolved all three bugs mentioned in my email.

Dmitry, should I wait for your testing as well, or should I just push
the changes?

Re: Bugs related to buffer-local tags-file-name

[Josiah Schwab]

Hi Eli,

On 29 November 2016 at 17:25 GMT, Eli Zaretskii wrote:

> should I just push the changes?

I saw that the 25.1.90 pretest was released yesterday.  After you've
pushed them, will these changes still be able make it into 25.2 or will
they have to wait until 26.1 (or whatever version is next)?

Thanks,
Josiah

Re: Bugs related to buffer-local tags-file-name

[Eli Zaretskii]

> From: Josiah Schwab <jschwab@gmail.com>
> Cc: dgutov@yandex.ru, emacs-devel@gnu.org
> Date: Tue, 29 Nov 2016 10:33:50 -0800
> 
> > should I just push the changes?
> 
> I saw that the 25.1.90 pretest was released yesterday.  After you've
> pushed them, will these changes still be able make it into 25.2 or will
> they have to wait until 26.1 (or whatever version is next)?

I'd feel uneasy about pushing this to the release branch.  For
starters, these issues don't fit the criteria for what should go into
25.2, criteria that I myself defined.  In addition, the changes are
quite pervasive, and might potentially cause problems in use cases I
didn't test.

So my intention was to push this to the master branch, which means
they will be available in Emacs 26.

Re: Bugs related to buffer-local tags-file-name

[Dmitry Gutov]

On 29.11.2016 19:25, Eli Zaretskii wrote:

> Dmitry, should I wait for your testing as well, or should I just push
> the changes?

Yes, sorry. I think you should go ahead with them, maybe with some minor 
tweaks.

My notes:

- In visit-tags-table-buffer's docstring, "optional arg CONT" should 
probably be just "CONT", all three times.

- More importantly, this patch contains a backward-incompatible change 
to the tags-completion-table function. Maybe we should consider how to 
make BUF an optional argument.

Although this might be not the end of the world: I only know of one 
external package that calls it (company-etags).

If we work around that by catching the wrong-number-of-arguments error, 
maybe the result will be to everybody's satisfaction.

Or maybe we'd just use tags-lazy-completion-table there.

- It would be great if let-binding tags-table-list around 
tags-lazy-completion-table didn't leak it to the global value either 
(which happens with company-etags). Alas, this seems to be a harder task.

Re: Bugs related to buffer-local tags-file-name

[Eli Zaretskii]

close 158
close 17326
close 23164
close 19741
thanks

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 30 Nov 2016 03:12:20 +0200
> 
> On 29.11.2016 19:25, Eli Zaretskii wrote:
> 
> > Dmitry, should I wait for your testing as well, or should I just push
> > the changes?
> 
> Yes, sorry. I think you should go ahead with them, maybe with some minor 
> tweaks.

Done.

> - In visit-tags-table-buffer's docstring, "optional arg CONT" should 
> probably be just "CONT", all three times.

Fixed.

> - More importantly, this patch contains a backward-incompatible change 
> to the tags-completion-table function. Maybe we should consider how to 
> make BUF an optional argument.

It is optional now.

> - It would be great if let-binding tags-table-list around 
> tags-lazy-completion-table didn't leak it to the global value either 
> (which happens with company-etags). Alas, this seems to be a harder task.

Right, patches welcome.

Re: Bugs related to buffer-local tags-file-name

[Dmitry Gutov]

On 01.12.2016 18:53, Eli Zaretskii wrote:

>> Yes, sorry. I think you should go ahead with them, maybe with some minor
>> tweaks.
>
> Done.
>
>> - In visit-tags-table-buffer's docstring, "optional arg CONT" should
>> probably be just "CONT", all three times.
>
> Fixed.

Thanks!