Issue 6494075: code review 6494075: vet: add range variable misuse detection (Closed)
|
Created:
12 years, 7 months ago by adg Modified:
12 years, 6 months ago CC:
r, remyoudompheng, minux1, gri, rsc, golang-dev Visibility:
Public. |
Descriptionvet: add range variable misuse detection
Patch Set 1 #Patch Set 2 : diff -r c2fb0e36f5e8 https://go.googlecode.com/hg #Patch Set 3 : diff -r e990cf183236 https://go.googlecode.com/hg #Patch Set 4 : diff -r e990cf183236 https://go.googlecode.com/hg #
Total comments: 1
Patch Set 5 : diff -r e990cf183236 https://go.googlecode.com/hg #Patch Set 6 : diff -r e990cf183236 https://go.googlecode.com/hg #Patch Set 7 : diff -r e990cf183236 https://go.googlecode.com/hg #
Total comments: 12
Patch Set 8 : diff -r 6c9c22a4b5c5 https://go.googlecode.com/hg #Patch Set 9 : diff -r 6c9c22a4b5c5 https://go.googlecode.com/hg #
Total comments: 4
Patch Set 10 : diff -r f48f1de09de5 https://go.googlecode.com/hg #
Total comments: 13
Patch Set 11 : diff -r f48f1de09de5 https://go.googlecode.com/hg #Patch Set 12 : diff -r f48f1de09de5 https://go.googlecode.com/hg #Patch Set 13 : diff -r 6cfab3a0935e https://go.googlecode.com/hg #Patch Set 14 : diff -r 6cfab3a0935e https://go.googlecode.com/hg #Patch Set 15 : diff -r 6cfab3a0935e https://go.googlecode.com/hg #
Total comments: 2
Patch Set 16 : diff -r 42b884930d7c https://go.googlecode.com/hg #
MessagesTotal messages: 27
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg
Sign in to reply to this message.
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#newc... src/cmd/vet/rangeloop.go:11: // checkRangeLoop walks the body of the provided range statment, checking if statment -> statement
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, fullung@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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#new... 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.
Sign in to reply to this message.
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#new... 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#new... src/cmd/vet/rangeloop.go:102: println("unforunately, we can't catch the error above because of this statement") unfortunately
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, fullung@gmail.com, r@golang.org, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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#new... 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.
Sign in to reply to this message.
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#new... 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#new... 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.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, fullung@gmail.com, r@golang.org, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On Tuesday, September 18, 2012, adg wrote > > http://codereview.appspot.com/**6494075/diff/12001/src/cmd/**vet/main.go<http... > 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?
Sign in to reply to this message.
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#newc... 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#newc... src/cmd/vet/rangeloop.go:28: // its index or value variables are used unsafely inside goroutine or deferred "goroutines"?
Sign in to reply to this message.
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
Sign in to reply to this message.
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#newc... 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#newc... 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.
Sign in to reply to this message.
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#newc... 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#newc... 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.
Sign in to reply to this message.
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#newc... 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#newc... 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).
Sign in to reply to this message.
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#newc... 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#newc... 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#newc... 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_.
Sign in to reply to this message.
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#newc... 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#newc... 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#newc... 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#newc... 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#newc... 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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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#newc... 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.
Sign in to reply to this message.
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#newc... > 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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
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#new... 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.
Sign in to reply to this message.
*** 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#new... 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.
Sign in to reply to this message.
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#new...
> 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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Sign in to reply to this message.
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#new... >> >> 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/
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
