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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Philipp Stephani


Philipp Stephani <[hidden email]> 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. 

0001-Have-cl-letf-restore-buffer-local-status-Bug-26624.txt (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky-2
Philipp Stephani <[hidden email]> 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?



Reply | Threaded
Open this post in threaded view
|

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

Michael Heerdegen
In reply to this post by Philipp Stephani
Philipp Stephani <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

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

Philipp Stephani


Michael Heerdegen <[hidden email]> schrieb am So., 18. Juni 2017 um 06:17 Uhr:
Philipp Stephani <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

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

Philipp Stephani


Philipp Stephani <[hidden email]> schrieb am So., 2. Juli 2017 um 18:53 Uhr:
Michael Heerdegen <[hidden email]> schrieb am So., 18. Juni 2017 um 06:17 Uhr:
Philipp Stephani <[hidden email]> 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. 

0001-Remove-buffer-local-value-as-generalized-variable-Bug-.txt (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Michael Heerdegen
Philipp Stephani <[hidden email]> writes:

> Here's a patch that does just that.

Thanks.  Looks good to me.


Michael.



Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky-2
In reply to this post by Philipp Stephani
Philipp Stephani <[hidden email]> writes:

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

Is it possible to just give an obsolete warning first?



Reply | Threaded
Open this post in threaded view
|

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

Philipp Stephani


Noam Postavsky <[hidden email]> schrieb am So., 24. Sep. 2017 um 17:44 Uhr:
Philipp Stephani <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

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

Noam Postavsky-2
Philipp Stephani <[hidden email]> writes:

> Noam Postavsky <[hidden email]> schrieb am So., 24.
> Sep. 2017 um 17:44 Uhr:
>
>     Philipp Stephani <[hidden email]> 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.




Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Noam Postavsky <[hidden email]>
> Date: Sun, 24 Sep 2017 13:43:20 -0400
> Cc: Michael Heerdegen <[hidden email]>, [hidden email]
>
> Philipp Stephani <[hidden email]> writes:
>
> > Noam Postavsky <[hidden email]> schrieb am So., 24.
> > Sep. 2017 um 17:44 Uhr:
> >
> >     Philipp Stephani <[hidden email]> 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).



Reply | Threaded
Open this post in threaded view
|

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

Philipp Stephani


Eli Zaretskii <[hidden email]> schrieb am Fr., 29. Sep. 2017 um 09:51 Uhr:
> From: Noam Postavsky <[hidden email]>
> Date: Sun, 24 Sep 2017 13:43:20 -0400
> Cc: Michael Heerdegen <[hidden email]>, [hidden email]
>
> Philipp Stephani <[hidden email]> writes:
>
> > Noam Postavsky <[hidden email]> schrieb am So., 24.
> > Sep. 2017 um 17:44 Uhr:
> >
> >     Philipp Stephani <[hidden email]> 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?  
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Fri, 29 Sep 2017 20:55:41 +0000
> Cc: [hidden email], [hidden email]
>
>  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.