bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Philipp Stephani]

In *scratch*, evaluate:

(defvar foo-test-var nil)
(with-temp-buffer
  (list (list (buffer-local-value 'foo-test-var (current-buffer))
              (local-variable-p 'foo-test-var)
              (local-variable-if-set-p 'foo-test-var))
        (cl-letf (((buffer-local-value 'foo-test-var (current-buffer)) 123))
          (list (buffer-local-value 'foo-test-var (current-buffer))
                (local-variable-p 'foo-test-var)
                (local-variable-if-set-p 'foo-test-var)))
        (list (buffer-local-value 'foo-test-var (current-buffer))
              (local-variable-p 'foo-test-var)
              (local-variable-if-set-p 'foo-test-var))))

The result is:

((nil nil nil) (123 t t) (nil t t))

But expected is:

((nil nil nil) (123 t t) (nil nil nil))

i.e. the local flag of the variable should be reset.



In GNU Emacs 26.0.50 (build 19, x86_64-apple-darwin16.5.0, NS appkit-1504.82 Version 10.12.4 (Build 16E195))
 of 2017-04-23 built on p
Repository revision: a1f93c1dfa53dbe007faa09ab0c6e913e86e3ffe
Windowing system distributor 'Apple', version 10.3.1504
Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --without-xml2 --with-modules --without-pop --with-mailutils
 --enable-checking --enable-check-lisp-object-type 'CFLAGS=-ggdb3 -O0'
 MAKEINFO=/usr/local/opt/texinfo/bin/makeinfo'

Configured features:
RSVG DBUS NOTIFY ACL GNUTLS ZLIB TOOLKIT_SCROLL_BARS NS MODULES

Important settings:
  value of $LANG: de_DE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message subr-x puny seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived epg
epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils time-date tooltip eldoc
electric uniquify ediff-hook vc-hooks lisp-float-type mwheel term/ns-win
ns-win ucs-normalize mule-util term/common-win tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
elisp-mode lisp-mode prog-mode register page menu-bar rfn-eshadow
isearch timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame cl-generic cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite charscript case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind kqueue cocoa ns multi-tty make-network-process emacs)

Memory information:
((conses 16 203864 9733)
 (symbols 48 19974 1)
 (miscs 40 57 190)
 (strings 32 18048 4861)
 (string-bytes 1 589547)
 (vectors 16 34976)
 (vector-slots 8 701022 3277)
 (floats 8 52 66)
 (intervals 56 199 0)
 (buffers 976 11))

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 1181 bytes --]

Philipp Stephani <p.stephani2@gmail.com> schrieb am So., 23. Apr. 2017 um
19:14 Uhr:

>
> In *scratch*, evaluate:
>
> (defvar foo-test-var nil)
> (with-temp-buffer
>   (list (list (buffer-local-value 'foo-test-var (current-buffer))
>               (local-variable-p 'foo-test-var)
>               (local-variable-if-set-p 'foo-test-var))
>         (cl-letf (((buffer-local-value 'foo-test-var (current-buffer))
> 123))
>           (list (buffer-local-value 'foo-test-var (current-buffer))
>                 (local-variable-p 'foo-test-var)
>                 (local-variable-if-set-p 'foo-test-var)))
>         (list (buffer-local-value 'foo-test-var (current-buffer))
>               (local-variable-p 'foo-test-var)
>               (local-variable-if-set-p 'foo-test-var))))
>
> The result is:
>
> ((nil nil nil) (123 t t) (nil t t))
>
> But expected is:
>
> ((nil nil nil) (123 t t) (nil nil nil))
>
> i.e. the local flag of the variable should be reset.
>
>
It's possible to fix this (see attached patch), but at the expense of
breaking other valid use cases such as (cl-incf (buffer-local-value ...)).
Not sure whether the bug can be fixed at all without breaking other stuff.

[-- Attachment #1.2: Type: text/html, Size: 1642 bytes --]

[-- Attachment #2: 0001-Have-cl-letf-restore-buffer-local-status-Bug-26624.txt --]
[-- Type: text/plain, Size: 3996 bytes --]

From 3789b7b843ec417ae3be5d0f24409559a46bcc93 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Fri, 16 Jun 2017 22:55:52 +0200
Subject: [PATCH 1/2] Have `cl-letf' restore buffer-local status (Bug#26624)

* lisp/emacs-lisp/gv.el (buffer-local-value): Record and restore
whether the variable was buffer-local; used for `cl-letf'.
* test/lisp/emacs-lisp/gv-tests.el (gv-tests--bug26624): Add unit
test.
---
 lisp/emacs-lisp/gv.el            | 15 +++++++++---
 test/lisp/emacs-lisp/gv-tests.el | 51 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 3 deletions(-)
 create mode 100644 test/lisp/emacs-lisp/gv-tests.el

diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
index c5c12a6414..a614d675af 100644
--- a/lisp/emacs-lisp/gv.el
+++ b/lisp/emacs-lisp/gv.el
@@ -372,9 +372,18 @@ setf
 (gv-define-setter window-point (v &optional w) `(set-window-point ,w ,v))
 (gv-define-setter window-start (v &optional w) `(set-window-start ,w ,v))
 
-(gv-define-setter buffer-local-value (val var buf)
-  (macroexp-let2 nil v val
-    `(with-current-buffer ,buf (set (make-local-variable ,var) ,v))))
+(gv-define-expander buffer-local-value
+  (lambda (do var buf)
+    (macroexp-let2* nil ((var var) (buf buf))
+      (funcall do
+               `(if (local-variable-p ,var ,buf)
+                    (buffer-local-value ,var ,buf)
+                  #1='#:unbound)
+               (lambda (val)
+                 `(with-current-buffer ,buf
+                    (if (eq ,val #1#)
+                        (kill-local-variable ,var)
+                      (set (make-local-variable ,var) ,val))))))))
 
 (gv-define-expander alist-get
   (lambda (do key alist &optional default remove)
diff --git a/test/lisp/emacs-lisp/gv-tests.el b/test/lisp/emacs-lisp/gv-tests.el
new file mode 100644
index 0000000000..b49c12ddf2
--- /dev/null
+++ b/test/lisp/emacs-lisp/gv-tests.el
@@ -0,0 +1,51 @@
+;;; gv-tests.el --- unit tests for gv.el             -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2017 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;; Unit tests for lisp/emacs-lisp/gv.el.
+
+;;; Code:
+
+(defvar gv-tests--var 'default "Test variable.")
+
+(ert-deftest gv-tests--bug26624 ()
+  "Checks that Bug#26624 is fixed."
+  (with-temp-buffer
+    (let ((var-calls 0) (buf-calls 0))
+      (should (equal (buffer-local-value 'gv-tests--var (current-buffer))
+                     'default))
+      (should-not (local-variable-p 'gv-tests--var))
+      (should-not (local-variable-if-set-p 'gv-tests--var))
+      (cl-letf (((buffer-local-value
+                  (progn (cl-incf var-calls) 'gv-tests--var)
+                  (progn (cl-incf buf-calls) (current-buffer)))
+                 'unbound))
+        (should (equal (buffer-local-value 'gv-tests--var (current-buffer))
+                       'unbound))
+        (should (local-variable-p 'gv-tests--var))
+        (should (local-variable-if-set-p 'gv-tests--var)))
+      (should (equal (buffer-local-value 'gv-tests--var (current-buffer))
+                     'default))
+      (should-not (local-variable-p 'gv-tests--var))
+      (should-not (local-variable-if-set-p 'gv-tests--var))
+      (should (equal var-calls 1))
+      (should (equal buf-calls 1)))))
+
+;;; gv-tests.el ends here
-- 
2.13.1

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[npostavs]

Philipp Stephani <p.stephani2@gmail.com> writes:

> It's possible to fix this (see attached patch), but at the expense of
> breaking other valid use cases such as (cl-incf (buffer-local-value ...)).
> Not sure whether the bug can be fixed at all without breaking other
> stuff.

I don't really understand the internals of the gv expander stuff, but
maybe we could special case the fix to cl-letf?

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Michael Heerdegen]

Philipp Stephani <p.stephani2@gmail.com> writes:

> It's possible to fix this (see attached patch), but at the expense of
> breaking other valid use cases such as (cl-incf (buffer-local-value
> ...)). Not sure whether the bug can be fixed at all without breaking
> other stuff.

I have no solution, but some thoughts.

The more I think about it, the more I come to the conclusion that
`buffer-local-value' does not have a well defined according place.

The function `buffer-local-value' is not injective: it maps different
states to the same value because it can't express whether the VARIABLE's
binding is buffer-local or not.  But we need this information because we
need to undo creating a buffer local binding in the setter when closing
the `letf'.

And the setter, accepting only a value for the binding, isn't
surjective, because the argument doesn't hold any information of
buffer-localness.  Moreover, we want the setter to always create a
buffer-local binding in one situation (setf), but this isn't true for
the setter we need to use for `cl-letf'.

We could widen the semantics of `cl-letf' to do what we want in this
case, but I'm not sure if it's worth the trouble.  Not if there are more
cases like this.


BTW, a completely different point of view would be to argument like
this: the manual describes generalized variables as "one of the many
places in Lisp memory where values can be stored".  If variable X has no
buffer local binding in some buffer B, that place in Lisp memory is the
one that holds the global binding of X.  That means that the place expression

  (buffer-local-binding X B)

is equivalent to the place expression X.

OTOH, If X has a buffer local binding in B, we can use X as getter
expression and `setq' as setter, so the place expression
(buffer-local-binding X B) is also equivalent to X.  So it is always
equivalent to just X and of no use.


Michael.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1495 bytes --]

Michael Heerdegen <michael_heerdegen@web.de> schrieb am So., 18. Juni 2017
um 06:17 Uhr:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > It's possible to fix this (see attached patch), but at the expense of
> > breaking other valid use cases such as (cl-incf (buffer-local-value
> > ...)). Not sure whether the bug can be fixed at all without breaking
> > other stuff.
>
> I have no solution, but some thoughts.
>
> The more I think about it, the more I come to the conclusion that
> `buffer-local-value' does not have a well defined according place.
>
> The function `buffer-local-value' is not injective: it maps different
> states to the same value because it can't express whether the VARIABLE's
> binding is buffer-local or not.  But we need this information because we
> need to undo creating a buffer local binding in the setter when closing
> the `letf'.
>
> And the setter, accepting only a value for the binding, isn't
> surjective, because the argument doesn't hold any information of
> buffer-localness.  Moreover, we want the setter to always create a
> buffer-local binding in one situation (setf), but this isn't true for
> the setter we need to use for `cl-letf'.
>
> We could widen the semantics of `cl-letf' to do what we want in this
> case, but I'm not sure if it's worth the trouble.  Not if there are more
> cases like this.
>
>
Thanks for this great analysis. Given this, it seems that the place
definition for `buffer-local-value' should be removed from gv.el.

[-- Attachment #2: Type: text/html, Size: 1996 bytes --]

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 1657 bytes --]

Philipp Stephani <p.stephani2@gmail.com> schrieb am So., 2. Juli 2017 um
18:53 Uhr:

> Michael Heerdegen <michael_heerdegen@web.de> schrieb am So., 18. Juni
> 2017 um 06:17 Uhr:
>
>> Philipp Stephani <p.stephani2@gmail.com> writes:
>>
>> > It's possible to fix this (see attached patch), but at the expense of
>> > breaking other valid use cases such as (cl-incf (buffer-local-value
>> > ...)). Not sure whether the bug can be fixed at all without breaking
>> > other stuff.
>>
>> I have no solution, but some thoughts.
>>
>> The more I think about it, the more I come to the conclusion that
>> `buffer-local-value' does not have a well defined according place.
>>
>> The function `buffer-local-value' is not injective: it maps different
>> states to the same value because it can't express whether the VARIABLE's
>> binding is buffer-local or not.  But we need this information because we
>> need to undo creating a buffer local binding in the setter when closing
>> the `letf'.
>>
>> And the setter, accepting only a value for the binding, isn't
>> surjective, because the argument doesn't hold any information of
>> buffer-localness.  Moreover, we want the setter to always create a
>> buffer-local binding in one situation (setf), but this isn't true for
>> the setter we need to use for `cl-letf'.
>>
>> We could widen the semantics of `cl-letf' to do what we want in this
>> case, but I'm not sure if it's worth the trouble.  Not if there are more
>> cases like this.
>>
>>
> Thanks for this great analysis. Given this, it seems that the place
> definition for `buffer-local-value' should be removed from gv.el.
>

Here's a patch that does just that.

[-- Attachment #1.2: Type: text/html, Size: 2456 bytes --]

[-- Attachment #2: 0001-Remove-buffer-local-value-as-generalized-variable-Bug-.txt --]
[-- Type: text/plain, Size: 2480 bytes --]

From 4ad3058476bdc7f5fa7e60d91c240a63e1f5f73d Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Fri, 16 Jun 2017 22:55:52 +0200
Subject: [PATCH] Remove `buffer-local-value' as generalized variable
 (Bug#26624)

* lisp/emacs-lisp/gv.el (buffer-local-value): Remove.
* lisp/files.el (file-name-non-special): Remove a stale comment.
---
 etc/NEWS              | 7 +++++++
 lisp/emacs-lisp/gv.el | 4 ----
 lisp/files.el         | 2 --
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index aacdf79b57..5b87939cf7 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -51,6 +51,13 @@ sets the XTerm window title.  The default is to set the window title.
 ** The FILENAME argument to 'file-name-base' is now mandatory and no
 longer defaults to 'buffer-file-name'.
 
+** 'buffer-local-value' is no longer a generalized variable.  This is
+because the buffer-local value of a variable always consists of two
+pieces: the actual value of the variable, and whether it is
+buffer-local.  But 'buffer-local-value' returns only one piece of
+information, which is not enough for a generalized variable.  In
+particular, 'cl-letf' can't be made to work with 'buffer-local-value'.
+
 \f
 * Lisp Changes in Emacs 27.1
 
diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
index 892d6e9716..f504ca43b0 100644
--- a/lisp/emacs-lisp/gv.el
+++ b/lisp/emacs-lisp/gv.el
@@ -367,10 +367,6 @@ setf
 (gv-define-setter window-point (v &optional w) `(set-window-point ,w ,v))
 (gv-define-setter window-start (v &optional w) `(set-window-start ,w ,v))
 
-(gv-define-setter buffer-local-value (val var buf)
-  (macroexp-let2 nil v val
-    `(with-current-buffer ,buf (set (make-local-variable ,var) ,v))))
-
 (gv-define-expander alist-get
   (lambda (do key alist &optional default remove testfn)
     (macroexp-let2 macroexp-copyable-p k key
diff --git a/lisp/files.el b/lisp/files.el
index fe7cb1a8a9..4bb611a3ba 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -7005,8 +7005,6 @@ file-name-non-special
            (when (and visit buffer-file-name)
              (setq buffer-file-name (concat "/:" buffer-file-name))))))
       (`unquote-then-quote
-       ;; We can't use `cl-letf' with `(buffer-local-value)' here
-       ;; because it wouldn't work during bootstrapping.
        (let ((buffer (current-buffer)))
          ;; `unquote-then-quote' is only used for the
          ;; `verify-visited-file-modtime' action, which takes a buffer
-- 
2.14.1

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Michael Heerdegen]

Philipp Stephani <p.stephani2@gmail.com> writes:

> Here's a patch that does just that. 

Thanks.  Looks good to me.


Michael.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Noam Postavsky]

Philipp Stephani <p.stephani2@gmail.com> writes:

> * lisp/emacs-lisp/gv.el (buffer-local-value): Remove.

Is it possible to just give an obsolete warning first?

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

Noam Postavsky <npostavs@users.sourceforge.net> schrieb am So., 24. Sep.
2017 um 17:44 Uhr:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > * lisp/emacs-lisp/gv.el (buffer-local-value): Remove.
>
> Is it possible to just give an obsolete warning first?
>

I don't think it's possible in the sense of `make-obsolete' because the
expander is not a named function.
It would be possible to use `display-warning' within the body of the
setter, but that would only annoy users.
If necessary, we might add additional code to the `setf' macro to warn
about this form in particular during byte compilation.

[-- Attachment #2: Type: text/html, Size: 1012 bytes --]

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Noam Postavsky]

Philipp Stephani <p.stephani2@gmail.com> writes:

> Noam Postavsky <npostavs@users.sourceforge.net> schrieb am So., 24.
> Sep. 2017 um 17:44 Uhr:
>
>     Philipp Stephani <p.stephani2@gmail.com> writes:
>    
>     > * lisp/emacs-lisp/gv.el (buffer-local-value): Remove.
>    
>     Is it possible to just give an obsolete warning first?
>
>
> I don't think it's possible in the sense of `make-obsolete' because
> the expander is not a named function.
> It would be possible to use `display-warning' within the body of the
> setter, but that would only annoy users.
> If necessary, we might add additional code to the `setf' macro to
> warn about this form in particular during byte compilation.

IMO, a compilation warning would be appropriate.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sun, 24 Sep 2017 13:43:20 -0400
> Cc: Michael Heerdegen <michael_heerdegen@web.de>, 26624@debbugs.gnu.org
> 
> Philipp Stephani <p.stephani2@gmail.com> writes:
> 
> > Noam Postavsky <npostavs@users.sourceforge.net> schrieb am So., 24.
> > Sep. 2017 um 17:44 Uhr:
> >
> >     Philipp Stephani <p.stephani2@gmail.com> writes:
> >    
> >     > * lisp/emacs-lisp/gv.el (buffer-local-value): Remove.
> >    
> >     Is it possible to just give an obsolete warning first?
> >
> >
> > I don't think it's possible in the sense of `make-obsolete' because
> > the expander is not a named function.
> > It would be possible to use `display-warning' within the body of the
> > setter, but that would only annoy users.
> > If necessary, we might add additional code to the `setf' macro to
> > warn about this form in particular during byte compilation.
> 
> IMO, a compilation warning would be appropriate.

I agree.  Removing some feature without due warning is not something
we should do, except in very rare cases (which this one isn't).

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Fr., 29. Sep. 2017 um 09:51 Uhr:

> > From: Noam Postavsky <npostavs@users.sourceforge.net>
> > Date: Sun, 24 Sep 2017 13:43:20 -0400
> > Cc: Michael Heerdegen <michael_heerdegen@web.de>, 26624@debbugs.gnu.org
> >
> > Philipp Stephani <p.stephani2@gmail.com> writes:
> >
> > > Noam Postavsky <npostavs@users.sourceforge.net> schrieb am So., 24.
> > > Sep. 2017 um 17:44 Uhr:
> > >
> > >     Philipp Stephani <p.stephani2@gmail.com> writes:
> > >
> > >     > * lisp/emacs-lisp/gv.el (buffer-local-value): Remove.
> > >
> > >     Is it possible to just give an obsolete warning first?
> > >
> > >
> > > I don't think it's possible in the sense of `make-obsolete' because
> > > the expander is not a named function.
> > > It would be possible to use `display-warning' within the body of the
> > > setter, but that would only annoy users.
> > > If necessary, we might add additional code to the `setf' macro to
> > > warn about this form in particular during byte compilation.
> >
> > IMO, a compilation warning would be appropriate.
>
> I agree.  Removing some feature without due warning is not something
> we should do, except in very rare cases (which this one isn't).
>

I fully agree, but I don't know how to correctly deprecate a generalized
variable. Should I add code to the byte compiler to deal with this case
explicitly?

[-- Attachment #2: Type: text/html, Size: 2283 bytes --]

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Fri, 29 Sep 2017 20:55:41 +0000
> Cc: michael_heerdegen@web.de, 26624@debbugs.gnu.org
> 
>  I agree. Removing some feature without due warning is not something
>  we should do, except in very rare cases (which this one isn't).
> 
> I fully agree, but I don't know how to correctly deprecate a generalized variable. Should I add code to the byte
> compiler to deal with this case explicitly? 

If no other idea comes up, yes, I think so.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> >  I agree. Removing some feature without due warning is not something
> >  we should do, except in very rare cases (which this one isn't).
> > 
> > I fully agree, but I don't know how to correctly deprecate a
> > generalized variable. Should I add code to the byte
> > compiler to deal with this case explicitly? 
>
> If no other idea comes up, yes, I think so.

I guess it's not really worth the time to implement an infrastructure
for gv-expander obsoletion, because we will probably make use of it only
every 150 years (estimation).  So it could be that nobody wants to do
this for quite a while.

Would it be acceptable if the gv setter of `buffer-local-value' would
just print a warning (i.e., solve it "by hand")?  Not perfect, admitted,
but still much better than leaving this unfixed.


Thanks,

Michael.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Philipp Stephani <p.stephani2@gmail.com>,  26624@debbugs.gnu.org,  npostavs@users.sourceforge.net
> Date: Tue, 26 Dec 2017 23:19:33 +0100
> 
> I guess it's not really worth the time to implement an infrastructure
> for gv-expander obsoletion, because we will probably make use of it only
> every 150 years (estimation).  So it could be that nobody wants to do
> this for quite a while.
> 
> Would it be acceptable if the gv setter of `buffer-local-value' would
> just print a warning (i.e., solve it "by hand")?  Not perfect, admitted,
> but still much better than leaving this unfixed.

Is it (better than leaving this unfixed)?

In any case, I don't think I understand the suggestion in detail.  Can
you show a patch or an idea of a patch?

Thanks.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Michael Heerdegen]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> In any case, I don't think I understand the suggestion in detail.  Can
> you show a patch or an idea of a patch?

Maybe simply something like this?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Obsolete-gv-setter-of-buffer-local-value.patch --]
[-- Type: text/x-diff, Size: 803 bytes --]

From 5bc85fadc201eb7d061fe585283f0f4e44f0d910 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Wed, 27 Dec 2017 20:46:20 +0100
Subject: [PATCH] Obsolete gv-setter of `buffer-local-value'

This solves Bug#26624.
---
 lisp/emacs-lisp/gv.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
index 777b955d90..f0aad6689d 100644
--- a/lisp/emacs-lisp/gv.el
+++ b/lisp/emacs-lisp/gv.el
@@ -370,6 +370,7 @@ setf
 (gv-define-setter window-start (v &optional w) `(set-window-start ,w ,v))
 
 (gv-define-setter buffer-local-value (val var buf)
+  (byte-compile-warn "Warning: obsolete gv-setter: `buffer-local-value'")
   (macroexp-let2 nil v val
     `(with-current-buffer ,buf (set (make-local-variable ,var) ,v))))
 
-- 
2.15.1


[-- Attachment #3: Type: text/plain, Size: 12 bytes --]



Michael.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: p.stephani2@gmail.com,  26624@debbugs.gnu.org,  npostavs@users.sourceforge.net
> Date: Wed, 27 Dec 2017 20:54:28 +0100
> 
> diff --git a/lisp/emacs-lisp/gv.el b/lisp/emacs-lisp/gv.el
> index 777b955d90..f0aad6689d 100644
> --- a/lisp/emacs-lisp/gv.el
> +++ b/lisp/emacs-lisp/gv.el
> @@ -370,6 +370,7 @@ setf
>  (gv-define-setter window-start (v &optional w) `(set-window-start ,w ,v))
>  
>  (gv-define-setter buffer-local-value (val var buf)
> +  (byte-compile-warn "Warning: obsolete gv-setter: `buffer-local-value'")
>    (macroexp-let2 nil v val
>      `(with-current-buffer ,buf (set (make-local-variable ,var) ,v))))

Does this warning pop up during bootstrap, and if so, how many times?

Thanks.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Does this warning pop up during bootstrap, and if so, how many times?

It currently would pop up five times:

| ../lisp/electric.el:Warning: Warning: obsolete gv-setter: ‘buffer-local-value’
| --------------------
| ../lisp/electric.el:Warning: Warning: obsolete gv-setter: ‘buffer-local-value’
| --------------------
| electric.el:350:40:Warning: Warning: obsolete gv-setter: ‘buffer-local-value’
| --------------------
| electric.el:580:39:Warning: Warning: obsolete gv-setter: ‘buffer-local-value’
| --------------------
| elec-pair.el:608:38:Warning: Warning: obsolete gv-setter: ‘buffer-local-value’


Yes, these would need to be treated.


Michael.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: p.stephani2@gmail.com,  26624@debbugs.gnu.org,  npostavs@users.sourceforge.net
> Date: Fri, 29 Dec 2017 15:08:41 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Does this warning pop up during bootstrap, and if so, how many times?
> 
> It currently would pop up five times:
> 
> | ../lisp/electric.el:Warning: Warning: obsolete gv-setter: ‘buffer-local-value’
> | --------------------
> | ../lisp/electric.el:Warning: Warning: obsolete gv-setter: ‘buffer-local-value’
> | --------------------
> | electric.el:350:40:Warning: Warning: obsolete gv-setter: ‘buffer-local-value’
> | --------------------
> | electric.el:580:39:Warning: Warning: obsolete gv-setter: ‘buffer-local-value’
> | --------------------
> | elec-pair.el:608:38:Warning: Warning: obsolete gv-setter: ‘buffer-local-value’
> 
> 
> Yes, these would need to be treated.

Can we treat them as part of fixing this issue?

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am Fr., 29. Dez. 2017 um 17:15 Uhr:

> > From: Michael Heerdegen <michael_heerdegen@web.de>
> > Cc: p.stephani2@gmail.com,  26624@debbugs.gnu.org,
> npostavs@users.sourceforge.net
> > Date: Fri, 29 Dec 2017 15:08:41 +0100
> >
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> > > Does this warning pop up during bootstrap, and if so, how many times?
> >
> > It currently would pop up five times:
> >
> > | ../lisp/electric.el:Warning: Warning: obsolete gv-setter:
> ‘buffer-local-value’
> > | --------------------
> > | ../lisp/electric.el:Warning: Warning: obsolete gv-setter:
> ‘buffer-local-value’
> > | --------------------
> > | electric.el:350:40:Warning: Warning: obsolete gv-setter:
> ‘buffer-local-value’
> > | --------------------
> > | electric.el:580:39:Warning: Warning: obsolete gv-setter:
> ‘buffer-local-value’
> > | --------------------
> > | elec-pair.el:608:38:Warning: Warning: obsolete gv-setter:
> ‘buffer-local-value’
> >
> >
> > Yes, these would need to be treated.
>
> Can we treat them as part of fixing this issue?
>

Yes, but I think changing them should be a separate commit because it's not
straightforward.
These are all modes that can be locally or globally enabled. I think
typically such modes would be defined using `define-minor-mode` and
`define-globalized-minor-mode`, but the electric modes are defined the
other way round, i.e. the main modes are global, and then there are local
modes that use `buffer-local-value` as mode variable. I'd suggest to turn
this around to use `define-minor-mode` for the local modes and
`define-globalized-minor-mode` for the global ones. Would that have any
downsides?

[-- Attachment #2: Type: text/html, Size: 2418 bytes --]

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Michael Heerdegen]

Philipp Stephani <p.stephani2@gmail.com> writes:

> Thanks for this great analysis. Given this, it seems that the place
> definition for `buffer-local-value' should be removed from gv.el.

But hmm - surely there are other functions where `cl-letf' doesn't
exactly restore the previous state - `alist-get', for example:

#+begin_src emacs-lisp
(setq my-alist '((x . 1)))
(ignore (cl-letf (((alist-get 'y my-alist) 17)) my-alist))
my-alist
==> ((y) (x . 1))
#+end_src

(admittedly, this is not as serious as the `buffer-local-value' case).
Also, as another, different problematic case, the manual warns about
`point' to be used with `cl-letf'.

So, I wonder if, instead of removing the gv-setter definition for
`buffer-local-value', we should instead add some more text about how
`cl-letf' can have surprising effects to the manual.


Michael.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

Michael Heerdegen <michael_heerdegen@web.de> schrieb am Mi., 24. Jan. 2018
um 15:33 Uhr:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > Thanks for this great analysis. Given this, it seems that the place
> > definition for `buffer-local-value' should be removed from gv.el.
>
> But hmm - surely there are other functions where `cl-letf' doesn't
> exactly restore the previous state - `alist-get', for example:
>
> #+begin_src emacs-lisp
> (setq my-alist '((x . 1)))
> (ignore (cl-letf (((alist-get 'y my-alist) 17)) my-alist))
> my-alist
> ==> ((y) (x . 1))
> #+end_src
>
> (admittedly, this is not as serious as the `buffer-local-value' case).
> Also, as another, different problematic case, the manual warns about
> `point' to be used with `cl-letf'.
>
> So, I wonder if, instead of removing the gv-setter definition for
> `buffer-local-value', we should instead add some more text about how
> `cl-letf' can have surprising effects to the manual.
>
>
>
I think we should spend significant efforts to avoid surprises. In this
case, if it means we should remove `alist-get' as well from the forms
supported by `cl-letf', then I think that's what we should do. The
documentation for `cl-letf' clearly states: "On exit, either normally or
because of a ‘throw’ or error, the PLACEs are set back to their original
values." If it can't do that for some place form, it shouldn't be allowed.

[-- Attachment #2: Type: text/html, Size: 1925 bytes --]

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Michael Heerdegen]

Philipp Stephani <p.stephani2@gmail.com> writes:

>  #+begin_src emacs-lisp
>  (setq my-alist '((x . 1)))
>  (ignore (cl-letf (((alist-get 'y my-alist) 17)) my-alist))
>  my-alist
>  ==> ((y) (x . 1))
>  #+end_src

> I think we should spend significant efforts to avoid surprises. In
> this case, if it means we should remove `alist-get' as well from the
> forms supported by `cl-letf', then I think that's what we should
> do. The documentation for `cl-letf' clearly states: "On exit, either
> normally or because of a ‘throw’ or error, the PLACEs are set back to
> their original values." If it can't do that for some place form, it
> shouldn't be allowed.

But

  (alist-get value my-alist)

doesn't change for any value (especially for y), so the alist, or the
`alist-get' place expressions, aren't effectively changed.  The object
that represents the alist changes, however.  Is that a problem or an
internal implementation detail?


Michael.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Philipp Stephani]

[-- Attachment #1: Type: text/plain, Size: 2042 bytes --]

Michael Heerdegen <michael_heerdegen@web.de> schrieb am So., 4. Feb. 2018
um 22:02 Uhr:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> >  #+begin_src emacs-lisp
> >  (setq my-alist '((x . 1)))
> >  (ignore (cl-letf (((alist-get 'y my-alist) 17)) my-alist))
> >  my-alist
> >  ==> ((y) (x . 1))
> >  #+end_src
>
> > I think we should spend significant efforts to avoid surprises. In
> > this case, if it means we should remove `alist-get' as well from the
> > forms supported by `cl-letf', then I think that's what we should
> > do. The documentation for `cl-letf' clearly states: "On exit, either
> > normally or because of a ‘throw’ or error, the PLACEs are set back to
> > their original values." If it can't do that for some place form, it
> > shouldn't be allowed.
>
> But
>
>   (alist-get value my-alist)
>
> doesn't change for any value (especially for y), so the alist, or the
> `alist-get' place expressions, aren't effectively changed.  The object
> that represents the alist changes, however.  Is that a problem or an
> internal implementation detail?
>
>
>
Since it affects user-visible behavior, I wouldn't classify it as internal
implementation detail.
It seems to me that the approach that `cl-letf` takes is too naive: binding
a generalized variable is never the same as setting it and later resetting
it to the previous value, not even for simple dynamic symbols (consider
unbound variables). Rather than having `(cl-letf ((place val)) body)`
expand to

(let ((oldval place))
  (setf place val)
  (unwind-protect body
    (setf place oldval)))

it should rather expand to

(let ((old-state (internal-get-state place)))
  (setf place val)
  (unwind-protect body
    (internal-reset-state place old-state)))

with suitably defined `internal-get-state` and `internal-reset-state`. For
most use cases `internal-get-state` and `internal-reset-state` could just
be `identity` and `setf`, but for the cases discussed here they would
contain additional information.

[-- Attachment #2: Type: text/html, Size: 2697 bytes --]

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Michael Heerdegen]

Philipp Stephani <p.stephani2@gmail.com> writes:

> it should rather expand to
>
> (let ((old-state (internal-get-state place)))
>   (setf place val)
>   (unwind-protect body
>     (internal-reset-state place old-state)))
>
> with suitably defined `internal-get-state` and
> `internal-reset-state`. For most use cases `internal-get-state` and
> `internal-reset-state` could just be `identity` and `setf `, but for
> the cases discussed here they would contain additional information.

Is that even well-defined?  What happens when the code inside `letf'
also alters this state?

For example, code like

#+begin_src emacs-lisp
(let ((my-alist '((x 1))))
  (cl-letf (((alist-get 'y my-alist) 2))
    (push (cons 'y 17) my-alist))
  my-alist)
#+end_src

or

#+begin_src emacs-lisp
(cl-letf (((buffer-local-value 'x my-buffer) 20))
  ...
  (with-current-buffer my-buffer
    (set (make-local-variable 'x) 0))
  ...)
#+end_src

what would "reset the state" mean?


Michael.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
>> * lisp/emacs-lisp/gv.el (buffer-local-value): Remove.
>
> Is it possible to just give an obsolete warning first?

I've added a mechanism for obsoletion, and I've now followed Michael's
recommendation about buffer-local-value not being well-defined as a
generalized variable, and obsoleted it in Emacs 29.

I'm therefore closing this bug report.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I've added a mechanism for obsoletion, and I've now followed Michael's
> recommendation about buffer-local-value not being well-defined as a
> generalized variable, and obsoleted it in Emacs 29.

It turns out that while not well-defined, it's useful here:

(define-minor-mode electric-indent-local-mode
  "Toggle `electric-indent-mode' only in this buffer."
  :variable (buffer-local-value 'electric-indent-mode (current-buffer))

Rewriting this to avoid this is slightly cumbersome, it turns out.  So
I'm not sure it's worth obsoleting the form, and we just have to live
with the somewhat odd semantics.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> It turns out that while not well-defined, it's useful here:
>
> (define-minor-mode electric-indent-local-mode
>   "Toggle `electric-indent-mode' only in this buffer."
>   :variable (buffer-local-value 'electric-indent-mode (current-buffer))
>
> Rewriting this to avoid this is slightly cumbersome, it turns out.

Why not use the (GET . SET) syntax for :variable?

Michael.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> (define-minor-mode electric-indent-local-mode
>>   "Toggle `electric-indent-mode' only in this buffer."
>>   :variable (buffer-local-value 'electric-indent-mode (current-buffer))
>>
>> Rewriting this to avoid this is slightly cumbersome, it turns out.
>
> Why not use the (GET . SET) syntax for :variable?

Let's see...  would this work?

:variable (electric-indent-mode .
           (lambda (val) (setq-local electric-indent-mode val)))
           

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Let's see...  would this work?
>
> :variable (electric-indent-mode .
>            (lambda (val) (setq-local electric-indent-mode val)))

Looks good - similar specs are already used in other places.

BTW: I find the definition of `electric-indent-local-mode' inelegant:
the handling of the variable is split between the :variable spec and the
body.  The body enables the global mode and sets the global variable
back to nil - quite hackish.

Anyway, if this is needed in more places it cries for a
`define-localized-mode'.

Michael.

bug#26624: 26.0.50; Generalized variable `buffer-local-value' does't restore local flag

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> :variable (electric-indent-mode .
>>            (lambda (val) (setq-local electric-indent-mode val)))
>
> Looks good - similar specs are already used in other places.

Now pushed to Emacs 29 (and I've made buffer-local-value obsolete again
as a generalized variable).

> BTW: I find the definition of `electric-indent-local-mode' inelegant:
> the handling of the variable is split between the :variable spec and the
> body.  The body enables the global mode and sets the global variable
> back to nil - quite hackish.

Yes, it's not ideal at all.

> Anyway, if this is needed in more places it cries for a
> `define-localized-mode'.

I hope it's not used a lot -- we'd rather have modes work in the
opposite direction.  That is, the mode is local, and then we have
globalized versions of it.