Old-style backquotes

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

Old-style backquotes

Philipp Stephani
Hi,

old-style backquotes were deprecated ten years ago (1d06469794a66d6c8b424e6f17da029ebc5bb295). Is it time to finally turn them into errors for Emacs 26, and remove them in Emacs 27?

Philipp
Reply | Threaded
Open this post in threaded view
|

Re: Old-style backquotes

Paul Eggert
On 10/03/2017 06:51 AM, Philipp Stephani wrote:
> old-style backquotes were deprecated ten years ago
> (1d06469794a66d6c8b424e6f17da029ebc5bb295). Is it time to finally turn
> them into errors for Emacs 26, and remove them in Emacs 27?

Ten years is long enough, so I'd say it's time.


Reply | Threaded
Open this post in threaded view
|

[PATCH] Raise an error when detecting old-style backquotes.

Philipp Stephani
They have been deprecated for a decade now.

* src/lread.c (Fload): Don’t use record_unwind_protect to warn about
old-style backquotes any more.  They now generate a hard error.
(read1): Signal an error when detecting old-style backquotes.  Remove
unused label.
(syms_of_lread): Remove unused internal variable
‘lread--old-style-backquotes’.
(load_error_old_style_backquotes): Rename from
‘load_warn_oldstyle_backquotes’.  Signal an error.

* test/src/lread-tests.el (lread-tests--old-style-backquotes): Adapt
unit test.
---
 src/lread.c             | 32 ++++++--------------------------
 test/src/lread-tests.el | 10 +++++-----
 2 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/src/lread.c b/src/lread.c
index 6bc93b1481..44953baa77 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1003,14 +1003,11 @@ load_error_handler (Lisp_Object data)
   return Qnil;
 }
 
-static void
-load_warn_old_style_backquotes (Lisp_Object file)
+static _Noreturn void
+load_error_old_style_backquotes (void)
 {
-  if (!NILP (Vlread_old_style_backquotes))
-    {
-      AUTO_STRING (format, "Loading `%s': old-style backquotes detected!");
-      CALLN (Fmessage, format, file);
-    }
+  AUTO_STRING (format, "Loading `%s': old-style backquotes detected!");
+  xsignal1 (Qerror, CALLN (Fformat_message, format, Vload_file_name));
 }
 
 static void
@@ -1282,10 +1279,6 @@ Return t if the file exists and loads successfully.  */)
 
   version = -1;
 
-  /* Check for the presence of old-style quotes and warn about them.  */
-  specbind (Qlread_old_style_backquotes, Qnil);
-  record_unwind_protect (load_warn_old_style_backquotes, file);
-
   /* Check for the presence of unescaped character literals and warn
      about them. */
   specbind (Qlread_unescaped_character_literals, Qnil);
@@ -3178,10 +3171,7 @@ read1 (Lisp_Object readcharfun, int *pch, bool first_in_list)
    first_in_list exception (old-style can still be obtained via
    "(\`" anyway).  */
  if (!new_backquote_flag && first_in_list && next_char == ' ')
-  {
-    Vlread_old_style_backquotes = Qt;
-    goto default_label;
-  }
+          load_error_old_style_backquotes ();
  else
   {
     Lisp_Object value;
@@ -3232,10 +3222,7 @@ read1 (Lisp_Object readcharfun, int *pch, bool first_in_list)
     return list2 (comma_type, value);
   }
  else
-  {
-    Vlread_old_style_backquotes = Qt;
-    goto default_label;
-  }
+          load_error_old_style_backquotes ();
       }
     case '?':
       {
@@ -3423,7 +3410,6 @@ read1 (Lisp_Object readcharfun, int *pch, bool first_in_list)
  row.  */
       FALLTHROUGH;
     default:
-    default_label:
       if (c <= 040) goto retry;
       if (c == NO_BREAK_SPACE)
  goto retry;
@@ -4996,12 +4982,6 @@ variables, this must be set in the first line of a file.  */);
        doc: /* List of buffers being read from by calls to `eval-buffer' and `eval-region'.  */);
   Veval_buffer_list = Qnil;
 
-  DEFVAR_LISP ("lread--old-style-backquotes", Vlread_old_style_backquotes,
-       doc: /* Set to non-nil when `read' encounters an old-style backquote.
-For internal use only.  */);
-  Vlread_old_style_backquotes = Qnil;
-  DEFSYM (Qlread_old_style_backquotes, "lread--old-style-backquotes");
-
   DEFVAR_LISP ("lread--unescaped-character-literals",
                Vlread_unescaped_character_literals,
                doc: /* List of deprecated unescaped character literals encountered by `read'.
diff --git a/test/src/lread-tests.el b/test/src/lread-tests.el
index ac730b4f00..241cebb6b1 100644
--- a/test/src/lread-tests.el
+++ b/test/src/lread-tests.el
@@ -173,13 +173,13 @@ lread-tests--last-message
     (should (string-suffix-p "/somelib.el" (caar load-history)))))
 
 (ert-deftest lread-tests--old-style-backquotes ()
-  "Check that loading warns about old-style backquotes."
+  "Check that loading doesn’t accept old-style backquotes."
   (lread-tests--with-temp-file file-name
     (write-region "(` (a b))" nil file-name)
-    (should (equal (load file-name nil :nomessage :nosuffix) t))
-    (should (equal (lread-tests--last-message)
-                   (concat (format-message "Loading `%s': " file-name)
-                           "old-style backquotes detected!")))))
+    (let ((data (should-error (load file-name nil :nomessage :nosuffix))))
+      (should (equal (cdr data)
+                     (list (concat (format-message "Loading `%s': " file-name)
+                                   "old-style backquotes detected!")))))))
 
 (ert-deftest lread-lread--substitute-object-in-subtree ()
   (let ((x (cons 0 1)))
--
2.14.2


Reply | Threaded
Open this post in threaded view
|

Re: Old-style backquotes

Eli Zaretskii
In reply to this post by Paul Eggert
> From: Paul Eggert <[hidden email]>
> Date: Tue, 3 Oct 2017 07:12:25 -0700
>
> On 10/03/2017 06:51 AM, Philipp Stephani wrote:
> > old-style backquotes were deprecated ten years ago
> > (1d06469794a66d6c8b424e6f17da029ebc5bb295). Is it time to finally turn
> > them into errors for Emacs 26, and remove them in Emacs 27?
>
> Ten years is long enough, so I'd say it's time.

On master, please (unless there are objections).

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Raise an error when detecting old-style backquotes.

Stefan Monnier
In reply to this post by Philipp Stephani
> * src/lread.c (Fload): Don’t use record_unwind_protect to warn about
> old-style backquotes any more.  They now generate a hard error.

Not sure what's the benefit of raising a hard error, over just removing
the obsolete feature altogether.


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Raise an error when detecting old-style backquotes.

Philipp Stephani


Stefan Monnier <[hidden email]> schrieb am Di., 3. Okt. 2017 um 19:44 Uhr:
> * src/lread.c (Fload): Don’t use record_unwind_protect to warn about
> old-style backquotes any more.  They now generate a hard error.

Not sure what's the benefit of raising a hard error, over just removing
the obsolete feature altogether.


Removing the special treatment would change the behavior. I'd rather have a time span where developers would get hard errors instead of subtle behavior changes. 
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Raise an error when detecting old-style backquotes.

Stefan Monnier
> Removing the special treatment would change the behavior.  I'd rather have a
> time span where developers would get hard errors instead of subtle behavior
> changes.

I don't think it'll make any real difference: in both cases the old code
won't work anymore: I'm hard pressed to think of a scenario where the
"subtle" behavior change is subtle enough not to be detected
fairly quickly.


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Raise an error when detecting old-style backquotes.

Richard Stallman
In reply to this post by Stefan Monnier
[[[ 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. ]]]

  > > * src/lread.c (Fload): Don’t use record_unwind_protect to warn about
  > > old-style backquotes any more.  They now generate a hard error.

  > Not sure what's the benefit of raising a hard error, over just removing
  > the obsolete feature altogether.

The hard error could have a specific error message
to inform users of where to look to find out what to change.

--
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Raise an error when detecting old-style backquotes.

Stefan Monnier
> The hard error could have a specific error message
> to inform users of where to look to find out what to change.

We've had such a specific warning (instead of error) for the last
10 years.


        Stefan

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Raise an error when detecting old-style backquotes.

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

  > > The hard error could have a specific error message
  > > to inform users of where to look to find out what to change.

  > We've had such a specific warning (instead of error) for the last
  > 10 years.

I suggest a specific error message as the next step in deprecation.

--
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Raise an error when detecting old-style backquotes.

Stefan Monnier
>> > The hard error could have a specific error message
>> > to inform users of where to look to find out what to change.
>> We've had such a specific warning (instead of error) for the last
>> 10 years.
> I suggest a specific error message as the next step in deprecation.

We've never done that when removing obsolete features.
Why should this be different?


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Raise an error when detecting old-style backquotes.

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

  > We've never done that when removing obsolete features.
  > Why should this be different?

I'm not insisting it should be different.

I just think that, given that this was a very general feature that
changed in a grossly incompatible way, it is worth keeping a little
code to make a specific error message.

--
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.


Reply | Threaded
Open this post in threaded view
|

Re: Old-style backquotes

Philipp Stephani
In reply to this post by Eli Zaretskii


Eli Zaretskii <[hidden email]> schrieb am Di., 3. Okt. 2017 um 17:02 Uhr:
> From: Paul Eggert <[hidden email]>
> Date: Tue, 3 Oct 2017 07:12:25 -0700
>
> On 10/03/2017 06:51 AM, Philipp Stephani wrote:
> > old-style backquotes were deprecated ten years ago
> > (1d06469794a66d6c8b424e6f17da029ebc5bb295). Is it time to finally turn
> > them into errors for Emacs 26, and remove them in Emacs 27?
>
> Ten years is long enough, so I'd say it's time.

On master, please (unless there are objections).

OK, here's a patch. 

0001-Raise-an-error-when-detecting-old-style-backquotes.txt (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Raise an error when detecting old-style backquotes.

Philipp Stephani
In reply to this post by Stefan Monnier


Stefan Monnier <[hidden email]> schrieb am Di., 3. Okt. 2017 um 20:52 Uhr:
> Removing the special treatment would change the behavior.  I'd rather have a
> time span where developers would get hard errors instead of subtle behavior
> changes.

I don't think it'll make any real difference: in both cases the old code
won't work anymore: I'm hard pressed to think of a scenario where the
"subtle" behavior change is subtle enough not to be detected
fairly quickly.


For example, code such as '(` foo) would change behavior. Whether a certain list is nested or not is definitely subtle enough. 
Reply | Threaded
Open this post in threaded view
|

Re: Old-style backquotes

Eli Zaretskii
In reply to this post by Philipp Stephani
> From: Philipp Stephani <[hidden email]>
> Date: Sun, 08 Oct 2017 14:58:45 +0000
> Cc: [hidden email]
>
> OK, here's a patch.

Thanks, a few minor comments:

> @@ -3178,10 +3171,7 @@ read1 (Lisp_Object readcharfun, int *pch, bool first_in_list)
>     first_in_list exception (old-style can still be obtained via
>     "(\`" anyway).  */
>   if (!new_backquote_flag && first_in_list && next_char == ' ')
> -  {
> -    Vlread_old_style_backquotes = Qt;
> -    goto default_label;
> -  }
> +          load_error_old_style_backquotes ();

Here (and elsewhere) the indentation should be fixed to be in line
with our style.

> --- a/test/src/lread-tests.el
> +++ b/test/src/lread-tests.el
> @@ -173,13 +173,13 @@ lread-tests--last-message
>      (should (string-suffix-p "/somelib.el" (caar load-history)))))
>  
>  (ert-deftest lread-tests--old-style-backquotes ()
> -  "Check that loading warns about old-style backquotes."
> +  "Check that loading doesn’t accept old-style backquotes."

Please don't use literal curved quotes in doc strings and message
text, they will be transformed (by default) when displayed.

Reply | Threaded
Open this post in threaded view
|

Re: Old-style backquotes

Eli Zaretskii
> Date: Sun, 08 Oct 2017 19:05:53 +0300
> From: Eli Zaretskii <[hidden email]>
> Cc: [hidden email], [hidden email]
>
> > From: Philipp Stephani <[hidden email]>
> > Date: Sun, 08 Oct 2017 14:58:45 +0000
> > Cc: [hidden email]
> >
> > OK, here's a patch.
>
> Thanks, a few minor comments:

Oh, and one more: this needs a NEWS entry.

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: Old-style backquotes

Philipp Stephani


Eli Zaretskii <[hidden email]> schrieb am So., 8. Okt. 2017 um 18:09 Uhr:
> Date: Sun, 08 Oct 2017 19:05:53 +0300
> From: Eli Zaretskii <[hidden email]>
> Cc: [hidden email], [hidden email]
>
> > From: Philipp Stephani <[hidden email]>
> > Date: Sun, 08 Oct 2017 14:58:45 +0000
> > Cc: [hidden email]
> >
> > OK, here's a patch.
>
> Thanks, a few minor comments:

Oh, and one more: this needs a NEWS entry.


Here's a new patch that also fixes the byte compiler tests. 

0001-Raise-an-error-when-detecting-old-style-backquotes.txt (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Old-style backquotes

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Sun, 08 Oct 2017 16:25:44 +0000
> Cc: [hidden email], [hidden email]
>
> Here's a new patch that also fixes the byte compiler tests.

Thanks, LGTM, but please mention the NEWS change in the log message.

Also, the log message should quote 'like this', not ‘like this’.

Reply | Threaded
Open this post in threaded view
|

Re: Old-style backquotes

Philipp Stephani


Eli Zaretskii <[hidden email]> schrieb am So., 8. Okt. 2017 um 18:38 Uhr:
> From: Philipp Stephani <[hidden email]>
> Date: Sun, 08 Oct 2017 16:25:44 +0000
> Cc: [hidden email], [hidden email]
>
> Here's a new patch that also fixes the byte compiler tests.

Thanks, LGTM, but please mention the NEWS change in the log message.

Also, the log message should quote 'like this', not ‘like this’.

Fixed and pushed as 9613690f6e to master. 
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Raise an error when detecting old-style backquotes.

Stefan Monnier
In reply to this post by Philipp Stephani
>> I don't think it'll make any real difference: in both cases the old code
>> won't work anymore: I'm hard pressed to think of a scenario where the
>> "subtle" behavior change is subtle enough not to be detected
>> fairly quickly.
> For example, code such as '(` foo) would change behavior.  Whether a certain
> list is nested or not is definitely subtle enough.

But this is not a complete scenario.  You'd need to give additional
context:
- why is there '(` foo) in the source code (it seems already contrived)?
- How is the result used (it's important that the result be
  used in a way which works differently but without signaling an error
  in both cases)?


        Stefan