code review 4534047: misc/vim: new Vim indentation script.

misc/vim: new Vim indentation script.

This uses a fully custom function for indenting Go code in Vim.
It provides a lot more flexibility than a cindent-based approach,
so this version gets the := operator correct, as well as switch
labels and jump labels.

One outstanding TODO is to handle lines immediately after jump labels.

[dsymonds, 2011-05-11 06:14:42]

Hello adg, niemeyer (cc: golang-dev@googlegroups.com, rivercheng@gmail.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dsymonds, 2011-05-11 06:16:01]

I'm going to start using this for regularly writing Go code,
so shortcomings should bug me enough to tune this.


Dave.

[jnw, 2011-05-11 09:22:11]

On 2011/05/11 06:16:01, dsymonds wrote:
> I'm going to start using this for regularly writing Go code,
> so shortcomings should bug me enough to tune this.
> 
> 
> Dave.

I'll do the same to get an idea for it. If I find time I'll also come up with a
small script that runs the indentation on a body of code and then checks it
against gofmt. That would give us a nice way to test any changes to the vim
indentation.

- Jim

[jnw, 2011-05-11 09:30:31]

On 2011/05/11 06:14:42, dsymonds wrote:
> Hello adg, niemeyer (cc: mailto:golang-dev@googlegroups.com,
mailto:rivercheng@gmail.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

I've dropped this into my vim distribution and it doesn't seem to be working.
Even a simple brace indent doesn't work:

func main() {
println("Hello world!")
}

The patch applied cleanly and I've verified that the indentation script is
loading properly and not being clobbered. Any ideas?

- Jim

[dsymonds, 2011-05-11 15:36:45]

On Wed, May 11, 2011 at 2:30 AM,  <jnwhiteh@gmail.com> wrote:

> Any ideas?

Have you got "syntax on" and "filetype indent on" in your .vimrc?
Follow the instructions in $GOROOT/misc/vim/readme.txt, and just
replace this indentation file.


Dave.

[jnw, 2011-05-11 15:39:53]

On Wed, May 11, 2011 at 4:36 PM, David Symonds <dsymonds@golang.org> wrote:
> On Wed, May 11, 2011 at 2:30 AM,  <jnwhiteh@gmail.com> wrote:
>
>> Any ideas?
>
> Have you got "syntax on" and "filetype indent on" in your .vimrc?
> Follow the instructions in $GOROOT/misc/vim/readme.txt, and just
> replace this indentation file.

Yes. I always have the plugin installed and all I did was apply your
patch. As I said I verified that the indentation script was loading
properly.

I wrote those directions, so I've definitely followed them =)

- Jim

[dsymonds, 2011-05-11 16:03:13]

Wow. I just tried it again, and it's broken for me too.
It must have been some late change I made last night. Damn.

I'll take a closer look later today. Sorry.

[dchest, 2011-05-11 16:06:24]

On 2011/05/11 09:30:31, jnw wrote:
> The patch applied cleanly and I've verified that the indentation script is
> loading properly and not being clobbered. Any ideas?

Same here, indentation doesn't work.
I checked that function GoIndent exists (it does).
Vim 7.3.107.

[jnw, 2011-05-11 16:10:57]

I was able to get this version of the indentation script working, using the fix
listed below. In most of my source files I saw no differences, so I took it to
the Go source to see how it would fare. I ran the following command on each Go
source file in the http package:

vim -c "normal gg=G" -c "wq" filename.go 

The following issues appear:

1. Following a switch statement, subsequent case statements will be indented one
level inside the switch statement. It should be on the same level.

2. On a multi-line string concatenation, lines following the first SHOULD be
indented, but this script matches the indentation of the first line.

 		"Set-Cookie: cookie-1=v$1\r\n" +
			"Set-Cookie: cookie-2=two; Max-Age=3600\r\n" +
			"Set-Cookie: cookie-3=three; Domain=.example.com\r\n" +
			"Set-Cookie: cookie-4=four; Path=/restricted/\r\n",

3. On a function invocation split across two lines, the second line should be
indented one level, i.e.:

 				t.Errorf("#%d: %d-th entry mismatch. Have {%s}, expect {%s}",
					k, i, result[i], l.Result[i])

4. Comments aren't handled properly, the following example fails to indent the
second line.

func main() { // this is a comment

There are a few more issues that I think need to be worked out before we push
this live, as the current version actually does reasonably well in most cases.
It would be much better to have a more flexible version written that doesn't
rely on C syntax, so if I can help please let me know.

- Jim

http://codereview.appspot.com/4534047/diff/5001/misc/vim/indent/go.vim
File misc/vim/indent/go.vim (right):

http://codereview.appspot.com/4534047/diff/5001/misc/vim/indent/go.vim#newcode25
misc/vim/indent/go.vim:25: let prevlnum = prevnonblank(lnum-1)
The vim manual indicates that in order to use an argument you must prefix its
name with a:. Indeed this script fails silently (but you can see the errors if
you invoke it manually using :call GoIndent(v:lnum). The fix is to change this
lnum to a:lnum.

[dsymonds, 2011-05-11 16:33:01]

D'oh. I did indeed change that late, trying to get the indentation
after jump labels working right. Fixed that.

I'm not too worried about covering every case perfectly. I think what
I've got here is a big improvement over the existing script simply due
to handling the := operator correctly. That's so very common in Go
code. This is never going to match gofmt perfectly, so I'm not willing
to invest massive amounts of effort in matching it in uncommon cases,
or even in common cases where go.vim will still give a
reasonable-looking indentation.


To your other points,

1. Switch statements seem to work fine for me. Can you give me a small
chunk of code that it fails on?

2. Yeah, that can be a later refinement, but it's not particularly common.

3. Later refinement; TODO added.

4. I don't think that particular comment placement is very common at
all, but I've added a comment to look into it.


Dave.

[jnw, 2011-05-11 16:37:19]

On Wed, May 11, 2011 at 5:32 PM, David Symonds <dsymonds@golang.org> wrote:
> D'oh. I did indeed change that late, trying to get the indentation
> after jump labels working right. Fixed that.
>
> I'm not too worried about covering every case perfectly. I think what
> I've got here is a big improvement over the existing script simply due
> to handling the := operator correctly. That's so very common in Go
> code. This is never going to match gofmt perfectly, so I'm not willing
> to invest massive amounts of effort in matching it in uncommon cases,
> or even in common cases where go.vim will still give a
> reasonable-looking indentation.
>
>
> To your other points,
>
> 1. Switch statements seem to work fine for me. Can you give me a small
> chunk of code that it fails on?

If you run your indent script against the http package, you'll see
each of these issues. The snippet is in cookie.go:

diff -r fcd501ee1ba4 src/pkg/http/cookie.go
--- a/src/pkg/http/cookie.go	Wed May 11 08:31:24 2011 -0700
+++ b/src/pkg/http/cookie.go	Wed May 11 17:34:27 2011 +0100
@@ -258,7 +258,7 @@

 func isCookieByte(c byte) bool {
 	switch true {
-	case c == 0x21, 0x23 <= c && c <= 0x2b, 0x2d <= c && c <= 0x3a,
+		case c == 0x21, 0x23 <= c && c <= 0x2b, 0x2d <= c && c <= 0x3a,
 		0x3c <= c && c <= 0x5b, 0x5d <= c && c <= 0x7e:
 		return true
 	}


> 2. Yeah, that can be a later refinement, but it's not particularly common.
>
> 3. Later refinement; TODO added.
>
> 4. I don't think that particular comment placement is very common at
> all, but I've added a comment to look into it.

Its not just there, its anytime a brace is followed by a comment which
is incredibly common. It also happens when a { or } is found within a
comment. Again, there are countless examples of this in the Go source
and it's very common in mine as well.

The rest I can give or take, but this one is probably the most
destructive.. it simply gets things wrong when comments are involved
it seems.

- Jim
>
>
> Dave.
>

[dsymonds, 2011-05-11 16:51:25]

On Wed, May 11, 2011 at 9:36 AM, Jim Whitehead II <jnwhiteh@gmail.com> wrote:

> If you run your indent script against the http package, you'll see
> each of these issues. The snippet is in cookie.go:
>
> diff -r fcd501ee1ba4 src/pkg/http/cookie.go
> --- a/src/pkg/http/cookie.go    Wed May 11 08:31:24 2011 -0700
> +++ b/src/pkg/http/cookie.go    Wed May 11 17:34:27 2011 +0100
> @@ -258,7 +258,7 @@
>
>  func isCookieByte(c byte) bool {
>        switch true {
> -       case c == 0x21, 0x23 <= c && c <= 0x2b, 0x2d <= c && c <= 0x3a,
> +               case c == 0x21, 0x23 <= c && c <= 0x2b, 0x2d <= c && c <=
0x3a,
>                0x3c <= c && c <= 0x5b, 0x5d <= c && c <= 0x7e:
>                return true
>        }

Okay, that's not a particularly common case. I think we can punt on
that for now.


>> 4. I don't think that particular comment placement is very common at
>> all, but I've added a comment to look into it.
>
> Its not just there, its anytime a brace is followed by a comment which
> is incredibly common. It also happens when a { or } is found within a
> comment. Again, there are countless examples of this in the Go source
> and it's very common in mine as well.

> The rest I can give or take, but this one is probably the most
> destructive.. it simply gets things wrong when comments are involved
> it seems.

I haven't seen it much, but I just grepped through the standard
package library, and it is there in plenty of places.
Let me hack a bit and see if it's easy to fix.

[dsymonds, 2011-05-11 17:00:05]

Cool, got it.

PTAL.

[niemeyer, 2011-05-11 18:59:12]

That's nice, thanks David.

A few minor suggestions:

http://codereview.appspot.com/4534047/diff/2003/misc/vim/indent/go.vim
File misc/vim/indent/go.vim (right):

http://codereview.appspot.com/4534047/diff/2003/misc/vim/indent/go.vim#newcode42
misc/vim/indent/go.vim:42: let ind = ind + &sw
let ind += &sw

http://codereview.appspot.com/4534047/diff/2003/misc/vim/indent/go.vim#newcode44
misc/vim/indent/go.vim:44: if prevl =~ '^\s*\(case .*\|default\):$'
This and the similar ones below should be =~# and !-#, so it checks for the
right casing.

http://codereview.appspot.com/4534047/diff/2003/misc/vim/indent/go.vim#newcode59
misc/vim/indent/go.vim:59: endif
I think something like this at that point would handle the TODO for functions
plus more:

" Indent sequences that were broken down in the middle.
if ind == previ && prevl =~ ',\s*$' && getline(prevlnum-1) !~ ',\s*$'
  let ind += &sw
endif

[dsymonds, 2011-05-11 23:57:47]

http://codereview.appspot.com/4534047/diff/2003/misc/vim/indent/go.vim
File misc/vim/indent/go.vim (right):

http://codereview.appspot.com/4534047/diff/2003/misc/vim/indent/go.vim#newcode42
misc/vim/indent/go.vim:42: let ind = ind + &sw
On 2011/05/11 18:59:13, niemeyer wrote:
> let ind += &sw

Done.

http://codereview.appspot.com/4534047/diff/2003/misc/vim/indent/go.vim#newcode44
misc/vim/indent/go.vim:44: if prevl =~ '^\s*\(case .*\|default\):$'
On 2011/05/11 18:59:13, niemeyer wrote:
> This and the similar ones below should be =~# and !-#, so it checks for the
> right casing.

Done.

http://codereview.appspot.com/4534047/diff/2003/misc/vim/indent/go.vim#newcode59
misc/vim/indent/go.vim:59: endif
On 2011/05/11 18:59:13, niemeyer wrote:
> I think something like this at that point would handle the TODO for functions
> plus more:
> 
> " Indent sequences that were broken down in the middle.
> if ind == previ && prevl =~ ',\s*$' && getline(prevlnum-1) !~ ',\s*$'
>   let ind += &sw
> endif

There are other ways of breaking lines than that, though. Let me punt it to a
future CL so we can exercise what I've got here a bit more widely first.

[rsc1, 2011-05-12 02:49:38]

>> let ind += &sw

This is wrong on so many levels.  I suppose it fits right in.

Russ

[jnw, 2011-05-12 08:05:02]

This looks great, only corner cases seem to different from gofmt and it gets the
:= operator right, which is a big win. Looks good to me, but might want a few
more days of wider use before we push it through.

http://codereview.appspot.com/4534047/diff/9002/misc/vim/indent/go.vim
File misc/vim/indent/go.vim (right):

http://codereview.appspot.com/4534047/diff/9002/misc/vim/indent/go.vim#newcode8
misc/vim/indent/go.vim:8: " - function invocations split across lines
Perhaps make a note that the same problem happens with trailing operators (like
concatenation on strings) that continue to multiple lines. It's possible that a
general solution to one will solve the other and it would greatly reduce the
difference between gofmt and the indentation script.

[Ross Light, 2011-05-13 03:08:42]

LGTM

[dsymonds, 2011-05-13 04:49:31]

Okay, I think this has converged. There's plenty of corner-cases still
to improve, but I think this is a reasonable basis.

I'll give this until at least tomorrow morning (~12h), and unless
there's a show-stopper I'll check this in and we can play with it and
iterate on the roughest parts gradually.


Dave.

[jnw, 2011-05-13 07:25:09]

On Fri, May 13, 2011 at 5:49 AM, David Symonds <dsymonds@golang.org> wrote:
> Okay, I think this has converged. There's plenty of corner-cases still
> to improve, but I think this is a reasonable basis.
>
> I'll give this until at least tomorrow morning (~12h), and unless
> there's a show-stopper I'll check this in and we can play with it and
> iterate on the roughest parts gradually.

Sounds great, my testing has all gone rather well. Once it's in the
repo, I'll make sure I update the vim-gofiles git mirror for those of
you who use pathogen and bundles.

- Jim

[jnw, 2011-05-13 13:34:51]

Sorry for the zero hour report, but I'm a bit concerned about the general
approach we're taking here, particularly with the handling of strings and
comments. The latest issue involves the use of colon in a string causing the
line to be dropped to the leftmost column, which is rather jarring when working
through code.

I'm far from a vim guru, but can't we piggy-back onto the already fantastic
syntax ftplugin and utilize that to strip out (or be aware of) strings and
comments from the indentation logic? I've done a small bit of research but the
world of indent scripts seems like voodoo to me.

- Jim

http://codereview.appspot.com/4534047/diff/9002/misc/vim/indent/go.vim
File misc/vim/indent/go.vim (right):

http://codereview.appspot.com/4534047/diff/9002/misc/vim/indent/go.vim#newcode61
misc/vim/indent/go.vim:61: if thisl =~ '^\s*\S\+:\s*$'
This code seems to cause any use of colon in a string to outdent the given line.
This can be seen quite easily:

func foo() {
  panic("NYI: foo")
}

Everything is fine until we type the :, at which point the line gets outdented.

[jnw, 2011-05-13 13:49:14]

On 2011/05/13 13:34:51, jnw wrote:
> Sorry for the zero hour report, but I'm a bit concerned about the general
> approach we're taking here, particularly with the handling of strings and
> comments. The latest issue involves the use of colon in a string causing the
> line to be dropped to the leftmost column, which is rather jarring when
working
> through code.
> 
> I'm far from a vim guru, but can't we piggy-back onto the already fantastic
> syntax ftplugin and utilize that to strip out (or be aware of) strings and
> comments from the indentation logic? I've done a small bit of research but the
> world of indent scripts seems like voodoo to me.
> 
> - Jim
> 
> http://codereview.appspot.com/4534047/diff/9002/misc/vim/indent/go.vim
> File misc/vim/indent/go.vim (right):
> 
>
http://codereview.appspot.com/4534047/diff/9002/misc/vim/indent/go.vim#newcode61
> misc/vim/indent/go.vim:61: if thisl =~ '^\s*\S\+:\s*$'
> This code seems to cause any use of colon in a string to outdent the given
line.
> This can be seen quite easily:
> 
> func foo() {
>   panic("NYI: foo")
> }
> 
> Everything is fine until we type the :, at which point the line gets
outdented.

I should note that running indent as soon as there is a non-space character
following the colon causes it to drop into the right place, so it gets it right
in the end, it is just very jarring when writing a flow of code.

- Jim

[dsymonds, 2011-05-13 14:57:57]

On Fri, May 13, 2011 at 6:34 AM,  <jnwhiteh@gmail.com> wrote:

> Sorry for the zero hour report, but I'm a bit concerned about the
> general approach we're taking here, particularly with the handling of
> strings and comments. The latest issue involves the use of colon in a
> string causing the line to be dropped to the leftmost column, which is
> rather jarring when working through code.

There's still plenty of edge cases, and colons inside strings is one of them.
That case is also handled badly by the current indent script, but at
least we have a basis to improve these kinds of cases.
Given that my indent script is not making this case worse, I don't
consider this a showstopper.
Let's proceed.


Dave.

[rsc1, 2011-05-13 15:08:18]

LGTM

[jnw, 2011-05-13 15:18:51]

On 2011/05/13 14:57:57, dsymonds wrote:
> On Fri, May 13, 2011 at 6:34 AM,  <mailto:jnwhiteh@gmail.com> wrote:
> 
> > Sorry for the zero hour report, but I'm a bit concerned about the
> > general approach we're taking here, particularly with the handling of
> > strings and comments. The latest issue involves the use of colon in a
> > string causing the line to be dropped to the leftmost column, which is
> > rather jarring when working through code.
> 
> There's still plenty of edge cases, and colons inside strings is one of them.
> That case is also handled badly by the current indent script, but at
> least we have a basis to improve these kinds of cases.
> Given that my indent script is not making this case worse, I don't
> consider this a showstopper.
> Let's proceed.

I am really not trying to be a pain, I'm just trying to give feedback on
something that I use daily.

This is new behaviour introduced with this vim script, the current script in the
repository handles it correctly. If everyone is fine with trading the behaviour
of := for this then we can absolutely commit it, it does quite a good job with
code. 

- Jim

[dsymonds, 2011-05-13 15:23:33]

Okay, you make a fair case, and it turns out it wasn't hard to fix.
I've uploaded a tweaked version. PTAL.

[jnw, 2011-05-13 15:27:50]

On 2011/05/13 15:23:33, dsymonds wrote:
> Okay, you make a fair case, and it turns out it wasn't hard to fix.
> I've uploaded a tweaked version. PTAL.

LGTM!

[dsymonds, 2011-05-13 15:29:58]

*** Submitted as http://code.google.com/p/go/source/detail?r=4a52fe751853 ***

misc/vim: new Vim indentation script.

This uses a fully custom function for indenting Go code in Vim.
It provides a lot more flexibility than a cindent-based approach,
so this version gets the := operator correct, as well as switch
labels and jump labels.

One outstanding TODO is to handle lines immediately after jump labels.

R=adg, n13m3y3r, jnwhiteh, dchest, rsc, rlight2
CC=golang-dev, rivercheng
http://codereview.appspot.com/4534047

[rivercheng_gmail.com, 2011-05-14 14:07:18]

The new indent file works very well now. Have not found any issues yet. Will
use it in my daily coding. Thank you all for this prompt help and wonderful
patch. Go community really rocks.


On Fri, May 13, 2011 at 11:29 PM, <dsymonds@golang.org> wrote:

> *** Submitted as
> http://code.google.com/p/go/source/detail?r=4a52fe751853 ***
>
>
> misc/vim: new Vim indentation script.
>
> This uses a fully custom function for indenting Go code in Vim.
> It provides a lot more flexibility than a cindent-based approach,
> so this version gets the := operator correct, as well as switch
> labels and jump labels.
>
> One outstanding TODO is to handle lines immediately after jump labels.
>
> R=adg, n13m3y3r, jnwhiteh, dchest, rsc, rlight2
> CC=golang-dev, rivercheng
>
> http://codereview.appspot.com/4534047
>
>
> http://codereview.appspot.com/4534047/
>