Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(58)

Issue 6494075: code review 6494075: vet: add range variable misuse detection (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 7 months ago by adg
Modified:
7 years, 6 months ago
CC:
r, remyoudompheng, minux1, gri, rsc, golang-dev
Visibility:
Public.

Description

vet: 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+122 lines, -10 lines) Patch
M src/cmd/vet/Makefile View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/vet/main.go View 1 2 3 4 5 6 7 5 chunks +17 lines, -9 lines 0 comments Download
A src/cmd/vet/rangeloop.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +104 lines, -0 lines 0 comments Download

Messages

Total messages: 27
adg
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg
7 years, 6 months ago (2012-09-16 05:54:15 UTC) #1
albert.strasheim
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 ...
7 years, 6 months ago (2012-09-16 06:34:53 UTC) #2
adg
Hello golang-dev@googlegroups.com, fullung@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 6 months ago (2012-09-17 17:16:38 UTC) #3
r
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, ...
7 years, 6 months ago (2012-09-17 18:09:59 UTC) #4
remyoudompheng
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 || ...
7 years, 6 months ago (2012-09-17 18:16:14 UTC) #5
adg
Hello golang-dev@googlegroups.com, fullung@gmail.com, r@golang.org, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 6 months ago (2012-09-17 18:16:46 UTC) #6
adg
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 ...
7 years, 6 months ago (2012-09-17 18:17:31 UTC) #7
adg
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 || ...
7 years, 6 months ago (2012-09-17 18:19:13 UTC) #8
adg
Hello golang-dev@googlegroups.com, fullung@gmail.com, r@golang.org, remyoudompheng@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
7 years, 6 months ago (2012-09-17 18:19:21 UTC) #9
minux1
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): > ...
7 years, 6 months ago (2012-09-17 18:22:26 UTC) #10
r
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: ...
7 years, 6 months ago (2012-09-17 18:23:31 UTC) #11
r
On Mon, Sep 17, 2012 at 11:22 AM, minux <minux.ma@gmail.com> wrote: > Could we use ...
7 years, 6 months ago (2012-09-17 18:24:14 UTC) #12
adg
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 ...
7 years, 6 months ago (2012-09-17 18:26:24 UTC) #13
gri
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 ...
7 years, 6 months ago (2012-09-17 19:35:31 UTC) #14
gri
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 ...
7 years, 6 months ago (2012-09-17 19:54:13 UTC) #15
rsc
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 ...
7 years, 6 months ago (2012-09-17 20:01:59 UTC) #16
adg
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 ...
7 years, 6 months ago (2012-09-17 20:17:49 UTC) #17
adg
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.
7 years, 6 months ago (2012-09-17 20:18:05 UTC) #18
rsc
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: > ...
7 years, 6 months ago (2012-09-17 20:29:28 UTC) #19
adg
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): > ...
7 years, 6 months ago (2012-09-17 22:32:37 UTC) #20
adg
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.
7 years, 6 months ago (2012-09-18 20:31:41 UTC) #21
rsc
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 ...
7 years, 6 months ago (2012-09-18 20:53:33 UTC) #22
adg
*** Submitted as http://code.google.com/p/go/source/detail?r=838111caecdd *** vet: add range variable misuse detection R=fullung, r, remyoudompheng, minux.ma, ...
7 years, 6 months ago (2012-09-18 21:22:31 UTC) #23
albert.strasheim
Hello This didn't survive our code. The file that blows it up has ranges like: ...
7 years, 6 months ago (2012-09-19 08:50:52 UTC) #24
dsymonds
I've reduced it to something like this: var x [2]int var f int for x[0], ...
7 years, 6 months ago (2012-09-19 13:00:07 UTC) #25
dsymonds
Fix: http://codereview.appspot.com/6540045
7 years, 6 months ago (2012-09-19 13:04:09 UTC) #26
adg
7 years, 6 months ago (2012-09-19 17:05:43 UTC) #27
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b