bug#20852: 24.3; update-file-autoloads doesn't accept unescaped parenthesis character literal

bug#20852: 24.3; update-file-autoloads doesn't accept unescaped parenthesis character literal

[Philipp Stephani]

Create an Elisp file such as

   (defvar foo ?()

and try to run `update-file-autoloads' on it.  This will fail with an
unbalanced parenthesis error.  However, this is valid (albeit
discouraged) Elisp code.


In GNU Emacs 24.3.1 (x86_64-pc-linux-gnu, GTK+ Version 3.10.7)
 of 2014-03-07 on lamiak, modified by Debian
Windowing system distributor `The X.Org Foundation', version 11.0.11501000
System Description:	Ubuntu 14.04 LTS

Configured using:
 `configure '--build' 'x86_64-linux-gnu' '--build' 'x86_64-linux-gnu'
 '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib'
 '--localstatedir=/var/lib' '--infodir=/usr/share/info'
 '--mandir=/usr/share/man' '--with-pop=yes'
 '--enable-locallisppath=/etc/emacs24:/etc/emacs:/usr/local/share/emacs/24.3/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.3/site-lisp:/usr/share/emacs/site-lisp'
 '--with-crt-dir=/usr/lib/x86_64-linux-gnu' '--with-x=yes'
 '--with-x-toolkit=gtk3' '--with-toolkit-scroll-bars'
 'build_alias=x86_64-linux-gnu' 'CFLAGS=-g -O2 -fstack-protector
 --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wall'
 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro'
 'CPPFLAGS=-D_FORTIFY_SOURCE=2''

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Emacs-Lisp

Minor modes in effect:
  tooltip-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

Recent input:
C-x C-f / t m p / a u t o l o a d t e s t . e l <return> 
( d e f v a r SPC f o o SPC ? ( ) <return> C-x C-s 
M-x b y t e - c o m <tab> <return> a u t o <tab> <return> 
M-x u p d a t e - a u <tab> <tab> f <tab> <return> 
a u <tab> <return> a u <backspace> <backspace> l o 
a <tab> d d e f s . e l <return> M-x r e p o r t <
return>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
(New file)
Saving file /tmp/autoloadtest.el...
Wrote /tmp/autoloadtest.el
Compiling /tmp/autoloadtest.el...done
Wrote /tmp/autoloadtest.elc
Making completion list...
Generating autoloads for /tmp/autoloadtest.el...
forward-sexp: Scan error: "Unbalanced parentheses", 1, 17

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message byte-opt format-spec
rfc822 mml mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils autoload help-fns help-mode easymenu
warnings compile comint ansi-color ring bytecomp byte-compile cconv
time-date tooltip ediff-hook vc-hooks lisp-float-type mwheel x-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list newcomment
lisp-mode register page menu-bar rfn-eshadow timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core frame cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese case-table epa-hook jka-cmpr-hook help simple abbrev
minibuffer loaddefs button faces cus-face macroexp files text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process dbusbind
dynamic-setting system-font-setting font-render-setting move-toolbar gtk
x-toolkit x multi-tty emacs)

-- 
Google Germany GmbH
Dienerstraße 12
80331 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores

Diese E-Mail ist vertraulich.  Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen
Sie die E-Mail und alle Anhänge.  Vielen Dank.
       
This e-mail is confidential.  If you are not the right addressee please do not
forward it, please inform the sender, and please erase this e-mail including
any attachments.  Thanks.

bug#20852: 24.3; update-file-autoloads doesn't accept unescaped parenthesis character literal

[Stefan Monnier]

> Create an Elisp file such as
>    (defvar foo ?()
> and try to run `update-file-autoloads' on it.  This will fail with an
> unbalanced parenthesis error.  However, this is valid (albeit
> discouraged) Elisp code.

Yes: don't do that.  Use (defvar foo ?\() instead.
This same error will appear during interactive editing of that code.


        Stefan

bug#20852: 24.3; update-file-autoloads doesn't accept unescaped parenthesis character literal

[Philipp Stephani]

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

Stefan Monnier <monnier@iro.umontreal.ca> schrieb am Fr., 19. Juni 2015 um
23:24 Uhr:

> > Create an Elisp file such as
> >    (defvar foo ?()
> > and try to run `update-file-autoloads' on it.  This will fail with an
> > unbalanced parenthesis error.  However, this is valid (albeit
> > discouraged) Elisp code.
>
> Yes: don't do that.  Use (defvar foo ?\() instead.
> This same error will appear during interactive editing of that code.
>
>
>
If this is generally unsupported, can we then remove it from the doc and
have the reader print a warning (or reject it altogether)?

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

bug#20852: 24.3; update-file-autoloads doesn't accept unescaped parenthesis character literal

[Stefan Monnier]

> If this is generally unsupported, can we then remove it from the doc and

Yes.  Do you happen to know where it's documented?

> have the reader print a warning (or reject it altogether)?

We can't reject it, because I'm pretty sure there's code out there which
uses it.  I'd accept a patch which adds a warning for it,


        Stefan

bug#20852: 24.3; update-file-autoloads doesn't accept unescaped parenthesis character literal

[Philipp Stephani]

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

Stefan Monnier <monnier@iro.umontreal.ca> schrieb am Mo., 22. Juni 2015 um
03:18 Uhr:

> > If this is generally unsupported, can we then remove it from the doc and
>
> Yes.  Do you happen to know where it's documented?
>

https://www.gnu.org/software/emacs/manual/html_node/elisp/Basic-Char-Syntax.html

"You can use the same syntax for punctuation characters, but it is often a
good idea to add a ‘\’ so that the Emacs commands for editing Lisp code
don’t get confused. For example, ‘?\(’ is the way to write the open-paren
character. If the character is ‘\’, you must use a second ‘\’ to quote it:
‘?\\’."

Here I would put ( and ) into the same category as \, so that it's not only
"a good idea" to escape it.

"However, you should add a backslash before any of the characters
‘()\|;'`"#.,’ to avoid confusing the Emacs commands for editing Lisp code."

Here I would make escaping mandatory for the first three characters (or
even for all of them).


>
> > have the reader print a warning (or reject it altogether)?
>
> We can't reject it, because I'm pretty sure there's code out there which
> uses it.  I'd accept a patch which adds a warning for it,
>
>
>
OK, I'll try to come up with a patch.

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

bug#20852: 24.3; update-file-autoloads doesn't accept unescaped parenthesis character literal

[Philipp Stephani]

[-- Attachment #1.1: Type: text/plain, Size: 219 bytes --]

Philipp Stephani <p.stephani2@gmail.com> schrieb am So., 28. Juni 2015 um
15:12 Uhr:

> OK, I'll try to come up with a patch.
>

I've attached a patch that uses the same mechanism as the check for
old-style backquotes.

[-- Attachment #1.2: Type: text/html, Size: 577 bytes --]

[-- Attachment #2: 0001-lread.c-Fload-Warn-about-missing-backslashes.patch --]
[-- Type: application/octet-stream, Size: 2653 bytes --]

From 570b3fa697a9597169a27ac47b1a56f5440203b9 Mon Sep 17 00:00:00 2001
From: Philipp Stephani <phst@google.com>
Date: Tue, 30 Jun 2015 22:38:35 +0200
Subject: [PATCH] * lread.c (Fload): Warn about missing backslashes

* lread.c (load_warn_unescaped_character_literals, Fload, read1)
(syms_of_lread): Warn if unescaped parenthesis character literals
are found (Bug#20152).
---
 src/lread.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/src/lread.c b/src/lread.c
index 11c8d00..c2df2bd 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -952,6 +952,15 @@ load_warn_old_style_backquotes (Lisp_Object file)
     }
 }
 
+static void
+load_warn_unescaped_character_literals (Lisp_Object file) {
+  if (!NILP (Vunescaped_character_literals))
+    {
+      AUTO_STRING (format, "Loading `%s': unescaped parenthesis character literals detected!");
+      CALLN (Fmessage, format, file);
+    }
+}
+
 DEFUN ("get-load-suffixes", Fget_load_suffixes, Sget_load_suffixes, 0, 0, 0,
        doc: /* Return the suffixes that `load' should try if a suffix is \
 required.
@@ -1194,6 +1203,11 @@ Return t if the file exists and loads successfully.  */)
   specbind (Qold_style_backquotes, Qnil);
   record_unwind_protect (load_warn_old_style_backquotes, file);
 
+  /* Check for the presence of unescaped parenthesis character literals
+     and warn about them. */
+  specbind (Qunescaped_character_literals, Qnil);
+  record_unwind_protect(load_warn_unescaped_character_literals, file);
+
   if (!memcmp (SDATA (found) + SBYTES (found) - 4, ".elc", 4)
       || (fd >= 0 && (version = safe_to_load_version (fd)) > 0))
     /* Load .elc files directly, but not when they are
@@ -2971,6 +2985,9 @@ read1 (Lisp_Object readcharfun, int *pch, bool first_in_list)
 	if (c == ' ' || c == '\t')
 	  return make_number (c);
 
+	if (c == '(' || c == ')')
+	  Vunescaped_character_literals = Qt;
+
 	if (c == '\\')
 	  c = read_escape (readcharfun, 0);
 	modifiers = c & CHAR_MODIFIER_MASK;
@@ -4678,6 +4695,10 @@ variables, this must be set in the first line of a file.  */);
   Vold_style_backquotes = Qnil;
   DEFSYM (Qold_style_backquotes, "old-style-backquotes");
 
+  DEFVAR_LISP ("unescaped-character-literals", Vunescaped_character_literals,
+               doc: /* Set to non-nil when `read' encounters a deprecated unescaped character literal.  */);
+  DEFSYM (Qunescaped_character_literals, "unescaped-character-literals");
+
   DEFVAR_BOOL ("load-prefer-newer", load_prefer_newer,
                doc: /* Non-nil means `load' prefers the newest version of a file.
 This applies when a filename suffix is not explicitly specified and
-- 
2.4.4

bug#20852: 24.3; update-file-autoloads doesn't accept unescaped parenthesis character literal

[Stefan Monnier]

> I've attached a patch that uses the same mechanism as the check for
> old-style backquotes.

Looks pretty good, thank you.
A few comments and nitpicks:

> +static void
> +load_warn_unescaped_character_literals (Lisp_Object file) {
> +  if (!NILP (Vunescaped_character_literals))
> +    {
> +      AUTO_STRING (format, "Loading `%s': unescaped parenthesis character literals detected!");
> +      CALLN (Fmessage, format, file);
> +    }
> +}

It would be even better to show the actual unescaped character literal(s).
Either just displaying one of the chars, or displaying them all.

>  	if (c == ' ' || c == '\t')
>  	  return make_number (c);
> 
> +	if (c == '(' || c == ')')
> +	  Vunescaped_character_literals = Qt;

I think we want to emit the same warning for several other chars, at
least ';', '"', '[', and ']' (we could also include ' ' and '\t', tho
I'm not sure it's worth the trouble.  If/when we want to do that, we'd
want to do it not just for ?<SPC> and ?<TAB> but also for ?\<SPC>
and ?\<TAB> and recommend the use of ?\s and ?\t instead).

>    DEFSYM (Qold_style_backquotes, "old-style-backquotes");
> +  DEFSYM (Qunescaped_character_literals, "unescaped-character-literals");

I think "old-style-backquotes" was a bad choice.  It should have been
"read--found-old-style-backquotes" or something along these lines.
AFAIK this variable is only used internally, so we can rename it without
any extra precautions.  If you could do that in your patch, that'd
be great.

One more thing: old-style-backquotes is not only used by `load' but also
by the byte-compiler.  So take a look at bytecomp.el where you'll
probably want to add similar code for your unescaped-character-literals.


        Stefan

bug#20852: [PATCH] Warn about missing backslashes during load

[Stefan Monnier]

Looks great, thank you very much.  See comments below.

> +	if (c == '(' || c == ')' || c == '[' || c == ']'
> +            || c == '"' || c == ';')

These are indeed the most important ones w.r.t elisp-mode's
syntax-table, but I think we should put #, ?, and ' in there as well.
This said, maybe we should add chars in there "little by little" to try
and not overwhelm people with warnings.
So the above set might actually be a good set with which to start.

> +               doc: /* List of deprecated unescaped character literals encountered by `read'.  For internal use only.  */);

Please keep the first line of docstrings with 80 columns.  I.e. move the
"For internal use only." to a second line.


        Stefan

bug#20852: [PATCH] Warn about missing backslashes during load

[Philipp Stephani]

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

Stefan Monnier <monnier@iro.umontreal.ca> schrieb am Mo., 1. Mai 2017 um
19:59 Uhr:

> Looks great, thank you very much.  See comments below.
>
> > +     if (c == '(' || c == ')' || c == '[' || c == ']'
> > +            || c == '"' || c == ';')
>
> These are indeed the most important ones w.r.t elisp-mode's
> syntax-table, but I think we should put #, ?, and ' in there as well.
> This said, maybe we should add chars in there "little by little" to try
> and not overwhelm people with warnings.
> So the above set might actually be a good set with which to start.
>

Yeah, I've left it as-is for now, the current set are those characters that
cause problems in practice (wrong paren highlighting, confusion with
paredit).


>
> > +               doc: /* List of deprecated unescaped character literals
> encountered by `read'.  For internal use only.  */);
>
> Please keep the first line of docstrings with 80 columns.  I.e. move the
> "For internal use only." to a second line.
>
>
Done and pushed as c2bbdc3316.

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