GNU bug report logs - #76851
29.4; cperl-mode builtin fcn indent bug and fix

Previous Next

Full log

Message #31 received at 76851 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Harald Jörg <haj <at> posteo.de>
Cc: John Ciolfi <ciolfi <at> mathworks.com>, 76851 <at> debbugs.gnu.org
Subject: Re: bug#76851: 29.4; cperl-mode builtin fcn indent bug and fix
Date: Sun, 9 Mar 2025 11:10:29 +0000

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

> Stefan Kangas <stefankangas <at> gmail.com> writes:
>
>> Harald Jörg <haj <at> posteo.de> writes:
>>
>>> Stefan Kangas <stefankangas <at> gmail.com> writes:
>>>
>>>> Harald Jörg <haj <at> posteo.de> writes:
>>>>
>>>>> Stefan Kangas <stefankangas <at> gmail.com> writes:
>>>>>
>>>>>> John Ciolfi via "Bug reports for GNU Emacs, the Swiss army knife of text
>>>>>> editors" <bug-gnu-emacs <at> gnu.org> writes:
>>>>>>
>>>>>>> Hi
>>>>>>>
>>>>>>> Given this perl file:
>>>>>>>
>>>>>>>     sub test {
>>>>>>>         exec '/bin/echo',
>>>>>>>             'Your arguments are: ', @ARGV;
>>>>>>>     }
>>>>>>>
>>>>>>>     sub exec_fcn {
>>>>>>>     }
>>>>>>>
>>>>>>>         sub other {
>>>>>>>         }
>>>>>>>
>>>>>>> The 'sub other' is indented incorrectly (and all code following it).
>>>>>>>
>>>>>>> The fix is in cperl-after-block-and-statement-beg to not match exec_, i.e. we should
>>>>>>> not treat exec_fcn as a builtin. Attached is the fix.
>>>>>>
>>>>>> Harald, any comments on this patch?
>>>>>
>>>>> Yes :)
>>>>>
>>>>> I confirm this is a bug, and one of those nasty ones which affect all
>>>>> following code.
>>>>>
>>>>> I suggest a slightly improved patch.  Instead of replacing \> (end of
>>>>> word) by [[:space:]] it should be replaced by \_> (end of symbol).
>>>>> A space is not required after the keywords in the list.  In the case of
>>>>> exec, this is handled elsewhere, but there are keywords in the list
>>>>> where it matters.
>>>>>
>>>>> With [[:space:]], the following indentation happens:
>>>>>
>>>>>     my %h = map{$_=>1}
>>>>>     @ARGV;
>>>>>
>>>>> With \_>, @ARGV is correctly indented as a continuation line:
>>>>>
>>>>>     my %h = map{$_=>1}
>>>>>         @ARGV;
>>>>>
>>>>> This is less severe than the bug reported because it does not affect
>>>>> following code, but still should not happen.
>>>>
>>>> I recommend adding tests for the above.  I think we already have good
>>>> erts tests in cperl-indents.erts, that could easily be expanded.
>>>
>>> Sure, I plan to do that.  CPerl indentation with its multitude of
>>> options is one of its messier corners, without extensive tests it is
>>> dangerous to change anything.
>>>
>>> How do we proceed? Shall I commit my patch to emacs-30 together with
>>> tests?
>>
>> I don't think I can fully assess the risks of installing or not
>> installing this change, as cperl-mode is quite complex and I'm not
>> too familiar with it.
>
> Replacing end-of-word by end-of-symbol here is very low risk.  The same
> change has been applied to other regular expressions in the past,
> e.g. to fix Bug#18985.  The function patched here is one of the last
> "tie-breakers" to distinguish whether a } character ends a statement.
>
>> If this is a regression, we would tend to favor installing the fix on
>> the release branch.  Otherwise, especially if this is a messy corner and
>> we feel that it's risky to change, perhaps we should install it on
>> master.
>
> I _guess_ this bug sits here since 1999, commit 8f2222484972.  Before
> that, the underscore character was considered a word character by CPerl
> mode, so the regexp was "correct" unless someone fiddled with the option
> cperl-under-as-char (deprecated since Emacs 24.4, and its replacement
> superword-mode affects only movement, not regexps).
>
>> What is your estimation of this issue?  Do you consider it important to
>> fix on the release branch?
>
> The chance to hit this bug is low, as indicated by the fact that it
> didn't surface for that long.  I find it nasty, because it affects the
> following code ... until it finds an extra semicolon which it takes as
> end of the statement.
>
> So, that's also a workaround: Add a semicolon after the closing brace.
>     sub exec_fcn {...};
>
> So, to summarize:
>  + Risk: Very low.
>  + Probability to hit: Very low.
>  + Bug impact: annoying.
>  + Workaround: exists and is easy (albeit ugly).
>
> Pushing it to master works for me just fine, too.

Based on the above, and John Ciolfi's explanation, installing the fix on
emacs-30 sounds okay to me.  Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.