bug#50946: Emacs-28: Inadequate coding in hack-elisp-shorthands

[Alan Mackenzie <acm@muc.de>]

Hello, Eli.

On Sat, Oct 02, 2021 at 17:19:17 +0300, Eli Zaretskii wrote:
> > Date: Sat, 2 Oct 2021 13:57:21 +0000
> > Cc: joaotavora@gmail.com, 50946@debbugs.gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > If you want me to take your critique seriously, please be specific:
> > > what are the aspects of that function that you think lack attention to
> > > detail, and what detail?

> > The five aspects I enumerated on my original bug report.  Not checking
> > for a properly formatted Local Variables: section

> That is not part of the function in question, is it?  It's in
> hack-local-variables--find-variables, which we use everywhere.

It is now.  It wasn't when I raised the bug a day or two ago.

> > not checking for lower-case in that variable being searched for

> That's also in hack-local-variables--find-variables, right?

It is now.  It wasn't when I raised the bug

> > not going back at least 3000 characters

> That is now fixed, right?

No, it's not.  In certain edge cases, it will go back fewer than 3000
characters.

> > Additionally, the original code didn't check for a page boundary.

> No longer relevant, right?

No longer relevant.

> > What worries me is that this lack of attention to detail might extend to
> > the new code in lread.c, or the documentation page.

> You are welcome to point out specific issues with that.

In other words, I should do the review myself, you mean?  I really don't
want to do this.  I am not at all enthusiastic about the shorthands
feature and think somebody with more enthusiasm should do it.

> > I still think somebody who isn't João and isn't me should check
> > this.

> I did that when reviewing the commit.

Ah, good!  Thanks!

> > I worry, to a lesser degree, it is not entirely clear whether setting
> > the elisp-shorthands variable in the first line of a short file should
> > be valid or not.  I don't think the current hack-elisp-shorthands is
> > careful enough about this.

> Why does it matter?

Because the first line definition should either be valid or not valid.
Currently it works for a sufficiently small file, but not for a normal
sized file.  This, I think, is a bug.

> > > And while at that, please try to distinguish between general problems
> > > of hack-local-variables--find-variables, which affect all of Emacs,
> > > and hack-elisp-shorthands, which is specific to this feature.

> > I think all the things I've said in this bug report are about
> > hack-elisp-shorthands and the other new code/doc in this feature.

> See above: I don't think I agree.

I've largely been talking about the version of hack-elisp-shorthands I
raised the bug on.

> > > So the invitation to calm down goes both ways here.  Please focus on
> > > technical issues and leave ad-hominem out of this, okay?

> > Is this bug to be fixed completely, or are the edge cases just to be
> > ignored as unimportant?  That is the current technical issue as I see
> > it.  I really don't want to fix the bug myself, but am prepared to do so
> > if nobody else is.  If you think the bug indeed needs completely fixing,
> > please reopen it - there's no point in me trying to reopen it again.  If
> > you don't, just leave things the way they are.

> Please forget what happened before, and let's pick up from the above
> points I mention.  What else needs to be fixed in shorthands.el, and
> why?

I documented those carefully in my post to this thread this morning.
Here are some excerpts from that:

#########################################################################
Say you have a file 3150 bytes long, which is less than 3000 characters
in Emacs.  Your function will load only 3100 bytes, less than 3000
characters, into the temporary buffer.  It thus may fail to find a Local
Variables section, even if this scenario is highly unusual.

Have you checked that things work if the first byte in your temporary
buffer isn't at the start of a character?

Also, it is unclear whether the elisp-shorthands symbol on the first
line of a file is valid.  I think the intention is that it not be valid.
However, the current version of the code will read it from the first
line of a sufficiently small file.  You should look at this.  Possibly
this needs to be clarified in the documentation for shorthands.
#########################################################################

-- 
Alan Mackenzie (Nuremberg, Germany).