GNU bug report logs - #753
[Fwd: sh-script.el: indentation of ( )]

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 753 in the body.
You can then email your comments to 753 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Daniel Pfeiffer <occitan <at> t-online.de>
To: bug-gnu-emacs <at> gnu.org
Cc: Darren Stuart Embry <dse <at> webonastick.com>
Subject: [Fwd: sh-script.el: indentation of ( )]
Date: Wed, 20 Aug 2008 23:36:40 +0200

[Message part 1 (text/plain, inline)]

Hi,

since I abandoned Shell in favour of Perl, over to you maintainers:

-------- Original-Nachricht --------
Betreff: 	sh-script.el: indentation of ( )
Datum: 	Wed, 20 Aug 2008 12:12:12 -0400
Von: 	Darren Stuart Embry <dse <at> webonastick.com>
An: 	Daniel Pfeiffer <occitan <at> esperanto.org>



Hello,

I'd like to report an issue regarding indentation of compound commands
surrounded with parenthesis.

Desired indentation:

	#!/bin/sh

	(
	    true
	)
	{
	    true
	}

What indent-region on the entire buffer yields:

	#!/bin/sh
	
	(
	    true
	    )
	{
	    true
	}

Note that the same behavior does not happen with the { } pair.

You have any idea why this is happening and/or how to fix this?

Version of emacs:
	GNU Emacs 22.2.1 (i486-pc-linux-gnu, X toolkit, Xaw3d scroll
	bars) of 2008-04-27 on raven, modified by Debian

Version of sh-script.el:
	;; Author: Daniel Pfeiffer <occitan <at> esperanto.org>
	;; Version: 2.0f

Thanks in advance.

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in email?



coralament / best Grötens / liebe Grüße / best regards / elkorajn salutojn
Daniel Pfeiffer

-- 
lerne / learn / apprends / lär dig / ucz się    Esperanto:
                   http://lernu.net  /  http://ikurso.net

[Message part 2 (text/html, inline)]

Message #8 received at 753 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: 753 <at> emacsbugs.donarmstrong.com
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Fri, 22 Aug 2008 20:12:00 -0400

Daniel Pfeiffer wrote:

> What indent-region on the entire buffer yields:
>
> 	#!/bin/sh
> 	
> 	(
> 	    true
> 	    )
> 	{
> 	    true
> 	}


Emacs 21.4 gets it right here, whereas 22.1 and later gets it wrong.

The difference seems to be caused by sh-get-kw, specifically:

2006-10-10  Stefan Monnier  <monnier <at> iro.umontreal.ca>

    * progmodes/sh-script.el (sh-get-kw): | is not among the allowed chars
    for a keyword.

In making this change (rev 1.191), you also added '()' to the
non-allowed characters along with '|'. Since it wasn't documented in
the ChangeLog, was this a mistake?

Message #11 received at 753 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 753 <at> debbugs.gnu.org
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Wed, 03 Sep 2008 13:34:22 -0400

Glenn Morris wrote:

> The difference seems to be caused by sh-get-kw, specifically:
>
> 2006-10-10  Stefan Monnier  <monnier <at> iro.umontreal.ca>
>
>     * progmodes/sh-script.el (sh-get-kw): | is not among the allowed chars
>     for a keyword.
>
> In making this change (rev 1.191), you also added '()' to the
> non-allowed characters along with '|'. Since it wasn't documented in
> the ChangeLog, was this a mistake?

Ping.

Can you remember why you added '()'?

Here's why the '|' got added:

http://lists.gnu.org/archive/html/emacs-pretest-bug/2006-10/msg00160.html

Message #16 received at 753-done <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: 753-done <at> debbugs.gnu.org
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Sat, 06 Sep 2008 14:59:20 -0400

2008-09-06  Glenn Morris  <rgm <at> gnu.org>

    * progmodes/sh-script.el (sh-get-kw): Remove '()' from the list of
      unallowed characters; added 2006-10-10 without comment.  (Bug#753)

Message #23 received at 753 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: 753 <at> debbugs.gnu.org
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Tue, 06 Jan 2009 23:47:23 -0500

>> The difference seems to be caused by sh-get-kw, specifically:
>> 
>> 2006-10-10  Stefan Monnier  <monnier <at> iro.umontreal.ca>
>> 
>> * progmodes/sh-script.el (sh-get-kw): | is not among the allowed chars
>> for a keyword.
>> 
>> In making this change (rev 1.191), you also added '()' to the
>> non-allowed characters along with '|'. Since it wasn't documented in
>> the ChangeLog, was this a mistake?

> Ping.

> Can you remember why you added '()'?

Because neither ( nor ) are allowed in keywords.


        Stefan

Message #26 received at 753 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 753 <at> debbugs.gnu.org
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Thu, 08 Jan 2009 03:00:53 -0500

[ You must unarchive old bugs before mailing them ]

Stefan Monnier wrote:

>> Can you remember why you added '()'?
>
> Because neither ( nor ) are allowed in keywords.

They _are_ keywords, along with { and }, as evidenced by their entries
in the constant `sh-kw'.

Message #31 received at 753 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 753 <at> debbugs.gnu.org
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Thu, 08 Jan 2009 11:39:55 -0500

> [ You must unarchive old bugs before mailing them ]

Yes: I didn't know it was old :-(

>>> Can you remember why you added '()'?
>> Because neither ( nor ) are allowed in keywords.
> They _are_ keywords, along with { and }, as evidenced by their entries
> in the constant `sh-kw'.

Hmm... I think I see what might be the problem, tho I don't understand
the code enough.  Basically, what I saw is that it recognized "done|" as
a keyword, which I fixed by adding ?| to the "not in keywords" chars.
By the same reasonging "done)" is not a keyword, so I added ?\)
(and ?\( as well for good measure).  I guess that sh-get-kw should be
fixed more robustly by recognizing ")" and "(" but not "(done", nor
"(done|toto)" for that matter.
Can you take care of that and make sure it fixes the problem at hand
without breaking the problem that prompted my misguided fix?


        Stefan

Message #34 received at 753 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 753 <at> debbugs.gnu.org
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Thu, 08 Jan 2009 21:39:01 -0500

Stefan Monnier wrote:

> Basically, what I saw is that it recognized "done|" as a keyword,
> which I fixed by adding ?| to the "not in keywords" chars. By the
> same reasonging "done)" is not a keyword, so I added ?\) (and ?\( as
> well for good measure).

Adding them causes the problem you are trying to avoid.

Applying (skip-chars-forward "^ \t\n;&|")

to "done) "

results in "done)", which is not a recognized keyword.

>  I guess that sh-get-kw should be fixed more robustly by recognizing
> ")" and "(" but not "(done", nor "(done|toto)" for that matter. Can
> you take care of that and make sure it fixes the problem at hand
> without breaking the problem that prompted my misguided fix?

It's tricky. Consider the following two examples:


for f in 1; do
    case $f in
        done) echo t ;;
    esac
done


(for f in 1; do
   echo $f
done)


In the second one, "done)" ends a for loop, in the first one it does not.

Both Emacs 22.3 and the current CVS get both examples wrong, in
different ways.

ii), though valid syntax, seems ugly (and hopefully uncommon) to me,
especially considering that in Bash at least, the same problem cannot
arise with {}, since these must be isolated by whitespace.

A simple fix for i) is as follows. I don't know how to fix both i) and ii).


Did you have any other examples of shell syntax to be considered?



*** sh-script.el	5 Jan 2009 03:23:50 -0000	1.220
--- sh-script.el	9 Jan 2009 02:22:31 -0000
***************
*** 1421,1427 ****
    "Make a regexp which matches WORD as a word.
  This specifically excludes an occurrence of WORD followed by
  punctuation characters like '-'."
!   (concat word "\\([^-[:alnum:]_]\\|$\\)"))
  
  (defconst sh-re-done (sh-mkword-regexpr "done"))
  
--- 1421,1428 ----
    "Make a regexp which matches WORD as a word.
  This specifically excludes an occurrence of WORD followed by
  punctuation characters like '-'."
!   ;; ")}" excludes things like "done)" in case statements.
!   (concat word "\\([^-[:alnum:]_)}]\\|$\\)"))
  
  (defconst sh-re-done (sh-mkword-regexpr "done"))

Message #39 received at 753 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 753 <at> debbugs.gnu.org
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Thu, 08 Jan 2009 22:58:08 -0500

>> Basically, what I saw is that it recognized "done|" as a keyword,
>> which I fixed by adding ?| to the "not in keywords" chars. By the
>> same reasonging "done)" is not a keyword, so I added ?\) (and ?\( as
>> well for good measure).

> Adding them causes the problem you are trying to avoid.

I don't follow you.

> Applying (skip-chars-forward "^ \t\n;&|")
> to "done) "
> results in "done)", which is not a recognized keyword.

So does the original code (skip-chars-forward "^ \t\n;&"), but so
doesn't my code (skip-chars-forward "^ \t\n;&|()").  In this sense my
code did make things more correct.

>> I guess that sh-get-kw should be fixed more robustly by recognizing
>> ")" and "(" but not "(done", nor "(done|toto)" for that matter. Can
>> you take care of that and make sure it fixes the problem at hand
>> without breaking the problem that prompted my misguided fix?

> It's tricky. Consider the following two examples:

> for f in 1; do
>     case $f in
>         done) echo t ;;
>     esac
> done


> (for f in 1; do
>    echo $f
> done)

> In the second one, "done)" ends a for loop, in the first one it does not.

It doesn't matter.  "done)" is not an entity in `sh', so we should not
treat it as one.  After all if you replace "done)" with "done )" in the
above 2 scripts, they still mean the same.

> Both Emacs 22.3 and the current CVS get both examples wrong, in
> different ways.

Yes, but it's a due to different bug than the one at hand, IIUC.


        Stefan

Message #44 received at 753 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 753 <at> debbugs.gnu.org
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Fri, 09 Jan 2009 00:02:28 -0500

Stefan Monnier wrote:

> It doesn't matter.  "done)" is not an entity in `sh', so we should not
> treat it as one.

Yes, but it is a useful shortcut to treat it as one IMO, since it
gets the indentation "mostly right" for little effort.

I'm afraid I don't know how to fix it (indentation) properly, so I
reopened this bug.

>  After all if you replace "done)" with "done )" in the above 2
> scripts, they still mean the same.

Message #49 received at 753 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 753 <at> debbugs.gnu.org
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Fri, 09 Jan 2009 12:24:02 -0500

>> It doesn't matter.  "done)" is not an entity in `sh', so we should not
>> treat it as one.
> Yes, but it is a useful shortcut to treat it as one IMO, since it
> gets the indentation "mostly right" for little effort.

It still gets it wrong for "done )".  And it introduces bugs elsewhere.
I'm pretty sure it was not done on purpose: it's basically a bug that
accidentally does the right thing in some corner cases.

> I'm afraid I don't know how to fix it (indentation) properly, so I
> reopened this bug.

We know how to fix the OP's bug without re-introducing the bug that my
misguided patch tried to fix.  So we should do that and close this bug.
Then we can open another bug about the "done )" case pattern since it's
a different bug.


        Stefan

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: Glenn Morris <rgm <at> gnu.org>, 753 <at> debbugs.gnu.org,
 Daniel Pfeiffer <occitan <at> t-online.de>
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Tue, 29 Dec 2015 19:45:28 -0500

+occitan (the original reported)

Stefan Monnier <monnier <at> IRO.UMontreal.CA> writes:

>>> It doesn't matter.  "done)" is not an entity in `sh', so we should not
>>> treat it as one.
>> Yes, but it is a useful shortcut to treat it as one IMO, since it
>> gets the indentation "mostly right" for little effort.
>
> It still gets it wrong for "done )".  And it introduces bugs elsewhere.
> I'm pretty sure it was not done on purpose: it's basically a bug that
> accidentally does the right thing in some corner cases.
>
>> I'm afraid I don't know how to fix it (indentation) properly, so I
>> reopened this bug.
>
> We know how to fix the OP's bug without re-introducing the bug that my
> misguided patch tried to fix.  So we should do that and close this bug.
> Then we can open another bug about the "done )" case pattern since it's
> a different bug.
>
>
>         Stefan

I can't reproduce the original bug on Emacs 25.  Can anyone reproduce
it?  If not, or I don't hear anything in a few weeks, I'll close this.

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

From: Glenn Morris <rgm <at> gnu.org>
To: Andrew Hyatt <ahyatt <at> gmail.com>
Cc: 753 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>,
 Daniel Pfeiffer <occitan <at> t-online.de>
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Thu, 31 Dec 2015 21:19:05 -0500

Andrew Hyatt wrote:

> I can't reproduce the original bug on Emacs 25.  Can anyone reproduce
> it?  If not, or I don't hear anything in a few weeks, I'll close this.

The remaining issue was not with the original recipe, it was with the
related stuff that was discussed later. You might like to check if any
of it is still relevant.

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

From: Andrew Hyatt <ahyatt <at> gmail.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: 753 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> IRO.UMontreal.CA>,
 Daniel Pfeiffer <occitan <at> t-online.de>
Subject: Re: bug#753: [Fwd: sh-script.el: indentation of ( )]
Date: Sat, 02 Jan 2016 15:55:32 -0500

Glenn Morris <rgm <at> gnu.org> writes:

> Andrew Hyatt wrote:
>
>> I can't reproduce the original bug on Emacs 25.  Can anyone reproduce
>> it?  If not, or I don't hear anything in a few weeks, I'll close this.
>
> The remaining issue was not with the original recipe, it was with the
> related stuff that was discussed later. You might like to check if any
> of it is still relevant.

There is a problem, but it seems fairly minor, and arguably not a bug -
if you use a case identifier that's also a keyword, it doesn't behave
correctly.  That has a trivial workaround - just wrap the keyword in
quotes.  I think to some extent, modes having issues with keywords
acting as identifiers or variables is pretty normal - at least hard to
fix without real semantic parsing.

I'm going to close this bug, and if anyone feels like there's still an
issue worth fixing here, please open a new bug.

Thanks for the response!

Previous Next

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