code review 98850043: cmd/vet: check that string literals in regexp.Compile a...

cmd/vet: check that string literals in regexp.Compile are valid.

[djd, 2014-06-12 05:25:02]

Hello dsymonds@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.tools

[josharian, 2014-06-12 16:27:38]

I'm not sure that this warrants a vet check, since users should be checking
errors anyway. But if you update this to clpatch cleanly against tip, I'll run
it over a corpus of public code to see how many cases it catches.

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/regexp.go
File cmd/vet/regexp.go (right):

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/regexp.go#newcode18
cmd/vet/regexp.go:18: if !vet("regexp") { //|| pkg != "regexp" ||
!strings.Contains(name, "Compile") {
Leftover debugging code?

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/regexp.go#newcode42
cmd/vet/regexp.go:42: if strings.Contains(name, "POSIX") {
Maybe s/Contains/HasSuffix/

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/testdata/regexp.go
File cmd/vet/testdata/regexp.go (right):

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/testdata/regexp.go...
cmd/vet/testdata/regexp.go:1: // Copyright 2010 The Go Authors. All rights
reserved.
New files should use the current year for copyright, here and in the other new
file.

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/testdata/regexp.go...
cmd/vet/testdata/regexp.go:18: // This function never executes, but it serves as
a simple test for the program.
I think you can omit this whole comment; the context makes it clear.

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/testdata/regexp.go...
cmd/vet/testdata/regexp.go:26: regexp.Compile(`(unmatched`)          // ERROR
"error parsing regexp.*"
No need for trailing .* here and below.

[djd, 2014-06-13 03:09:39]

All done, and sync'd to head.

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/regexp.go
File cmd/vet/regexp.go (right):

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/regexp.go#newcode18
cmd/vet/regexp.go:18: if !vet("regexp") { //|| pkg != "regexp" ||
!strings.Contains(name, "Compile") {
On 2014/06/12 16:27:38, josharian wrote:
> Leftover debugging code?

Done.

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/regexp.go#newcode42
cmd/vet/regexp.go:42: if strings.Contains(name, "POSIX") {
On 2014/06/12 16:27:37, josharian wrote:
> Maybe s/Contains/HasSuffix/

Done.

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/testdata/regexp.go
File cmd/vet/testdata/regexp.go (right):

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/testdata/regexp.go...
cmd/vet/testdata/regexp.go:1: // Copyright 2010 The Go Authors. All rights
reserved.
On 2014/06/12 16:27:38, josharian wrote:
> New files should use the current year for copyright, here and in the other new
> file.

Done.

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/testdata/regexp.go...
cmd/vet/testdata/regexp.go:18: // This function never executes, but it serves as
a simple test for the program.
On 2014/06/12 16:27:38, josharian wrote:
> I think you can omit this whole comment; the context makes it clear.

Done.

https://codereview.appspot.com/98850043/diff/80001/cmd/vet/testdata/regexp.go...
cmd/vet/testdata/regexp.go:26: regexp.Compile(`(unmatched`)          // ERROR
"error parsing regexp.*"
On 2014/06/12 16:27:38, josharian wrote:
> No need for trailing .* here and below.

Done.

[djd, 2014-06-13 07:25:25]

I've just updated this to account for dsymond's refactor of cmd/vet.

[dsymonds, 2014-06-13 07:41:29]

LGTM, but wait for r to weigh in.

https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go
File cmd/vet/regexp.go (right):

https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go#newcode34
cmd/vet/regexp.go:34: if i, ok := x.X.(*ast.Ident); !ok || i.Name != "regexp" {
i -> pkg

https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go#newcode37
cmd/vet/regexp.go:37: if len(call.Args) != 1 {
move this up higher, since it's the cheapest early-out.

https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go#newcode44
cmd/vet/regexp.go:44: re := lit.Value
I think for a BasicLit of kind token.String the lit.Value is a quoted string, so
you'll want to strconv.Unquote before checking if it is a valid regular
expression.

https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go#newcode53
cmd/vet/regexp.go:53: f.Badf(call.Pos(), err.Error())
Anchor it to lit.Pos() instead. That's going to be more useful if someone splits
an expression over multiple lines.

A little more context in the error message would be nice.

[djd, 2014-06-13 08:28:26]

https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go
File cmd/vet/regexp.go (right):

https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go#newcode34
cmd/vet/regexp.go:34: if i, ok := x.X.(*ast.Ident); !ok || i.Name != "regexp" {
On 2014/06/13 07:41:29, dsymonds wrote:
> i -> pkg

Done.

https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go#newcode37
cmd/vet/regexp.go:37: if len(call.Args) != 1 {
On 2014/06/13 07:41:29, dsymonds wrote:
> move this up higher, since it's the cheapest early-out.

Done.

https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go#newcode44
cmd/vet/regexp.go:44: re := lit.Value
On 2014/06/13 07:41:29, dsymonds wrote:
> I think for a BasicLit of kind token.String the lit.Value is a quoted string,
so
> you'll want to strconv.Unquote before checking if it is a valid regular
> expression.

You're right, added tests to verify too.

https://codereview.appspot.com/98850043/diff/140001/cmd/vet/regexp.go#newcode53
cmd/vet/regexp.go:53: f.Badf(call.Pos(), err.Error())
On 2014/06/13 07:41:29, dsymonds wrote:
> Anchor it to lit.Pos() instead. That's going to be more useful if someone
splits
> an expression over multiple lines.
> 
> A little more context in the error message would be nice.

Done the anchoring. The regexp errors start with "error parsing regexp", and I
can't think of anything more useful to add than that. A full example output is:

testdata/regexp.go:23: error parsing regexp: invalid escape sequence: `\s`

[dsymonds, 2014-06-13 09:22:04]

LGTM

https://codereview.appspot.com/98850043/diff/180001/cmd/vet/regexp.go
File cmd/vet/regexp.go (right):

https://codereview.appspot.com/98850043/diff/180001/cmd/vet/regexp.go#newcode57
cmd/vet/regexp.go:57: f.Badf(lit.Pos(), err.Error())
either use f.Bad, or use a format string, otherwise a % in a bad regexp will
yield a mess.

[djd, 2014-06-13 10:40:14]

https://codereview.appspot.com/98850043/diff/180001/cmd/vet/regexp.go
File cmd/vet/regexp.go (right):

https://codereview.appspot.com/98850043/diff/180001/cmd/vet/regexp.go#newcode57
cmd/vet/regexp.go:57: f.Badf(lit.Pos(), err.Error())
On 2014/06/13 09:22:04, dsymonds wrote:
> either use f.Bad, or use a format string, otherwise a % in a bad regexp will
> yield a mess.

Done.

[r, 2014-06-13 13:20:27]

I don't think this is worth it, since it's checking something that's already
verifiable by existing means.

I'm worried that people are turning vet into a little playground for ideas in
static analysis. Every new check should add genuine value. Vet needs to stay
focused on issues that cause real bugs that are otherwise hard to find.

[josharian, 2014-06-13 15:52:45]

I ran this on a large corpus. It found only one instance:

github.com/coocood/jas/router_test.go:161: error parsing regexp: invalid escape
sequence: `\1`

And that appears to have been fixed since I last updated the corpus.

I'm with r on this one.

[mtj1, 2014-06-13 19:52:18]

Rob,

What about a Goadvise or Golint for more general static analysis? (fork
govet and accept most anything in the new one and greatest hits in the
former.)


On Fri, Jun 13, 2014 at 5:52 PM, <josharian@gmail.com> wrote:

> I ran this on a large corpus. It found only one instance:
>
> github.com/coocood/jas/router_test.go:161: error parsing regexp: invalid
> escape sequence: `\1`
>
> And that appears to have been fixed since I last updated the corpus.
>
> I'm with r on this one.
>
> https://codereview.appspot.com/98850043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>



-- 
*Michael T. Jones | Chief Technology Advocate  | mtj@google.com
<mtj@google.com> |  +1 650-335-5765*

[mtj1, 2014-06-13 19:53:55]

"One man's toy is another's real system, and every tool adds to the
arsenal."


On Fri, Jun 13, 2014 at 9:51 PM, Michael Jones <mtj@google.com> wrote:

> Rob,
>
> What about a Goadvise or Golint for more general static analysis? (fork
> govet and accept most anything in the new one and greatest hits in the
> former.)
>
>
> On Fri, Jun 13, 2014 at 5:52 PM, <josharian@gmail.com> wrote:
>
>> I ran this on a large corpus. It found only one instance:
>>
>> github.com/coocood/jas/router_test.go:161: error parsing regexp: invalid
>> escape sequence: `\1`
>>
>> And that appears to have been fixed since I last updated the corpus.
>>
>> I'm with r on this one.
>>
>> https://codereview.appspot.com/98850043/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>>
>
>
>
> --
> *Michael T. Jones | Chief Technology Advocate  | mtj@google.com
> <mtj@google.com> |  +1 650-335-5765 <%2B1%20650-335-5765>*
>



-- 
*Michael T. Jones | Chief Technology Advocate  | mtj@google.com
<mtj@google.com> |  +1 650-335-5765*

[alsodave, 2014-06-19 03:56:04]

I was mostly thinking of this check in the context of vet being used as
part of a code review tool. However, it does only pick up issues which even
a tiny amount of testing should uncover, so happy to go with the crowd and
drop it.


On 14 June 2014 01:52, <josharian@gmail.com> wrote:

> I ran this on a large corpus. It found only one instance:
>
> github.com/coocood/jas/router_test.go:161: error parsing regexp: invalid
> escape sequence: `\1`
>
> And that appears to have been fixed since I last updated the corpus.
>
> I'm with r on this one.
>
> https://codereview.appspot.com/98850043/
>

[r, 2014-06-19 05:27:32]

If it's code review you want, suggest it for golint instead of vet.

-rob


On Wed, Jun 18, 2014 at 8:55 PM, Dave Day <davidday@google.com> wrote:
> I was mostly thinking of this check in the context of vet being used as part
> of a code review tool. However, it does only pick up issues which even a
> tiny amount of testing should uncover, so happy to go with the crowd and
> drop it.
>
>
> On 14 June 2014 01:52, <josharian@gmail.com> wrote:
>>
>> I ran this on a large corpus. It found only one instance:
>>
>> github.com/coocood/jas/router_test.go:161: error parsing regexp: invalid
>> escape sequence: `\1`
>>
>> And that appears to have been fixed since I last updated the corpus.
>>
>> I'm with r on this one.
>>
>> https://codereview.appspot.com/98850043/
>
>

[r, 2014-06-26 18:17:35]

NOT LGTM

Clearing this from the dashboard. Perhaps it belongs in golint? But I don' think
it's worthwhile anywhere and josharian's statistic supports that.

[djd, 2014-06-26 23:53:14]

*** Abandoned ***