bug#59345: 28.2; peg.el: syntax-class PEX does not advance point

bug#59345: 28.2; peg.el: syntax-class PEX does not advance point

[Sean Devlin]

Hi folks,

In version 1.0 of peg.el, there is a PEX to match the syntax class of
the character at point. I expected it to advance the point after
recognizing the correct class, but it seems that it does not. I expected
its behavior to be similar to character classes, sets and ranges, and
all of these seem to advance the point. Instead, it seems it is more
like a guard expression.

To demonstrate this, you can evaluate the following forms:

(require 'peg)

(with-temp-buffer
  (save-excursion
    (insert " "))
  (list 
   (save-excursion
     (cons
      (peg-run (peg (syntax-class whitespace)))
      (point)))
   (save-excursion
     (cons
      (peg-run (peg " "))
      (point)))))

While both PEGs match the character at point, only the latter advances
the point.

This might just be a matter of documenting the behavior of the
syntax-class PEX, but I think it would make more sense if it behaved
like character classes, sets, ranges and literals.

Thanks!

In GNU Emacs 28.2
System Description:  macOS

Configured using:
'configure --disable-silent-rules
--enable-locallisppath=/opt/homebrew/share/emacs/site-lisp
--infodir=/opt/homebrew/Cellar/emacs/28.2/share/info/emacs
--prefix=/opt/homebrew/Cellar/emacs/28.2 --with-gnutls --without-x
--with-xml2 --without-dbus --with-modules --without-ns
--without-imagemagick --without-selinux'

Configured features:
ACL GMP GNUTLS JSON LIBXML2 MODULES NOTIFY KQUEUE PDUMPER THREADS 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
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow regexp-opt sort mail-extr emacsbug message rmc puny dired
dired-loaddefs rfc822 mml mml-sec epa derived epg rfc6068 epg-config
gnus-util rmail tool-bar rmail-loaddefs auth-source cl-seq eieio
eieio-core cl-macs eieio-loaddefs password-cache json map
text-property-search time-date subr-x seq 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
term/xterm xterm byte-opt gv bytecomp byte-compile cconv iso-transl
tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode tabulated-list replace newcomment text-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer cl-generic 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 simple abbrev obarray cl-preloaded nadvice
button loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote threads kqueue multi-tty
make-network-process emacs)

Memory information:
((conses 16 53226 5880)
(symbols 48 6605 1)
(strings 32 18480 957)
(string-bytes 1 607151)
(vectors 16 10649)
(vector-slots 8 126128 10593)
(floats 8 24 302)
(intervals 56 220 0)
(buffers 992 11))

bug#59345: 28.2; peg.el: syntax-class PEX does not advance point

[Michael Heerdegen]

Sean Devlin <spd@toadstyle.org> writes:

> In version 1.0 of peg.el, there is a PEX to match the syntax class of
> the character at point. I expected it to advance the point after
> recognizing the correct class, but it seems that it does not.

Yes, I too think this is a bug.

PEGs that match something after the char class matching part even fail,
which is definitely not correct: compare in a buffer containing "  a":

  (peg-run (peg (* [blank]) "a"))

vs.

  (peg-run (peg (* (syntax-class whitespace)) "a"))

The fist one matches up to the "a", the seconds doesn't, but should.

I CC Helmut Eller, the author of peg.el.


Michael.

bug#59345: 28.2; peg.el: syntax-class PEX does not advance point

[Helmut Eller]

On Fri, Nov 18 2022, Michael Heerdegen wrote:

> Sean Devlin <spd@toadstyle.org> writes:
>
>> In version 1.0 of peg.el, there is a PEX to match the syntax class of
>> the character at point. I expected it to advance the point after
>> recognizing the correct class, but it seems that it does not.
>
> Yes, I too think this is a bug.

Yes, looks like an oversight.

> I CC Helmut Eller, the author of peg.el.

I may be the original author (long ago), but the current maintainer is
Sefan Monnier.

Helmut

bug#59345: 28.2; peg.el: syntax-class PEX does not advance point

[Michael Heerdegen]

Helmut Eller <eller.helmut@gmail.com> writes:

> >> In version 1.0 of peg.el, there is a PEX to match the syntax class of
> >> the character at point. I expected it to advance the point after
> >> recognizing the correct class, but it seems that it does not.

> Yes, looks like an oversight.
> [...]
> I may be the original author (long ago), but the current maintainer is
> Sefan Monnier.

Ok, thanks for confirming, Helmut.  Then I CC Stefan.

Michael.

bug#59345: 28.2; peg.el: syntax-class PEX does not advance point

[Eric Abrahamsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Helmut Eller <eller.helmut@gmail.com> writes:
>
>> >> In version 1.0 of peg.el, there is a PEX to match the syntax class of
>> >> the character at point. I expected it to advance the point after
>> >> recognizing the correct class, but it seems that it does not.
>
>> Yes, looks like an oversight.
>> [...]
>> I may be the original author (long ago), but the current maintainer is
>> Sefan Monnier.
>
> Ok, thanks for confirming, Helmut.  Then I CC Stefan.

I'm finally getting around to adding this package to core, so I'll
stick myself in the Cc.

bug#59345: 28.2; peg.el: syntax-class PEX does not advance point

[Eric Abrahamsen]

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

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>> Helmut Eller <eller.helmut@gmail.com> writes:
>>
>>> >> In version 1.0 of peg.el, there is a PEX to match the syntax class of
>>> >> the character at point. I expected it to advance the point after
>>> >> recognizing the correct class, but it seems that it does not.
>>
>>> Yes, looks like an oversight.
>>> [...]
>>> I may be the original author (long ago), but the current maintainer is
>>> Sefan Monnier.
>>
>> Ok, thanks for confirming, Helmut.  Then I CC Stefan.
>
> I'm finally getting around to adding this package to core, so I'll
> stick myself in the Cc.

I guess the fix would look like the attached?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: peg_syntax_class.diff --]
[-- Type: text/x-patch, Size: 586 bytes --]

diff --git a/peg.el b/peg.el
index 0e4221eeb7..3f86a0bf19 100644
--- a/peg.el
+++ b/peg.el
@@ -588,7 +588,9 @@ of PEG expressions, implicitly combined with `and'."
 
 (cl-defmethod peg--translate ((_ (eql syntax-class)) class)
   (let ((probe (assoc class peg-syntax-classes)))
-    (cond (probe `(looking-at ,(format "\\s%c" (cadr probe))))
+    (cond (probe `(when (looking-at ,(format "\\s%c" (cadr probe)))
+                    (forward-char)
+                    t)
 	  (t (error "Invalid syntax class: %S\nMust be one of: %s" class
 		    (mapcar #'car peg-syntax-classes))))))
 

bug#59345: 28.2; peg.el: syntax-class PEX does not advance point

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> diff --git a/peg.el b/peg.el
> index 0e4221eeb7..3f86a0bf19 100644
> --- a/peg.el
> +++ b/peg.el
> @@ -588,7 +588,9 @@ of PEG expressions, implicitly combined with `and'."
>  
>  (cl-defmethod peg--translate ((_ (eql syntax-class)) class)
>    (let ((probe (assoc class peg-syntax-classes)))
> -    (cond (probe `(looking-at ,(format "\\s%c" (cadr probe))))
> +    (cond (probe `(when (looking-at ,(format "\\s%c" (cadr probe)))
> +                    (forward-char)
> +                    t)
>  	  (t (error "Invalid syntax class: %S\nMust be one of: %s" class
>  		    (mapcar #'car peg-syntax-classes))))))
>  

LGTM,


        Stefan

bug#59345: 28.2; peg.el: syntax-class PEX does not advance point

[Michael Heerdegen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> I guess the fix would look like the attached?

Looks also good to me.  And I tested and can confirm that the patch
fixes the reported issue.


Thanks,

Michael.

bug#59345: 28.2; peg.el: syntax-class PEX does not advance point

[Eric Abrahamsen]

On 11/29/22 10:46 AM, Michael Heerdegen wrote:
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> I guess the fix would look like the attached?
>
> Looks also good to me.  And I tested and can confirm that the patch
> fixes the reported issue.

Thanks for testing! I've added this to peg-tests.el for good measure.
Bumped the version number and pushed, and now closing this bug report,
thanks to Sean for reporting.

Eric