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

[Philipp Stephani <p.stephani2@gmail.com>]

[-- 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 --]