If you open a session either return it or close it (#16)

If you call `sess := x.NewSession()` you must call `sess.Close()` or return the session.

This is not quite completely perfect for example if you return the output of x.NewSession() immediately it will falsely claim that you're not closing it. However it shows the beginnings of how to do it.

Signed-off-by: Andrew Thornton <art27@cantab.net>

close #15

Reviewed-on: https://gitea.com/gitea/gitea-vet/pulls/16
Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com>
Reviewed-by: 6543 <6543@obermui.de>
Co-authored-by: Andrew Thornton <art27@cantab.net>
Co-committed-by: Andrew Thornton <art27@cantab.net>

checks/models.go

@@ -5,7 +5,11 @@
 package checks
 
 import (
+	"bytes"
 	"errors"
+	"go/ast"
+	"go/format"
+	"go/token"
 	"os/exec"
 	"strings"
 
@@ -18,6 +22,12 @@ var Models = &analysis.Analyzer{
 	Run:  checkModels,
 }
 
+var ModelsSession = &analysis.Analyzer{
+	Name: "modelssession",
+	Doc:  "check models for misuse of session.",
+	Run:  checkModelsSession,
+}
+
 var (
 	modelsImpBlockList = []string{
 		"code.gitea.io/gitea/modules/git",
@@ -49,3 +59,186 @@ func checkModels(pass *analysis.Pass) (interface{}, error) {
 
 	return nil, nil
 }
+
+func checkModelsSession(pass *analysis.Pass) (interface{}, error) {
+	if !strings.EqualFold(pass.Pkg.Path(), "code.gitea.io/gitea/models") {
+		return nil, nil
+	}
+
+	for _, file := range pass.Files {
+		for _, decl := range file.Decls {
+			// We only care about function declarations
+			fnDecl, ok := decl.(*ast.FuncDecl)
+			if !ok {
+				continue
+			}
+
+			fnname := formatFunctionName(fnDecl)
+
+			// OK now we to step through each line in the function and ensure that if we open a session we close it or return the session
+			w := walker{
+				fname:  file.Name.String(),
+				fnname: fnname,
+				pass:   pass,
+			}
+			ast.Walk(&w, fnDecl.Body)
+
+			// Finally we may have a named return so we need to check if the session is returned as one of these
+			if w.HasUnclosedSession() && w.sessionName != "" {
+				w.closesSession = fnDeclHasNamedReturn(fnDecl, w.sessionName)
+			}
+
+			if w.HasUnclosedSession() {
+				pass.Reportf(fnDecl.Pos(), "%s opens session but does not close it", fnname)
+			}
+		}
+	}
+
+	return nil, nil
+}
+
+// fnDeclHasNamedReturn checks if the function declaration has a named return with the provided name
+func fnDeclHasNamedReturn(fnDecl *ast.FuncDecl, name string) bool {
+	if fnDecl.Type.Results == nil {
+		return false
+	}
+	for _, result := range fnDecl.Type.Results.List {
+		if len(result.Names) != 1 {
+			continue
+		}
+		if result.Names[0].Name == name {
+			return true
+		}
+	}
+	return false
+}
+
+// formatNode is a convenience function for printing a node
+func formatNode(node ast.Node) string {
+	buf := new(bytes.Buffer)
+	_ = format.Node(buf, token.NewFileSet(), node)
+	return buf.String()
+}
+
+// formatFunctionName returns the function name as called by the source
+func formatFunctionName(fnDecl *ast.FuncDecl) string {
+	fnname := fnDecl.Name.Name
+	if fnDecl.Recv != nil && fnDecl.Recv.List[fnDecl.Recv.NumFields()-1] != nil {
+		ns := formatNode(fnDecl.Recv.List[fnDecl.Recv.NumFields()-1].Type)
+		if ns[0] == '*' {
+			ns = ns[1:]
+		}
+		fnname = ns + "." + fnname
+	}
+	return fnname
+}
+
+// walker looks for unclosed sessions
+type walker struct {
+	fname          string
+	fnname         string
+	pass           *analysis.Pass
+	createsSession bool
+	closesSession  bool
+	sessionName    string
+}
+
+func (w *walker) HasUnclosedSession() bool {
+	return w.createsSession && !w.closesSession
+}
+
+func (w *walker) Visit(node ast.Node) ast.Visitor {
+	if node == nil {
+		return nil
+	}
+
+	switch t := node.(type) {
+
+	case *ast.ExprStmt:
+		if isCloseSessionExpr(t.X, w.sessionName) {
+			w.closesSession = true
+			return nil
+		}
+	case *ast.AssignStmt:
+		if len(t.Lhs) != 1 && len(t.Rhs) != 1 {
+			break
+		}
+
+		name, ok := t.Lhs[0].(*ast.Ident)
+		if !ok {
+			break
+		}
+
+		if isNewSession(t.Rhs[0]) {
+			w.createsSession = true
+			w.sessionName = name.Name
+			return nil
+		}
+		if isCloseSessionExpr(t.Rhs[0], w.sessionName) {
+			w.closesSession = true
+			return nil
+		}
+	case *ast.DeferStmt:
+		if isCloseSessionExpr(t.Call, w.sessionName) {
+			w.closesSession = true
+			return nil
+		}
+	case *ast.ReturnStmt:
+		for _, expr := range t.Results {
+			id, ok := expr.(*ast.Ident)
+			if !ok {
+				continue
+			}
+			if w.sessionName != "" && id.Name == w.sessionName {
+				w.closesSession = true
+			}
+		}
+	}
+
+	return w
+}
+
+// isCloseSessionExpr checks whether a provided expression represents a call to sess.Close
+func isCloseSessionExpr(expr ast.Expr, name string) bool {
+	if name == "" {
+		return false
+	}
+	call, ok := expr.(*ast.CallExpr)
+	if ok {
+		expr = call.Fun
+	}
+	sel, ok := expr.(*ast.SelectorExpr)
+	if !ok {
+		return false
+	}
+	id, ok := sel.X.(*ast.Ident)
+	if !ok {
+		return false
+	}
+
+	if id.Name != name || sel.Sel.Name != "Close" {
+		return false
+	}
+
+	return true
+}
+
+// isNewSession checks whether a provided expression represents a call to x.NewSession()
+func isNewSession(expr ast.Expr) bool {
+	value, ok := expr.(*ast.CallExpr)
+	if !ok {
+		return false
+	}
+	sel, ok := value.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return false
+	}
+	id, ok := sel.X.(*ast.Ident)
+	if !ok {
+		return false
+	}
+	if id.Name != "x" || sel.Sel.Name != "NewSession" {
+		return false
+	}
+	return true
+}

main.go

@@ -15,5 +15,6 @@ func main() {
 		checks.Imports,
 		checks.License,
 		checks.Migrations,
+		checks.ModelsSession,
 	)
 }