bug#22867: cperl mode highlights %d, but misses %.6f

bug#22867: cperl mode highlights %d, but misses %.6f

[積丹尼 Dan Jacobson]

cperl mode highlights %d, but misses %.6f in
printf "%.6f,%.6f,%d\n" ...

bug#22867: cperl mode highlights %d, but misses %.6f

[Alexis]

積丹尼 Dan Jacobson <jidanni@jidanni.org> writes:

> cperl mode highlights %d, but misses %.6f in printf 
> "%.6f,%.6f,%d\n" ...

Reproduced on emacs-25 branch as at commit 0e7bcec1 
(emacs-version: 25.0.92.2).

Reproduced on master branch as at commit d6ea6453 (emacs-version: 
25.1.50.3).

bug#22867: cperl mode highlights %d, but misses %.6f

[Lars Ingebrigtsen]

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

積丹尼 Dan Jacobson <jidanni@jidanni.org> writes:

> cperl mode highlights %d, but misses %.6f in
> printf "%.6f,%.6f,%d\n" ...


[-- Attachment #2: Type: image/png, Size: 8309 bytes --]

[-- Attachment #3: Type: text/plain, Size: 571 bytes --]


That font locking is actually a bug:

	     ("\\(\\([@%]\\|\\$#\\)[a-zA-Z_:][a-zA-Z0-9_:]*\\)" 1
	      (if (eq (char-after (match-beginning 2)) ?%)
		  'cperl-hash-face
		'cperl-array-face)
	      t)			; arrays and hashes

It's interpreting this as a hash called %d...  but we're inside a
string, so it's not a hash.

At this point, the string has already been fontised...  So how do we
avoid doing this stuff if we're in a string?  Cc'd Stefan for
expertise.  :-)

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

bug#22867: cperl mode highlights %d, but misses %.6f

[Andreas Schwab]

On Aug 07 2020, Lars Ingebrigtsen wrote:

> That font locking is actually a bug:
>
> 	     ("\\(\\([@%]\\|\\$#\\)[a-zA-Z_:][a-zA-Z0-9_:]*\\)" 1
> 	      (if (eq (char-after (match-beginning 2)) ?%)
> 		  'cperl-hash-face
> 		'cperl-array-face)
> 	      t)			; arrays and hashes
>
> It's interpreting this as a hash called %d...  but we're inside a
> string, so it's not a hash.
>
> At this point, the string has already been fontised...  So how do we
> avoid doing this stuff if we're in a string?  Cc'd Stefan for
> expertise.  :-)

Replace t by nil or 'keep.

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."

bug#22867: cperl mode highlights %d, but misses %.6f

[Lars Ingebrigtsen]

Andreas Schwab <schwab@linux-m68k.org> writes:

>> At this point, the string has already been fontised...  So how do we
>> avoid doing this stuff if we're in a string?  Cc'd Stefan for
>> expertise.  :-)
>
> Replace t by nil or 'keep.

Thanks; nil fixed the problem.  (With 'keep %d = {}; wasn't fontified
either.)

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

bug#22867: cperl-mode: Commit d0ad6306 suppresses fontification of hash/array declarations

[Harald Jörg]

The recent fix d0ad6306 to cperl-mode fixes a lot of unwanted
fontification of arrays and hashes, in particular in comments, POD,
and strings.  However, as a side effect it also prevents fontification
in a variable declaration (this has been observed by choroba):

   my @arr = (1,2,3); # @arr fontified as a scalar - bad
   push @arr, 4;      # @arr fontified as an array - good
   print $arr[4];     # $arr fontified as an array - good

   my %hash = ( foo => 'bar' ); # %hash fontified as a scalar - bad
   $hash{baz => 'quux'};        # $hash fontified as a hash - good

Can this be easily fixed?

---

Of minor importance: There are some cases where fontification of an
array in a string makes sense, but is gone now:

   my $mail = "me@somewhere.net";

This was previously fontified as an array.  This makes sense because
in doubly-quoted strings, the value of arrays (here: @somewhere) will
be interpolated into the string.  In the example above, the
fontification of @somewhere as an array was a welcome warning that
this is not what you want.

Fontification of a hash in a string, on the other hand, never makes
sense because hashes aren't interpolated.

---

Also, this bug can be merged with (Bug#11054), which in turn had been
merged with (Bug#3091).

bug#22867: cperl-mode: Commit d0ad6306 suppresses fontification of hash/array declarations

[Stefan Kangas]

forcemerge 11054 22867
thanks

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

> The recent fix d0ad6306 to cperl-mode fixes a lot of unwanted
> fontification of arrays and hashes, in particular in comments, POD,
> and strings.  However, as a side effect it also prevents fontification
> in a variable declaration (this has been observed by choroba):

It would be very nice to have test cases for all this.

Is the fix worse than the problem?  If yes, maybe it should be reverted?

> Also, this bug can be merged with (Bug#11054), which in turn had been
> merged with (Bug#3091).

Done.

bug#22867: cperl-mode: Commit d0ad6306 suppresses fontification of hash/array declarations

[Harald Jörg]

On 9/1/20 6:34 PM, Stefan Kangas wrote:
> forcemerge 11054 22867
> thanks
> 
> Harald Jörg <haj@posteo.de> writes:
> 
>> The recent fix d0ad6306 to cperl-mode fixes a lot of unwanted
>> fontification of arrays and hashes, in particular in comments, POD,
>> and strings.  However, as a side effect it also prevents fontification
>> in a variable declaration (this has been observed by choroba):
> 
> It would be very nice to have test cases for all this.

Indeed.  That will take a while, too :)

Not too long ago I stumbled over (or was pointed to?) a suite
https://github.com/Lindydancer/font-lock-regression-suite which also
contains an example in Perl.  Testing this example against its
faceup-counterpart would fail as of today.

> Is the fix worse than the problem?  If yes, maybe it should be reverted?

The fix isn't _worse_.  Strings like "%s" and "%d" in the argument
list of printf being fontified as hashes is indeed idiotic.  Also,
more than once I was annoyed by fontification in POD sections.

On the other hand, declarations of hashes and arrays occur rather
frequently, and there's a lot more in that area that would deserve
some love and care.  So, while this fix was a necessary first step in
any case, some refinement might be desirable to cut off the rough
edges it left.  I don't see an easy way, but this might be well due to
my limited elisp experience.
-- 
Cheers,
haj

bug#22867: cperl-mode: Commit d0ad6306 suppresses fontification of hash/array declarations

[Lars Ingebrigtsen]

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

>> Is the fix worse than the problem?  If yes, maybe it should be reverted?
>
> The fix isn't _worse_.  Strings like "%s" and "%d" in the argument
> list of printf being fontified as hashes is indeed idiotic.  Also,
> more than once I was annoyed by fontification in POD sections.

The change in the offending commit just ensured that we don't re-fontify
already-fontified stuff as hashes.  This means that the reason stuff
like "my %foo = ..." isn't fontified as a hash is because something has
already fontified it as something else, so I wondered whether just
flipping the order of some of these regexp would do the trick.  After
poking around a bit, I came up with this patch:

diff --git a/lisp/progmodes/cperl-mode.el b/lisp/progmodes/cperl-mode.el
index 5dee5007e2..7a1c2e4d24 100644
--- a/lisp/progmodes/cperl-mode.el
+++ b/lisp/progmodes/cperl-mode.el
@@ -5776,8 +5776,8 @@ cperl-init-faces
 		  t-font-lock-keywords)
 		cperl-font-lock-keywords cperl-font-lock-keywords-1
 		cperl-font-lock-keywords-2 (append
-					   cperl-font-lock-keywords-1
-					   t-font-lock-keywords-1)))
+					   t-font-lock-keywords-1
+					   cperl-font-lock-keywords-1)))
 	(if (fboundp 'ps-print-buffer) (cperl-ps-print-init))
 	(if (or (featurep 'choose-color) (featurep 'font-lock-extra))
 	    (eval			; Avoid a warning


It seems to fix all the test cases in Harald's .pl file, and the cperl
test file still passes (but its coverage isn't, ahem, extensive).

Does anybody see any problems with just doing fixing it like this?  I
had a peek at a couple of other perl files here, and nothing immediately
looked wonky, but I'm a bit out of practice writing perl...

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

bug#22867: cperl-mode: Commit d0ad6306 suppresses fontification of hash/array declarations

[Harald Jörg]

On 9/4/20 5:46 AM, Lars Ingebrigtsen wrote:
> Harald Jörg <haj@posteo.de> writes:
> 
>>> Is the fix worse than the problem?  If yes, maybe it should be reverted?
>>
>> The fix isn't _worse_.  Strings like "%s" and "%d" in the argument
>> list of printf being fontified as hashes is indeed idiotic.  Also,
>> more than once I was annoyed by fontification in POD sections.
> [...]
> After poking around a bit, I came up with this patch:
> 
> diff --git a/lisp/progmodes/cperl-mode.el b/lisp/progmodes/cperl-mode.el
> index 5dee5007e2..7a1c2e4d24 100644
> --- a/lisp/progmodes/cperl-mode.el
> +++ b/lisp/progmodes/cperl-mode.el
> @@ -5776,8 +5776,8 @@ cperl-init-faces
>  		  t-font-lock-keywords)
>  		cperl-font-lock-keywords cperl-font-lock-keywords-1
>  		cperl-font-lock-keywords-2 (append
> -					   cperl-font-lock-keywords-1
> -					   t-font-lock-keywords-1)))
> +					   t-font-lock-keywords-1
> +					   cperl-font-lock-keywords-1)))
>  	(if (fboundp 'ps-print-buffer) (cperl-ps-print-init))
>  	(if (or (featurep 'choose-color) (featurep 'font-lock-extra))
>  	    (eval			; Avoid a warning
> 
> 
> It seems to fix all the test cases in Harald's .pl file, and the cperl
> test file still passes (but its coverage isn't, ahem, extensive).
> 
> Does anybody see any problems with just doing fixing it like this?  I
> had a peek at a couple of other perl files here, and nothing immediately
> looked wonky, but I'm a bit out of practice writing perl...

That change is fine with me!
-- 
Cheers,
haj

bug#22867: cperl-mode: Commit d0ad6306 suppresses fontification of hash/array declarations

[Lars Ingebrigtsen]

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

>> Does anybody see any problems with just doing fixing it like this?  I
>> had a peek at a couple of other perl files here, and nothing immediately
>> looked wonky, but I'm a bit out of practice writing perl...
>
> That change is fine with me!

OK; applied to Emacs 28.  

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