bug#29857: 27.0.50; error: "Loading `nil': old-style backquotes detected!"

bug#29857: 27.0.50; error: "Loading `nil': old-style backquotes detected!"

[Michael Heerdegen]

Hello,

Insert into a fresh buffer (e.g. *scratch*):

#+begin_src emacs-lisp
`(progn
   (add-to-list 'rcirc-client-commands ,(concat "/" (symbol-name command)))
   (defun ,(intern (concat "rcirc-cmd-" (symbol-name command)))
       (,@argument &optional process target)
     ,(concat docstring "\n\nNote: If PROCESS or TARGET are nil, the values given"
	      "\nby `rcirc-buffer-process' and `rcirc-target' will be used.")
     ,interactive-form
     (let ((process (or process (rcirc-buffer-process)))
	   (target (or target rcirc-target)))
       (ignore target) ; mark `target' variable as ignorable
       ,@body)))
#+end_src

(that's the body of the `defun-rcirc-command' macro in rcirc.el).  Put
point after the starting backquote character, at the position of the
outermost opening paren.  Then (read (current-buffer)) errors with

| Debugger entered--Lisp error: (error "Loading `nil': old-style backquotes detected!")
|   read(#<buffer *scratch*>)
|   [...]

I think raising this error is not justified, as there are no old-style
backquotes there AFAIK.  Also, the error message saying loading `nil'
failed is confusing, it took me a while to find out where I tried to
load `nil' (nowhere).

Finally, let me say that `read' raising such errors about old-style
backquotes, may it be justified or not, breaks "el-search" which relies
heavily on `read' at diverse buffer positions to succeed.


TIA,

Michael.



In GNU Emacs 27.0.50 (build 25, x86_64-pc-linux-gnu, GTK+ Version 3.22.24)
 of 2017-12-21 built on drachen
Repository revision: e45499a6c1bae4577af2e86397e9bab23fd63bf1
Windowing system distributor 'The X.Org Foundation', version 11.0.11905000
System Description: Debian GNU/Linux testing (buster)

bug#29857: 27.0.50; error: "Loading `nil': old-style backquotes detected!"

[Philipp Stephani]

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

Michael Heerdegen <michael_heerdegen@web.de> schrieb am Di., 26. Dez. 2017
um 15:39 Uhr:

>
> Hello,
>
> Insert into a fresh buffer (e.g. *scratch*):
>
> #+begin_src emacs-lisp
> `(progn
>    (add-to-list 'rcirc-client-commands ,(concat "/" (symbol-name command)))
>    (defun ,(intern (concat "rcirc-cmd-" (symbol-name command)))
>        (,@argument &optional process target)
>      ,(concat docstring "\n\nNote: If PROCESS or TARGET are nil, the
> values given"
>               "\nby `rcirc-buffer-process' and `rcirc-target' will be
> used.")
>      ,interactive-form
>      (let ((process (or process (rcirc-buffer-process)))
>            (target (or target rcirc-target)))
>        (ignore target) ; mark `target' variable as ignorable
>        ,@body)))
> #+end_src
>
> (that's the body of the `defun-rcirc-command' macro in rcirc.el).  Put
> point after the starting backquote character, at the position of the
> outermost opening paren.  Then (read (current-buffer)) errors with
>
> | Debugger entered--Lisp error: (error "Loading `nil': old-style
> backquotes detected!")
> |   read(#<buffer *scratch*>)
> |   [...]
>
> I think raising this error is not justified, as there are no old-style
> backquotes there AFAIK.


It's somewhat subtle, but if you skip ` there is an oldstyle backquote
here. There's a comment in lread.c:
           Because it's more difficult to peek 2 chars ahead, a new-style
   ,@ can still not be used outside of a `, unless it's in the middle
   of a list.
That is, in the construct (,@argument the ,@ is oldstyle unless the ` has
been read before.


> Also, the error message saying loading `nil'
> failed is confusing, it took me a while to find out where I tried to
> load `nil' (nowhere).
>

It would be reasonable and easy to remove the "Loading `nil'" part in the
case where no file is being loaded.


>
> Finally, let me say that `read' raising such errors about old-style
> backquotes, may it be justified or not, breaks "el-search" which relies
> heavily on `read' at diverse buffer positions to succeed.
>
>
That's a bummer. It means that el-search currently relies on an
underspecified legacy feature. Could el-search be changed to always include
reading the initial ` in such cases?
Otherwise I'd accept introducing a variable to control whether oldstyle
backquotes should raise an error or get interpreted as newstyle. El-search
would need to adapt, though, because the newstyle interpretation is
different. However, that will need to happen anyway because at some point
in time (Emacs 28?) we want to get rid of oldstyle backquotes altogether.

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

[PATCH 1/2] Add new variable to force new-style backquote interpretation.

[Philipp Stephani]

* src/lread.c (syms_of_lread): Add new variable
'force-new-style-backquotes'.
(read_internal_start): Use it.

* test/src/lread-tests.el (lread-tests--force-new-style-backquotes):
New test.

* etc/NEWS: Document new variable.
---
 etc/NEWS                |  3 ++-
 src/lread.c             | 19 ++++++++++++++-----
 test/src/lread-tests.el |  8 ++++++++
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 64c74c0d56..33841c087d 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -181,7 +181,8 @@ support, you should set 'eldoc-documentation-function' instead of
 calling 'eldoc-message' directly.
 
 ** Old-style backquotes now generate an error.  They have been
-generating warnings for a decade.
+generating warnings for a decade.  To interpret old-style backquotes
+as new-style, bind the new variable 'force-new-style-backquotes' to t.
 
 \f
 * Lisp Changes in Emacs 27.1
diff --git a/src/lread.c b/src/lread.c
index 52897b4fcc..57cb0a5e48 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -147,10 +147,10 @@ static ptrdiff_t prev_saved_doc_string_length;
 /* This is the file position that string came from.  */
 static file_offset prev_saved_doc_string_position;
 
-/* True means inside a new-style backquote
-   with no surrounding parentheses.
-   Fread initializes this to false, so we need not specbind it
-   or worry about what happens to it when there is an error.  */
+/* True means inside a new-style backquote with no surrounding
+   parentheses.  Fread initializes this to the value of
+   `force_new_style_backquotes', so we need not specbind it or worry
+   about what happens to it when there is an error.  */
 static bool new_backquote_flag;
 
 /* A list of file names for files being loaded in Fload.  Used to
@@ -2187,7 +2187,7 @@ read_internal_start (Lisp_Object stream, Lisp_Object start, Lisp_Object end)
   Lisp_Object retval;
 
   readchar_count = 0;
-  new_backquote_flag = 0;
+  new_backquote_flag = force_new_style_backquotes;
   /* We can get called from readevalloop which may have set these
      already.  */
   if (! HASH_TABLE_P (read_objects_map)
@@ -5006,6 +5006,15 @@ Note that if you customize this, obviously it will not affect files
 that are loaded before your customizations are read!  */);
   load_prefer_newer = 0;
 
+  DEFVAR_BOOL ("force-new-style-backquotes", force_new_style_backquotes,
+               doc: /* Non-nil means to always use the current syntax for backquotes.
+If nil, `load' and `read' raise errors when encountering some
+old-style variants of backquote and comma.  If non-nil, these
+constructs are always interpreted as described in the Info node
+`(elisp)Backquotes', even if that interpretation is incompatible with
+previous versions of Emacs.  */);
+  force_new_style_backquotes = false;
+
   /* Vsource_directory was initialized in init_lread.  */
 
   DEFSYM (Qcurrent_load_list, "current-load-list");
diff --git a/test/src/lread-tests.el b/test/src/lread-tests.el
index 3f41982eba..4dfed13be7 100644
--- a/test/src/lread-tests.el
+++ b/test/src/lread-tests.el
@@ -181,6 +181,14 @@ lread-tests--last-message
                      (list (concat (format-message "Loading `%s': " file-name)
                                    "old-style backquotes detected!")))))))
 
+(ert-deftest lread-tests--force-new-style-backquotes ()
+  (let ((data (should-error (read "(` (a b))"))))
+    (should (equal (cdr data)
+                   '("Loading `nil': old-style backquotes detected!"))))
+  (should (equal (let ((force-new-style-backquotes t))
+                   (read "(` (a b))"))
+                 '(`(a b)))))
+
 (ert-deftest lread-lread--substitute-object-in-subtree ()
   (let ((x (cons 0 1)))
     (setcar x x)
-- 
2.15.1

[PATCH 2/2] Improve error message for old-style backquotes

[Philipp Stephani]

* src/lread.c (load_error_old_style_backquotes): Improve error message
if no file is being loaded.

* test/src/lread-tests.el (lread-tests--force-new-style-backquotes):
Adapt test.
---
 src/lread.c             | 12 ++++++++++--
 test/src/lread-tests.el |  3 +--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/lread.c b/src/lread.c
index 57cb0a5e48..5f8c85e769 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1006,8 +1006,16 @@ load_error_handler (Lisp_Object data)
 static _Noreturn void
 load_error_old_style_backquotes (void)
 {
-  AUTO_STRING (format, "Loading `%s': old-style backquotes detected!");
-  xsignal1 (Qerror, CALLN (Fformat_message, format, Vload_file_name));
+  if (NILP (Vload_file_name))
+    {
+      AUTO_STRING (message, "Old-style backquotes detected!");
+      xsignal1 (Qerror, message);
+    }
+  else
+    {
+      AUTO_STRING (format, "Loading `%s': old-style backquotes detected!");
+      xsignal1 (Qerror, CALLN (Fformat_message, format, Vload_file_name));
+    }
 }
 
 static void
diff --git a/test/src/lread-tests.el b/test/src/lread-tests.el
index 4dfed13be7..2ac3cdb2f0 100644
--- a/test/src/lread-tests.el
+++ b/test/src/lread-tests.el
@@ -183,8 +183,7 @@ lread-tests--last-message
 
 (ert-deftest lread-tests--force-new-style-backquotes ()
   (let ((data (should-error (read "(` (a b))"))))
-    (should (equal (cdr data)
-                   '("Loading `nil': old-style backquotes detected!"))))
+    (should (equal (cdr data) '("Old-style backquotes detected!"))))
   (should (equal (let ((force-new-style-backquotes t))
                    (read "(` (a b))"))
                  '(`(a b)))))
-- 
2.15.1

Re: [PATCH 1/2] Add new variable to force new-style backquote interpretation.

[Paul Eggert]

Why not simply take the new interpretation for backquotes? That would be simpler 
than adding a backward-compatibility variable that I suspect nobody will ever need.

Re: [PATCH 1/2] Add new variable to force new-style backquote interpretation.

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am Sa., 30. Dez. 2017 um 07:19 Uhr:

> Why not simply take the new interpretation for backquotes? That would be
> simpler
> than adding a backward-compatibility variable that I suspect nobody will
> ever need.
>

As discussed in
http://lists.gnu.org/archive/html/emacs-devel/2017-10/msg00051.html, we
(mostly) agreed to first turn them into errors because otherwise the
interpretation would suddenly silently change.

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

bug#29857: 27.0.50; error: "Loading `nil': old-style backquotes detected!"

[Michael Heerdegen]

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

> It's somewhat subtle, but if you skip ` there is an oldstyle backquote here. There's a
> comment in lread.c:
>            Because it's more difficult to peek 2 chars ahead, a new-style
>    ,@ can still not be used outside of a `, unless it's in the middle
>    of a list.
> That is, in the construct (,@argument the ,@ is oldstyle unless the ` has been read
> before.

Hmm, ok.
 
>  Also, the error message saying loading `nil'
>  failed is confusing, it took me a while to find out where I tried to
>  load `nil' (nowhere).
>
> It would be reasonable and easy to remove the "Loading `nil'" part in
> the case where no file is being loaded.

Yes, I think that would be good.
  
>  Finally, let me say that `read' raising such errors about old-style
>  backquotes, may it be justified or not, breaks "el-search" which relies
>  heavily on `read' at diverse buffer positions to succeed.
>
> That's a bummer. It means that el-search currently relies on an
> underspecified legacy feature. Could el-search be changed to always
> include reading the initial ` in such cases?

Not really.  It would mean that the "construct" after any backquote
can't be matched or replaced.

E.g. if you have a function `foo' accepting three arguments, and you
decide to change the definition of `foo' so that the meaning of the
second and third arguments are interchanged, you want to replace all
calls in your code with the rule

  `(foo ,a ,b ,c) -> `(foo ,a ,c ,b)

to adopt to the new signature.

(Note that the backquote here is part of `pcase' pattern semantics and
there is no relation with this issue).

But in this occurrence:

#+begin_src emacs-lisp
(defmacro bar (form)
  `(foo 1 1 ,@form))
#+end_src

this replacement rule would fail because the according form would be
unmatchable (and the backquoted thing doesn't match).  One could work
around this...there are always workarounds.  To need to do that would be
very bad.

> Otherwise I'd accept introducing a variable to control whether
> oldstyle backquotes should raise an error or get interpreted as
> newstyle.

That would be optimal for my case.

> El-search would need to adapt, though, because the newstyle
> interpretation is different.

In which way would el-search need to adapt?  It doesn't interpret code.
It is a tool for matching and transforming lists, which, in most cases,
happen to be code.  The user would need to know how `read' interprets
what's matched, of course.


Thanks,

Michael.

bug#29857: 27.0.50; error: "Loading `nil': old-style backquotes detected!"

[Philipp Stephani]

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

Michael Heerdegen <michael_heerdegen@web.de> schrieb am Sa., 30. Dez. 2017
um 15:00 Uhr:

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> >  Also, the error message saying loading `nil'
> >  failed is confusing, it took me a while to find out where I tried to
> >  load `nil' (nowhere).
> >
> > It would be reasonable and easy to remove the "Loading `nil'" part in
> > the case where no file is being loaded.
>
> Yes, I think that would be good.
>

OK, I've sent a patch to do that:
https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00901.html


>
> >  Finally, let me say that `read' raising such errors about old-style
> >  backquotes, may it be justified or not, breaks "el-search" which relies
> >  heavily on `read' at diverse buffer positions to succeed.
> >
> > That's a bummer. It means that el-search currently relies on an
> > underspecified legacy feature. Could el-search be changed to always
> > include reading the initial ` in such cases?
>
> Not really.  It would mean that the "construct" after any backquote
> can't be matched or replaced.
>
> E.g. if you have a function `foo' accepting three arguments, and you
> decide to change the definition of `foo' so that the meaning of the
> second and third arguments are interchanged, you want to replace all
> calls in your code with the rule
>
>   `(foo ,a ,b ,c) -> `(foo ,a ,c ,b)
>
> to adopt to the new signature.
>
> (Note that the backquote here is part of `pcase' pattern semantics and
> there is no relation with this issue).
>
> But in this occurrence:
>
> #+begin_src emacs-lisp
> (defmacro bar (form)
>   `(foo 1 1 ,@form))
> #+end_src
>
> this replacement rule would fail because the according form would be
> unmatchable (and the backquoted thing doesn't match).  One could work
> around this...there are always workarounds.  To need to do that would be
> very bad.
>

OK


>
> > Otherwise I'd accept introducing a variable to control whether
> > oldstyle backquotes should raise an error or get interpreted as
> > newstyle.
>
> That would be optimal for my case.
>

I've sent a patch for this as well:
https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00902.html


>
> > El-search would need to adapt, though, because the newstyle
> > interpretation is different.
>
> In which way would el-search need to adapt?  It doesn't interpret code.
> It is a tool for matching and transforming lists, which, in most cases,
> happen to be code.  The user would need to know how `read' interprets
> what's matched, of course.
>
>
>
Then I guess el-search should document the new behavior. Previously (up to
Emacs 26) (read "(,@ x)") returns (\,@ x), now (with the new variable) it
returns ((\,@ x)).
However, if el-search uses `read` for both the buffer text and the search
pattern, the interpretations should still match (within one version of
Emacs).

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

bug#29857: 27.0.50; error: "Loading `nil': old-style backquotes detected!"

[Michael Heerdegen]

Hi Philipp,

thanks for working on this.

>  > It would be reasonable and easy to remove the "Loading `nil'" part in
>  > the case where no file is being loaded.
>
>  Yes, I think that would be good.
>
> OK, I've sent a patch to do that:
> https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00901.html

Yes, that should be ok, I think.

>  > Otherwise I'd accept introducing a variable to control whether
>  > oldstyle backquotes should raise an error or get interpreted as
>  > newstyle.
>
>  That would be optimal for my case.
>
> I've sent a patch for this as well:
> https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00902.html

Thanks.  Looks like it does what I want.

> Then I guess el-search should document the new behavior. Previously
> (up to Emacs 26) (read "(,@ x)") returns (\,@ x), now (with the new
> variable) it returns ((\,@ x)).  However, if el-search uses `read` for
> both the buffer text and the search pattern, the interpretations
> should still match (within one version of Emacs).

Yes, it does.  I think there is no problem with this new behavior (well,
unless you either try to make use of old-style backquotes, or need the
semantics of old-style backquotes to transform old code that makes use
of old-style backquotes - I don't need to support these cases).


Thanks,

Michael.

Re: [PATCH 1/2] Add new variable to force new-style backquote interpretation.

[Paul Eggert]

Philipp Stephani wrote:
> As discussed in
> http://lists.gnu.org/archive/html/emacs-devel/2017-10/msg00051.html, we
> (mostly) agreed to first turn them into errors

And we've done that in Emacs 26, right? So in Emacs 27 we can remove support for 
old-style backquotes; we don't need a new variable to enable old-style 
backquotes dynamically.

Re: [PATCH 1/2] Add new variable to force new-style backquote interpretation.

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am So., 31. Dez. 2017 um 00:07 Uhr:

> Philipp Stephani wrote:
> > As discussed in
> > http://lists.gnu.org/archive/html/emacs-devel/2017-10/msg00051.html, we
> > (mostly) agreed to first turn them into errors
>
> And we've done that in Emacs 26, right?


No, only in master (i.e. Emacs 27), see
http://lists.gnu.org/archive/html/emacs-devel/2017-10/msg00056.html


> So in Emacs 27 we can remove support for
> old-style backquotes;


Only in Emacs 28, because 26 will still have old-style backquotes only.


> we don't need a new variable to enable old-style
> backquotes dynamically.
>

My patch conditionally enables new-style backquotes; there's no way to
re-enable old-style backquotes once they generate errors. The assumption is
that everyone should migrate to new-style once they get the errors, and not
re-enable old-style.

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

Re: [PATCH 1/2] Add new variable to force new-style backquote interpretation.

[Paul Eggert]

Philipp Stephani wrote:
> 
> Only in Emacs 28, because 26 will still have old-style backquotes only.

Ah, OK, I was turned around then: this variable is for forward-compatibility and 
not backward. I suggest mentioning in the doc string that setting the variable 
makes Emacs compatible with planned-for-28 behavior, and that the variable is 
planned to go away in Emacs 28. (It's a temporary variable, so to speak....)

Re: [PATCH 1/2] Add new variable to force new-style backquote interpretation.

[Stefan Monnier]

>> > As discussed in
>> > http://lists.gnu.org/archive/html/emacs-devel/2017-10/msg00051.html, we
>> > (mostly) agreed to first turn them into errors

Still looks like a complete waste of time to me.  I still haven't seen
a single example of code which works in Emacs-25 (say) and fails in
a byzantine manner if we silently drop support for old-style backquotes.

OTOH we have already seen at least one example of code which works
correctly in Emacs-25, works correctly if we drop support for old-style
backquotes, and yet fails with this "in-between" support.

So not only it's extra work for very little benefit (i.e. try and
protect us from a hypothetical case no-one has ever seen in the wild)
but it even costs us a known bug.

Sorry, but the tradeoff really doesn't look good from here.


        Stefan

Re: [PATCH 1/2] Add new variable to force new-style backquote interpretation.

[Philipp Stephani]

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

Paul Eggert <eggert@cs.ucla.edu> schrieb am So., 31. Dez. 2017 um 01:40 Uhr:

> Philipp Stephani wrote:
> >
> > Only in Emacs 28, because 26 will still have old-style backquotes only.
>
> Ah, OK, I was turned around then: this variable is for
> forward-compatibility and
> not backward. I suggest mentioning in the doc string that setting the
> variable
> makes Emacs compatible with planned-for-28 behavior, and that the variable
> is
> planned to go away in Emacs 28. (It's a temporary variable, so to
> speak....)
>

Good point, I've amended the docstring and pushed the patch to master as
ddb74b2027.

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

bug#29857: 27.0.50; error: "Loading `nil': old-style backquotes detected!"

[Philipp Stephani]

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

Michael Heerdegen <michael_heerdegen@web.de> schrieb am Sa., 30. Dez. 2017
um 23:33 Uhr:

> Hi Philipp,
>
> thanks for working on this.
>
> >  > It would be reasonable and easy to remove the "Loading `nil'" part in
> >  > the case where no file is being loaded.
> >
> >  Yes, I think that would be good.
> >
> > OK, I've sent a patch to do that:
> > https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00901.html
>
> Yes, that should be ok, I think.
>
> >  > Otherwise I'd accept introducing a variable to control whether
> >  > oldstyle backquotes should raise an error or get interpreted as
> >  > newstyle.
> >
> >  That would be optimal for my case.
> >
> > I've sent a patch for this as well:
> > https://lists.gnu.org/archive/html/emacs-devel/2017-12/msg00902.html
>
> Thanks.  Looks like it does what I want.
>
>
OK, I've now pushed both patches to master.

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