From ddc9c995c0816b62f0aa9dac71d25a9dbf784941 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Wed, 20 Mar 2024 17:20:18 +0100 Subject: [PATCH 1/9] chore: update x/tools to support recent Go versions --- go.mod | 5 ++--- go.sum | 12 +++++------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/go.mod b/go.mod index 666545f..a702fc9 100644 --- a/go.mod +++ b/go.mod @@ -4,11 +4,10 @@ go 1.20 require ( github.com/gostaticanalysis/analysisutil v0.7.1 - golang.org/x/tools v0.8.0 + golang.org/x/tools v0.19.0 ) require ( github.com/gostaticanalysis/comment v1.4.2 // indirect - golang.org/x/mod v0.10.0 // indirect - golang.org/x/sys v0.7.0 // indirect + golang.org/x/mod v0.16.0 // indirect ) diff --git a/go.sum b/go.sum index f213be7..898832e 100644 --- a/go.sum +++ b/go.sum @@ -24,20 +24,18 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/mod v0.4.1/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= -golang.org/x/mod v0.10.0 h1:lFO9qtOdlre5W1jxS3r/4szv2/6iXxScdzjoBMXNhYk= -golang.org/x/mod v0.10.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= +golang.org/x/mod v0.16.0 h1:QX4fJ0Rr5cPQCF7O9lh9Se4pmwfwskqZfq5moyldzic= +golang.org/x/mod v0.16.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20201021035429-f5854403a974/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20201020160332-67f06af15bc9/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o= +golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.7.0 h1:3jlCCIQZPdOYu1h8BkNvLz8Kgwtae2cagcG/VamtZRU= -golang.org/x/sys v0.7.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= golang.org/x/text v0.3.3 h1:cokOdA+Jmi5PJGXLlLllQSgYigAEfHXJAERHVMaCc2k= @@ -46,8 +44,8 @@ golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGm golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.1.1-0.20210205202024-ef80cdb6ec6d/go.mod h1:9bzcO0MWcOuT0tm1iBGzDVPshzfwoVvREIui8C+MHqU= golang.org/x/tools v0.1.1-0.20210302220138-2ac05c832e1a/go.mod h1:9bzcO0MWcOuT0tm1iBGzDVPshzfwoVvREIui8C+MHqU= -golang.org/x/tools v0.8.0 h1:vSDcovVPld282ceKgDimkRSC8kpaH1dgyc9UMzlt84Y= -golang.org/x/tools v0.8.0/go.mod h1:JxBZ99ISMI5ViVkT1tr6tdNmXeTrcpVSD3vZ1RsRdN4= +golang.org/x/tools v0.19.0 h1:tfGCXNR1OsFG+sVdLAitlpjAvD/I6dHDKnYrpEZUHkw= +golang.org/x/tools v0.19.0/go.mod h1:qoJWxmGSIBmAeriMx19ogtrEPrGtDbPK634QFIcLAhc= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= From 9dd73ac38ce7dd4a6bfbbe621a01705e2594165e Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Wed, 20 Mar 2024 17:22:03 +0100 Subject: [PATCH 2/9] chore: ignore binary --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index fc1b400..1c2ffa5 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,5 @@ .idea .DS_Store + +/contextcheck From d442b7c02209bedb02b819655e807054a4d737fa Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Wed, 20 Mar 2024 17:22:23 +0100 Subject: [PATCH 3/9] fix: don't report issue related to invalid position --- contextcheck.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/contextcheck.go b/contextcheck.go index c9ad010..f7a9579 100644 --- a/contextcheck.go +++ b/contextcheck.go @@ -455,6 +455,10 @@ func (r *runner) collectCtxRef(f *ssa.Function, isHttpHandler bool) (refMap map[ } for instr := range storeInstrs { + if !instr.Pos().IsValid() { + continue + } + if !checkedRefMap[instr.Val] { r.pass.Reportf(instr.Pos(), "Non-inherited new context, use function like `context.WithXXX` instead") ok = false @@ -462,6 +466,10 @@ func (r *runner) collectCtxRef(f *ssa.Function, isHttpHandler bool) (refMap map[ } for instr := range phiInstrs { + if !instr.Pos().IsValid() { + continue + } + for _, v := range instr.Edges { if !checkedRefMap[v] { r.pass.Reportf(instr.Pos(), "Non-inherited new context, use function like `context.WithXXX` instead") @@ -551,6 +559,10 @@ func (r *runner) checkFuncWithCtx(f *ssa.Function, tp entryType) { for _, b := range f.Blocks { for _, instr := range b.Instrs { + if !instr.Pos().IsValid() { + continue + } + tp, ok := r.getCtxType(instr) if !ok { continue From fbe3efab72987debc2629496ac9719b0549ab265 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Wed, 20 Mar 2024 18:05:41 +0100 Subject: [PATCH 4/9] tests: add tests --- Makefile | 14 ++++++++++++-- testdata/src/a/a.go | 8 ++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 9321e9d..613d35e 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,15 @@ +.PHONY: clean test build + +default: test build + +clean: + rm -rf dist/ cover.out + +test: clean + go test -v -cover ./... + build: - @GO111MODULE=on go build -ldflags '-s -w' -o contextcheck ./cmd/contextcheck/main.go + go build -ldflags '-s -w' -o contextcheck ./cmd/contextcheck/main.go install: - @GO111MODULE=on go install -ldflags '-s -w' ./cmd/contextcheck + go install -ldflags '-s -w' ./cmd/contextcheck diff --git a/testdata/src/a/a.go b/testdata/src/a/a.go index 4c12875..e51bcd4 100644 --- a/testdata/src/a/a.go +++ b/testdata/src/a/a.go @@ -155,3 +155,11 @@ func f13[T int | int8 | int16 | int32 | int64 | uint | uint8 | uint16 | uint32 | return b } + +/* ----------------- issue 21 ----------------- */ + +func f16(ctx context.Context, k string) func() { + return func() { + f16(context.Background(), k) + } +} From 7fdfd4d5fdae48ed479e0ccf5771bd03d0cc248e Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Thu, 21 Mar 2024 16:09:59 +0100 Subject: [PATCH 5/9] feat: use parent position if current position invalid --- contextcheck.go | 35 ++++++++++++++++++----------------- testdata/src/a/a.go | 2 +- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/contextcheck.go b/contextcheck.go index f7a9579..35806a7 100644 --- a/contextcheck.go +++ b/contextcheck.go @@ -455,24 +455,16 @@ func (r *runner) collectCtxRef(f *ssa.Function, isHttpHandler bool) (refMap map[ } for instr := range storeInstrs { - if !instr.Pos().IsValid() { - continue - } - if !checkedRefMap[instr.Val] { - r.pass.Reportf(instr.Pos(), "Non-inherited new context, use function like `context.WithXXX` instead") + r.Reportf(instr, "Non-inherited new context, use function like `context.WithXXX` instead") ok = false } } for instr := range phiInstrs { - if !instr.Pos().IsValid() { - continue - } - for _, v := range instr.Edges { if !checkedRefMap[v] { - r.pass.Reportf(instr.Pos(), "Non-inherited new context, use function like `context.WithXXX` instead") + r.Reportf(instr, "Non-inherited new context, use function like `context.WithXXX` instead") ok = false } } @@ -559,10 +551,6 @@ func (r *runner) checkFuncWithCtx(f *ssa.Function, tp entryType) { for _, b := range f.Blocks { for _, instr := range b.Instrs { - if !instr.Pos().IsValid() { - continue - } - tp, ok := r.getCtxType(instr) if !ok { continue @@ -576,9 +564,9 @@ func (r *runner) checkFuncWithCtx(f *ssa.Function, tp entryType) { if tp&CtxIn != 0 { if !refMap[instr] { if isHttpHandler { - r.pass.Reportf(instr.Pos(), "Non-inherited new context, use function like `context.WithXXX` or `r.Context` instead") + r.Reportf(instr, "Non-inherited new context, use function like `context.WithXXX` or `r.Context` instead") } else { - r.pass.Reportf(instr.Pos(), "Non-inherited new context, use function like `context.WithXXX` instead") + r.Reportf(instr, "Non-inherited new context, use function like `context.WithXXX` instead") } } } @@ -592,13 +580,26 @@ func (r *runner) checkFuncWithCtx(f *ssa.Function, tp entryType) { res, ok := r.getValue(key, ff) if ok { if !res.Valid { - r.pass.Reportf(instr.Pos(), "Function `%s` should pass the context parameter", strings.Join(reverse(res.Funcs), "->")) + r.Reportf(instr, "Function `%s` should pass the context parameter", strings.Join(reverse(res.Funcs), "->")) } } } } } +func (r *runner) Reportf(instr ssa.Instruction, format string, args ...interface{}) { + pos := instr.Pos() + if !pos.IsValid() && instr.Parent() != nil { + pos = instr.Parent().Pos() + } + + if !pos.IsValid() { + return + } + + r.pass.Reportf(pos, format, args...) +} + func (r *runner) checkFuncWithoutCtx(f *ssa.Function, checkingMap map[string]bool) (ret bool) { ret = true orgKey := f.RelString(nil) diff --git a/testdata/src/a/a.go b/testdata/src/a/a.go index e51bcd4..7758690 100644 --- a/testdata/src/a/a.go +++ b/testdata/src/a/a.go @@ -158,7 +158,7 @@ func f13[T int | int8 | int16 | int32 | int64 | uint | uint8 | uint16 | uint32 | /* ----------------- issue 21 ----------------- */ -func f16(ctx context.Context, k string) func() { +func f16(ctx context.Context, k string) func() { // want "Function `f16\\$1` should pass the context parameter" return func() { f16(context.Background(), k) } From 3cc6ce5912f65c9b9f7fd9921d51af3a1cde3558 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Thu, 21 Mar 2024 21:24:53 +0100 Subject: [PATCH 6/9] feat: improve position for closure --- contextcheck.go | 16 ++++++++++++++++ testdata/src/a/a.go | 4 ++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/contextcheck.go b/contextcheck.go index 35806a7..dac8d49 100644 --- a/contextcheck.go +++ b/contextcheck.go @@ -589,6 +589,22 @@ func (r *runner) checkFuncWithCtx(f *ssa.Function, tp entryType) { func (r *runner) Reportf(instr ssa.Instruction, format string, args ...interface{}) { pos := instr.Pos() + + if !pos.IsValid() && instr.Block() != nil { + if closure, ok := instr.(*ssa.MakeClosure); ok { + for _, in := range closure.Block().Instrs { + if !in.Pos().IsValid() { + continue + } + + if r, ok := in.(*ssa.Return); ok { + pos = r.Pos() + break + } + } + } + } + if !pos.IsValid() && instr.Parent() != nil { pos = instr.Parent().Pos() } diff --git a/testdata/src/a/a.go b/testdata/src/a/a.go index 7758690..d927548 100644 --- a/testdata/src/a/a.go +++ b/testdata/src/a/a.go @@ -158,8 +158,8 @@ func f13[T int | int8 | int16 | int32 | int64 | uint | uint8 | uint16 | uint32 | /* ----------------- issue 21 ----------------- */ -func f16(ctx context.Context, k string) func() { // want "Function `f16\\$1` should pass the context parameter" - return func() { +func f16(ctx context.Context, k string) func() { + return func() { // want "Function `f16\\$1` should pass the context parameter" f16(context.Background(), k) } } From b08c73506c8f2688bf5e3706d4dde0c97ed7527d Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Fri, 22 Mar 2024 15:49:11 +0100 Subject: [PATCH 7/9] tests: add tests about closure of closure --- testdata/src/a/a.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/testdata/src/a/a.go b/testdata/src/a/a.go index d927548..adb8f85 100644 --- a/testdata/src/a/a.go +++ b/testdata/src/a/a.go @@ -163,3 +163,11 @@ func f16(ctx context.Context, k string) func() { f16(context.Background(), k) } } + +func f17(ctx context.Context, k string) func() func() { + return func() func() { // want "Function `f17\\$1->f17\\$1\\$1` should pass the context parameter" + return func() { + f16(context.Background(), k) + } + } +} From 925e56d7505d9565533998ce4cb02148abd78082 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Fri, 22 Mar 2024 17:43:37 +0100 Subject: [PATCH 8/9] chore: simplify --- contextcheck.go | 55 ++++++++++++++++++++++--------------------------- 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/contextcheck.go b/contextcheck.go index dac8d49..5bdb081 100644 --- a/contextcheck.go +++ b/contextcheck.go @@ -2,6 +2,7 @@ package contextcheck import ( "go/ast" + "go/token" "go/types" "regexp" "strconv" @@ -68,6 +69,11 @@ var ( pkgFactMu sync.RWMutex ) +type element interface { + Pos() token.Pos + Parent() *ssa.Function +} + type resInfo struct { Valid bool Funcs []string @@ -580,42 +586,17 @@ func (r *runner) checkFuncWithCtx(f *ssa.Function, tp entryType) { res, ok := r.getValue(key, ff) if ok { if !res.Valid { - r.Reportf(instr, "Function `%s` should pass the context parameter", strings.Join(reverse(res.Funcs), "->")) + if instr.Pos().IsValid() { + r.Reportf(instr, "Function `%s` should pass the context parameter", strings.Join(reverse(res.Funcs), "->")) + } else { + r.Reportf(ff, "Function `%s` should pass the context parameter", strings.Join(reverse(res.Funcs), "->")) + } } } } } } -func (r *runner) Reportf(instr ssa.Instruction, format string, args ...interface{}) { - pos := instr.Pos() - - if !pos.IsValid() && instr.Block() != nil { - if closure, ok := instr.(*ssa.MakeClosure); ok { - for _, in := range closure.Block().Instrs { - if !in.Pos().IsValid() { - continue - } - - if r, ok := in.(*ssa.Return); ok { - pos = r.Pos() - break - } - } - } - } - - if !pos.IsValid() && instr.Parent() != nil { - pos = instr.Parent().Pos() - } - - if !pos.IsValid() { - return - } - - r.pass.Reportf(pos, format, args...) -} - func (r *runner) checkFuncWithoutCtx(f *ssa.Function, checkingMap map[string]bool) (ret bool) { ret = true orgKey := f.RelString(nil) @@ -835,6 +816,20 @@ func (r *runner) setFact(key string, valid bool, funcs ...string) { } } +func (r *runner) Reportf(instr element, format string, args ...interface{}) { + pos := instr.Pos() + + if !pos.IsValid() && instr.Parent() != nil { + pos = instr.Parent().Pos() + } + + if !pos.IsValid() { + return + } + + r.pass.Reportf(pos, format, args...) +} + // setPkgFact save fact to mem func setPkgFact(pkg *types.Package, fact ctxFact) { pkgFactMu.Lock() From 9f1f2d196d6a1ac24fcb3b622174310d7913bda8 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Fri, 22 Mar 2024 17:47:45 +0100 Subject: [PATCH 9/9] chore: simplify again --- contextcheck.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/contextcheck.go b/contextcheck.go index 5bdb081..50e11f3 100644 --- a/contextcheck.go +++ b/contextcheck.go @@ -584,13 +584,11 @@ func (r *runner) checkFuncWithCtx(f *ssa.Function, tp entryType) { key := ff.RelString(nil) res, ok := r.getValue(key, ff) - if ok { - if !res.Valid { - if instr.Pos().IsValid() { - r.Reportf(instr, "Function `%s` should pass the context parameter", strings.Join(reverse(res.Funcs), "->")) - } else { - r.Reportf(ff, "Function `%s` should pass the context parameter", strings.Join(reverse(res.Funcs), "->")) - } + if ok && !res.Valid { + if instr.Pos().IsValid() { + r.Reportf(instr, "Function `%s` should pass the context parameter", strings.Join(reverse(res.Funcs), "->")) + } else { + r.Reportf(ff, "Function `%s` should pass the context parameter", strings.Join(reverse(res.Funcs), "->")) } } }