Code review - Issue 6494075: code review 6494075: vet: add range variable misuse detectionhttps://codereview.appspot.com/2012-09-19T17:05:43+00:00rietveld
Message from unknown
2012-09-04T01:22:17+00:00adgurn:md5:b559c2dca29c621ce1461db4b138a5e4
Message from unknown
2012-09-04T01:22:39+00:00adgurn:md5:e356e2a96bf8f6f92a5db453ed662223
Message from unknown
2012-09-16T05:44:48+00:00adgurn:md5:0f56a96253e885423b33c215e7f2b9e6
Message from unknown
2012-09-16T05:54:07+00:00adgurn:md5:1998fe332b15c609c0c398da2095eb64
Message from adg@golang.org
2012-09-16T05:54:15+00:00adgurn:md5:c8456acbe67378daecf2ff13623fc02c
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://go.googlecode.com/hg
Message from fullung@gmail.com
2012-09-16T06:34:53+00:00albert.strasheimurn:md5:e7549d1a81d1c6e311af6b03e79fabf8
minor typo
http://codereview.appspot.com/6494075/diff/6001/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/6001/src/cmd/vet/rangeloop.go#newcode11
src/cmd/vet/rangeloop.go:11: // checkRangeLoop walks the body of the provided range statment, checking if
statment -> statement
Message from unknown
2012-09-17T00:24:38+00:00adgurn:md5:db6cda62834b1685bc25b95fb04d4150
Message from unknown
2012-09-17T17:16:35+00:00adgurn:md5:ecba9c484d385e2d6136cda46f51f07e
Message from adg@golang.org
2012-09-17T17:16:38+00:00adgurn:md5:29984fada1a319e2fd8e9b2528ef18a2
Hello golang-dev@googlegroups.com, fullung@gmail.com (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2012-09-17T17:16:52+00:00adgurn:md5:6a26dc6ea0766fc84a0730467b928378
Message from r@golang.org
2012-09-17T18:09:59+00:00rurn:md5:485391083953de439296bdad00b3427b
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go
File src/cmd/vet/main.go (right):
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go#newcode192
src/cmd/vet/main.go:192: f.walkRangeStmt(n)
these were in alphabetical order.
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go#newcode208
src/cmd/vet/main.go:208: checkRangeLoop(f, n)
ditto, almost. please fix them up
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/rangeloop.go#newcode5
src/cmd/vet/rangeloop.go:5: // This file contains the code to check the use of range loop variables.
maybe an example - it's not clear what's being checked.
Message from remyoudompheng@gmail.com
2012-09-17T18:16:14+00:00remyoudomphengurn:md5:baeff90019cae880374775088051791b
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go
File src/cmd/vet/main.go (right):
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go#newcode64
src/cmd/vet/main.go:64: if *vetMethods || *vetPrintf || *vetStructTags || *vetUntaggedLiteral || *vetRangeLoops {
this is becoming hard to read, I suggest (if you think it looks better):
switch {
case *vetMethods,
*vetPrintf,
*vetRangeLoops,
*vetStructTags,
*vetUntaggedLiteral:
*vetAll = false
}
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/rangeloop.go#newcode63
src/cmd/vet/rangeloop.go:63: // If found, it is issues a warning.
it issues ?
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/rangeloop.go#newcode102
src/cmd/vet/rangeloop.go:102: println("unforunately, we can't catch the error above because of this statement")
unfortunately
Message from unknown
2012-09-17T18:16:41+00:00adgurn:md5:12cb85bc39ff75b1c54b291d12e0577d
Message from adg@golang.org
2012-09-17T18:16:46+00:00adgurn:md5:d7a45f63a3adf36fa959a3613d1fa261
Hello golang-dev@googlegroups.com, fullung@gmail.com, r@golang.org, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com),
Please take another look.
Message from adg@golang.org
2012-09-17T18:17:31+00:00adgurn:md5:7219b3e51b96d12542568e70ab57ad8f
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go
File src/cmd/vet/main.go (right):
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go#newcode192
src/cmd/vet/main.go:192: f.walkRangeStmt(n)
On 2012/09/17 18:09:59, r wrote:
> these were in alphabetical order.
Done.
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go#newcode208
src/cmd/vet/main.go:208: checkRangeLoop(f, n)
On 2012/09/17 18:09:59, r wrote:
> ditto, almost. please fix them up
Done.
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/rangeloop.go#newcode5
src/cmd/vet/rangeloop.go:5: // This file contains the code to check the use of range loop variables.
On 2012/09/17 18:09:59, r wrote:
> maybe an example - it's not clear what's being checked.
Done.
Message from adg@golang.org
2012-09-17T18:19:13+00:00adgurn:md5:54288bf8244b3f768c896ab7a1c33a8f
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go
File src/cmd/vet/main.go (right):
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go#newcode64
src/cmd/vet/main.go:64: if *vetMethods || *vetPrintf || *vetStructTags || *vetUntaggedLiteral || *vetRangeLoops {
That's messier than what's here, IMO.
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/rangeloop.go#newcode63
src/cmd/vet/rangeloop.go:63: // If found, it is issues a warning.
On 2012/09/17 18:16:14, remyoudompheng wrote:
> it issues ?
Done.
http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/rangeloop.go#newcode102
src/cmd/vet/rangeloop.go:102: println("unforunately, we can't catch the error above because of this statement")
On 2012/09/17 18:16:14, remyoudompheng wrote:
> unfortunately
Done.
Message from unknown
2012-09-17T18:19:18+00:00adgurn:md5:815f106df83f2bff8d27bc219c5d62c9
Message from adg@golang.org
2012-09-17T18:19:21+00:00adgurn:md5:fff23b393289b6bf562b2ba6f7f46ee2
Hello golang-dev@googlegroups.com, fullung@gmail.com, r@golang.org, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com),
Please take another look.
Message from minux.ma@gmail.com
2012-09-17T18:22:26+00:00minux1urn:md5:5aa4097670f56ea30f621dbe5def301d
On Tuesday, September 18, 2012, adg wrote
>
> http://codereview.appspot.com/**6494075/diff/12001/src/cmd/**vet/main.go<http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go>
> File src/cmd/vet/main.go (right):
> http://codereview.appspot.com/**6494075/diff/12001/src/cmd/**
> vet/main.go#newcode64<http://codereview.appspot.com/6494075/diff/12001/src/cmd/vet/main.go#newcode64>
> src/cmd/vet/main.go:64: if *vetMethods || *vetPrintf || *vetStructTags
> || *vetUntaggedLiteral || *vetRangeLoops {
> That's messier than what's here, IMO.
Could we use an array of flags here so that we do not
need to add every bool flag here?
Message from r@golang.org
2012-09-17T18:23:31+00:00rurn:md5:1f34d614013751fb8b7ac4ecb89ff5cb
LGTM but let rsc or gri take a look
http://codereview.appspot.com/6494075/diff/4005/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/4005/src/cmd/vet/rangeloop.go#newcode9
src/cmd/vet/rangeloop.go:9: body, as otherwise we would need whole program analysis to reliably detect
unsplit the verb or just drop the whole "to ... " end of the sentence.
http://codereview.appspot.com/6494075/diff/4005/src/cmd/vet/rangeloop.go#newcode28
src/cmd/vet/rangeloop.go:28: // its index or value variables are used unsafely inside goroutine or deferred
"goroutines"?
Message from r@golang.org
2012-09-17T18:24:14+00:00rurn:md5:a1bb7158d0c1c120edb3d4ae41a6ac4c
On Mon, Sep 17, 2012 at 11:22 AM, minux <minux.ma@gmail.com> wrote:
> Could we use an array of flags here so that we do not
> need to add every bool flag here?
sounds good but let's make that a separate CL.
-rob
Message from adg@golang.org
2012-09-17T18:26:24+00:00adgurn:md5:0751660be5b002ded1516c2a39f6c06b
http://codereview.appspot.com/6494075/diff/4005/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/4005/src/cmd/vet/rangeloop.go#newcode9
src/cmd/vet/rangeloop.go:9: body, as otherwise we would need whole program analysis to reliably detect
On 2012/09/17 18:23:31, r wrote:
> unsplit the verb or just drop the whole "to ... " end of the sentence.
Done.
http://codereview.appspot.com/6494075/diff/4005/src/cmd/vet/rangeloop.go#newcode28
src/cmd/vet/rangeloop.go:28: // its index or value variables are used unsafely inside goroutine or deferred
On 2012/09/17 18:23:31, r wrote:
> "goroutines"?
>
Done.
Message from unknown
2012-09-17T18:26:31+00:00adgurn:md5:11bcf5cf919f839d9833680cc8a0181e
Message from gri@golang.org
2012-09-17T19:35:31+00:00griurn:md5:754dc9377d4bde6cab92751877f930d8
FYI.
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode64
src/cmd/vet/rangeloop.go:64: if g, ok := node.(*ast.GoStmt); ok {
switch n := node.(type) {
case nil:
case *ast.GoStmt:
case *ast.DeferStmt:
default:
}
?
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode79
src/cmd/vet/rangeloop.go:79: type rangeLoopVisitor struct {
FYI: There's nothing wrong with using a visitor, but for simple tree inspections like this you might be able to just use ast.Inspect with a closure. There's no need for a visitor type and the all the walk setup. Might be less code and easier to understand.
Message from gri@golang.org
2012-09-17T19:54:13+00:00griurn:md5:14137dce9eb5475d448659e724a247cf
PS:
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode81
src/cmd/vet/rangeloop.go:81: id *ast.Ident
You only need the ast.Object I think - no need to always do id.Obj.
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode85
src/cmd/vet/rangeloop.go:85: if n, ok := node.(*ast.Ident); ok && n.Name == v.id.Name && n.Obj == v.id.Obj {
The n.Name == v.id.Name shouldn't be required: Two identifiers that denote the same object have the same Obj field - if it's the same, the names must be the same. Instead I'd check n.Obj != nil && n.Obj == v.id.Obj - just as a precaution. If the identifier isn't resolved (due to an error), it's Obj will be nil.
Background: I think go vet works on individual files and thus doesn't always have full package information (or imports). Thus there is plenty of potential for unresolved identifiers and thus nil objects. In general, for the ones interesting here, the identifier is resolved because it's in the same file and inside a function. However, a program could refer to a globally declared range iteration variable which might be in another package; in that case Obj would be nil (and should be ignored).
Message from rsc@golang.org
2012-09-17T20:01:59+00:00rscurn:md5:c6af6e0a2c477e49decb9e6d3211ab11
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode33
src/cmd/vet/rangeloop.go:33: walkRangeFor(f, n.Key, n)
It is not required to use walk for everything. If the data structure is fixed, like here where we know it's a range statement, then it is usually easier to inspect its fields directly. Only the final walk, over the closure body, is truly useful:
if len(n.Body.List) == 0 {
return
}
var call *ast.CallExpr
switch stmt := n.Body.List[len(n.Body.List)-1].(type) {
case *ast.GoStmt:
call = stmt.Call
case *ast.DeferStmt:
call = stmt.Call
default:
return
}
lit, ok := call.Fun.(*ast.FuncLit)
if !ok {
return
}
v := &rangeLoopVisitor{file: f, key: key, val: val}
ast.Walk(v, lit.Body)
(And even there you could possibly use Inspect and a closure).
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode85
src/cmd/vet/rangeloop.go:85: if n, ok := node.(*ast.Ident); ok && n.Name == v.id.Name && n.Obj == v.id.Obj {
On 2012/09/17 19:54:13, gri wrote:
> The n.Name == v.id.Name shouldn't be required: Two identifiers that denote the
> same object have the same Obj field - if it's the same, the names must be the
> same. Instead I'd check n.Obj != nil && n.Obj == v.id.Obj - just as a
> precaution. If the identifier isn't resolved (due to an error), it's Obj will be
> nil.
>
> Background: I think go vet works on individual files and thus doesn't always
> have full package information (or imports). Thus there is plenty of potential
> for unresolved identifiers and thus nil objects. In general, for the ones
> interesting here, the identifier is resolved because it's in the same file and
> inside a function. However, a program could refer to a globally declared range
> iteration variable which might be in another package; in that case Obj would be
> nil (and should be ignored).
I think the check is good as written (with .Name). It's just as wrong to use globals this way as it is to use locals this way.
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode117
src/cmd/vet/rangeloop.go:117: println("unfortunately, we can't catch the error above because of this statement")
s/unfortunately, we can't/we don't/
You certainly _can_.
Message from adg@golang.org
2012-09-17T20:17:49+00:00adgurn:md5:181a122b4ff8c991c26b924858e650a7
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode33
src/cmd/vet/rangeloop.go:33: walkRangeFor(f, n.Key, n)
I had it written this way initially, before I introduced the lastEscapingCallVisitor. I added it because it means I can catch those additional cases where the defer or go statement is inside another block.
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode64
src/cmd/vet/rangeloop.go:64: if g, ok := node.(*ast.GoStmt); ok {
On 2012/09/17 19:35:31, gri wrote:
> switch n := node.(type) {
> case nil:
> case *ast.GoStmt:
> case *ast.DeferStmt:
> default:
> }
>
> ?
Done.
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode79
src/cmd/vet/rangeloop.go:79: type rangeLoopVisitor struct {
On 2012/09/17 19:35:31, gri wrote:
> FYI: There's nothing wrong with using a visitor, but for simple tree inspections
> like this you might be able to just use ast.Inspect with a closure. There's no
> need for a visitor type and the all the walk setup. Might be less code and
> easier to understand.
Done.
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode81
src/cmd/vet/rangeloop.go:81: id *ast.Ident
On 2012/09/17 19:54:13, gri wrote:
> You only need the ast.Object I think - no need to always do id.Obj.
Done.
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode117
src/cmd/vet/rangeloop.go:117: println("unfortunately, we can't catch the error above because of this statement")
On 2012/09/17 20:01:59, rsc wrote:
> s/unfortunately, we can't/we don't/
>
> You certainly _can_.
Done.
Message from unknown
2012-09-17T20:18:01+00:00adgurn:md5:65a1afe243086bbf7bc929d2c9eab14b
Message from adg@golang.org
2012-09-17T20:18:05+00:00adgurn:md5:7c90e09fa283d85433eea6f485fa661f
Hello fullung@gmail.com, r@golang.org, remyoudompheng@gmail.com, minux.ma@gmail.com, gri@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2012-09-17T20:20:25+00:00adgurn:md5:ea5f7f6bc540caa8ef7f1a1e0cccb934
Message from rsc@golang.org
2012-09-17T20:29:28+00:00rscurn:md5:c61975c7fdeced86f2434359699d8666
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode33
src/cmd/vet/rangeloop.go:33: walkRangeFor(f, n.Key, n)
On 2012/09/17 20:17:50, adg wrote:
> I had it written this way initially, before I introduced the
> lastEscapingCallVisitor. I added it because it means I can catch those
> additional cases where the defer or go statement is inside another block.
They're not worth it.
Message from adg@golang.org
2012-09-17T22:32:37+00:00adgurn:md5:71f666be99286402fba6285809ba1e42
On 17 September 2012 13:29, <rsc@golang.org> wrote:
>
> http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go
> File src/cmd/vet/rangeloop.go (right):
>
> http://codereview.appspot.com/6494075/diff/1002/src/cmd/vet/rangeloop.go#newcode33
> src/cmd/vet/rangeloop.go:33: walkRangeFor(f, n.Key, n)
> On 2012/09/17 20:17:50, adg wrote:
>>
>> I had it written this way initially, before I introduced the
>> lastEscapingCallVisitor. I added it because it means I can catch those
>> additional cases where the defer or go statement is inside another
>
> block.
>
> They're not worth it.
Not worth the extra 10 lines of code? Or are you worried about
execution speed? Or false positives? FWIW, you'd have to write pretty
contrived code to trigger a false positive.
Message from unknown
2012-09-18T20:31:37+00:00adgurn:md5:0a6cd28e653af994853dde3e66b7efb4
Message from adg@golang.org
2012-09-18T20:31:41+00:00adgurn:md5:9730c0f314b07085f664756893b200fb
Hello fullung@gmail.com, r@golang.org, remyoudompheng@gmail.com, minux.ma@gmail.com, gri@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),
Please take another look.
Message from unknown
2012-09-18T20:33:32+00:00adgurn:md5:104737e7004f100cfab49d75b9984034
Message from unknown
2012-09-18T20:34:52+00:00adgurn:md5:89258ed3adeda2045bf313f7ead660ad
Message from rsc@golang.org
2012-09-18T20:53:33+00:00rscurn:md5:e1676c8286c2d1e77c38db7f4c10879b
LGTM
http://codereview.appspot.com/6494075/diff/20007/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/20007/src/cmd/vet/rangeloop.go#newcode33
src/cmd/vet/rangeloop.go:33: checkRangeLoopFor(f, n, n.Key)
It's odd to do the work twice just because there are two variables.
Pass both into one function.
Message from unknown
2012-09-18T21:19:25+00:00adgurn:md5:5765ba4734f1842edb874b9b24c86ab0
Message from adg@golang.org
2012-09-18T21:22:31+00:00adgurn:md5:edd01cdb04be6a51037ef54e1347b0ac
*** Submitted as http://code.google.com/p/go/source/detail?r=838111caecdd ***
vet: add range variable misuse detection
R=fullung, r, remyoudompheng, minux.ma, gri, rsc
CC=golang-dev
http://codereview.appspot.com/6494075
http://codereview.appspot.com/6494075/diff/20007/src/cmd/vet/rangeloop.go
File src/cmd/vet/rangeloop.go (right):
http://codereview.appspot.com/6494075/diff/20007/src/cmd/vet/rangeloop.go#newcode33
src/cmd/vet/rangeloop.go:33: checkRangeLoopFor(f, n, n.Key)
On 2012/09/18 20:53:33, rsc wrote:
> It's odd to do the work twice just because there are two variables.
> Pass both into one function.
Done.
Message from fullung@gmail.com
2012-09-19T08:50:52+00:00albert.strasheimurn:md5:026a869d3815cca0d8599c4c29a51dc8
Hello
This didn't survive our code.
The file that blows it up has ranges like:
for b:= range bb {}
for i, c := range cc {}
for _, f := range ff {}
It's quite a big file though. Don't have time to reduce it right now unfortunately.
Cheers
Albert
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x18 pc=0x405fbb]
goroutine 1 [running]:
main.funcĀ·002(0xf84006efb8, 0xf84006efc0, 0xf84006efb0, 0x43b973, 0xf84008b840, ...)
/build/go/go/src/cmd/vet/rangeloop.go:56 +0x8b
go/ast.inspector.Visit(0xf840090870, 0xf84008b840, 0xf840087dc0, 0xf84008bc00, 0xf840090870, ...)
/build/go/go/src/pkg/go/ast/walk.go:367 +0x33
go/ast.Walk(0xf84008bc00, 0xf840090870, 0xf84008b840, 0xf840087dc0, 0x7f9ccb67e100, ...)
/build/go/go/src/pkg/go/ast/walk.go:52 +0x59
----- stack segment boundary -----
go/ast.walkExprList(0xf84008bc00, 0xf840090870, 0xf840078d80, 0x400000004, 0xf840087d80, ...)
/build/go/go/src/pkg/go/ast/walk.go:26 +0x8a
go/ast.Walk(0xf84008bc00, 0xf840090870, 0xf84008b9f0, 0xf840085d50, 0xf840085d50, ...)
/build/go/go/src/pkg/go/ast/walk.go:134 +0x1fc8
go/ast.Walk(0xf84008bc00, 0xf840090870, 0xf84008ba50, 0xf840088330, 0x7f9ccb67d100, ...)
/build/go/go/src/pkg/go/ast/walk.go:191 +0x41ab
----- stack segment boundary -----
go/ast.walkStmtList(0xf84008bc00, 0xf840090870, 0xf840078e00, 0x400000004, 0xf840090801, ...)
/build/go/go/src/pkg/go/ast/walk.go:32 +0x8a
go/ast.Walk(0xf84008bc00, 0xf840090870, 0xf84008b990, 0xf8400890c0, 0x405461, ...)
/build/go/go/src/pkg/go/ast/walk.go:219 +0x4117
go/ast.Inspect(0xf84008b990, 0xf8400890c0, 0xf840090870, 0xf84006efc0, 0xf84006efb0, ...)
/build/go/go/src/pkg/go/ast/walk.go:378 +0x52
main.checkRangeLoop(0xf84005d0e0, 0xf840001c80, 0x4020ce, 0xf84005d0e0)
/build/go/go/src/cmd/vet/rangeloop.go:60 +0x261
main.(*File).walkRangeStmt(0xf84005d0e0, 0xf840001c80, 0xf840001c80, 0xf840001c80)
/build/go/go/src/cmd/vet/main.go:260 +0x2f
main.(*File).Visit(0xf84005d0e0, 0xf84008bba0, 0xf840001c80, 0xf84008b7e0, 0xf84005d0e0, ...)
/build/go/go/src/cmd/vet/main.go:202 +0x24e
go/ast.Walk(0xf84008b7e0, 0xf84005d0e0, 0xf84008bba0, 0xf840001c80, 0x7f9ccb67c100, ...)
/build/go/go/src/pkg/go/ast/walk.go:52 +0x59
----- stack segment boundary -----
go/ast.walkStmtList(0xf84008b7e0, 0xf84005d0e0, 0xf840068400, 0x4000000030, 0xf84005d001, ...)
/build/go/go/src/pkg/go/ast/walk.go:32 +0x8a
go/ast.Walk(0xf84008b7e0, 0xf84005d0e0, 0xf84008b990, 0xf84008f100, 0xf84005d001, ...)
/build/go/go/src/pkg/go/ast/walk.go:219 +0x4117
go/ast.Walk(0xf84008b7e0, 0xf84005d0e0, 0xf84008b900, 0xf84008b450, 0x7f9ccb67a100, ...)
/build/go/go/src/pkg/go/ast/walk.go:337 +0xd0c
----- stack segment boundary -----
go/ast.walkDeclList(0xf84008b7e0, 0xf84005d0e0, 0xf840080400, 0x1000000009, 0xf84005d001, ...)
/build/go/go/src/pkg/go/ast/walk.go:38 +0x8a
go/ast.Walk(0xf84008b7e0, 0xf84005d0e0, 0xf84008b810, 0xf8400760c0, 0xf840088d20, ...)
/build/go/go/src/pkg/go/ast/walk.go:346 +0x2c7b
main.(*File).walkFile(0xf84005d0e0, 0x7fff31ccd602, 0xe, 0xf8400760c0, 0x0, ...)
/build/go/go/src/cmd/vet/main.go:185 +0x11c
main.doFile(0x7fff31ccd602, 0xe, 0x0, 0x0, 0x0, ...)
/build/go/go/src/cmd/vet/main.go:116 +0x1f5
main.main()
/build/go/go/src/cmd/vet/main.go:99 +0x522
On 2012/09/18 21:22:31, adg wrote:
> *** Submitted as http://code.google.com/p/go/source/detail?r=838111caecdd ***
>
> vet: add range variable misuse detection
>
> R=fullung, r, remyoudompheng, minux.ma, gri, rsc
> CC=golang-dev
> http://codereview.appspot.com/6494075
>
> http://codereview.appspot.com/6494075/diff/20007/src/cmd/vet/rangeloop.go
> File src/cmd/vet/rangeloop.go (right):
>
> http://codereview.appspot.com/6494075/diff/20007/src/cmd/vet/rangeloop.go#newcode33
> src/cmd/vet/rangeloop.go:33: checkRangeLoopFor(f, n, n.Key)
> On 2012/09/18 20:53:33, rsc wrote:
> > It's odd to do the work twice just because there are two variables.
> > Pass both into one function.
>
> Done.
Message from dsymonds@golang.org
2012-09-19T13:00:07+00:00dsymondsurn:md5:73a80d35119c94dffb08d9268e7c07d0
I've reduced it to something like this:
var x [2]int
var f int
for x[0], f = range s {
go func() {
_ = f
}()
}
The code is assuming that if one of key and value are non-nil then
(value!=nil) => (key!=nil) [true], and therefore (value is an
*ast.Ident) => (key is an *ast.Ident) [false].
Easy to fix.
Message from dsymonds@golang.org
2012-09-19T13:04:09+00:00dsymondsurn:md5:eeede8b6443118956392182d86f7558b
Fix: http://codereview.appspot.com/6540045
Message from adg@golang.org
2012-09-19T17:05:43+00:00adgurn:md5:5b598cbde741a64465a94b1edfb575a9
Sorry about that. Thanks for the report.
On 19 September 2012 01:50, <fullung@gmail.com> wrote:
> Hello
>
> This didn't survive our code.
>
> The file that blows it up has ranges like:
>
> for b:= range bb {}
>
> for i, c := range cc {}
>
> for _, f := range ff {}
>
> It's quite a big file though. Don't have time to reduce it right now
> unfortunately.
>
> Cheers
>
> Albert
>
> panic: runtime error: invalid memory address or nil pointer dereference
> [signal 0xb code=0x1 addr=0x18 pc=0x405fbb]
>
> goroutine 1 [running]:
> main.funcĀ·002(0xf84006efb8, 0xf84006efc0, 0xf84006efb0, 0x43b973,
> 0xf84008b840, ...)
> /build/go/go/src/cmd/vet/rangeloop.go:56 +0x8b
> go/ast.inspector.Visit(0xf840090870, 0xf84008b840, 0xf840087dc0,
> 0xf84008bc00, 0xf840090870, ...)
> /build/go/go/src/pkg/go/ast/walk.go:367 +0x33
> go/ast.Walk(0xf84008bc00, 0xf840090870, 0xf84008b840, 0xf840087dc0,
> 0x7f9ccb67e100, ...)
> /build/go/go/src/pkg/go/ast/walk.go:52 +0x59
> ----- stack segment boundary -----
> go/ast.walkExprList(0xf84008bc00, 0xf840090870, 0xf840078d80,
> 0x400000004, 0xf840087d80, ...)
> /build/go/go/src/pkg/go/ast/walk.go:26 +0x8a
> go/ast.Walk(0xf84008bc00, 0xf840090870, 0xf84008b9f0, 0xf840085d50,
> 0xf840085d50, ...)
> /build/go/go/src/pkg/go/ast/walk.go:134 +0x1fc8
> go/ast.Walk(0xf84008bc00, 0xf840090870, 0xf84008ba50, 0xf840088330,
> 0x7f9ccb67d100, ...)
> /build/go/go/src/pkg/go/ast/walk.go:191 +0x41ab
> ----- stack segment boundary -----
> go/ast.walkStmtList(0xf84008bc00, 0xf840090870, 0xf840078e00,
> 0x400000004, 0xf840090801, ...)
> /build/go/go/src/pkg/go/ast/walk.go:32 +0x8a
> go/ast.Walk(0xf84008bc00, 0xf840090870, 0xf84008b990, 0xf8400890c0,
> 0x405461, ...)
> /build/go/go/src/pkg/go/ast/walk.go:219 +0x4117
> go/ast.Inspect(0xf84008b990, 0xf8400890c0, 0xf840090870, 0xf84006efc0,
> 0xf84006efb0, ...)
> /build/go/go/src/pkg/go/ast/walk.go:378 +0x52
> main.checkRangeLoop(0xf84005d0e0, 0xf840001c80, 0x4020ce, 0xf84005d0e0)
> /build/go/go/src/cmd/vet/rangeloop.go:60 +0x261
> main.(*File).walkRangeStmt(0xf84005d0e0, 0xf840001c80, 0xf840001c80,
> 0xf840001c80)
> /build/go/go/src/cmd/vet/main.go:260 +0x2f
> main.(*File).Visit(0xf84005d0e0, 0xf84008bba0, 0xf840001c80,
> 0xf84008b7e0, 0xf84005d0e0, ...)
> /build/go/go/src/cmd/vet/main.go:202 +0x24e
> go/ast.Walk(0xf84008b7e0, 0xf84005d0e0, 0xf84008bba0, 0xf840001c80,
> 0x7f9ccb67c100, ...)
> /build/go/go/src/pkg/go/ast/walk.go:52 +0x59
> ----- stack segment boundary -----
> go/ast.walkStmtList(0xf84008b7e0, 0xf84005d0e0, 0xf840068400,
> 0x4000000030, 0xf84005d001, ...)
> /build/go/go/src/pkg/go/ast/walk.go:32 +0x8a
> go/ast.Walk(0xf84008b7e0, 0xf84005d0e0, 0xf84008b990, 0xf84008f100,
> 0xf84005d001, ...)
> /build/go/go/src/pkg/go/ast/walk.go:219 +0x4117
> go/ast.Walk(0xf84008b7e0, 0xf84005d0e0, 0xf84008b900, 0xf84008b450,
> 0x7f9ccb67a100, ...)
> /build/go/go/src/pkg/go/ast/walk.go:337 +0xd0c
> ----- stack segment boundary -----
> go/ast.walkDeclList(0xf84008b7e0, 0xf84005d0e0, 0xf840080400,
> 0x1000000009, 0xf84005d001, ...)
> /build/go/go/src/pkg/go/ast/walk.go:38 +0x8a
> go/ast.Walk(0xf84008b7e0, 0xf84005d0e0, 0xf84008b810, 0xf8400760c0,
> 0xf840088d20, ...)
> /build/go/go/src/pkg/go/ast/walk.go:346 +0x2c7b
> main.(*File).walkFile(0xf84005d0e0, 0x7fff31ccd602, 0xe, 0xf8400760c0,
> 0x0, ...)
> /build/go/go/src/cmd/vet/main.go:185 +0x11c
> main.doFile(0x7fff31ccd602, 0xe, 0x0, 0x0, 0x0, ...)
> /build/go/go/src/cmd/vet/main.go:116 +0x1f5
> main.main()
> /build/go/go/src/cmd/vet/main.go:99 +0x522
>
>
> On 2012/09/18 21:22:31, adg wrote:
>>
>> *** Submitted as
>
> http://code.google.com/p/go/source/detail?r=838111caecdd ***
>
>> vet: add range variable misuse detection
>
>
>> R=fullung, r, remyoudompheng, minux.ma, gri, rsc
>> CC=golang-dev
>> http://codereview.appspot.com/6494075
>
>
>
> http://codereview.appspot.com/6494075/diff/20007/src/cmd/vet/rangeloop.go
>>
>> File src/cmd/vet/rangeloop.go (right):
>
>
>
> http://codereview.appspot.com/6494075/diff/20007/src/cmd/vet/rangeloop.go#newcode33
>>
>> src/cmd/vet/rangeloop.go:33: checkRangeLoopFor(f, n, n.Key)
>> On 2012/09/18 20:53:33, rsc wrote:
>> > It's odd to do the work twice just because there are two variables.
>> > Pass both into one function.
>
>
>> Done.
>
>
>
> http://codereview.appspot.com/6494075/