:alnum: broken?

:alnum: broken?

[Stephen Leake]

I'm attempting to generalize the regular expressions in ada-mode to
handle utf-8; some are currently limited to ASCII.

However, :alnum: appears to be broken:

(let ((text "01abCDE")
      res1 res2)
  (string-match "[:alnum:]+" text)
  (setq res1 (match-string 0 text))
  (string-match "[0-9a-zA-Z]+" text)
  (setq res2 (match-string 0 text))
  (list res1 res2))

gives:

("a" "01abCDE")

I'm running emacs master on Windows 8 x64.

I'm guessing this might be affected by the font: (frame-parameter nil 'font)
reports:
"-raster-Courier-normal-normal-normal-mono-13-*-*-*-c-*-iso8859-1"


On the other hand, running in a Debian testing VM on the same machine
gives the same results, and the font is
"-PfEd-DejaVu Sans Mono-normal-normal-normal-*-11-*-*-*-m-0-iso10646-1"

What am I missing?

-- 
-- Stephe

Re: :alnum: broken?

[Paul Eggert]

On 2/21/20 10:58 AM, Stephen Leake wrote:
>    (string-match "[:alnum:]+" text)

You meant "[[:alnum:]]+".

Re: :alnum: broken?

[Noam Postavsky]

On Fri, 21 Feb 2020 at 13:58, Stephen Leake
<stephen_leake@stephe-leake.org> wrote:

>   (string-match "[:alnum:]+" text)

> What am I missing?

You're missing set of brackets. It should be "[[:alnum:]]+" rather
than "[:alnum:]".

Re: :alnum: broken?

[Andreas Schwab]

On Feb 21 2020, Stephen Leake wrote:

>   (string-match "[:alnum:]+" text)

That's the same as (string-match "[:almnu]+" text).  If you want to
match alphanumeric characters, use "[[:alnum:]]+".

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: :alnum: broken?

[Mattias Engdegård]

21 feb. 2020 kl. 20.00 skrev Paul Eggert <eggert@cs.ucla.edu>:

> On 2/21/20 10:58 AM, Stephen Leake wrote:
>>   (string-match "[:alnum:]+" text)
> 
> You meant "[[:alnum:]]+".

Or maybe (rx (+ alnum)).

Running relint is also a good idea -- it would have caught the mistake. There is a similar one in csv-mode in GNU ELPA.

Re: :alnum: broken?

[Stephen Leake]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 2/21/20 10:58 AM, Stephen Leake wrote:
>>    (string-match "[:alnum:]+" text)
>
> You meant "[[:alnum:]]+".

Ah, that makes sense.

I propose this patch to help others avoid the same mistake:

--- a/doc/lispref/searching.texi
+++ b/doc/lispref/searching.texi
@@ -582,8 +582,10 @@ Char Classes
 @cindex alpha character class, regexp
 @cindex xdigit character class, regexp
 
-  Here is a table of the classes you can use in a character alternative,
-and what they mean:
+  Here is a table of the classes you can use in a character
+alternative, and what they mean. Note that the characters @samp{[]}
+are part of the character class name, so a regular expression using
+one would be @samp{[[:alnum:]]+}.
 
 @table @samp
 @item [:ascii:]

-- 
-- Stephe

Re: :alnum: broken?

[Paul Eggert]

On 2/21/20 1:28 PM, Stephen Leake wrote:
> I propose this patch to help others avoid the same mistake:

Perhaps better yet would be for Emacs to do what GNU grep does:

$ grep '[:space:]'
grep: character class syntax is [[:space:]], not [:space:]
$ echo $?
2

That is, GNU grep treats a bracket expression like '[:space:]' as an 
error, since it's inevitably a typo. (POSIX does not allow this behavior 
so the diagnostic is suppressed if POSIXLY_CORRECT is set.)

Re: :alnum: broken?

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 21 Feb 2020 17:09:20 -0800
> 
> Perhaps better yet would be for Emacs to do what GNU grep does:
> 
> $ grep '[:space:]'
> grep: character class syntax is [[:space:]], not [:space:]
> $ echo $?
> 2

But "[:space:]" is a valid, though peculiar, character class, so
erroring out for it would be inappropriate, IMO.

Re: :alnum: broken?

[Eli Zaretskii]

> From: Stephen Leake <stephen_leake@stephe-leake.org>
> Date: Fri, 21 Feb 2020 13:28:19 -0800
> 
> I propose this patch to help others avoid the same mistake:

Thanks, I installed a note along these lines.

Re: :alnum: broken?

[Paul Eggert]

On 2/21/20 11:48 PM, Eli Zaretskii wrote:
> "[:space:]" is a valid, though peculiar, character class

Yes, but point of my suggestion was to suggest that we change Emacs to make it 
not a valid character class. In practice it would be a win to change Emacs in 
this way, since the aggravation of the current approach (which regularly bites 
people as this thread illustrates) far outweighs the aggravation of making 
classes like "[:space:]" invalid when they're intended (which they're invariably 
not).

A similar change went into GNU grep a decade ago, and in practice it's been a 
win. It would also be a win with Emacs.

Re: :alnum: broken?

[Eli Zaretskii]

> Cc: stephen_leake@stephe-leake.org, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 22 Feb 2020 13:28:58 -0800
> 
> A similar change went into GNU grep a decade ago, and in practice it's been a 
> win. It would also be a win with Emacs.

I don't think I see a win here, sorry.

Re: :alnum: broken?

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

Can we do something to report such errors?  For instance, the compiler
could warn when it sees an argument that is a regexp which is a
constant string with a character class containing two colons.

-- 
Dr Richard Stallman
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: :alnum: broken?

[Paul Eggert]

On 2/22/20 7:49 PM, Richard Stallman wrote:
> Can we do something to report such errors?

Yes, it would be easy to modify the Emacs regular expression parser to signal an 
error for patterns like [:alnum:]. GNU grep has been reporting these as errors 
ever since Paolo Bonzini changed it to do so in 2010, and it's been a win there. 
However, Eli's the Emacs maintainer and he has not yet been convinced to change 
GNU Emacs to be consistent with GNU grep.

Re: :alnum: broken?

[Mattias Engdegård]

22 feb. 2020 kl. 02.09 skrev Paul Eggert <eggert@cs.ucla.edu>:

> That is, GNU grep treats a bracket expression like '[:space:]' as an error, since it's inevitably a typo. (POSIX does not allow this behavior so the diagnostic is suppressed if POSIXLY_CORRECT is set.)

Such a check is obviously unsound, strictly speaking, which may be a reason for objecting to it. But in practice? It would have to be naïve regexp-building code that puts characters inside square brackets without eliminating duplicates, like

(defun make-char-regexp (char-list)
  (concat "[" (apply #'string char-list) "]"))

and given our experience in regexp mistakes, such code probably exists in use somewhere. I'm still in favour of the check since it would be cheap and helpful, and false positives unlikely. It is a fairly infrequent blunder, though.

Re: :alnum: broken?

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 22 Feb 2020 23:51:11 -0800
> Cc: emacs-devel@gnu.org
> 
> On 2/22/20 7:49 PM, Richard Stallman wrote:
> > Can we do something to report such errors?
> 
> Yes, it would be easy to modify the Emacs regular expression parser to signal an 
> error for patterns like [:alnum:]. GNU grep has been reporting these as errors 
> ever since Paolo Bonzini changed it to do so in 2010, and it's been a win there. 
> However, Eli's the Emacs maintainer and he has not yet been convinced to change 
> GNU Emacs to be consistent with GNU grep.

AFAIU, Richard suggested to emit a warning from the byte compiler.
That might be a good-enough solution, better than signaling an error
at run time, IMO.

RE: :alnum: broken?

[Drew Adams]

> > signal an error for patterns like [:alnum:].
> 
> AFAIU, Richard suggested to emit a warning from the byte compiler.
> That might be a good-enough solution, better than signaling an error
> at run time, IMO.

+1.  Not only better, but appropriate.

It should be a warning, because it is not an
invalid regexp.

It might generally be a useless regexp, and
it's most likely not what the user intended,
but it's not invalid.

Raising an error would, well, be an error, IMHO.
Displaying a warning during byte-compilation
would be very helpful.

Re: :alnum: broken?

[Paul Eggert]

On 2/23/20 2:21 AM, Mattias Engdegård wrote:
>> GNU grep treats a bracket expression like '[:space:]' as an error, since it's inevitably a typo....
> Such a check is obviously unsound, strictly speaking, which may be a reason for objecting to it.

I understand the objection, but the check is not unsound. The syntax of regexps 
was not carved in stone by God. It is something that we decide, and we can 
change our minds if the change would be an overall win, as it would be if Emacs 
behaved like Grep.

When the [[:alnum:]] syntax was added to regular expressions many years ago, it 
invalidated some astronomically-unlikely but formerly-valid expressions. For 
example, "[[:x:]" formerly was a valid regexp (with the same meaning as "[:[x]") 
but it is now invalid. So there is precedent for invalidating some 
astronomically-unlikely regexps when the overall change is a net benefit, as it 
would be if Emacs behaved like Grep.

The byte-compiler could warn about some of these blunders and if someone wants 
to change the byte-compiler to do that, it would be an improvement. However, 
this would necessarily either cry wolf or let blunders through, because the 
byte-compiler cannot reliably determine whether a string will be used as a 
regular expression.

Re: :alnum: broken?

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 23 Feb 2020 10:13:16 -0800
> Cc: Stephen Leake <stephen_leake@stephe-leake.org>,
>  emacs-devel <emacs-devel@gnu.org>
> 
> The byte-compiler could warn about some of these blunders and if someone wants 
> to change the byte-compiler to do that, it would be an improvement. However, 
> this would necessarily either cry wolf or let blunders through, because the 
> byte-compiler cannot reliably determine whether a string will be used as a 
> regular expression.

Indeed, the byte compiler cannot, which is why issuing a warning is
appropriate.  Experience shows that we do pay attention to warnings
and try to have our sources compile warning-free.

Re: :alnum: broken?

[Mattias Engdegård]

23 feb. 2020 kl. 19.13 skrev Paul Eggert <eggert@cs.ucla.edu>:

> I understand the objection, but the check is not unsound. The syntax of regexps was not carved in stone by God. It is something that we decide, and we can change our minds if the change would be an overall win, as it would be if Emacs behaved like Grep.

We seem to be in full agreement. (I meant incomplete rather than unsound, of course, and you were graceful enough to understand that.)

> The byte-compiler could warn about some of these blunders and if someone wants to change the byte-compiler to do that, it would be an improvement. However, this would necessarily either cry wolf or let blunders through, because the byte-compiler cannot reliably determine whether a string will be used as a regular expression.

Quite, it would be rather elaborate code for finding a small subset of what relint does. In contrast, doing it in the regexp compiler is both simple and accurate, although only mistakes encountered dynamically are found.

Re: :alnum: broken?

[Óscar Fuentes]

Eli Zaretskii <eliz@gnu.org> writes:

> Indeed, the byte compiler cannot, which is why issuing a warning is
> appropriate.  Experience shows that we do pay attention to warnings
> and try to have our sources compile warning-free.

The less experienced users, which are those who fall the most on the
trap (me raises hand) hardly would benefit from a compiler warning. We
use regexps on interactive *-regexp commands, not on Elisp code that we
later compile. That's package writers.

RE: :alnum: broken?

[Drew Adams]

> The less experienced users, which are those who fall the most on the
> trap (me raises hand) hardly would benefit from a compiler warning. We
> use regexps on interactive *-regexp commands, not on Elisp code that we
> later compile. That's package writers.

That's definitely true, IMO.  But:

1. Warning during regexp input (e.g. regexp Isearch)
   would likely, itself, be quite confusing or
   annoying.

2. There are _lots_ of regexp-learning gotchas. The
   case currently discussed is just one such.  Any
   attempt to interrupt input with some guidance
   wrt what a user might be doing wrong would truly
   be a distraction to lots of other users, and
   even the same user sometimes, and at some point.

   It could be imagined that Emacs could provide a
   mode that helps users in such ways, but it would
   need to be quite smart and configurable.

   The differences in help level needed for different
   users make that alone problematic.  Ideally,
   perhaps, such help would watch a given user and
   learn over time (machine-learning) just what
   kinds of help might be most appropriate.

Re: :alnum: broken?

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

  > 1. Warning during regexp input (e.g. regexp Isearch)
  >    would likely, itself, be quite confusing or
  >    annoying.

Maybe there is a way to do it that won't be confusing.
If it waits until you type RET to finish the regexp.
then flashes a warning on the screen for two seconds or until there is
more input, it might be clear and not annoying.

-- 
Dr Richard Stallman
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: :alnum: broken?

[Andreas Schwab]

On Feb 23 2020, Drew Adams wrote:

> 1. Warning during regexp input (e.g. regexp Isearch)
>    would likely, itself, be quite confusing or
>    annoying.

It's not unlike the error you get during input of an incomplete or
otherwise malformed regexp.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: :alnum: broken?

[Clément Pit-Claudel]

On 2020-02-25 04:33, Andreas Schwab wrote:
> On Feb 23 2020, Drew Adams wrote:
> 
>> 1. Warning during regexp input (e.g. regexp Isearch)
>>    would likely, itself, be quite confusing or
>>    annoying.
> 
> It's not unlike the error you get during input of an incomplete or
> otherwise malformed regexp.

Yes, handling it that way would work quite fine, I think.

Re: :alnum: broken?

[Stefan Monnier]

> Maybe there is a way to do it that won't be confusing.
> If it waits until you type RET to finish the regexp.
> then flashes a warning on the screen for two seconds or until there is
> more input, it might be clear and not annoying.

We could also have `read-regexp` show a warning *on the side* while you
type the input, a bit like isearch does when the regexp is
incomplete/invalid.

It would also make sense to make `read-regexp` emit such warnings about
incomplete/invalid regexps.


        Stefan

RE: :alnum: broken?

[Drew Adams]

>   > 1. Warning during regexp input (e.g. regexp Isearch)
>   >    would likely, itself, be quite confusing or
>   >    annoying.
> 
> Maybe there is a way to do it that won't be confusing.
> If it waits until you type RET to finish the regexp.
> then flashes a warning on the screen for two seconds or until there is
> more input, it might be clear and not annoying.

If such a way is found, and especially if it's
user-configurable (e.g. users can turn it off,
without just turning off all warnings), that
would be a good thing, indeed.

Wrt RET: I was thinking mostly of regexp isearch.
In that case, RET ends searching, so I don't see
that particular part as being appropriate.  But
I take the general idea that perhaps something
could be done.

RE: :alnum: broken?

[Drew Adams]

> > 1. Warning during regexp input (e.g. regexp Isearch)
> >    would likely, itself, be quite confusing or
> >    annoying.
> 
> It's not unlike the error you get during input of
> an incomplete or otherwise malformed regexp.

Yes, I guess that could work.  (I guess you, like I,
are thinking about regexp Isearch, at least as one
such use case.)

The message should, in that case, be quite different
from the incomplete/malformed message.  It should
essentially point out that what you've typed so far
is a character class, not a character alternative.
E.g. "Did you perhaps mean [[:alnum:]]?"

Maybe even better might be to query the user to
just continue (y or SPC) or to go to the manual
node that talks about this.

(And the manual should probably explicitly point
out the gotcha of using a char-class outside of
a char alternative.  Actually, I maybe think it
did, at one time, and that was removed (?).)

Since different users will likely see such
interactive help as more or less helpful/annoying,
it should be configurable.  E.g. (1) no warning,
(2) on-the-fly brief reminder, (3) query to let
you visit the manual or get more explanation in
some other way (e.g. *Help*).

RE: :alnum: broken?

[Drew Adams]

> > Maybe there is a way to do it that won't be confusing.
> > If it waits until you type RET to finish the regexp.
> > then flashes a warning on the screen for two seconds or until there is
> > more input, it might be clear and not annoying.
> 
> We could also have `read-regexp` show a warning *on the side* while you
> type the input, a bit like isearch does when the regexp is
> incomplete/invalid.
>
> It would also make sense to make `read-regexp` emit such warnings about
> incomplete/invalid regexps.

Both suggestions sound good.  

Re: :alnum: broken?

[Mattias Engdegård]

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

I just made this very mistake while adding a new regexp-error checking feature to xr. Needless to say I now am strongly in favour of turning it into a hard error.

Patch attached. It was written for master, but I would suggest it go in emacs-27.

The error message could be improved. For the benefit of isearch-forward-regexp, it's probably a good idea if it doesn't start or end in a square bracket.


[-- Attachment #2: 0001-Signal-an-error-for-the-regexp-alnum.patch --]
[-- Type: application/octet-stream, Size: 3737 bytes --]

From 014a7a7dce5ae23b8a47dd68eaaef0a5cb985b46 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 26 Feb 2020 14:46:01 +0100
Subject: [PATCH] Signal an error for the regexp "[:alnum:]"

Omitting the extra brackets is a common mistake; see discussion at
https://lists.gnu.org/archive/html/emacs-devel/2020-02/msg00215.html

* src/regex-emacs.c (reg_errcode_t, re_error_msgid): Add REG_ECLASSBR.
(regex_compile): Check for the mistake.
* test/src/regex-emacs-tests.el (regexp-invalid): Test.
* etc/NEWS: Announce.
---
 etc/NEWS                      |  5 +++++
 src/regex-emacs.c             | 21 ++++++++++++++++++++-
 test/src/regex-emacs-tests.el |  4 ++++
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 54aab1a5b6..404b4b9ebd 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -190,6 +190,11 @@ Emacs now supports bignums so this old glitch is no longer needed.
 'previous-system-time-locale' have been removed, as they were created
 by mistake and were not useful to Lisp code.
 
+** The regexp mistake '[:digit:]' is now an error.
+The correct syntax is '[[:digit:]]'.  Previously, forgetting the extra
+brackets silently resulted in a regexp that did not at all work as
+intended.
+
 \f
 * Lisp Changes in Emacs 28.1
 
diff --git a/src/regex-emacs.c b/src/regex-emacs.c
index 694431c95e..2648e1d6ae 100644
--- a/src/regex-emacs.c
+++ b/src/regex-emacs.c
@@ -818,7 +818,8 @@ print_double_string (re_char *where, re_char *string1, ptrdiff_t size1,
   REG_ESIZE,		/* Compiled pattern bigger than 2^16 bytes.  */
   REG_ERPAREN,		/* Unmatched ) or \); not returned from regcomp.  */
   REG_ERANGEX,		/* Range striding over charsets.  */
-  REG_ESIZEBR           /* n or m too big in \{n,m\} */
+  REG_ESIZEBR,          /* n or m too big in \{n,m\} */
+  REG_ECLASSBR,         /* Missing [] around [:class:].  */
 } reg_errcode_t;
 
 static const char *re_error_msgid[] =
@@ -842,6 +843,7 @@ print_double_string (re_char *where, re_char *string1, ptrdiff_t size1,
    [REG_ERPAREN] = "Unmatched ) or \\)",
    [REG_ERANGEX ] = "Range striding over charsets",
    [REG_ESIZEBR ] = "Invalid content of \\{\\}",
+   [REG_ECLASSBR] = "Class syntax is [[:digit:]], not [:digit:]",
   };
 
 /* For 'regs_allocated'.  */
@@ -2000,6 +2002,23 @@ regex_compile (re_char *pattern, ptrdiff_t size,
 
 	    laststart = b;
 
+            /* Check for the mistake of forgetting the extra square brackets,
+               as in "[:alpha:]".  */
+            if (*p == ':')
+              {
+                re_char *q = p + 1;
+                while (q != pend && *q != ']')
+                  {
+                    if (*q == ':')
+                      {
+                        if (q + 1 != pend && q[1] == ']' && q > p + 1)
+                          FREE_STACK_RETURN (REG_ECLASSBR);
+                        break;
+                      }
+                    q++;
+                  }
+              }
+
 	    /* Test '*p == '^' twice, instead of using an if
 	       statement, so we need only one BUF_PUSH.  */
 	    BUF_PUSH (*p == '^' ? charset_not : charset);
diff --git a/test/src/regex-emacs-tests.el b/test/src/regex-emacs-tests.el
index f9372e37b1..d268b97080 100644
--- a/test/src/regex-emacs-tests.el
+++ b/test/src/regex-emacs-tests.el
@@ -803,4 +803,8 @@ regexp-multibyte-unibyte
   (should-not (string-match "å" "\xe5"))
   (should-not (string-match "[å]" "\xe5")))
 
+(ert-deftest regexp-invalid ()
+  (should-error (string-match "[:space:]" "")
+                :type 'invalid-regexp))
+
 ;;; regex-emacs-tests.el ends here
-- 
2.21.1 (Apple Git-122.3)

RE: :alnum: broken?

[Drew Adams]

> I just made this very mistake while adding a new regexp-error checking
> feature to xr. Needless to say I now am strongly in favour of turning it
> into a hard error.

Why "needless to say", and why wouldn't a
warning have alerted you?

It's not an error.  It's not an invalid
regexp, even though it has unnecessary
duplication and it doesn't do at all
what you were likely expecting.  That
faulty expectation is why a warning is
appropriate.

Re: :alnum: broken?

[Stefan Monnier]

> Patch attached. It was written for master, but I would suggest it go in emacs-27.

FWIW, you have a +1 from me, tho I don't see any urgency so I'd keep it
for `master`.


        Stefan


Mattias Engdegård [2020-02-26 15:10:46] wrote:

> I just made this very mistake while adding a new regexp-error checking
> feature to xr. Needless to say I now am strongly in favour of turning it
> into a hard error.
>
>
> The error message could be improved. For the benefit of
> isearch-forward-regexp, it's probably a good idea if it doesn't start or end
> in a square bracket.
>
> From 014a7a7dce5ae23b8a47dd68eaaef0a5cb985b46 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
> Date: Wed, 26 Feb 2020 14:46:01 +0100
> Subject: [PATCH] Signal an error for the regexp "[:alnum:]"
>
> Omitting the extra brackets is a common mistake; see discussion at
> https://lists.gnu.org/archive/html/emacs-devel/2020-02/msg00215.html
>
> * src/regex-emacs.c (reg_errcode_t, re_error_msgid): Add REG_ECLASSBR.
> (regex_compile): Check for the mistake.
> * test/src/regex-emacs-tests.el (regexp-invalid): Test.
> * etc/NEWS: Announce.
> ---
>  etc/NEWS                      |  5 +++++
>  src/regex-emacs.c             | 21 ++++++++++++++++++++-
>  test/src/regex-emacs-tests.el |  4 ++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/etc/NEWS b/etc/NEWS
> index 54aab1a5b6..404b4b9ebd 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -190,6 +190,11 @@ Emacs now supports bignums so this old glitch is no longer needed.
>  'previous-system-time-locale' have been removed, as they were created
>  by mistake and were not useful to Lisp code.
>  
> +** The regexp mistake '[:digit:]' is now an error.
> +The correct syntax is '[[:digit:]]'.  Previously, forgetting the extra
> +brackets silently resulted in a regexp that did not at all work as
> +intended.
> +
>  \f
>  * Lisp Changes in Emacs 28.1
>  
> diff --git a/src/regex-emacs.c b/src/regex-emacs.c
> index 694431c95e..2648e1d6ae 100644
> --- a/src/regex-emacs.c
> +++ b/src/regex-emacs.c
> @@ -818,7 +818,8 @@ print_double_string (re_char *where, re_char *string1, ptrdiff_t size1,
>    REG_ESIZE,		/* Compiled pattern bigger than 2^16 bytes.  */
>    REG_ERPAREN,		/* Unmatched ) or \); not returned from regcomp.  */
>    REG_ERANGEX,		/* Range striding over charsets.  */
> -  REG_ESIZEBR           /* n or m too big in \{n,m\} */
> +  REG_ESIZEBR,          /* n or m too big in \{n,m\} */
> +  REG_ECLASSBR,         /* Missing [] around [:class:].  */
>  } reg_errcode_t;
>  
>  static const char *re_error_msgid[] =
> @@ -842,6 +843,7 @@ print_double_string (re_char *where, re_char *string1, ptrdiff_t size1,
>     [REG_ERPAREN] = "Unmatched ) or \\)",
>     [REG_ERANGEX ] = "Range striding over charsets",
>     [REG_ESIZEBR ] = "Invalid content of \\{\\}",
> +   [REG_ECLASSBR] = "Class syntax is [[:digit:]], not [:digit:]",
>    };
>  
>  /* For 'regs_allocated'.  */
> @@ -2000,6 +2002,23 @@ regex_compile (re_char *pattern, ptrdiff_t size,
>  
>  	    laststart = b;
>  
> +            /* Check for the mistake of forgetting the extra square brackets,
> +               as in "[:alpha:]".  */
> +            if (*p == ':')
> +              {
> +                re_char *q = p + 1;
> +                while (q != pend && *q != ']')
> +                  {
> +                    if (*q == ':')
> +                      {
> +                        if (q + 1 != pend && q[1] == ']' && q > p + 1)
> +                          FREE_STACK_RETURN (REG_ECLASSBR);
> +                        break;
> +                      }
> +                    q++;
> +                  }
> +              }
> +
>  	    /* Test '*p == '^' twice, instead of using an if
>  	       statement, so we need only one BUF_PUSH.  */
>  	    BUF_PUSH (*p == '^' ? charset_not : charset);
> diff --git a/test/src/regex-emacs-tests.el b/test/src/regex-emacs-tests.el
> index f9372e37b1..d268b97080 100644
> --- a/test/src/regex-emacs-tests.el
> +++ b/test/src/regex-emacs-tests.el
> @@ -803,4 +803,8 @@ regexp-multibyte-unibyte
>    (should-not (string-match "å" "\xe5"))
>    (should-not (string-match "[å]" "\xe5")))
>  
> +(ert-deftest regexp-invalid ()
> +  (should-error (string-match "[:space:]" "")
> +                :type 'invalid-regexp))
> +
>  ;;; regex-emacs-tests.el ends here

Re: :alnum: broken?

[Andreas Schwab]

On Feb 26 2020, Mattias Engdegård wrote:

> @@ -2000,6 +2002,23 @@ regex_compile (re_char *pattern, ptrdiff_t size,
>  
>  	    laststart = b;
>  
> +            /* Check for the mistake of forgetting the extra square brackets,
> +               as in "[:alpha:]".  */
> +            if (*p == ':')
> +              {
> +                re_char *q = p + 1;
> +                while (q != pend && *q != ']')
> +                  {
> +                    if (*q == ':')
> +                      {
> +                        if (q + 1 != pend && q[1] == ']' && q > p + 1)
> +                          FREE_STACK_RETURN (REG_ECLASSBR);
> +                        break;
> +                      }
> +                    q++;
> +                  }
> +              }
> +

That would break "[:[:alpha:]]".

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: :alnum: broken?

[Paul Eggert]

On 2/26/20 7:48 AM, Stefan Monnier wrote:
> FWIW, you have a +1 from me, tho I don't see any urgency so I'd keep it
> for `master`.

I agree with Stefan on both points.

The bug noted by Andreas should also be fixed. More specifically, I 
suggest having Emacs diagnose the typo only when GNU grep diagnoses it, 
as it's better to be consistent among GNU applications. Grep diagnoses 
the contents of [...] if all the following are true:

   * The first character is ":".
   * The last character is ":".
   * There is some non-":" character.
   * There are no ranges, char/equiv classes or collation elements.

GNU grep has done this for about a decade and this has worked well. If 
we don't like these rules we can change them but I would then suggest we 
also change GNU grep to be consistent.

Re: :alnum: broken?

[Mattias Engdegård]

26 feb. 2020 kl. 17.01 skrev Andreas Schwab <schwab@suse.de>:

> That would break "[:[:alpha:]]".

No, it seems all right.

Re: :alnum: broken?

[Mattias Engdegård]

26 feb. 2020 kl. 22.00 skrev Paul Eggert <eggert@cs.ucla.edu>:

> The bug noted by Andreas should also be fixed. More specifically, I suggest having Emacs diagnose the typo only when GNU grep diagnoses it, as it's better to be consistent among GNU applications. Grep diagnoses the contents of [...] if all the following are true:
> 
>  * The first character is ":".
>  * The last character is ":".
>  * There is some non-":" character.
>  * There are no ranges, char/equiv classes or collation elements.

Thank you -- there was no bug that I could see, and the patch already worked according to your above criteria; now pushed to master.

I changed the error message, but feel free to make it better if you are able to. As mentioned, I didn't want it to end in a square bracket, because regexp I-search displays the error inside square brackets.

Re: :alnum: broken?

[Clément Pit-Claudel]

On 2020-02-26 16:18, Mattias Engdegård wrote:
> 26 feb. 2020 kl. 22.00 skrev Paul Eggert <eggert@cs.ucla.edu>:
> 
>> The bug noted by Andreas should also be fixed. More specifically, I suggest having Emacs diagnose the typo only when GNU grep diagnoses it, as it's better to be consistent among GNU applications. Grep diagnoses the contents of [...] if all the following are true:
>>
>>  * The first character is ":".
>>  * The last character is ":".
>>  * There is some non-":" character.
>>  * There are no ranges, char/equiv classes or collation elements.
> 
> the patch already worked according to your above criteria

I think your patch disallows thing like [:-:], which grep allows.

Re: :alnum: broken?

[Mattias Engdegård]

26 feb. 2020 kl. 22.24 skrev Clément Pit-Claudel <cpitclaudel@gmail.com>:

> I think your patch disallows thing like [:-:], which grep allows.

Very well, I've now made it more forgiving in this respect. Thank you!

Paul, I believe I misunderstood you at first; the change does not check for POSIX equivalence classes or collation elements, on the grounds that these are not accepted syntax in Emacs regexps anyway. If you think it really is worth the trouble to add the analysis then say so, but there is a smell of gold-plating (mixing metaphors) here.

Re: :alnum: broken?

[Eli Zaretskii]

On February 26, 2020 10:01:24 PM GMT, "Mattias Engdegård" <mattiase@acm.org> wrote:
> 26 feb. 2020 kl. 22.24 skrev Clément Pit-Claudel
> <cpitclaudel@gmail.com>:
> 
> > I think your patch disallows thing like [:-:], which grep allows.
> 
> Very well, I've now made it more forgiving in this respect. Thank you!
> 
> Paul, I believe I misunderstood you at first; the change does not
> check for POSIX equivalence classes or collation elements, on the
> grounds that these are not accepted syntax in Emacs regexps anyway. If
> you think it really is worth the trouble to add the analysis then say
> so, but there is a smell of gold-plating (mixing metaphors) here.

Please revert these changes.  I already said that I wasn't interested in making these regular expressions signal an error.

 If we want to flag these as potential mistakes, let's make the byte compiler warn about them, as Richard proposed.  But I'm against making this a run-time error.

Thanks.

Re: :alnum: broken?

[Paul Eggert]

On 2/26/20 2:01 PM, Mattias Engdegård wrote:
> the change does not check for POSIX equivalence classes or collation elements, on the grounds that these are not accepted syntax in Emacs regexps anyway.

Sounds good. Thanks for the patches; they improve Emacs overall. 
Unfortunately I doubt whether Eli will accept them.

Re: :alnum: broken?

[Andreas Schwab]

On Feb 26 2020, Mattias Engdegård wrote:

> 26 feb. 2020 kl. 17.01 skrev Andreas Schwab <schwab@suse.de>:
>
>> That would break "[:[:alpha:]]".
>
> No, it seems all right.

Right, I missed the break.  But I think this is too strict, it should
not reject "[:a-z$%&/()=:]'

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: :alnum: broken?

[Mattias Engdegård]

26 feb. 2020 kl. 23.38 skrev Eli Zaretskii <eliz@gnu.org>:

> Please revert these changes.  I already said that I wasn't interested in making these regular expressions signal an error.

Sorry Eli, I didn't realise it was a belief strongly held. The changes have been reverted, of course.

But perhaps you will let me attempt to sway your opinion? I was a bit lukewarm to the idea myself, but the irony was not lost on me after making this very mistake in the implementation of code designed to find regexp errors. In short, the check saves time for beginners and experienced users alike, with no downside worth speaking about at all.

There is no way a byte-compiler warning could come close to the precision of a run-time check, and I speak with some modest experience on the subject. A compiler warning wouldn't have found my error, nor would it find common non-code use such as interactive search.

Initially I was worried about someone's regexp-composing code falling victim of a more stringent check, but Paul convinced me that this is unlikely to be an actual concern. Besides, we do break absolute compatibility now and then for good reasons, and this is one. There is also GNU grep as a precedence.

Re: :alnum: broken?

[Mattias Engdegård]

27 feb. 2020 kl. 09.43 skrev Andreas Schwab <schwab@suse.de>:

> Right, I missed the break.  But I think this is too strict, it should
> not reject "[:a-z$%&/()=:]'

Thank you, and with the later committed addendum that example is accepted as well. The code still rejects "[:az$%&/()=:]" (no hyphen) but so does GNU grep. Perhaps we could restrict the filter to letters between the colons; I'm open for suggestions.

The point is moot now, of course, as the changes have been reverted altogether.

Re: :alnum: broken?

[Óscar Fuentes]

Mattias Engdegård <mattiase@acm.org> writes:

> 26 feb. 2020 kl. 23.38 skrev Eli Zaretskii <eliz@gnu.org>:
>
>> Please revert these changes.  I already said that I wasn't interested in making these regular expressions signal an error.
>
> Sorry Eli, I didn't realise it was a belief strongly held. The changes have been reverted, of course.
>
> But perhaps you will let me attempt to sway your opinion? I was a bit
> lukewarm to the idea myself, but the irony was not lost on me after
> making this very mistake in the implementation of code designed to
> find regexp errors. In short, the check saves time for beginners and
> experienced users alike, with no downside worth speaking about at all.
>
> There is no way a byte-compiler warning could come close to the
> precision of a run-time check, and I speak with some modest experience
> on the subject. A compiler warning wouldn't have found my error, nor
> would it find common non-code use such as interactive search.
>
> Initially I was worried about someone's regexp-composing code falling
> victim of a more stringent check, but Paul convinced me that this is
> unlikely to be an actual concern. Besides, we do break absolute
> compatibility now and then for good reasons, and this is one. There is
> also GNU grep as a precedence.

Okay, I'll be the one who says it: make it optional, disabled as
default.

This will nullify any concern about backwards compatibility and make it
possible to test the feature on the field. Plus hopefully being useful
for some of those that know about its existence.

Re: :alnum: broken?

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Thu, 27 Feb 2020 18:57:50 +0100
> Cc: emacs-devel@gnu.org,
>         Clément Pit-Claudel <cpitclaudel@gmail.com>,
>         Paul Eggert <eggert@cs.ucla.edu>
> 
> > Please revert these changes.  I already said that I wasn't interested in making these regular expressions signal an error.
> 
> Sorry Eli, I didn't realise it was a belief strongly held. The changes have been reverted, of course.

Thank you.

> But perhaps you will let me attempt to sway your opinion? I was a bit lukewarm to the idea myself, but the irony was not lost on me after making this very mistake in the implementation of code designed to find regexp errors. In short, the check saves time for beginners and experienced users alike, with no downside worth speaking about at all.
> 
> There is no way a byte-compiler warning could come close to the precision of a run-time check, and I speak with some modest experience on the subject. A compiler warning wouldn't have found my error, nor would it find common non-code use such as interactive search.
> 
> Initially I was worried about someone's regexp-composing code falling victim of a more stringent check, but Paul convinced me that this is unlikely to be an actual concern. Besides, we do break absolute compatibility now and then for good reasons, and this is one. There is also GNU grep as a precedence.

I see you POV, and understand it.  Granted, years ago, when these
classes were introduced, I had my share of "forget the brackets"
mistake.

But I don't believe it is right for us to reject questionable but
valid code.  This is a job for linters and for special warning
options, used by those who want this and don't mind extra level of
noise and annoyances.  That's why doing this in the byte compiler
sounds like a good solution to me.  I can even agree to reject this at
run time under a non-default value of some special variable (a moral
equivalent of a compiler warning option).  But doing this by default
is simply wrong, IMO, the precedent of GCC and other compilers that
become noisier and noisier with each release notwithstanding.

Thanks.

Re: :alnum: broken?

[Paul Eggert]

On 2/28/20 12:09 AM, Eli Zaretskii wrote:
> I don't believe it is right for us to reject questionable but
> valid code.

That begs the question. The code is valid only if we continue to insist that it 
be valid, despite the clear drawbacks of doing so. Instead, we can easily change 
the definition of Emacs regular expressions so that the code is invalid. Since 
such code is invariably a mistake, it's a win to make such a change. That's what 
GNU grep has done for many years, and it works.

> I can even agree to reject this at
> run time under a non-default value of some special variable

That would be better than nothing, but it's not very good since most people 
won't know about the variable and thus will continue to suffer from these 
errors. Better would be to make the default reject these buggy regexps, which is 
what GNU grep does (it accepts the buggy regexps only if POSIXLY_CORRECT is set).

Re: :alnum: broken?

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org, cpitclaudel@gmail.com
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 28 Feb 2020 00:48:32 -0800
> 
> On 2/28/20 12:09 AM, Eli Zaretskii wrote:
> > I don't believe it is right for us to reject questionable but
> > valid code.
> 
> That begs the question. The code is valid only if we continue to insist that it 
> be valid, despite the clear drawbacks of doing so.

I think it's valid due to regexp specification.

> Instead, we can easily change the definition of Emacs regular
> expressions so that the code is invalid. Since such code is
> invariably a mistake, it's a win to make such a change. That's what
> GNU grep has done for many years, and it works.

So what is your question?

> > I can even agree to reject this at
> > run time under a non-default value of some special variable
> 
> That would be better than nothing, but it's not very good since most people 
> won't know about the variable and thus will continue to suffer from these 
> errors. Better would be to make the default reject these buggy regexps, which is 
> what GNU grep does (it accepts the buggy regexps only if POSIXLY_CORRECT is set).

We disagree (as has been established already).

Re: :alnum: broken?

[Paul Eggert]

On 2/28/20 5:11 AM, Eli Zaretskii wrote:
> it's valid due to regexp specification.

I volunteer to make the minor changes to the Emacs regexp documentation 
so that these buggy regexps no longer valid. That will fix the problem 
of the revised code not matching the documentation, and then we can 
reinstall the patch.

I also volunteer to write the NEWS entry.

Re: :alnum: broken?

[Eli Zaretskii]

> Cc: mattiase@acm.org, emacs-devel@gnu.org, cpitclaudel@gmail.com
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 28 Feb 2020 09:41:22 -0800
> 
> On 2/28/20 5:11 AM, Eli Zaretskii wrote:
> > it's valid due to regexp specification.
> 
> I volunteer to make the minor changes to the Emacs regexp documentation 
> so that these buggy regexps no longer valid.

The regexp specification is not an Emacs-only feature, and I don't
think we should invent a variant of regexp spec where these particular
regexps are disallowed.  It sounds like a step in the wrong direction,
since there are already too many variants of regular expressions.  And
the particular reason for which you propose this change sounds
backwards to me.

So thanks for volunteering, but I don't think we should do this.

Re: :alnum: broken?

[Paul Eggert]

On 2/28/20 12:09 PM, Eli Zaretskii wrote:
> The regexp specification is not an Emacs-only feature, and I don't
> think we should invent a variant of regexp spec where these particular
> regexps are disallowed.

It would not be an invention of Emacs. It is a variant used in Gnu grep 
(and GNU grep surely does more regexp processing than Emacs does, if we 
look at all the world's computation), and it works fine there. So even 
if we took a strict stance against invention in Emacs regexps (a stance 
that we haven't taken in the past), that stance would not preclude the 
proposed change.

> the particular reason for which you propose this change sounds
> backwards to me.

The goal of this change is to improve the reliability of Elisp code, by 
having Emacs reject invariably-incorrect regexps. It's not "backwards" 
to improve reliability.

Re: :alnum: broken?

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 28 Feb 2020 12:25:56 -0800
> Cc: mattiase@acm.org, cpitclaudel@gmail.com, emacs-devel@gnu.org
> 
> On 2/28/20 12:09 PM, Eli Zaretskii wrote:
> > The regexp specification is not an Emacs-only feature, and I don't
> > think we should invent a variant of regexp spec where these particular
> > regexps are disallowed.
> 
> It would not be an invention of Emacs. It is a variant used in Gnu grep 
> (and GNU grep surely does more regexp processing than Emacs does, if we 
> look at all the world's computation), and it works fine there. So even 
> if we took a strict stance against invention in Emacs regexps (a stance 
> that we haven't taken in the past), that stance would not preclude the 
> proposed change.
> 
> > the particular reason for which you propose this change sounds
> > backwards to me.
> 
> The goal of this change is to improve the reliability of Elisp code, by 
> having Emacs reject invariably-incorrect regexps. It's not "backwards" 
> to improve reliability.

I suggest that we agree to disagree on this.

A couple of solutions was proposed that could be regarded as
compromises, and allow us to flag these suspicious regexps in at least
some of the use cases.  I'm okay with those proposals, but not with
the radical one you described.

Re: :alnum: broken?

[Mattias Engdegård]

28 feb. 2020 kl. 21.38 skrev Eli Zaretskii <eliz@gnu.org>:

> A couple of solutions was proposed that could be regarded as
> compromises, and allow us to flag these suspicious regexps in at least
> some of the use cases.  I'm okay with those proposals, but not with
> the radical one you described.

What about adding a variable controlling the change, defaulting to the stricter semantics? Users who depend on the looser interpretation, or find that the change would cramp their style, will happily set that variable permanently and suffer no ill effects. Everyone get what they want!

Re: :alnum: broken?

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 28 Feb 2020 22:04:12 +0100
> Cc: Paul Eggert <eggert@cs.ucla.edu>, cpitclaudel@gmail.com,
>         emacs-devel@gnu.org
> 
> What about adding a variable controlling the change, defaulting to the stricter semantics?

No, let's leave the default as it is now.  Those who want such
suspicious regular expressions flagged will customize the variable to
a non-default value.  Like they do with the warning options of a
compiler.

Re: :alnum: broken?

[Mattias Engdegård]

28 feb. 2020 kl. 22.40 skrev Eli Zaretskii <eliz@gnu.org>:

> No, let's leave the default as it is now.  Those who want such
> suspicious regular expressions flagged will customize the variable to
> a non-default value.  Like they do with the warning options of a
> compiler.

In Emacs's own compiler, all warnings are on by default:

(defcustom byte-compile-warnings t
  "List of warnings that the byte-compiler should issue (t for all).
  ...

We all agree that this is as it should be, because although experienced users would know how to enable the warnings, it is those who don't that need them the most.

Re: :alnum: broken?

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 29 Feb 2020 12:43:38 +0100
> Cc: cpitclaudel@gmail.com, eggert@cs.ucla.edu, emacs-devel@gnu.org
> 
> 28 feb. 2020 kl. 22.40 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > No, let's leave the default as it is now.  Those who want such
> > suspicious regular expressions flagged will customize the variable to
> > a non-default value.  Like they do with the warning options of a
> > compiler.
> 
> In Emacs's own compiler, all warnings are on by default:

Because they are always spot-on.

Re: :alnum: broken?

[Stefan Monnier]

> In Emacs's own compiler, all warnings are on by default:
[...]
> We all agree that this is as it should be, because although experienced
> users would know how to enable the warnings, it is those who don't that need
> them the most.

Actually, from where I stand the warnings have two purposes:

1- Communicate with our users.
2- Help catch errors.

And from that point of view, the first is the main reason why they're
enabled by default.


        Stefan

Re: :alnum: broken?

[Stefan Monnier]

> Because they are always spot-on.

As confirmed by `grep 'with-\(no\|suppressed\)-warnings' **/*.el`  ;-)


        Stefan

Re: :alnum: broken?

[Óscar Fuentes]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> In Emacs's own compiler, all warnings are on by default:
> [...]
>> We all agree that this is as it should be, because although experienced
>> users would know how to enable the warnings, it is those who don't that need
>> them the most.
>
> Actually, from where I stand the warnings have two purposes:
>
> 1- Communicate with our users.
> 2- Help catch errors.
>
> And from that point of view, the first is the main reason why they're
> enabled by default.

byte-compiler warnings is a terrible method for communicating with users.

Re: :alnum: broken?

[Stefan Monnier]

> byte-compiler warnings is a terrible method for communicating with users.

I'd love to use better ones.


        Stefan

Re: :alnum: broken?

[Óscar Fuentes]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> byte-compiler warnings is a terrible method for communicating with users.
>
> I'd love to use better ones.

Don't assume that the user compiles his .el files, for starts, implement
the warnings on the interpreter too. Show a prompt or prominent notice
about the presence of the warnings (similar to what is shown when the
init files has errors). Show a special buffer with those warnings (and
only those) which are targeted to the user. Ignore files which are
likely authored by package writers (a good hint consists on containing a
`provide' or living on certain directories). Add detailed information
about how to fix the warning, assuming that the user is not proficient
with Elisp.

Re: :alnum: broken?

[Marcin Borkowski]

On 2020-02-29, at 22:12, Óscar Fuentes <ofv@wanadoo.es> wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> byte-compiler warnings is a terrible method for communicating with users.
>>
>> I'd love to use better ones.
>
> Don't assume that the user compiles his .el files, for starts, implement
> the warnings on the interpreter too. Show a prompt or prominent notice
> about the presence of the warnings (similar to what is shown when the
> init files has errors). Show a special buffer with those warnings (and
> only those) which are targeted to the user. Ignore files which are
> likely authored by package writers (a good hint consists on containing a
> `provide' or living on certain directories). Add detailed information
> about how to fix the warning, assuming that the user is not proficient
> with Elisp.

+1.  I consider myself fairly proficient in Elisp; I authored quitee
a few packages, some of them distributed on Github, some of them for
private use for a few people.  I admit that even that I know about
compiler warnings, I never got into a habit of compiling my files.  (I
know I should, but consider me as a datapoint suggesting that compiler
warnings are not enough.)

My 2 cents.

-- 
Marcin Borkowski
http://mbork.pl

Re: :alnum: broken?

[Clément Pit-Claudel]

On 2020-02-29 17:22, Marcin Borkowski wrote:
> +1.  I consider myself fairly proficient in Elisp; I authored quitee
> a few packages, some of them distributed on Github, some of them for
> private use for a few people.  I admit that even that I know about
> compiler warnings, I never got into a habit of compiling my files.  (I
> know I should, but consider me as a datapoint suggesting that compiler
> warnings are not enough.)

Did you consider flycheck or flymake?  Both will compile your files in the background and report errors.

Re: :alnum: broken?

[Stefan Monnier]

> Don't assume that the user compiles his .el files,

I don't.  I only know that some do, and that compiler warnings reach at
least these people.

> for starts, implement the warnings on the interpreter too.

The interpreter is often not a convenient place since it can't pin point
the origin of the code, because adding checks there slows down
execution, because in many cases the interpreter doesn't have enough
info to perform the needed checks efficiently (e.g. it'd be hard/costly
to figure out if a variable is unused), because such warnings will tend
to be repeated many many times since the code is likely to be executed
many many times, ... (and because it won't catch problems in code that's
been compiled ;-).

> Show a prompt or prominent notice about the presence of the warnings
> (similar to what is shown when the init files has errors).

When/where?  My best hope for non-compiling authors is that they use
tools like flymake which let them see the byte-compiler's warnings even
when the user doesn't compile the file.

> Ignore files which are likely authored by package writers (a good hint
> consists on containing a `provide' or living on certain
> directories).

On the contrary, these are the people I mostly care to reach.


        Stefan

Re: :alnum: broken?

[Andrea Corallo]

Marcin Borkowski <mbork@mbork.pl> writes:

> +1.  I consider myself fairly proficient in Elisp; I authored quitee
> a few packages, some of them distributed on Github, some of them for
> private use for a few people.  I admit that even that I know about
> compiler warnings, I never got into a habit of compiling my files.  (I
> know I should, but consider me as a datapoint suggesting that compiler
> warnings are not enough.)
>
> My 2 cents.

My experience is quite the opposite.  I can't imagine my self writing
large non trivial Elisp and going for a first run without double
checking against the byte-compiler output.

I agree in that respect that, given the cheap cpu time involved in the
byte-compilation process, we should move towards using this as default.

  Andrea

--
akrl@sdf.org

Re: :alnum: broken?

[Óscar Fuentes]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Ignore files which are likely authored by package writers (a good hint
>> consists on containing a `provide' or living on certain
>> directories).
>
> On the contrary, these are the people I mostly care to reach.

Well, for me users is a vastly larger and heterogeneous group than
package writers. IMHO those who author and distribute code should look
at compiler warnings, so what I wrote previously was the result of
misunderstanding your "communicating with our users."

Re: :alnum: broken?

[Marcin Borkowski]

On 2020-03-01, at 00:02, Andrea Corallo <akrl@sdf.org> wrote:

> Marcin Borkowski <mbork@mbork.pl> writes:
>
>> +1.  I consider myself fairly proficient in Elisp; I authored quitee
>> a few packages, some of them distributed on Github, some of them for
>> private use for a few people.  I admit that even that I know about
>> compiler warnings, I never got into a habit of compiling my files.  (I
>> know I should, but consider me as a datapoint suggesting that compiler
>> warnings are not enough.)
>>
>> My 2 cents.
>
> My experience is quite the opposite.  I can't imagine my self writing
> large non trivial Elisp and going for a first run without double
> checking against the byte-compiler output.
>
> I agree in that respect that, given the cheap cpu time involved in the
> byte-compilation process, we should move towards using this as default.

Well, this probably means I'll have to change my habit, and the sooner
the better.

By the way, is the advice about using the byte-compiler (even if only
for the warnings) anywhere in the Elisp intro and/or the Elisp
reference?  Because I think it should be there, maybe even in both
places.

Best,

--
Marcin Borkowski
http://mbork.pl

Re: :alnum: broken?

[Marcin Borkowski]

On 2020-02-29, at 23:34, Clément Pit-Claudel <cpitclaudel@gmail.com> wrote:

> On 2020-02-29 17:22, Marcin Borkowski wrote:
>> +1.  I consider myself fairly proficient in Elisp; I authored quitee
>> a few packages, some of them distributed on Github, some of them for
>> private use for a few people.  I admit that even that I know about
>> compiler warnings, I never got into a habit of compiling my files.  (I
>> know I should, but consider me as a datapoint suggesting that compiler
>> warnings are not enough.)
>
> Did you consider flycheck or flymake?  Both will compile your files in the background and report errors.

Interesting.  I didn't know that.  I tried enablng flycheck on ne of my
larger Elisp files.  It gave me one error in a apparently totally
correct (and working!) line (no idea why) and a lot of warnings about my
docstrings.  Still, this is a valuable advice, thanks!

Best,

--
Marcin Borkowski
http://mbork.pl

Re: :alnum: broken?

[Stefan Monnier]

> Interesting.  I didn't know that.  I tried enablng flycheck on one of my
> larger Elisp files.  It gave me one error in a apparently totally
> correct (and working!)

Most of the warnings don't claim that the code is likely wrong, but
their aim is rather to make your code more reliable and future-proof.


        Stefan

Re: :alnum: broken?

[Marcin Borkowski]

On 2020-03-02, at 04:07, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>> Interesting.  I didn't know that.  I tried enablng flycheck on one of my
>> larger Elisp files.  It gave me one error in a apparently totally
>> correct (and working!)
>
> Most of the warnings don't claim that the code is likely wrong, but
> their aim is rather to make your code more reliable and future-proof.

That makes me cringe.  If I use flycheck, I want my files to be 100%
warning-free.  What should I do with the line

(require 'request)

then, when it gives the error (not even warning!):

"Cannot open load file: No such file or directory, request (emacs-lisp)"?

(Of course, I have the `request' package installed.)

TIA,

-- 
Marcin Borkowski
http://mbork.pl

Re: :alnum: broken?

[Emanuel Berg via Emacs development discussions.]

Marcin Borkowski wrote:

> [...] I want my files to be 100%
> warning-free.

Yeah, optimally one would!

Here is what I do when `require' and `defvar'
don't it - well, I do it all the time! But
that's when it works, perhaps...

In the Makefile, put

  $(MAKE) 2>&1 > /dev/null; $(MAKE)

or in the shell

  $ \make > /dev/null; \make

(right, it seems there's no output to stderr to
care about anyway. maybe errors in
make itself?)

Anyway compile twice and even more warnings go
away :)

-- 
underground experts united
http://user.it.uu.se/~embe8573
https://dataswamp.org/~incal

Re: :alnum: broken?

[Joost Kremers]

On Mon, Mar 02 2020, Marcin Borkowski wrote:
> That makes me cringe.  If I use flycheck, I want my files to be 
> 100%
> warning-free.  What should I do with the line
>
> (require 'request)
>
> then, when it gives the error (not even warning!):
>
> "Cannot open load file: No such file or directory, request 
> (emacs-lisp)"?
>
> (Of course, I have the `request' package installed.)

Yeah, that's been an issue with flycheck that I've had since I 
first started using it. `require` statements for packages 
installed through Melpa or for local files (even in the same 
directory) are always flagged. I've been meaning to ask the 
flycheck devs about this for a long time but I never got round to 
it.

-- 
Joost Kremers
Life has its moments

Re: :alnum: broken?

[Štěpán Němec]

On Mon, 02 Mar 2020 08:56:08 +0100
Joost Kremers wrote:

> On Mon, Mar 02 2020, Marcin Borkowski wrote:
>> That makes me cringe.  If I use flycheck, I want my files to be 100%
>> warning-free.  What should I do with the line
>>
>> (require 'request)
>>
>> then, when it gives the error (not even warning!):
>>
>> "Cannot open load file: No such file or directory, request
>> (emacs-lisp)"?
>>
>> (Of course, I have the `request' package installed.)
>
> Yeah, that's been an issue with flycheck that I've had since I first
> started using it. `require` statements for packages installed through
> Melpa or for local files (even in the same directory) are always
> flagged. I've been meaning to ask the flycheck devs about this for a
> long time but I never got round to it.

You might be interested in the variable 'flycheck-emacs-lisp-load-path'.

-- 
Štěpán

Re: :alnum: broken?

[Joost Kremers]

On Mon, Mar 02 2020, Štěpán Němec wrote:
> On Mon, 02 Mar 2020 08:56:08 +0100
> Joost Kremers wrote:
>> Yeah, that's been an issue with flycheck that I've had since I 
>> first
>> started using it. `require` statements for packages installed 
>> through
>> Melpa or for local files (even in the same directory) are 
>> always
>> flagged. I've been meaning to ask the flycheck devs about this 
>> for a
>> long time but I never got round to it.
>
> You might be interested in the variable 
> 'flycheck-emacs-lisp-load-path'.

I might indeed...

Thanks!

-- 
Joost Kremers
Life has its moments

Re: :alnum: broken?

[Clément Pit-Claudel]

On 2020-03-02 02:15, Marcin Borkowski wrote:
> That makes me cringe.  If I use flycheck, I want my files to be 100%
> warning-free.  What should I do with the line
> 
> (require 'request)
> 
> then, when it gives the error [...]

Same as you would do with the byte-compiler: tell it where to find packages.

See flycheck-emacs-lisp-initialize-packages:

flycheck-emacs-lisp-initialize-packages is a variable defined in ‘flycheck.el’.
Its value is ‘auto’

Documentation:
Whether to initialize packages in the Emacs Lisp syntax checker.

When nil, never initialize packages.  When ‘auto’, initialize
packages only when checking ‘user-init-file’ or files from
‘user-emacs-directory’.  For any other non-nil value, always
initialize packages.

When initializing packages is enabled the ‘emacs-lisp’ syntax
checker calls ‘package-initialize’ before byte-compiling the file
to be checked.  It also sets ‘package-user-dir’ according to
‘flycheck-emacs-lisp-package-user-dir’.

This variable is an option for the following syntax checkers:

  - ‘emacs-lisp’

RE: :alnum: broken?

[Drew Adams]

> > [...] I want my files to be 100%
> > warning-free.
> 
> Yeah, optimally one would!

No, not necessarily.  Not if a file is
intended to support multiple releases
and that means having conditional code
that can use "obsolete" constructs in
some branches, for the benefit of an
older release.

Re: :alnum: broken?

[Emanuel Berg via Emacs development discussions.]

Drew Adams wrote:

>>> [...] I want my files to be 100%
>>> warning-free.
>> 
>> Yeah, optimally one would!
>
> No, not necessarily. Not if a file is
> intended to support multiple releases and
> that means having conditional code that can
> use "obsolete" constructs in some branches,
> for the benefit of an older release.

That's right, because then, you WANT warnings!

Ööö

-- 
underground experts united
http://user.it.uu.se/~embe8573
https://dataswamp.org/~incal

Re: :alnum: broken?

[Stefan Monnier]

>> Most of the warnings don't claim that the code is likely wrong, but
>> their aim is rather to make your code more reliable and future-proof.
> That makes me cringe.  If I use flycheck, I want my files to be 100%
> warning-free.

That's indeed what we're aiming for, which is why we provide
`with-suppressed-warnings`.

> What should I do with the line
> (require 'request)
> then, when it gives the error (not even warning!):
> "Cannot open load file: No such file or directory, request (emacs-lisp)"?

That's "unrelated": it's an error, not a warning.  In general it
indicates either an error in your code or an error in the way the
compiler is called, or an error in the compiler.  In this case it seems
to be a problem linked to flycheck.  Have you tried `flymake` instead?
Does it suffer from the same problem?


        Stefan

Re: :alnum: broken?

[Clément Pit-Claudel]

On 2020-03-02 12:03, Stefan Monnier wrote:
>>> Most of the warnings don't claim that the code is likely wrong, but
>>> their aim is rather to make your code more reliable and future-proof.
>> That makes me cringe.  If I use flycheck, I want my files to be 100%
>> warning-free.
> 
> That's indeed what we're aiming for, which is why we provide
> `with-suppressed-warnings`.
> 
>> What should I do with the line
>> (require 'request)
>> then, when it gives the error (not even warning!):
>> "Cannot open load file: No such file or directory, request (emacs-lisp)"?
> 
> That's "unrelated": it's an error, not a warning.  In general it
> indicates either an error in your code or an error in the way the
> compiler is called, or an error in the compiler.  In this case it seems
> to be a problem linked to flycheck.

Indeed, it's a problem in the way the compiler is called.

As I said, by default, the compiler is called in a clean environment (without loading your local packages), except when compiling your configuration files.  We could change that default, of course, but it's not a mistake or an oversight that it currently behaves the way it does.