bug#36970: 26.2; invalid-read-syntax could print the location of the error

bug#36970: 26.2; invalid-read-syntax could print the location of the error

[ndame]

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

It only says:

    (invalid-read-syntax ". in wrong context")

It could also print the character location or even the line number and
the position in the line to make it easy to find the error.
 

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

bug#36970: 26.2; invalid-read-syntax could print the location of the error

[Lars Ingebrigtsen]

ndame <emacsuser@freemail.hu> writes:

> It only says:
>
>     (invalid-read-syntax ". in wrong context")
>
> It could also print the character location or even the line number and
> the position in the line to make it easy to find the error.

Poking around in read1 and friends, this doesn't seem very difficult to
implement -- the errors are signalled from the invalid_syntax function.

The wrinkle is that we may be reading from a string or something else
based on readcharfun.  However, the common case is reading from a
buffer, and we could improve the error message there.

So my idea here would be to change all the calls that are like this:

	      invalid_syntax (". in wrong context");

into

	      invalid_syntax (". in wrong context", readcharfun);

and then, in that function do

  if (BUFFERP (readcharfun))
    make_string_based_on_point ()

Since point is at the problematic element, that could just do a
Fcount_lines/Fcurrent_column to get the line number/column number,
presumably.

Before starting to hack away at this, does anybody see any problems with
this approach?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36970: 26.2; invalid-read-syntax could print the location of the error

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>   if (BUFFERP (readcharfun))
>     make_string_based_on_point ()
>
> Since point is at the problematic element, that could just do a
> Fcount_lines/Fcurrent_column to get the line number/column number,
> presumably.

I started tinkering with this, but there's a couple of things that are
less obvious to me than I thought:

Should the line/column numbers be based on what Fcount_lines return, or
should they be the actual line number of the buffer?  That is, taking
selective display etc into consideration or not?  Ignoring all that and
just counting newlines would make sense in many situations (it's more
like when compiling a file), but not in others?

And...  I'm slightly nervous about calling out to something as
complicated as Fcount_lines.  There's no efficiency considerations here
(this is for error handling only), so I think calling that would be OK,
but it'd be un-fun if the error handling bugged out.

So I was poking through the Emacs lisp code to see whether there's any
function that will just tell me what line I'm on, and there's...
display_count_lines in xdisp.c, but that doesn't really seem like a good
fit, since that's in redisplay, and not lread.c context.

Isn't there any function that will just return the line number at point?
I've been grepping for half an hour without finding anything.

So I wrote a current_line function, which is trivial enough (and fast)
if we're just looking for newlines and ignoring everything else...
which...  may be what we want here?  Or not?

Any opinions?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36970: 26.2; invalid-read-syntax could print the location of the error

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Should the line/column numbers be based on what Fcount_lines return, or
> should they be the actual line number of the buffer?  That is, taking
> selective display etc into consideration or not? 

I think perhaps not?  Getting better error reporting here is mostly
useful when doing batch compilation etc.  If we're just saying (read
(current-buffer)), then point will be left at the offending character,
so we don't really need the info?  But we do need the info when doing
that from a byte-compile-file or the like...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36970: 26.2; invalid-read-syntax could print the location of the error

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 31 Jan 2021 10:14:59 +0100
> Cc: 36970@debbugs.gnu.org
> 
> Should the line/column numbers be based on what Fcount_lines return, or
> should they be the actual line number of the buffer?  That is, taking
> selective display etc into consideration or not?  Ignoring all that and
> just counting newlines would make sense in many situations (it's more
> like when compiling a file), but not in others?
> 
> And...  I'm slightly nervous about calling out to something as
> complicated as Fcount_lines.

What is Fcount_lines?

> So I was poking through the Emacs lisp code to see whether there's any
> function that will just tell me what line I'm on, and there's...
> display_count_lines in xdisp.c, but that doesn't really seem like a good
> fit, since that's in redisplay, and not lread.c context.

Can you tell why did you think display_count_lines will not do the job
you need it to do?

> Isn't there any function that will just return the line number at point?

That's display_count_lines, IMO.

bug#36970: 26.2; invalid-read-syntax could print the location of the error

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> And...  I'm slightly nervous about calling out to something as
>> complicated as Fcount_lines.
>
> What is Fcount_lines?

Sorry, I meant `count-lines'.

>> So I was poking through the Emacs lisp code to see whether there's any
>> function that will just tell me what line I'm on, and there's...
>> display_count_lines in xdisp.c, but that doesn't really seem like a good
>> fit, since that's in redisplay, and not lread.c context.
>
> Can you tell why did you think display_count_lines will not do the job
> you need it to do?

Because it's only used in xdisp.c, and I was guessing there was some
reason for that, and:

   Set *BYTE_POS_PTR to the byte position where we stopped.  This is
   either the position COUNT lines after/before START_BYTE, if we
   found COUNT lines, or LIMIT_BYTE if we hit the limit before finding
   COUNT lines.  */

So from the comments it seemed like this had some other, more complex
use case.  But I see from the actual call sites that it looks just like
what I need:

	nlines = display_count_lines (startpos_byte,
				      PT_BYTE, PT, &junk);

So I guess I should just put the prototype in lisp.h and use it?  (It's
kinda surprising that no other part of Emacs has felt the need to
compute a line number before...)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#36970: 26.2; invalid-read-syntax could print the location of the error

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacsuser@freemail.hu,  36970@debbugs.gnu.org
> Date: Mon, 01 Feb 2021 09:40:07 +0100
> 
> 	nlines = display_count_lines (startpos_byte,
> 				      PT_BYTE, PT, &junk);
> 
> So I guess I should just put the prototype in lisp.h and use it?

Yes (and make it non-static).  Alternatively, define a wrapper for it
that doesn't accept that last arg, and make _that_ wrapper extern and
more suitably named.

> (It's kinda surprising that no other part of Emacs has felt the need
> to compute a line number before...)

Emacs almost never counts lines, except for 2 display features: the
line-number display in the mode line and display-line-numbers-mode.
The buffer-with-gap paradigm makes a point of not knowing where each
line ends and how many lines are there.

bug#36970: 26.2; invalid-read-syntax could print the location of the error

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes (and make it non-static).  Alternatively, define a wrapper for it
> that doesn't accept that last arg, and make _that_ wrapper extern and
> more suitably named.

Yup.  I've made the wrapper even simpler, and dropped the count, too.

>> (It's kinda surprising that no other part of Emacs has felt the need
>> to compute a line number before...)
>
> Emacs almost never counts lines, except for 2 display features: the
> line-number display in the mode line and display-line-numbers-mode.
> The buffer-with-gap paradigm makes a point of not knowing where each
> line ends and how many lines are there.

Emacs almost never counts lines while running normally, but line numbers
are useful when reporting errors (like here), so I was surprised not to
find a convenient function to use.  On the other hand, perhaps this
explains why errors like these didn't use to report lines.  :-)

Anyway, I'm running some more tests, and then I'll push the change.  The
error message could be improved: It's

		CALLN (Fformat, build_string ("%s (line %d, column %d)"),

which looks a bit awkward -- feel free to tweak, everybody.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no