Merge pull request #1115 from nevkontakte/issue955

nevkontakte · web-flow · commit 3f8f90ca7731 · 2022-04-19T15:28:16.000+01:00
Propagate blocking function information through function literal calls.
diff --git a/compiler/analysis/info.go b/compiler/analysis/info.go
@@ -61,11 +61,12 @@ type Info struct {
 
 func (info *Info) newFuncInfo(n ast.Node) *FuncInfo {
 	funcInfo := &FuncInfo{
-		pkgInfo:      info,
-		Flattened:    make(map[ast.Node]bool),
-		Blocking:     make(map[ast.Node]bool),
-		GotoLabel:    make(map[*types.Label]bool),
-		localCallees: make(map[*types.Func][]astPath),
+		pkgInfo:            info,
+		Flattened:          make(map[ast.Node]bool),
+		Blocking:           make(map[ast.Node]bool),
+		GotoLabel:          make(map[*types.Label]bool),
+		localNamedCallees:  make(map[*types.Func][]astPath),
+		literalFuncCallees: make(map[*ast.FuncLit][]astPath),
 	}
 
 	// Register the function in the appropriate map.
@@ -128,15 +129,30 @@ func AnalyzePkg(files []*ast.File, fileSet *token.FileSet, typesInfo *types.Info
 	}
 
 	// Propagate information about blocking calls to the caller functions.
+	// For each function we check all other functions it may call and if any of
+	// them are blocking, we mark the caller blocking as well. The process is
+	// repeated until no new blocking functions is detected.
 	for {
 		done := true
 		for _, caller := range info.allInfos {
-			for callee, callSites := range caller.localCallees {
+			// Check calls to named functions and function-typed variables.
+			for callee, callSites := range caller.localNamedCallees {
 				if info.IsBlocking(callee) {
 					for _, callSite := range callSites {
 						caller.markBlocking(callSite)
 					}
-					delete(caller.localCallees, callee)
+					delete(caller.localNamedCallees, callee)
+					done = false
+				}
+			}
+
+			// Check direct calls to function literals.
+			for callee, callSites := range caller.literalFuncCallees {
+				if len(info.FuncLitInfos[callee].Blocking) > 0 {
+					for _, callSite := range callSites {
+						caller.markBlocking(callSite)
+					}
+					delete(caller.literalFuncCallees, callee)
 					done = false
 				}
 			}
@@ -177,9 +193,15 @@ type FuncInfo struct {
 	continueStmts []continueStmt
 	// List of return statements in the function.
 	returnStmts []astPath
-	// List of other functions from the current package this function calls. If
-	// any of them are blocking, this function will become blocking too.
-	localCallees map[*types.Func][]astPath
+	// List of other named functions from the current package this function calls.
+	// If any of them are blocking, this function will become blocking too.
+	localNamedCallees map[*types.Func][]astPath
+	// List of function literals directly called from this function (for example:
+	// `func() { /* do stuff */ }()`). This is distinct from function literals
+	// assigned to named variables (for example: `doStuff := func() {};
+	// doStuff()`), which are handled by localNamedCallees. If any of them are
+	// identified as blocking, this function will become blocking too.
+	literalFuncCallees map[*ast.FuncLit][]astPath
 
 	pkgInfo      *Info // Function's parent package.
 	visitorStack astPath
@@ -296,13 +318,13 @@ func (fi *FuncInfo) Visit(node ast.Node) ast.Visitor {
 func (fi *FuncInfo) visitCallExpr(n *ast.CallExpr) ast.Visitor {
 	switch f := astutil.RemoveParens(n.Fun).(type) {
 	case *ast.Ident:
-		fi.callTo(fi.pkgInfo.Uses[f])
+		fi.callToNamedFunc(fi.pkgInfo.Uses[f])
 	case *ast.SelectorExpr:
 		if sel := fi.pkgInfo.Selections[f]; sel != nil && typesutil.IsJsObject(sel.Recv()) {
 			// js.Object methods are known to be non-blocking, but we still must
 			// check its arguments.
 		} else {
-			fi.callTo(fi.pkgInfo.Uses[f.Sel])
+			fi.callToNamedFunc(fi.pkgInfo.Uses[f.Sel])
 		}
 	case *ast.FuncLit:
 		// Collect info about the function literal itself.
@@ -312,13 +334,8 @@ func (fi *FuncInfo) visitCallExpr(n *ast.CallExpr) ast.Visitor {
 		for _, arg := range n.Args {
 			ast.Walk(fi, arg)
 		}
-		// If the function literal is blocking, this function is blocking to.
-		// FIXME(nevkontakte): What if the function literal is calling a blocking
-		// function through several layers of indirection? This will only become
-		// known at a later stage of analysis.
-		if len(fi.pkgInfo.FuncLitInfos[f].Blocking) != 0 {
-			fi.markBlocking(fi.visitorStack)
-		}
+		// Register literal function call site in case it is identified as blocking.
+		fi.literalFuncCallees[f] = append(fi.literalFuncCallees[f], fi.visitorStack.copy())
 		return nil // No need to walk under this CallExpr, we already did it manually.
 	default:
 		if astutil.IsTypeExpr(f, fi.pkgInfo.Info) {
@@ -334,12 +351,12 @@ func (fi *FuncInfo) visitCallExpr(n *ast.CallExpr) ast.Visitor {
 	return fi
 }
 
-func (fi *FuncInfo) callTo(callee types.Object) {
+func (fi *FuncInfo) callToNamedFunc(callee types.Object) {
 	switch o := callee.(type) {
 	case *types.Func:
 		if recv := o.Type().(*types.Signature).Recv(); recv != nil {
 			if _, ok := recv.Type().Underlying().(*types.Interface); ok {
-				// Conservatively assume that an interfact implementation might be blocking.
+				// Conservatively assume that an interface implementation may be blocking.
 				fi.markBlocking(fi.visitorStack)
 				return
 			}
@@ -352,7 +369,7 @@ func (fi *FuncInfo) callTo(callee types.Object) {
 		}
 		// We probably don't know yet whether the callee function is blocking.
 		// Record the calls site for the later stage.
-		fi.localCallees[o] = append(fi.localCallees[o], fi.visitorStack.copy())
+		fi.localNamedCallees[o] = append(fi.localNamedCallees[o], fi.visitorStack.copy())
 	case *types.Var:
 		// Conservatively assume that a function in a variable might be blocking.
 		fi.markBlocking(fi.visitorStack)
diff --git a/compiler/analysis/info_test.go b/compiler/analysis/info_test.go
@@ -0,0 +1,79 @@
+package analysis
+
+import (
+	"go/ast"
+	"go/token"
+	"go/types"
+	"testing"
+
+	"github.com/gopherjs/gopherjs/internal/srctesting"
+)
+
+// See: https://github.com/gopherjs/gopherjs/issues/955.
+func TestBlockingFunctionLiteral(t *testing.T) {
+	src := `
+package test
+
+func blocking() {
+	c := make(chan bool)
+	<-c
+}
+
+func indirectlyBlocking() {
+	func() { blocking() }()
+}
+
+func directlyBlocking() {
+	func() {
+		c := make(chan bool)
+		<-c
+	}()
+}
+
+func notBlocking() {
+	func() { println() } ()
+}
+`
+	fset := token.NewFileSet()
+	file := srctesting.Parse(t, fset, src)
+	typesInfo, typesPkg := srctesting.Check(t, fset, file)
+
+	pkgInfo := AnalyzePkg([]*ast.File{file}, fset, typesInfo, typesPkg, func(f *types.Func) bool {
+		panic("isBlocking() should be never called for imported functions in this test.")
+	})
+
+	assertBlocking(t, file, pkgInfo, "blocking")
+	assertBlocking(t, file, pkgInfo, "indirectlyBlocking")
+	assertBlocking(t, file, pkgInfo, "directlyBlocking")
+	assertNotBlocking(t, file, pkgInfo, "notBlocking")
+}
+
+func assertBlocking(t *testing.T, file *ast.File, pkgInfo *Info, funcName string) {
+	typesFunc := getTypesFunc(t, file, pkgInfo, funcName)
+	if !pkgInfo.IsBlocking(typesFunc) {
+		t.Errorf("Got: %q is not blocking. Want: %q is blocking.", typesFunc, typesFunc)
+	}
+}
+
+func assertNotBlocking(t *testing.T, file *ast.File, pkgInfo *Info, funcName string) {
+	typesFunc := getTypesFunc(t, file, pkgInfo, funcName)
+	if pkgInfo.IsBlocking(typesFunc) {
+		t.Errorf("Got: %q is blocking. Want: %q is not blocking.", typesFunc, typesFunc)
+	}
+}
+
+func getTypesFunc(t *testing.T, file *ast.File, pkgInfo *Info, funcName string) *types.Func {
+	obj := file.Scope.Lookup(funcName)
+	if obj == nil {
+		t.Fatalf("Declaration of %q is not found in the AST.", funcName)
+	}
+	decl, ok := obj.Decl.(*ast.FuncDecl)
+	if !ok {
+		t.Fatalf("Got: %q is %v. Want: a function declaration.", funcName, obj.Kind)
+	}
+	blockingType, ok := pkgInfo.Defs[decl.Name]
+	if !ok {
+		t.Fatalf("No type information is found for %v.", decl.Name)
+	}
+	return blockingType.(*types.Func)
+}
diff --git a/compiler/astutil/astutil_test.go b/compiler/astutil/astutil_test.go
@@ -1,10 +1,10 @@
 package astutil
 
 import (
-	"go/ast"
-	"go/parser"
 	"go/token"
 	"testing"
+
+	"github.com/gopherjs/gopherjs/internal/srctesting"
 )
 
 func TestImportsUnsafe(t *testing.T) {
@@ -43,7 +43,7 @@ func TestImportsUnsafe(t *testing.T) {
 		t.Run(test.desc, func(t *testing.T) {
 			src := "package testpackage\n\n" + test.imports
 			fset := token.NewFileSet()
-			file := parse(t, fset, src)
+			file := srctesting.Parse(t, fset, src)
 			got := ImportsUnsafe(file)
 			if got != test.want {
 				t.Fatalf("ImportsUnsafe() returned %t, want %t", got, test.want)
@@ -74,7 +74,7 @@ func TestFuncKey(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
-			fdecl := parseFuncDecl(t, test.src)
+			fdecl := srctesting.ParseFuncDecl(t, test.src)
 			if got := FuncKey(fdecl); got != test.want {
 				t.Errorf("Got %q, want %q", got, test.want)
 			}
@@ -122,7 +122,7 @@ func TestPruneOriginal(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
-			fdecl := parseFuncDecl(t, test.src)
+			fdecl := srctesting.ParseFuncDecl(t, test.src)
 			if got := PruneOriginal(fdecl); got != test.want {
 				t.Errorf("PruneOriginal() returned %t, want %t", got, test.want)
 			}
@@ -173,34 +173,11 @@ func TestEndsWithReturn(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.desc, func(t *testing.T) {
-			fdecl := parseFuncDecl(t, "package testpackage\n"+test.src)
+			fdecl := srctesting.ParseFuncDecl(t, "package testpackage\n"+test.src)
 			got := EndsWithReturn(fdecl.Body.List)
 			if got != test.want {
 				t.Errorf("EndsWithReturn() returned %t, want %t", got, test.want)
 			}
 		})
 	}
 }
-
-func parse(t *testing.T, fset *token.FileSet, src string) *ast.File {
-	t.Helper()
-	f, err := parser.ParseFile(fset, "test.go", src, parser.ParseComments)
-	if err != nil {
-		t.Fatalf("Failed to parse test source: %s", err)
-	}
-	return f
-}
-
-func parseFuncDecl(t *testing.T, src string) *ast.FuncDecl {
-	t.Helper()
-	fset := token.NewFileSet()
-	file := parse(t, fset, src)
-	if l := len(file.Decls); l != 1 {
-		t.Fatalf("Got %d decls in the sources, expected exactly 1", l)
-	}
-	fdecl, ok := file.Decls[0].(*ast.FuncDecl)
-	if !ok {
-		t.Fatalf("Got %T decl, expected *ast.FuncDecl", file.Decls[0])
-	}
-	return fdecl
-}
diff --git a/internal/srctesting/srctesting.go b/internal/srctesting/srctesting.go
@@ -0,0 +1,65 @@
+// Package srctesting contains common helpers for unit testing source code
+// analysis and transformation.
+package srctesting
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"go/types"
+	"testing"
+)
+
+// Parse source from the string and return complete AST.
+//
+// Assumes source file name `test.go`. Fails the test on parsing error.
+func Parse(t *testing.T, fset *token.FileSet, src string) *ast.File {
+	t.Helper()
+	f, err := parser.ParseFile(fset, "test.go", src, parser.ParseComments)
+	if err != nil {
+		t.Fatalf("Failed to parse test source: %s", err)
+	}
+	return f
+}
+
+// Check type correctness of the provided AST.
+//
+// Assumes "test" package import path. Fails the test if type checking fails.
+// Provided AST is expected not to have any imports.
+func Check(t *testing.T, fset *token.FileSet, files ...*ast.File) (*types.Info, *types.Package) {
+	t.Helper()
+	typesInfo := &types.Info{
+		Types:      make(map[ast.Expr]types.TypeAndValue),
+		Defs:       make(map[*ast.Ident]types.Object),
+		Uses:       make(map[*ast.Ident]types.Object),
+		Implicits:  make(map[ast.Node]types.Object),
+		Selections: make(map[*ast.SelectorExpr]*types.Selection),
+		Scopes:     make(map[ast.Node]*types.Scope),
+	}
+	config := &types.Config{
+		Sizes: &types.StdSizes{WordSize: 4, MaxAlign: 8},
+	}
+	typesPkg, err := config.Check("test", fset, files, typesInfo)
+	if err != nil {
+		t.Fatalf("Filed to type check test source: %s", err)
+	}
+	return typesInfo, typesPkg
+}
+
+// ParseFuncDecl parses source with a single function defined and returns the
+// function AST.
+//
+// Fails the test if there isn't exactly one function declared in the source.
+func ParseFuncDecl(t *testing.T, src string) *ast.FuncDecl {
+	t.Helper()
+	fset := token.NewFileSet()
+	file := Parse(t, fset, src)
+	if l := len(file.Decls); l != 1 {
+		t.Fatalf("Got %d decls in the sources, expected exactly 1", l)
+	}
+	fdecl, ok := file.Decls[0].(*ast.FuncDecl)
+	if !ok {
+		t.Fatalf("Got %T decl, expected *ast.FuncDecl", file.Decls[0])
+	}
+	return fdecl
+}
