Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add defer in loop checker #1145

Merged
merged 3 commits into from Oct 26, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions checkers/checkers_test.go
Expand Up @@ -136,6 +136,7 @@ func TestStableList(t *testing.T) {
"unslice",
"valSwap",
"wrapperFunc",
"deferInLoop",
peakle marked this conversation as resolved.
Show resolved Hide resolved
}

m := make(map[string]bool)
Expand Down
88 changes: 88 additions & 0 deletions checkers/deferInLoop_checker.go
@@ -0,0 +1,88 @@
package checkers

import (
"go/ast"

"golang.org/x/tools/go/ast/astutil"

"github.com/go-critic/go-critic/checkers/internal/astwalk"
"github.com/go-critic/go-critic/framework/linter"
)

func init() {
var info linter.CheckerInfo
info.Name = "deferInLoop"
info.Tags = []string{"diagnostic"}
quasilyte marked this conversation as resolved.
Show resolved Hide resolved
info.Summary = "Detects cycles inside functions that use defer"
peakle marked this conversation as resolved.
Show resolved Hide resolved
info.Before = `
for _, filename := range []string{"kek", "shrek"} {
peakle marked this conversation as resolved.
Show resolved Hide resolved
f, err := os.Open(filename)

defer f.Close()
}
`
info.After = `
func process(filename string) {
f, err := os.Open(filename)

defer f.Close()
}
...
...
peakle marked this conversation as resolved.
Show resolved Hide resolved
for _, filename := range []string{"kek", "shrek"} {
process(filename)
}`

collection.AddChecker(&info, func(ctx *linter.CheckerContext) (linter.FileWalker, error) {
return astwalk.WalkerForFuncDecl(&deferInLoopChecker{ctx: ctx}), nil
})
}

type deferInLoopChecker struct {
astwalk.WalkHandler
ctx *linter.CheckerContext
}

func (c *deferInLoopChecker) VisitFuncDecl(fn *ast.FuncDecl) {
// TODO: add func args check
// example: t.Run(123, func() { for { defer println(); break } })
var blockParser func(*ast.BlockStmt, bool)
blockParser = func(block *ast.BlockStmt, inFor bool) {
for _, cur := range block.List {
switch n := cur.(type) {
case *ast.DeferStmt:
if inFor {
c.warn(n)
}
case *ast.RangeStmt:
blockParser(n.Body, true)
case *ast.ForStmt:
blockParser(n.Body, true)
case *ast.GoStmt:
if f, ok := n.Call.Fun.(*ast.FuncLit); ok {
blockParser(f.Body, false)
}
case *ast.ExprStmt:
if f, ok := n.X.(*ast.CallExpr); ok {
if anon, ok := f.Fun.(*ast.FuncLit); ok {
blockParser(anon.Body, false)
}
}
case *ast.BlockStmt:
blockParser(n, inFor)
}
}
}
pre := func(cur *astutil.Cursor) bool {
if n, ok := cur.Node().(*ast.BlockStmt); ok {
blockParser(n, false)
}
return false
}

astutil.Apply(fn.Body, pre, nil)
}

func (c *deferInLoopChecker) warn(cause *ast.DeferStmt) {
c.ctx.Warn(cause, "Possible resource leak, 'defer' is called in the 'for' loop")
}
112 changes: 112 additions & 0 deletions checkers/testdata/deferInLoop/negative_tests.go
@@ -0,0 +1,112 @@
package checker_test

func testForStmt() {
defer println(111)

for range []int{1, 2, 3, 4} {
println(222)
break
}

defer println(222)
}

func testRangeStmt() {
defer println(222)

for i := 0; i < 10; i++ {
println(111)
}

defer println(222)
}

func testClosure() {
func() {
for {
break
}
defer println(1)

for {
for {
break
}
break
}

defer println(1)
}()

func() {
defer println(123)

for range []int{1, 2, 3, 4} {
println(222)
}

defer println(123)

for range []int{1, 2, 3, 4} {
for range []int{1, 2, 3, 4} {
println(222)
}
}

defer println(123)
}()

for {
func() {
defer println(123)
}()
break
}

for {
go func() {
defer println()
}()

break
}
}

func testBlock() {
{
for {
func() {
defer println()
}()
break
}
}
{
for {
go func() {
defer println()
}()
break
}
}
{
for range []int{1, 2, 3, 4} {
go func() {
defer println()
}()
break
}
}
{
for range []int{1, 2, 3, 4} {
{
func() {
{
defer println()
}
}()
}
break
}
}
}
180 changes: 180 additions & 0 deletions checkers/testdata/deferInLoop/positive_tests.go
@@ -0,0 +1,180 @@
package checker_test

import "fmt"

func deferWithCall() {
for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer fmt.Println("test")
break
}

for range []int{1, 2, 3, 4} {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer fmt.Println("test")
}
}

func deferWithClosure() {
for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer func() {}()

break
}

for range []int{1, 2, 3, 4} {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer func() {}()
}
}

func innerLoops() {
for {
for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer func() {}()

break
}
break
}

for {
for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer fmt.Println(123)

break
}
break
}

for range []int{1, 2, 3, 4} {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer func() {}()

for range []int{1, 2, 3, 4} {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer func() {}()
}
}
}

func anonFunc() {
func() {
for range []int{1, 2, 3, 4} {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer func() {}()

for range []int{1, 2, 3, 4} {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer func() {}()
}
}
}()

for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer println(123)
break
}

func() {
for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer func() {}()

for range []int{1, 2, 3, 4} {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer func() {}()
}

break
}

for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer func() {}()
break
}
}()

go func() {
for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer println(123)
break
}
}()
}

func contextBlock() {
{
go func() {
for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer println(123)
break
}
}()
}

{
func() {
for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer println(123)
break
}
}()
}

{
{
func() {
for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer println(123)
break
}
}()
}
}

{
for {
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer println(123)
break
}
}

for {
{
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer println(123)
break
}
}

for range []int{1, 2, 3} {
{
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer println(123)
break
}
}

go func() {
{
for {
{
/*! Possible resource leak, 'defer' is called in the 'for' loop */
defer println(123)
break
}
}
}
}()
}