Merge pull request #255 from justinfx/254_err_check

bind: Check for exceptions and propagate errors back to python caller

Author: rcoreilly

README.md

@@ -179,7 +179,7 @@ package hi exposes a few Go functions to be wrapped and used from Python.
 ```sh
 $ gopy build -output=out -vm=`which python3` github.com/go-python/gopy/_examples/hi
 $ ls out
-Makefile  __init__.py  __pycache__/  _hi.so*  build.py  go.py  hi.c  hi.go  hi.py  hi_go.h  hi_go.so  patch-leaks.go
+Makefile  __init__.py  __pycache__/  _hi.so*  build.py  go.py  hi.c  hi.go  hi.py  hi_go.h  hi_go.so
 ```
 
 

_examples/pyerrors/pyerrors.go

@@ -5,7 +5,10 @@
 // Package pyerrors holds functions returning an error.
 package pyerrors
 
-import "errors"
+import (
+	"errors"
+	"fmt"
+)
 
 // Div is a function for detecting errors.
 func Div(i, j int) (int, error) {
@@ -14,3 +17,18 @@ func Div(i, j int) (int, error) {
 	}
 	return i / j, nil
 }
+
+type Stringer fmt.Stringer
+type MyString string
+
+func (t MyString) String() string { return string(t) }
+
+// NewMyString converts a string to a custom MyString type.
+// It is an error to pass an empty string value.
+func NewMyString(val string) (stringer Stringer, err error) {
+	if val == "" {
+		err = errors.New("Empty string value.")
+		return
+	}
+	return MyString(val), nil
+}

_examples/pyerrors/test.py

@@ -14,7 +14,19 @@ def div(a, b):
     except Exception as e:
         print(e)
 
-div(5,0)
+
+def new_mystring(s):
+    try:
+        ms = pyerrors.NewMyString(s)
+        print('pyerrors.NewMyString("%s") = "%s"'% (s, ms.String()))
+    except Exception as e:
+        print(e)
+
+
+div(5,0)  # error
 div(5,2)
 
+new_mystring("")  # error
+new_mystring("hello")
+
 print("OK")

bind/gen.go

@@ -203,9 +203,43 @@ func GoPyMainRun() {
 # File is generated by gopy. Do not edit.
 # %[2]s
 
-from pybindgen import retval, param, Module
+from pybindgen import retval, param, Function, Module
 import sys
 
+class CheckedFunction(Function):
+    def __init__(self, *a, **kw):
+        super(CheckedFunction, self).__init__(*a, **kw)
+        self._failure_expression = kw.get('failure_expression', '')
+        self._failure_cleanup = kw.get('failure_cleanup', '')
+
+    def set_failure_expression(self, expr):
+        self._failure_expression = expr
+
+    def set_failure_cleanup(self, expr):
+        self._failure_cleanup = expr
+
+    def generate_call(self):
+        super(CheckedFunction, self).generate_call()
+        check = "PyErr_Occurred()"
+        if self._failure_expression:
+            check = "{} && {}".format(self._failure_expression, check)
+        failure_cleanup = self._failure_cleanup or None
+        self.before_call.write_error_check(check, failure_cleanup)
+
+def add_checked_function(mod, name, retval, params, failure_expression='', *a, **kw):
+    fn = CheckedFunction(name, retval, params, *a, **kw)
+    fn.set_failure_expression(failure_expression)
+    mod._add_function_obj(fn)
+    return fn
+
+def add_checked_string_function(mod, name, retval, params, failure_expression='', *a, **kw):
+    fn = CheckedFunction(name, retval, params, *a, **kw)
+    fn.set_failure_cleanup('if (retval != NULL) free(retval);')
+    fn.after_call.add_cleanup_code('free(retval);')
+    fn.set_failure_expression(failure_expression)
+    mod._add_function_obj(fn)
+    return fn
+
 mod = Module('_%[1]s')
 mod.add_include('"%[1]s_go.h"')
 mod.add_function('GoPyInit', None, [])
@@ -226,6 +260,11 @@ mod.add_function('NumHandles', retval('int'), [])
 
 # the following is required to enable dlopen to open the _go.so file
 import os,sys,inspect,collections
+try:
+	import collections.abc as _collections_abc
+except ImportError:
+	_collections_abc = collections
+
 cwd = os.getcwd()
 currentdir = os.path.dirname(os.path.abspath(inspect.getfile(inspect.currentframe())))
 os.chdir(currentdir)
@@ -250,6 +289,10 @@ os.chdir(cwd)
 # %[2]s
 
 import collections
+try:
+	import collections.abc as _collections_abc
+except ImportError:
+	_collections_abc = collections
 %[6]s
 
 # to use this code in your end-user python file, import it as follows:
@@ -263,6 +306,10 @@ import collections
 
 	GoPkgDefs = `
 import collections
+try:
+	import collections.abc as _collections_abc
+except ImportError:
+	_collections_abc = collections
 	
 class GoClass(object):
 	"""GoClass is the base class for all GoPy wrapper classes"""
@@ -317,8 +364,6 @@ build:
 	# use pybindgen to build the %[1]s.c file which are the CPython wrappers to cgo wrappers..
 	# note: pip install pybindgen to get pybindgen if this fails
 	$(PYTHON) build.py
-	# patch storage leaks in pybindgen output
-	go run patch-leaks.go %[1]s.c
 	# build the _%[1]s$(LIBEXT) library that contains the cgo and CPython wrappers
 	# generated %[1]s.py python wrapper imports this c-code package
 	$(GCC) %[1]s.c %[6]s %[1]s_go$(LIBEXT) -o _%[1]s$(LIBEXT) $(CFLAGS) $(LDFLAGS) -fPIC --shared -w
@@ -360,91 +405,11 @@ build:
 	# use pybindgen to build the %[1]s.c file which are the CPython wrappers to cgo wrappers..
 	# note: pip install pybindgen to get pybindgen if this fails
 	$(PYTHON) build.py
-	# patch storage leaks in pybindgen output
-	go run patch-leaks.go %[1]s.c
 	# build the executable
 	- rm %[1]s_go$(LIBEXT)
 	$(GOBUILD) -o py%[1]s
 	
 `
-
-	patchLeaksPreamble = `// patch-leaks.go for post-processing %[1]s.
-// File is generated by gopy. Do not edit.
-// %[2]s
-// +build ignore
-
-package main
-
-import (
-	"bufio"
-	"bytes"
-	"io/ioutil"
-	"log"
-	"os"
-	"strings"
-)
-
-const (
-	cStringLine = "    py_retval = Py_BuildValue((char *) \"s\", retval);"
-)
-var cstringFunctions = []string{
-`
-
-	patchLeaksPostamble = `
-}
-
-func isCString(line string, names []string) bool {
-	for _, cfn := range names {
-		if strings.HasPrefix(line, cfn) {
-			return true
-		}
-	}
-	return false
-}
-
-func patchCString(line string, out *bytes.Buffer) bool {
-	out.WriteString(line)
-	out.Write([]byte{'\n'})
-	switch line {
-	case "}":
-		return false
-	case cStringLine:
-		out.WriteString("    free(retval);\n")
-		return false
-	}
-	return true
-}
-
-func main() {
-	file := os.Args[1]
-	buf, err := ioutil.ReadFile(file)
-	if err != nil {
-		log.Fatal(err)
-	}
-	sc := bufio.NewScanner(bytes.NewBuffer(buf))
-	obuf := &bytes.Buffer{}
-	var cstring bool
-	for sc.Scan() {
-		line := sc.Text()
-		if cstring {
-			cstring = patchCString(line, obuf)
-			continue
-		}
-		cstring = isCString(line, cstringFunctions)
-		obuf.WriteString(line)
-		obuf.Write([]byte{'\n'})
-	}
-
-	if err := sc.Err(); err != nil {
-		log.Fatal(err)
-	}
-
-	err = ioutil.WriteFile(file, obuf.Bytes(), 0644)
-	if err != nil {
-		log.Fatal(err)
-	}
-}
- `
 )
 
 // thePyGen is the current pyGen which is needed in symbols to lookup
@@ -536,7 +501,6 @@ func (g *pyGen) genPre() {
 		g.makefile = &printer{buf: new(bytes.Buffer), indentEach: []byte("\t")}
 	}
 	g.genGoPreamble()
-	g.genLeaksPreamble()
 	g.genPyBuildPreamble()
 	if !NoMake {
 		g.genMakefile()
@@ -559,9 +523,7 @@ func (g *pyGen) genPrintOut(outfn string, pr *printer) {
 func (g *pyGen) genOut() {
 	g.pybuild.Printf("\nmod.generate(open('%v.c', 'w'))\n\n", g.cfg.Name)
 	g.gofile.Printf("\n\n")
-	g.genLeaksPostamble()
 	g.genPrintOut(g.cfg.Name+".go", g.gofile)
-	g.genPrintOut("patch-leaks.go", g.leakfile)
 	g.genPrintOut("build.py", g.pybuild)
 	if !NoMake {
 		g.makefile.Printf("\n\n")
@@ -635,14 +597,6 @@ func (g *pyGen) genGoPreamble() {
 	g.gofile.Printf("\n// --- generated code for package: %[1]s below: ---\n\n", g.cfg.Name)
 }
 
-func (g *pyGen) genLeaksPreamble() {
-	g.leakfile.Printf(patchLeaksPreamble, g.cfg.Name, g.cfg.Cmd)
-}
-
-func (g *pyGen) genLeaksPostamble() {
-	g.leakfile.Printf(patchLeaksPostamble)
-}
-
 func (g *pyGen) genPyBuildPreamble() {
 	g.pybuild.Printf(PyBuildPreamble, g.cfg.Name, g.cfg.Cmd)
 }

bind/gen_func.go

@@ -10,51 +10,10 @@ import (
 	"strings"
 )
 
-// genPatchLeakFuncName outputs the name of either the function or the class
-// 'getter' represented by its arguments that need to have a memory
-// leak induced by the interaction of pybindgen and gopy.
-// These leaks occur because the cgo bindings use C.CString
-// to allocate a string on the C heap, as in:
-//
-//	return C.CString(cstrings.StringValue(C.GoString(s), int(n)))
-//
-// but the calling pybindgen code then takes a copy of this
-// newly allocated string indirectly by calling Py_BuildValue (see below)
-// without freeing the copy returned by the function or getter method.
-// Arguably pybindgen should provide a means of directly requesting
-// this behaviour (simularly to the transfer_ownership options it provides),
-// but as of now there is no means of doing so. As a work around the
-// output of pybindgen is modified to insert the free in the appropriate
-// functions identified by genPatchLeakFuncName.
-//
-// The patch-leaks.go (see gen.go) file patches these code fragments
-// to insert free(retval) immediately after the call to PyBuildValue.
-// The patch-leaks.go generated file will perform this patch for
-// functions whose names are embedded in it by genPatchLeakFuncName.
-//
-//  .....
-//  retval = cstrings_StringValue(s, n);
-//  py_retval = Py_BuildValue((char *) "s", retval);
-//  .....
-//
-// This patching is required for top level functions that return
-// strings directly and also for getters that return strings that
-// are embedded in maps, structs etc.
-// NOTE that it's ok to output the same function name multiple times (eg.
-// for a shared structs).
-func (g *pyGen) genPatchLeakFuncName(ret *Var, fsym *Func) {
-	if ret == nil || fsym == nil || ret.GoType() == nil {
-		return
-	}
-	g.recurse(ret.GoType(), "_wrap__"+fsym.pkg.Name()+"_", fsym.ID())
-}
-
 func (g *pyGen) recurse(gotype types.Type, prefix, name string) {
 	switch t := gotype.(type) {
 	case *types.Basic:
-		if t.Kind() == types.String {
-			g.leakfile.Printf("    %q,\n", prefix+name)
-		}
+		// pass
 	case *types.Map:
 		g.recurse(t.Elem(), prefix, "Map_"+t.Key().String()+"_string_elem")
 	case *types.Named:
@@ -149,21 +108,38 @@ func (g *pyGen) genFuncSig(sym *symbol, fsym *Func) bool {
 		wpArgs = append(wpArgs, "goRun=False")
 	}
 
+	// When building the pybindgen builder code, we start with
+	// a function that adds function calls with exception checking.
+	// But given specific return types, we may want to add more
+	// behavior to the wrapped function code gen.
+	addFuncName := "add_checked_function"
+	if len(res) > 0 {
+		ret := res[0]
+		switch t := ret.GoType().(type) {
+		case *types.Basic:
+			// string return types need special memory leak patches
+			// to free the allocated char*
+			if t.Kind() == types.String {
+				addFuncName = "add_checked_string_function"
+			}
+		}
+	}
+
 	switch {
 	case isMethod:
 		mnm := sym.id + "_" + fsym.GoName()
 
 		g.gofile.Printf("\n//export %s\n", mnm)
 		g.gofile.Printf("func %s(", mnm)
 
-		g.pybuild.Printf("mod.add_function('%s', ", mnm)
+		g.pybuild.Printf("%s(mod, '%s', ", addFuncName, mnm)
 
 		g.pywrap.Printf("def %s(", gname)
 	default:
 		g.gofile.Printf("\n//export %s\n", fsym.ID())
 		g.gofile.Printf("func %s(", fsym.ID())
 
-		g.pybuild.Printf("mod.add_function('%s', ", fsym.ID())
+		g.pybuild.Printf("%s(mod, '%s', ", addFuncName, fsym.ID())
 
 		g.pywrap.Printf("def %s(", gname)
 	}
@@ -180,8 +156,6 @@ func (g *pyGen) genFuncSig(sym *symbol, fsym *Func) bool {
 			))
 		}
 
-		g.genPatchLeakFuncName(ret, fsym)
-
 		if sret.cpyname == "PyObject*" {
 			g.pybuild.Printf("retval('%s', caller_owns_return=True)", sret.cpyname)
 		} else {
@@ -420,6 +394,15 @@ if __err != nil {
 			g.gofile.Printf("return estr\n") // NOTE: leaked string
 		} else {
 			g.gofile.Printf("C.free(unsafe.Pointer(estr))\n") // python should have converted, safe
+			ret := res[0]
+			if ret.sym.zval == "" {
+				fmt.Printf("gopy: programmer error: empty zval zero value in symbol: %v\n", ret.sym)
+			}
+			if ret.sym.go2py != "" {
+				g.gofile.Printf("return %s(%s)%s\n", ret.sym.go2py, ret.sym.zval, ret.sym.go2pyParenEx)
+			} else {
+				g.gofile.Printf("return %s\n", ret.sym.zval)
+			}
 		}
 		g.gofile.Outdent()
 		g.gofile.Printf("}\n")