bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b

bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b

[Harald Jörg]

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

How to reproduce:
In a buffer, activate cperl-mode (M-x cperl-mode), then enter:

$a++ / $b;

This is supposed to be a division.  However, cperl-mode interprets the
slash as the start of a regular expression.  Accordingly, the following
text is displayed with the wrong face, and also cperl-mode might
complain that it can't find the end of the regular expression.

Emacs does not crash.

Attached: A patch which fixes the issue for $a++ and $a--, and also
includes a test to verify that the very similar text "$a+ / $b" is still
correctly interpreted as a regular expression.

Disclaimer: I'm rather new to elisp and even newer to ERT.

Cheers,
haj


[-- Attachment #2: postincrement.diff --]
[-- Type: text/x-patch, Size: 3201 bytes --]

diff --git a/lisp/progmodes/cperl-mode.el b/lisp/progmodes/cperl-mode.el
index cdbb59a5ad..2ba6921643 100644
--- a/lisp/progmodes/cperl-mode.el
+++ b/lisp/progmodes/cperl-mode.el
@@ -3978,6 +3978,9 @@ the sections using `cperl-pod-head-face', `cperl-pod-face',
 				    (and (eq (preceding-char) ?.)
 					 (eq (char-after (- (point) 2)) ?.))
 				    (bobp))
+				;; { $a++ / $b } doesn't start a regex, nor does $a--
+				(not (and (memq (preceding-char) '(?+ ?-))
+					  (eq (preceding-char) (char-before (1- (point))))))
 				;;  m|blah| ? foo : bar;
 				(not
 				 (and (eq c ?\?)
diff --git a/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el b/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el
new file mode 100644
index 0000000000..4148db036c
--- /dev/null
+++ b/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el
@@ -0,0 +1,55 @@
+;;; cperl-fontification-tests.el --- Test fontification in cperl-mode -*- lexical-binding: t -*-
+
+;; Copyright (C) 2020-2020 ...to be decided ...
+
+;; Author: Harald Jörg <haj@posteo.de>
+;; Maintainer: Harald Jörg
+;; Keywords:       internal
+;; Human-Keywords: internal
+;; Homepage: https://github.com/HaraldJoerg/cperl-mode
+
+;;; Commentary:
+
+;; This is a collection of Tests for the fontification of CPerl-mode.
+;; The primary target is to verify that the refactoring we're doing
+;; right now (May 2020 - ...) doesn't change any behavior, or does the
+;; right thing in cases where new fontification rules are enabled.
+
+;; Run these tests interactively:
+;; (ert-run-tests-interactively '(tag :fontification))
+
+(defun cperl-test-face (text regexp)
+  "Returns the face of the first character matched by REGEXP in TEXT."
+  (interactive)
+  (with-temp-buffer
+    (let ((cperl-hairy nil)
+	  (cperl-font-lock nil)) ;; Does this matter?
+      (insert text)
+      (cperl-mode)
+      (font-lock-fontify-buffer)
+      (goto-char (point-min))
+      (re-search-forward regexp)
+      (message "%s" (match-string 0))
+      (get-text-property (match-beginning 0) 'face))))
+
+(ert-deftest jrockway-issue-45 ()
+  "Verify that '/' is a division after ++ or --, not a regexp.
+Reported in https://github.com/jrockway/cperl-mode/issues/45.
+If seen as regular expression, then the slash is displayed using
+font-lock-constant-face.  If seen as a division, then it doesn't
+have a face property."
+  :tags '(:fontification)
+  ;; The next two Perl expressions have divisions.  Perl "punctuation"
+  ;; operators don't get a face.  The comment at the end of line
+  ;; prevents cperl-mode from tripping over "End of ‘/ ... /’
+  ;; string/RE not found" if it gets it wrong
+  (let ((code "{ $a++ / $b } # /"))
+    (should (equal (cperl-test-face code "/" ) nil)))
+  (let ((code "{ $a-- / $b } # /"))
+    (should (equal (cperl-test-face code "/" ) nil)))
+  ;; The next two Perl expressions have regular expressions.  The
+  ;; delimiter of a RE is fontified with font-lock-constant-face.
+  (let ((code "{ $a+ / $b } # /"))
+    (should (equal (cperl-test-face code "/" ) font-lock-constant-face)))
+  (let ((code "{ $a- / $b } # /"))
+    (should (equal (cperl-test-face code "/" ) font-lock-constant-face))))

bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b

[Eli Zaretskii]

> From: Harald Jörg <haj@posteo.de>
> Date: Thu, 2 Jul 2020 20:19:21 +0200
> 
> How to reproduce:
> In a buffer, activate cperl-mode (M-x cperl-mode), then enter:
> 
> $a++ / $b;
> 
> This is supposed to be a division.  However, cperl-mode interprets the
> slash as the start of a regular expression.  Accordingly, the following
> text is displayed with the wrong face, and also cperl-mode might
> complain that it can't find the end of the regular expression.
> 
> Emacs does not crash.
> 
> Attached: A patch which fixes the issue for $a++ and $a--, and also
> includes a test to verify that the very similar text "$a+ / $b" is still
> correctly interpreted as a regular expression.

Thanks.

To accept a substantial contribution such as this one, we will need
you to assign the copyright of your code to the FSF.  If you are okay
with starting your legal paperwork, I will send you the form to sign
and email.

Thank you for your interest in Emacs.

bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Harald Jörg <haj@posteo.de>
>> Date: Thu, 2 Jul 2020 20:19:21 +0200
>>
>> How to reproduce:
>> In a buffer, activate cperl-mode (M-x cperl-mode), then enter:
>>
>> $a++ / $b;
>>
>> This is supposed to be a division.  However, cperl-mode interprets the
>> slash as the start of a regular expression.  Accordingly, the following
>> text is displayed with the wrong face, and also cperl-mode might
>> complain that it can't find the end of the regular expression.
>>
>> Emacs does not crash.
>>
>> Attached: A patch which fixes the issue for $a++ and $a--, and also
>> includes a test to verify that the very similar text "$a+ / $b" is still
>> correctly interpreted as a regular expression.
>
> Thanks.
>
> To accept a substantial contribution such as this one, we will need
> you to assign the copyright of your code to the FSF.  If you are okay
> with starting your legal paperwork, I will send you the form to sign
> and email.
>
> Thank you for your interest in Emacs.

Harald, would you be willing to sign the copyright papers?  Please find
out more about the reason for this here:

    https://www.gnu.org/licenses/why-assign.html

Best regards,
Stefan Kangas

bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b

[Harald Jörg]

On 8/13/20 2:40 AM, Stefan Kangas wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>>> From: Harald Jörg <haj@posteo.de>
>>> [...]
>> Thanks.
>>
>> To accept a substantial contribution such as this one, we will need
>> you to assign the copyright of your code to the FSF.  If you are okay
>> with starting your legal paperwork, I will send you the form to sign
>> and email.
>>
>> Thank you for your interest in Emacs.
> 
> Harald, would you be willing to sign the copyright papers?  Please find
> out more about the reason for this here:
> 
>     https://www.gnu.org/licenses/why-assign.html

I forgot to inform anyone, but this process has already been completed!
According to Lars Ingebrigtsen my name is already on "the list", so the
legal stuff should be out of the way.
-- 
Cheers,
haj

bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b

[Stefan Kangas]

Harald Jörg <haj@posteo.de> writes:

> I forgot to inform anyone, but this process has already been completed!
> According to Lars Ingebrigtsen my name is already on "the list", so the
> legal stuff should be out of the way.

Excellent, welcome to Emacs!  I will review your patch now.

Best regards,
Stefan Kangas

bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b

[Stefan Kangas]

Hi Harald,

Please see my comments on your patch below.

Harald Jörg <haj@posteo.de> writes:

> diff --git a/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el b/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el
> new file mode 100644
> index 0000000000..4148db036c
> --- /dev/null
> +++ b/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el

Great initiative to also write tests for this bug.

For now, I think you should simply place the file directly at:

lisp/progmodes/cperl-mode-tests.el

We could always break it up into several files later when it gets bigger
and there is a need to do so.

> @@ -0,0 +1,55 @@
> +;;; cperl-fontification-tests.el --- Test fontification in cperl-mode -*- lexical-binding: t -*-
> +
> +;; Copyright (C) 2020-2020 ...to be decided ...

All files in GNU Emacs should be following this template:

;; Copyright (C) 2020 Free Software Foundation, Inc.

(Note that you only need a range if there's more than one year.)

> +;; Author: Harald Jörg <haj@posteo.de>
> +;; Maintainer: Harald Jörg
> +;; Keywords:       internal
> +;; Human-Keywords: internal

Nit: I think we can skip Human-Keywords here.

> +;; Homepage: https://github.com/HaraldJoerg/cperl-mode
> +
> +;;; Commentary:
> +
> +;; This is a collection of Tests for the fontification of CPerl-mode.

Typo: "tests" shouldn't have a capital letter.

> +;; The primary target is to verify that the refactoring we're doing
> +;; right now (May 2020 - ...) doesn't change any behavior, or does the
> +;; right thing in cases where new fontification rules are enabled.

I think these three lines here could be removed, maybe?  It seems to me
that they are describing the purpose of all tests, namely to stop
regressions from happening.

> +;; Run these tests interactively:
> +;; (ert-run-tests-interactively '(tag :fontification))

Nit: Missing this header here:

;;; Code:

> +
> +(defun cperl-test-face (text regexp)
> +  "Returns the face of the first character matched by REGEXP in TEXT."
> +  (interactive)
> +  (with-temp-buffer
> +    (let ((cperl-hairy nil)
> +	  (cperl-font-lock nil)) ;; Does this matter?

Does it matter?  Not sure, what happens if you remove it?  :-)

> +      (insert text)
> +      (cperl-mode)
> +      (font-lock-fontify-buffer)
> +      (goto-char (point-min))
> +      (re-search-forward regexp)
> +      (message "%s" (match-string 0))
> +      (get-text-property (match-beginning 0) 'face))))

It is good practice to remove calls to 'message' in the tests, since
they mostly make the tests more noisy.  If it's really useful during
development of tests, you could comment it out or make it dependent upon
a new variable like

> +(ert-deftest jrockway-issue-45 ()

Is probably better named as: cperl-mode-test-bug-42168 to refer back to
our own bug in the name.  We already have the link to jrockway in the
doc string, which is useful if one needs to dig deeper.

> +  "Verify that '/' is a division after ++ or --, not a regexp.
> +Reported in https://github.com/jrockway/cperl-mode/issues/45.
> +If seen as regular expression, then the slash is displayed using
> +font-lock-constant-face.  If seen as a division, then it doesn't
> +have a face property."
> +  :tags '(:fontification)
> +  ;; The next two Perl expressions have divisions.  Perl "punctuation"
> +  ;; operators don't get a face.  The comment at the end of line
> +  ;; prevents cperl-mode from tripping over "End of ‘/ ... /’
> +  ;; string/RE not found" if it gets it wrong

Do we need the comment at the end of the below code fragments with your
patch as well?  If not, couldn't we just remove them?  I think that
would make the intention a little bit clearer, maybe.

> +  (let ((code "{ $a++ / $b } # /"))
> +    (should (equal (cperl-test-face code "/" ) nil)))
> +  (let ((code "{ $a-- / $b } # /"))
> +    (should (equal (cperl-test-face code "/" ) nil)))
> +  ;; The next two Perl expressions have regular expressions.  The
> +  ;; delimiter of a RE is fontified with font-lock-constant-face.
> +  (let ((code "{ $a+ / $b } # /"))
> +    (should (equal (cperl-test-face code "/" ) font-lock-constant-face)))
> +  (let ((code "{ $a- / $b } # /"))
> +    (should (equal (cperl-test-face code "/" ) font-lock-constant-face))))

The rest looks good to me.

Best regards,
Stefan Kangas

bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b

[Harald Jörg]

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

Hello Stefan,

Thank you for your thorough review.  I'm rather new to Emacs Lisp, and
also to the Emacs development workflows, so this is highly
appreciated.

Here I've attached a new version of the patch.
 - Copyright notice is now as it should be (now that the paperwork
   permits it)
 - All the stuff you suggested to be removed has been removed
 - The test has been renamed as suggested

In one place I've changed your suggestion:

> For now, I think you should simply place the file directly at:
>
> lisp/progmodes/cperl-mode-tests.el

I guess this was meant to read test/lisp/progmodes/cperl-mode-tests.el.

Also, I've added a ChangeLog-style attachment, as suggested by Lars
Ingebrigtsen in his comment to Bug#42355.
--
Cheers,
haj

[-- Attachment #2: postincrement_1.diff --]
[-- Type: text/x-patch, Size: 2663 bytes --]

diff --git a/lisp/progmodes/cperl-mode.el b/lisp/progmodes/cperl-mode.el
index 6122caf518..2d2713a36a 100644
--- a/lisp/progmodes/cperl-mode.el
+++ b/lisp/progmodes/cperl-mode.el
@@ -3979,6 +3979,9 @@ cperl-find-pods-heres
 				    (and (eq (preceding-char) ?.)
 					 (eq (char-after (- (point) 2)) ?.))
 				    (bobp))
+				;; { $a++ / $b } doesn't start a regex, nor does $a--
+				(not (and (memq (preceding-char) '(?+ ?-))
+					  (eq (preceding-char) (char-before (1- (point))))))
 				;;  m|blah| ? foo : bar;
 				(not
 				 (and (eq c ?\?)
diff --git a/test/lisp/progmodes/cperl-mode-tests.el b/test/lisp/progmodes/cperl-mode-tests.el
new file mode 100644
index 0000000000..030bdbe3db
--- /dev/null
+++ b/test/lisp/progmodes/cperl-mode-tests.el
@@ -0,0 +1,48 @@
+;;; cperl-mode-tests --- Test for cperl-mode -*- lexical-binding: t -*-
+
+;; Copyright (C) 2020 Free Software Foundation, Inc.
+
+;; Author: Harald Jörg <haj@posteo.de>
+;; Maintainer: Harald Jörg
+;; Keywords:       internal
+;; Homepage: https://github.com/HaraldJoerg/cperl-mode
+
+;;; Commentary:
+
+;; This is a collection of tests for the fontification of CPerl-mode.
+
+;; Run these tests interactively:
+;; (ert-run-tests-interactively '(tag :fontification))
+
+;;; Code:
+
+(defun cperl-test-face (text regexp)
+  "Returns the face of the first character matched by REGEXP in TEXT."
+  (interactive)
+  (with-temp-buffer
+      (insert text)
+      (cperl-mode)
+      (font-lock-fontify-buffer)
+      (goto-char (point-min))
+      (re-search-forward regexp)
+      (get-text-property (match-beginning 0) 'face)))
+
+(ert-deftest cperl-mode-test-bug-42168 ()
+  "Verify that '/' is a division after ++ or --, not a regexp.
+Reported in https://github.com/jrockway/cperl-mode/issues/45.
+If seen as regular expression, then the slash is displayed using
+font-lock-constant-face.  If seen as a division, then it doesn't
+have a face property."
+  :tags '(:fontification)
+  ;; The next two Perl expressions have divisions.  Perl "punctuation"
+  ;; operators don't get a face.
+  (let ((code "{ $a++ / $b }"))
+    (should (equal (cperl-test-face code "/" ) nil)))
+  (let ((code "{ $a-- / $b }"))
+    (should (equal (cperl-test-face code "/" ) nil)))
+  ;; The next two Perl expressions have regular expressions.  The
+  ;; delimiter of a RE is fontified with font-lock-constant-face.
+  (let ((code "{ $a+ / $b } # /"))
+    (should (equal (cperl-test-face code "/" ) font-lock-constant-face)))
+  (let ((code "{ $a- / $b } # /"))
+    (should (equal (cperl-test-face code "/" ) font-lock-constant-face))))

[-- Attachment #3: ChangeLog.Bug42168 --]
[-- Type: text/plain, Size: 253 bytes --]

2020-08-13  Harald Jörg  <haj@posteo.de>

	* lisp/progmodes/cperl-mode.el (cperl-find-pods-heres): Fix (Bug#42168)
	Recognize {$a++ / $b} correctly as division.

	* test/lisp/progmodes/cperl-mode-tests.el: New test verifying the fix
	for (Bug#42168).

bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b

[Stefan Kangas]

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

close 42168 28.1
thanks

Hi Harald,

Harald Jörg <haj@posteo.de> writes:

> Thank you for your thorough review.  I'm rather new to Emacs Lisp, and
> also to the Emacs development workflows, so this is highly
> appreciated.

Happy to help, we've all been there at one point.

>> lisp/progmodes/cperl-mode-tests.el
>
> I guess this was meant to read test/lisp/progmodes/cperl-mode-tests.el.

Indeed.

> Also, I've added a ChangeLog-style attachment, as suggested by Lars
> Ingebrigtsen in his comment to Bug#42355.

Thanks.

Your diff looks good, so I've installed the attached patch with some
minor fixes, and I'm closing the bug with this message.

Next time, could you please add the ChangeLog entry to the git commit
message and send the result of `git format-patch -1` as an attachment?
That saves time when installing the change.  Please see the attached for
an example, and you can read more in CONTRIBUTE in emacs.git.  It is
also useful to consult the git log for more examples.

Welcome again as an Emacs contributor!

Best regards,
Stefan Kangas

[-- Attachment #2: 0001-cperl-mode-Highlight-a-b-correctly.patch --]
[-- Type: text/x-diff, Size: 3297 bytes --]

From f3ff51288fa0370a9ea33312b188565e4f2b595e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Harald=20J=C3=B6rg?= <haj@posteo.de>
Date: Fri, 14 Aug 2020 10:01:30 +0200
Subject: [PATCH] cperl-mode: Highlight '{$a++ / $b}' correctly

* lisp/progmodes/cperl-mode.el (cperl-find-pods-heres):
Recognize {$a++ / $b} correctly as division.  (Bug#42168)
* test/lisp/progmodes/cperl-mode-tests.el: New file with test
verifying the fix.
---
 lisp/progmodes/cperl-mode.el            |  3 ++
 test/lisp/progmodes/cperl-mode-tests.el | 50 +++++++++++++++++++++++++
 2 files changed, 53 insertions(+)
 create mode 100644 test/lisp/progmodes/cperl-mode-tests.el

diff --git a/lisp/progmodes/cperl-mode.el b/lisp/progmodes/cperl-mode.el
index 6122caf518..2d2713a36a 100644
--- a/lisp/progmodes/cperl-mode.el
+++ b/lisp/progmodes/cperl-mode.el
@@ -3979,6 +3979,9 @@ cperl-find-pods-heres
 				    (and (eq (preceding-char) ?.)
 					 (eq (char-after (- (point) 2)) ?.))
 				    (bobp))
+				;; { $a++ / $b } doesn't start a regex, nor does $a--
+				(not (and (memq (preceding-char) '(?+ ?-))
+					  (eq (preceding-char) (char-before (1- (point))))))
 				;;  m|blah| ? foo : bar;
 				(not
 				 (and (eq c ?\?)
diff --git a/test/lisp/progmodes/cperl-mode-tests.el b/test/lisp/progmodes/cperl-mode-tests.el
new file mode 100644
index 0000000000..f39f1ba658
--- /dev/null
+++ b/test/lisp/progmodes/cperl-mode-tests.el
@@ -0,0 +1,50 @@
+;;; cperl-mode-tests --- Test for cperl-mode  -*- lexical-binding: t -*-
+
+;; Copyright (C) 2020 Free Software Foundation, Inc.
+
+;; Author: Harald Jörg <haj@posteo.de>
+;; Maintainer: Harald Jörg
+;; Keywords: internal
+;; Homepage: https://github.com/HaraldJoerg/cperl-mode
+
+;;; Commentary:
+
+;; This is a collection of tests for the fontification of CPerl-mode.
+
+;; Run these tests interactively:
+;; (ert-run-tests-interactively '(tag :fontification))
+
+;;; Code:
+
+(defun cperl-test-face (text regexp)
+  "Returns the face of the first character matched by REGEXP in TEXT."
+  (interactive)
+  (with-temp-buffer
+      (insert text)
+      (cperl-mode)
+      (font-lock-ensure (point-min) (point-max))
+      (goto-char (point-min))
+      (re-search-forward regexp)
+      (get-text-property (match-beginning 0) 'face)))
+
+(ert-deftest cperl-mode-test-bug-42168 ()
+  "Verify that '/' is a division after ++ or --, not a regexp.
+Reported in https://github.com/jrockway/cperl-mode/issues/45.
+If seen as regular expression, then the slash is displayed using
+font-lock-constant-face.  If seen as a division, then it doesn't
+have a face property."
+  :tags '(:fontification)
+  ;; The next two Perl expressions have divisions.  Perl "punctuation"
+  ;; operators don't get a face.
+  (let ((code "{ $a++ / $b }"))
+    (should (equal (cperl-test-face code "/" ) nil)))
+  (let ((code "{ $a-- / $b }"))
+    (should (equal (cperl-test-face code "/" ) nil)))
+  ;; The next two Perl expressions have regular expressions.  The
+  ;; delimiter of a RE is fontified with font-lock-constant-face.
+  (let ((code "{ $a+ / $b } # /"))
+    (should (equal (cperl-test-face code "/" ) font-lock-constant-face)))
+  (let ((code "{ $a- / $b } # /"))
+    (should (equal (cperl-test-face code "/" ) font-lock-constant-face))))
+
+;;; cperl-mode-tests.el ends here
-- 
2.28.0