bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

[Spencer Baugh]

The end-of-file error signaled by read has incorrect data if it's
signaled during a load, by a read unrelated to that load.

1. Create a file "~/read-empty.el" containing:
(read "")
2. (load "~/read-empty.el")
3. Observe the error message:
End of file during parsing: /home/sbaugh/read-empty.el

This error message suggests that there's a syntax error in
read-empty.el, when in fact the syntax error is in something else
entirely.  This is quite confusing.

This happens because end_of_file_error uses load-true-file-name if it's
non-nil, even if the actual read call is unrelated.

One possible fix: a new variable read-file-name could be introduced, and
end_of_file_error could use that instead of load-true-file-name, and
load can just bind read-file-name around read.

As one particular example of the confusing current behavior, a user had
corrupted their ~/.emacs.d/projects so that reading it failed.  Also,
they had a call to (project-forget-zombie-projects) in their init.el.
In combination, this meant Emacs startup errored with:

End of file during parsing: /home/user/.emacs.d/init.el

even though there was no syntax error in init.el at all.

To resolve that, perhaps project--read-project-list could bind
read-file-name to project-list-file, so we'd get an error message which
properly mentions the project file instead.


In GNU Emacs 29.1.90 (build 8, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2024-01-04 built on
 igm-qws-u22796a
Repository revision: 57fa5a53f74e489702825045832f52730c5d550f
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.9 (Green Obsidian)

Configured using:
 'configure 'CFLAGS=-O0 -g3' --with-gif=ifavailable
 --with-x-toolkit=lucid'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM
XINPUT2 XPM LUCID ZLIB

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

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-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
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
xinput2 x multi-tty make-network-process emacs)

Memory information:
((conses 16 63231 9564)
 (symbols 48 9475 0)
 (strings 32 22767 1134)
 (string-bytes 1 677438)
 (vectors 16 9316)
 (vector-slots 8 148900 13842)
 (floats 8 34 25)
 (intervals 56 241 0)
 (buffers 976 10))

bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

[Dmitry Gutov]

On 17/01/2024 21:04, Spencer Baugh wrote:
> As one particular example of the confusing current behavior, a user had
> corrupted their ~/.emacs.d/projects so that reading it failed.  Also,
> they had a call to (project-forget-zombie-projects) in their init.el.
> In combination, this meant Emacs startup errored with:
> 
> End of file during parsing:/home/user/.emacs.d/init.el
> 
> even though there was no syntax error in init.el at all.

Would something like this help with this particular sub-problem?

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index a6f14a0865c..196a82757b2 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1694,7 +1694,9 @@ project--read-project-list
                   (let ((name (car elem)))
                     (list (if (file-remote-p name) name
                             (abbreviate-file-name name)))))
-               (read (current-buffer))))))
+               (condition-case nil
+                   (read (current-buffer))
+                 (end-of-file (warn "Failed to read the projects list 
file")))))))
      (unless (seq-every-p
               (lambda (elt) (stringp (car-safe elt)))
               project--list)

bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

[Spencer Baugh]

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

On Wed, Jan 17, 2024 at 3:55 PM Dmitry Gutov <dmitry@gutov.dev> wrote:

> On 17/01/2024 21:04, Spencer Baugh wrote:
> > As one particular example of the confusing current behavior, a user had
> > corrupted their ~/.emacs.d/projects so that reading it failed.  Also,
> > they had a call to (project-forget-zombie-projects) in their init.el.
> > In combination, this meant Emacs startup errored with:
> >
> > End of file during parsing:/home/user/.emacs.d/init.el
> >
> > even though there was no syntax error in init.el at all.
>
> Would something like this help with this particular sub-problem?
>
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index a6f14a0865c..196a82757b2 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -1694,7 +1694,9 @@ project--read-project-list
>                    (let ((name (car elem)))
>                      (list (if (file-remote-p name) name
>                              (abbreviate-file-name name)))))
> -               (read (current-buffer))))))
> +               (condition-case nil
> +                   (read (current-buffer))
> +                 (end-of-file (warn "Failed to read the projects list
> file")))))))
>       (unless (seq-every-p
>                (lambda (elt) (stringp (car-safe elt)))
>                project--list)
>
>
Yes, I think that would be great.  Especially because this allows startup
to continue.  If it's okay with you too, I think it's reasonable to push.

This de-facto resets the contents of the projects list file (since the
corrupted version will get saved over), but I think that's fine -  it's not
especially hard information to rebuild, and if it's corrupt anyway then
it's probably already at least partially lost (in my case, a user had an
empty projects file for some reason, not sure why).  Oh and I guess we
already reset the contents if it's in the wrong format.  So yeah, this
seems great.

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

bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

[Spencer Baugh]

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

On Thu, Jan 25, 2024 at 9:05 AM Spencer Baugh <sbaugh@janestreet.com> wrote:

> On Wed, Jan 17, 2024 at 3:55 PM Dmitry Gutov <dmitry@gutov.dev> wrote:
>
>> On 17/01/2024 21:04, Spencer Baugh wrote:
>> > As one particular example of the confusing current behavior, a user had
>> > corrupted their ~/.emacs.d/projects so that reading it failed.  Also,
>> > they had a call to (project-forget-zombie-projects) in their init.el.
>> > In combination, this meant Emacs startup errored with:
>> >
>> > End of file during parsing:/home/user/.emacs.d/init.el
>> >
>> > even though there was no syntax error in init.el at all.
>>
>> Would something like this help with this particular sub-problem?
>>
>> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>> index a6f14a0865c..196a82757b2 100644
>> --- a/lisp/progmodes/project.el
>> +++ b/lisp/progmodes/project.el
>> @@ -1694,7 +1694,9 @@ project--read-project-list
>>                    (let ((name (car elem)))
>>                      (list (if (file-remote-p name) name
>>                              (abbreviate-file-name name)))))
>> -               (read (current-buffer))))))
>> +               (condition-case nil
>> +                   (read (current-buffer))
>> +                 (end-of-file (warn "Failed to read the projects list
>> file")))))))
>>       (unless (seq-every-p
>>                (lambda (elt) (stringp (car-safe elt)))
>>                project--list)
>>
>>
> Yes, I think that would be great.  Especially because this allows startup
> to continue.  If it's okay with you too, I think it's reasonable to push.
>
> This de-facto resets the contents of the projects list file (since the
> corrupted version will get saved over), but I think that's fine -  it's not
> especially hard information to rebuild, and if it's corrupt anyway then
> it's probably already at least partially lost (in my case, a user had an
> empty projects file for some reason, not sure why).  Oh and I guess we
> already reset the contents if it's in the wrong format.  So yeah, this
> seems great.
>

Ah, I think the cause (in at least one case) is that the user ran out of
disk space, and so the write-region call in project--write-project-list
failed when writing, which happens after truncating the existing file.  I
guess there should be some atomic version of write-region which writes to a
temp file and renames it over the original.

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

bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

[Eli Zaretskii]

> Cc: 68546@debbugs.gnu.org
> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Thu, 25 Jan 2024 09:05:19 -0500
> 
>  Would something like this help with this particular sub-problem?
> 
>  diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>  index a6f14a0865c..196a82757b2 100644
>  --- a/lisp/progmodes/project.el
>  +++ b/lisp/progmodes/project.el
>  @@ -1694,7 +1694,9 @@ project--read-project-list
>                     (let ((name (car elem)))
>                       (list (if (file-remote-p name) name
>                               (abbreviate-file-name name)))))
>  -               (read (current-buffer))))))
>  +               (condition-case nil
>  +                   (read (current-buffer))
>  +                 (end-of-file (warn "Failed to read the projects list 
>  file")))))))
>        (unless (seq-every-p
>                 (lambda (elt) (stringp (car-safe elt)))
>                 project--list)
> 
> Yes, I think that would be great.  Especially because this allows startup to continue.  If it's okay with
> you too, I think it's reasonable to push.

I don't object to the change, of course, but perhaps we could make the
error message friendlier?  For example:

   Failed to read the projects list file due to unexpected EOF

bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

[Dmitry Gutov]

On 25/01/2024 16:29, Eli Zaretskii wrote:
>> Cc:68546@debbugs.gnu.org
>> From: Spencer Baugh<sbaugh@janestreet.com>
>> Date: Thu, 25 Jan 2024 09:05:19 -0500
>>
>>   Would something like this help with this particular sub-problem?
>>
>>   diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>>   index a6f14a0865c..196a82757b2 100644
>>   --- a/lisp/progmodes/project.el
>>   +++ b/lisp/progmodes/project.el
>>   @@ -1694,7 +1694,9 @@ project--read-project-list
>>                      (let ((name (car elem)))
>>                        (list (if (file-remote-p name) name
>>                                (abbreviate-file-name name)))))
>>   -               (read (current-buffer))))))
>>   +               (condition-case nil
>>   +                   (read (current-buffer))
>>   +                 (end-of-file (warn "Failed to read the projects list
>>   file")))))))
>>         (unless (seq-every-p
>>                  (lambda (elt) (stringp (car-safe elt)))
>>                  project--list)
>>
>> Yes, I think that would be great.  Especially because this allows startup to continue.  If it's okay with
>> you too, I think it's reasonable to push.
> I don't object to the change, of course, but perhaps we could make the
> error message friendlier?  For example:
> 
>     Failed to read the projects list file due to unexpected EOF

Sure, the patch's message was more of a draft.

bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

[Dmitry Gutov]

On 25/01/2024 16:05, Spencer Baugh wrote:
> Yes, I think that would be great.  Especially because this allows 
> startup to continue.  If it's okay with you too, I think it's reasonable 
> to push.

Now pushed, thanks.

I'm not closing the bug, it's up to you to decide whether there's more 
to be done.

bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

[Dmitry Gutov]

On 25/01/2024 16:12, Spencer Baugh wrote:
> Ah, I think the cause (in at least one case) is that the user ran out of 
> disk space, and so the write-region call in project--write-project-list 
> failed when writing, which happens after truncating the existing file.

Makes sense.

> I 
> guess there should be some atomic version of write-region which writes 
> to a temp file and renames it over the original.

Yep, that would be an improvement.

bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

[Spencer Baugh]

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


Here is a straightforward fix.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Prevent-incorrect-error-message-when-calling-read-in.patch --]
[-- Type: text/x-patch, Size: 3151 bytes --]

From d850108fa309701e4899dfbcfd5a20d1e17f86af Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Fri, 16 Feb 2024 16:53:28 -0500
Subject: [PATCH] Prevent incorrect error message when calling read inside load

Previously, if `load' eval'd a `read' expression which raised
end-of-file, the error would include load-true-file-name, even
though the `read' may be reading something completely different.

Now, end-of-file errors raised by `read' will only include
load-true-file-name if it's actually reading that file.

We do this by having read include read-end-of-file-name in the error
instead of load-true-file-name, and only binding read-end-of-file-name
around the "read" parts of readevalloop, not the "eval" parts.
(load-true-file-name is still bound throughout)

Also, when reading a file (or some other source), it is now
possible to bind read-end-of-file-name so that end-of-file
errors raised by read will include the filename (or the string
of your choice).  Previously, an end-of-file error raised by
read outside of load would never include the filename.

* src/lread.c (syms_of_lread): Add read-end-of-file-name.
(readevalloop): Bind read-end-of-file-name to
load-true-file-name around read.
(end_of_file_error): Use read-end-of-file-name instead of
load-true-file-name.  (bug#68546)
---
 src/lread.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/lread.c b/src/lread.c
index 0e67a3f8879..54ae88c7eab 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -2385,8 +2385,8 @@ readevalloop_1 (int old)
 static AVOID
 end_of_file_error (void)
 {
-  if (STRINGP (Vload_true_file_name))
-    xsignal1 (Qend_of_file, Vload_true_file_name);
+  if (!NILP (Vread_end_of_file_name))
+    xsignal1 (Qend_of_file, Vread_end_of_file_name);
 
   xsignal0 (Qend_of_file);
 }
@@ -2490,6 +2490,8 @@ readevalloop (Lisp_Object readcharfun,
   while (continue_reading_p)
     {
       specpdl_ref count1 = SPECPDL_INDEX ();
+      if (NILP (Vread_end_of_file_name))
+	specbind (Qread_end_of_file_name, Vload_true_file_name);
 
       if (b != 0 && !BUFFER_LIVE_P (b))
 	error ("Reading from killed buffer");
@@ -2585,7 +2587,7 @@ readevalloop (Lisp_Object readcharfun,
       if (!NILP (start) && continue_reading_p)
 	start = Fpoint_marker ();
 
-      /* Restore saved point and BEGV.  */
+      /* Restore saved point and BEGV, and unbind read_stream_for_error.  */
       unbind_to (count1, Qnil);
 
       /* Now eval what we just read.  */
@@ -5843,6 +5845,12 @@ syms_of_lread (void)
 	       doc: /* Full name of file being loaded by `load'.  */);
   Vload_true_file_name = Qnil;
 
+  DEFVAR_LISP ("read-end-of-file-name", Vread_end_of_file_name,
+	       doc: /* String to be included when `read' signals `end-of-file'.
+When loading a file, this is bound to the filename.  */);
+  Vread_end_of_file_name = Qnil;
+  DEFSYM (Qread_end_of_file_name, "read-end-of-file-name");
+
   DEFVAR_LISP ("user-init-file", Vuser_init_file,
 	       doc: /* File name, including directory, of user's initialization file.
 If the file loaded had extension `.elc', and the corresponding source file
-- 
2.39.3

bug#68546: 29.1.90; end-of-file has incorrect data when signaled within a load

[Eli Zaretskii]

> Cc: dmitry@gutov.dev
> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Fri, 16 Feb 2024 16:56:26 -0500
> 
> Here is a straightforward fix.

Adding Stefan to the discussion, since this is no longer specific to
project.el.  Stefan, any comments?

> >From d850108fa309701e4899dfbcfd5a20d1e17f86af Mon Sep 17 00:00:00 2001
> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Fri, 16 Feb 2024 16:53:28 -0500
> Subject: [PATCH] Prevent incorrect error message when calling read inside load
> 
> Previously, if `load' eval'd a `read' expression which raised
> end-of-file, the error would include load-true-file-name, even
> though the `read' may be reading something completely different.
> 
> Now, end-of-file errors raised by `read' will only include
> load-true-file-name if it's actually reading that file.
> 
> We do this by having read include read-end-of-file-name in the error
> instead of load-true-file-name, and only binding read-end-of-file-name
> around the "read" parts of readevalloop, not the "eval" parts.
> (load-true-file-name is still bound throughout)
> 
> Also, when reading a file (or some other source), it is now
> possible to bind read-end-of-file-name so that end-of-file
> errors raised by read will include the filename (or the string
> of your choice).  Previously, an end-of-file error raised by
> read outside of load would never include the filename.

This needs the obvious documentation changes.

Also, I'm not sure I understand the rationale well enough: what other
use cases do we envision where read-end-of-file-name will be bound to
some other string than the name of the file being read?  IOW, this is
a general infrastructure change, but the use cases that justify such
generality are not clear to me.

> --- a/src/lread.c
> +++ b/src/lread.c
> @@ -2385,8 +2385,8 @@ readevalloop_1 (int old)
>  static AVOID
>  end_of_file_error (void)
>  {
> -  if (STRINGP (Vload_true_file_name))
> -    xsignal1 (Qend_of_file, Vload_true_file_name);
> +  if (!NILP (Vread_end_of_file_name))
> +    xsignal1 (Qend_of_file, Vread_end_of_file_name);

Why !NILP instead of STRINGP? read-end-of-file-name is documented to
take string values.

>  
>    xsignal0 (Qend_of_file);
>  }
> @@ -2490,6 +2490,8 @@ readevalloop (Lisp_Object readcharfun,
>    while (continue_reading_p)
>      {
>        specpdl_ref count1 = SPECPDL_INDEX ();
> +      if (NILP (Vread_end_of_file_name))
> +	specbind (Qread_end_of_file_name, Vload_true_file_name);
>  
>        if (b != 0 && !BUFFER_LIVE_P (b))
>  	error ("Reading from killed buffer");
> @@ -2585,7 +2587,7 @@ readevalloop (Lisp_Object readcharfun,
>        if (!NILP (start) && continue_reading_p)
>  	start = Fpoint_marker ();
>  
> -      /* Restore saved point and BEGV.  */
> +      /* Restore saved point and BEGV, and unbind read_stream_for_error.  */

read_stream_for_error or read-end-of-file-name?  If the former, I
don't understand the rationale for this hunk.

> +  DEFVAR_LISP ("read-end-of-file-name", Vread_end_of_file_name,
> +	       doc: /* String to be included when `read' signals `end-of-file'.
                              ^^^^^^^^^^^^^^
"included" where?  This sentence needs to be clarified, either in it
or in the following text.

Also, the general nature of the feature is not reflected in the
variable's name: if this can be any string, why does it have
"file-name" in its name?

Thanks.