From fa1029b018fe817d827925904703534bf3fb3575 Mon Sep 17 00:00:00 2001 From: Takafumi Arakaki Date: Sat, 1 Sep 2018 10:42:45 -0700 Subject: [PATCH 1/4] Don't steal reference from existing Python object closes #551 --- src/callback.jl | 10 +++++++--- test/runtests.jl | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/callback.jl b/src/callback.jl index 0cd7152c..d72a85f8 100644 --- a/src/callback.jl +++ b/src/callback.jl @@ -25,7 +25,7 @@ function _pyjlwrap_call(f, args_::PyPtr, kw_::PyPtr) # we need to use invokelatest to get execution in newest world if kw_ == C_NULL - ret = PyObject(Base.invokelatest(f, jlargs...)) + ret = Base.invokelatest(f, jlargs...) else kw = PyDict{Symbol,PyObject}(pyincref(kw_)) kwargs = [ (k,julia_kwarg(f,k,v)) for (k,v) in kw ] @@ -34,10 +34,14 @@ function _pyjlwrap_call(f, args_::PyPtr, kw_::PyPtr) # use a closure over kwargs. see: # https://github.com/JuliaLang/julia/pull/22646 f_kw_closure() = f(jlargs...; kwargs...) - ret = PyObject(Core._apply_latest(f_kw_closure)) + ret = Core._apply_latest(f_kw_closure) end - return pystealref!(ret) + if ret isa PyObject + # Don't steal reference from existing Python object (#551) + return pyincref_(ret.o) + end + return pystealref!(PyObject(ret)) catch e pyraise(e) finally diff --git a/test/runtests.jl b/test/runtests.jl index 4da2865f..4a080eba 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -573,4 +573,22 @@ end @test (@pywith IgnoreError(true) error(); true) end +@testset "callback" begin + # Returning existing PyObject in Julia should not invalidate it. + # https://github.com/JuliaPy/PyCall.jl/pull/552 + anonymous = Module() + Base.eval( + anonymous, quote + using PyCall + obj = pyimport("sys") # get some PyObject + end) + py""" + ns = {} + def set(name): + ns[name] = $include_string($anonymous, name) + """ + py"set"("obj") + @test anonymous.obj != PyNULL() +end + include("test_pyfncall.jl") From cfaeb32199b4ada038c2bc3cad3626366d897a14 Mon Sep 17 00:00:00 2001 From: Takafumi Arakaki Date: Sat, 1 Sep 2018 12:33:28 -0700 Subject: [PATCH 2/4] Refactoring: add pyreturn function --- src/PyCall.jl | 10 ++++++++++ src/callback.jl | 6 +----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/PyCall.jl b/src/PyCall.jl index 8c137294..71bb65e4 100644 --- a/src/PyCall.jl +++ b/src/PyCall.jl @@ -133,6 +133,16 @@ function pystealref!(o::PyObject) return optr end +""" + pyreturn(x) :: PyPtr + +Prepare `PyPtr` from `x` for passing it to Python. If `x` is already +a `PyObject`, the refcount is incremented. Otherwise a `PyObject` +wrapping/converted from `x` is created. +""" +pyreturn(x::Any) = pystealref!(PyObject(x)) +pyreturn(x::PyObject) = pyincref_(x.o) + function Base.copy!(dest::PyObject, src::PyObject) pydecref(dest) dest.o = src.o diff --git a/src/callback.jl b/src/callback.jl index d72a85f8..55d84936 100644 --- a/src/callback.jl +++ b/src/callback.jl @@ -37,11 +37,7 @@ function _pyjlwrap_call(f, args_::PyPtr, kw_::PyPtr) ret = Core._apply_latest(f_kw_closure) end - if ret isa PyObject - # Don't steal reference from existing Python object (#551) - return pyincref_(ret.o) - end - return pystealref!(PyObject(ret)) + return pyreturn(ret) catch e pyraise(e) finally From 562686c37a2f98e70455f35f9ed0c578a0ab3578 Mon Sep 17 00:00:00 2001 From: Takafumi Arakaki Date: Sat, 1 Sep 2018 12:43:14 -0700 Subject: [PATCH 3/4] Fix pystealref! in pyjlwrap_getattr --- src/pytype.jl | 2 +- test/runtests.jl | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/pytype.jl b/src/pytype.jl index cd1adc2c..a05d5f8c 100644 --- a/src/pytype.jl +++ b/src/pytype.jl @@ -377,7 +377,7 @@ function pyjlwrap_getattr(self_::PyPtr, attr__::PyPtr) else fidx = Base.fieldindex(typeof(f), Symbol(attr), false) if fidx != 0 - return pystealref!(PyObject(getfield(f, fidx))) + return pyreturn(getfield(f, fidx)) else return ccall(@pysym(:PyObject_GenericGetAttr), PyPtr, (PyPtr,PyPtr), self_, attr__) end diff --git a/test/runtests.jl b/test/runtests.jl index 4a080eba..8697d7b7 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -589,6 +589,24 @@ end """ py"set"("obj") @test anonymous.obj != PyNULL() + + # Test above for pyjlwrap_getattr too: + anonymous = Module() + Base.eval( + anonymous, quote + using PyCall + struct S + x + end + obj = S(pyimport("sys")) + end) + py""" + ns = {} + def set(name): + ns[name] = $include_string($anonymous, name).x + """ + py"set"("obj") + @test anonymous.obj.x != PyNULL() end include("test_pyfncall.jl") From 74ba3c3714f884d8603e56e94c63374094dddeb8 Mon Sep 17 00:00:00 2001 From: Takafumi Arakaki Date: Sat, 1 Sep 2018 12:57:34 -0700 Subject: [PATCH 4/4] Fix pystealref! in pyjlwrap_iternext --- src/pyiterator.jl | 4 ++-- test/runtests.jl | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/pyiterator.jl b/src/pyiterator.jl index 0e03e4b1..ccce277e 100644 --- a/src/pyiterator.jl +++ b/src/pyiterator.jl @@ -65,7 +65,7 @@ const jlWrapIteratorType = PyTypeObject() if !done(iter, state) item, state′ = next(iter, state) stateref[] = state′ # stores new state in the iterator object - return pystealref!(PyObject(item)) + return pyreturn(item) end catch e pyraise(e) @@ -80,7 +80,7 @@ else if iter_result !== nothing item, state = iter_result iter_result_ref[] = iterate(iter, state) - return pystealref!(PyObject(item)) + return pyreturn(item) end catch e pyraise(e) diff --git a/test/runtests.jl b/test/runtests.jl index 8697d7b7..6b3099d2 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -607,6 +607,22 @@ end """ py"set"("obj") @test anonymous.obj.x != PyNULL() + + # Test above for pyjlwrap_iternext too: + anonymous = Module() + Base.eval( + anonymous, quote + using PyCall + sys = pyimport("sys") + obj = (sys for _ in 1:1) + end) + py""" + ns = {} + def set(name): + ns[name] = list(iter($include_string($anonymous, name))) + """ + py"set"("obj") + @test anonymous.sys != PyNULL() end include("test_pyfncall.jl")