bug#36157: 27.0.50; ediff-files does not work after lexical-binding was enabled for ediff

bug#36157: 27.0.50; ediff-files does not work after lexical-binding was enabled for ediff

[Kaushal Modi]

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

 X-Debbugs-CC:  alex.branham@gmail.com

Hello,

After lexical-binding was enabled in ediff, doing ediff-files gives this
error:

Debugger entered--Lisp error: (void-variable file-A)
  ediff-find-file(file-A buf-A ediff-last-dir-A startup-hooks)
  ediff-files-internal("~/temp/d" "~/temp/a" nil nil ediff-files)
  ediff-files("~/temp/d" "~/temp/a")
  eval((ediff-files "~/temp/d" "~/temp/a") t)
  eval-expression((ediff-files "~/temp/d" "~/temp/a") nil nil 127)
  funcall-interactively(eval-expression (ediff-files "~/temp/d" "~/temp/a")
nil nil 127)
  call-interactively(eval-expression nil nil)
  command-execute(eval-expression)

Recipe:

Take any 2 files and do M-: (ediff-files "file1" "file2")

Commit:
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=963d4e24263b0ff2add1a223f00387ca53d0658f


In GNU Emacs 27.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version 2.24.23)
 of 2019-06-09
Repository revision: e4f12a1b1ffba07cc7d6f6e1aec5de9f09af616f
Repository branch: master
Windowing system distributor 'Open Text', version 11.0.11505
System Description: Red Hat Enterprise Linux Workstation release 6.8
(Santiago)

Configured using:
 'configure --with-modules --with-imagemagick
 --prefix=/home/kmodi/usr_local/apps/6/emacs/master
 '--program-transform-name=s/^ctags$/ctags_emacs/' --with-harfbuzz
 --enable-checking=yes,glyphs --enable-check-lisp-object-type
 'CPPFLAGS=-I/home/kmodi/stowed/include
 -I/cad/adi/apps/gnu/linux/x86_64/6/include/
 -I/home/kmodi/usr_local/6/include -I/usr/include/freetype2
 -I/usr/include' 'CFLAGS=-ggdb3 -Og' 'CXXFLAGS=-ggdb3 -Og'
 'LDFLAGS=-L/home/kmodi/stowed/lib -L/home/kmodi/stowed/lib64
 -L/cad/adi/apps/gnu/linux/x86_64/6/lib/ -L/home/kmodi/usr_local/6/lib
 -L/home/kmodi/usr_local/6/lib64 -ggdb3''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS GLIB
NOTIFY INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT
LIBOTF XFT ZLIB TOOLKIT_SCROLL_BARS GTK2 X11 XDBE XIM MODULES THREADS
PDUMPER GMP

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


--
Kaushal Modi

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

bug#36157: 27.0.50; ediff-files does not work after lexical-binding was enabled for ediff

[Kaushal Modi]

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

It seems like properly enabling lexical binding in ediff will be a bit
tricky.

If you look at the code of ediff-find-file, you will see the use of
`symbol-value' everywhere.

This line is the source of the error in the bug report:

(defun ediff-find-file (file-var buffer-name &optional last-dir hooks-var)
  "Visit FILE and arrange its buffer to Ediff's liking.
...

deleted."
  (let* ((file (symbol-value file-var))  ;<------------------

I see symbol-value used at 4 places in ediff.el. So all of those will need
replaced with variable access in lexical scope.

--
Kaushal Modi

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

bug#36157: 27.0.50; ediff-files does not work after lexical-binding was enabled for ediff

[Alex Branham]

On Sun 09 Jun 2019 at 22:21, Kaushal Modi <kaushal.modi@gmail.com> wrote:

> It seems like properly enabling lexical binding in ediff will be a bit
> tricky.
>
> If you look at the code of ediff-find-file, you will see the use of
> `symbol-value' everywhere.

Indeed, it looks like portions of ediff expect symbols like file-A and
file-B to be dynamically bound, but also passes them through as
arguments to functions. I'm not sure what the best way to deal with this
is, since if we just mark them as special the lexical binding will
shadow the global binding anyway. 

In the meantime, feel free to revert that patch if its causing issues.

Alex

bug#36157: 27.0.50; ediff-files does not work after lexical-binding was enabled for ediff

[Kaushal Modi]

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

Hi Alex,

I have reverted that patch in my local emacs build.

But I am copying Stefan and Eli here for ideas so that it doesn't need to
be reverted on master.

--
Kaushal Modi


On Mon, Jun 10, 2019 at 9:41 AM Alex Branham <alex.branham@gmail.com> wrote:

>
> On Sun 09 Jun 2019 at 22:21, Kaushal Modi <kaushal.modi@gmail.com> wrote:
>
> > It seems like properly enabling lexical binding in ediff will be a bit
> > tricky.
> >
> > If you look at the code of ediff-find-file, you will see the use of
> > `symbol-value' everywhere.
>
> Indeed, it looks like portions of ediff expect symbols like file-A and
> file-B to be dynamically bound, but also passes them through as
> arguments to functions. I'm not sure what the best way to deal with this
> is, since if we just mark them as special the lexical binding will
> shadow the global binding anyway.
>
> In the meantime, feel free to revert that patch if its causing issues.
>
> Alex
>

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

bug#36157: 27.0.50; ediff-files does not work after lexical-binding was enabled for ediff

[Eli Zaretskii]

> From: Alex Branham <alex.branham@gmail.com>
> Date: Mon, 10 Jun 2019 08:41:50 -0500
> Cc: 36157@debbugs.gnu.org
> 
> > It seems like properly enabling lexical binding in ediff will be a bit
> > tricky.
> >
> > If you look at the code of ediff-find-file, you will see the use of
> > `symbol-value' everywhere.
> 
> Indeed, it looks like portions of ediff expect symbols like file-A and
> file-B to be dynamically bound, but also passes them through as
> arguments to functions. I'm not sure what the best way to deal with this
> is, since if we just mark them as special the lexical binding will
> shadow the global binding anyway. 
> 
> In the meantime, feel free to revert that patch if its causing issues.

If there are no ideas how to fix this in a week or so, I suggest to
revert the changeset while we consider the possible solutions, and add
a comment that converting these files to lexical-binding is hairy.

Thanks.

bug#36157: 27.0.50; ediff-files does not work after lexical-binding was enabled for ediff

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > If you look at the code of ediff-find-file, you will see the use of
  > `symbol-value' everywhere.

Which variables would this operate on?

If they are file-local variables, won't they have to be dynamic?
Perhaps that code need not be concerned about lexical bindings.


(Someone should check this for security too.)

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#36157: 27.0.50; ediff-files does not work after lexical-binding was enabled for ediff

[Alex Branham]

On Mon 10 Jun 2019 at 21:26, Richard Stallman <rms@gnu.org> wrote:

>   > If you look at the code of ediff-find-file, you will see the use of
>   > `symbol-value' everywhere.
>
> Which variables would this operate on?

I'm not 100% sure what you're asking here, but ediff uses variables to
store information and then passes those to functions. So in one function
it'll do something like this:

(setq file-A "/path/to/foo.el")
(setq buff-A (get-buffer-create "foo.el"))
(ediff-find-file file-A buff-A)

And then ediff-find-file uses (symbol-value 'file-A) to get the string
back. This breaks under lexical binding since the file-A argument
shadows the file-A global value.

I think it would be relatively easy to fix by just passing the values
themselves, but that'll break backwards compatibility if people outside
Emacs are relying on calling ediff functions this way.

Hope that clears things up,
Alex

bug#36157: 27.0.50; ediff-files does not work after lexical-binding was enabled for ediff

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > I'm not 100% sure what you're asking here, but ediff uses variables to
  > store information and then passes those to functions. So in one function
  > it'll do something like this:

  > (setq file-A "/path/to/foo.el")
  > (setq buff-A (get-buffer-create "foo.el"))
  > (ediff-find-file file-A buff-A)

It should add prefixes to those variable names, to avoid conflict.

Then it could defvar those variables to make them always-dynamic.

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

bug#36157: 27.0.50; ediff-files does not work after lexical-binding was enabled for ediff

[Glenn Morris]

Eli Zaretskii wrote:

> If there are no ideas how to fix this in a week or so, I suggest to
> revert the changeset while we consider the possible solutions,

Done in ef23c8b.