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.

Sign up for GitHub

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

Jump to bottom

refactor py_str macro to allow passing custom global/locals #777

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

refactor py_str macro to allow passing custom global/locals #777

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55e5159
refactor py_str macro to allow passing custom global/locals
marius311 May 12, 2020
eb58257
get rid of unnecessary macroexpand, fix fname argument
marius311 May 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions src/pyeval.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,6 @@ function pyeval(s::AbstractString, returntype::TypeTuple=PyAny,
return convert(returntype, pyeval_(s, pynamespace(Main), locals, input_type))
end

# get filename from @__FILE__ macro, which returns nothing in the REPL
make_fname(fname::AbstractString) = String(fname)
make_fname(fname::Any) = "REPL"

# a little finite-state-machine dictionary to keep track of where
# we are in Python code, since $ is ignored in string literals and comments.
# 'p' = Python code, '#' = comment, '$' = Julia interpolation
Expand Down Expand Up @@ -202,32 +198,37 @@ pasted into the Python code. This allows you to evaluate code
where the code itself is generated by a Julia expression.
"""
macro py_str(code, options...)
m = :(pynamespace($__module__))
fname = String(__source__.file)
esc(:(PyCall.@_py_str($m, $m, $fname, $code, $(options...))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this call to esc necessary, since there are esc calls within @_py_str?

Indeed, the call to esc here seems like it might cause problems, because it prevents the names pylocals, pyglobals, and ret from being hygienized.

Copy link
Contributor Author

@marius311 marius311 May 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, pylocals etc... get hygenized by @_py_str, the esc here makes it so the interpolated variables don't also get hygenized, e.g. with the code in this PR:

julia> @macroexpand py"1+$i"
quote
    #= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:229 =#
    (var"#18#pyglobals", var"#19#pylocals") = (PyCall.pynamespace(Main), PyCall.pynamespace(Main))
    #= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:230 =#
    begin
        var"#19#pylocals"["__julia_localvar_2_1"] = PyCall.PyObject(i)
    end
    #= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:231 =#
    var"#20#ret" = (PyAny)(PyCall.pyeval_((string)("1+__julia_localvar_2_1"), var"#18#pyglobals", var"#19#pylocals", 258, "none"))
    #= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:232 =#
    begin
        PyCall.delete!(var"#19#pylocals", "__julia_localvar_2_1")
    end
    #= /home/marius/src/pyjulia/dev/PyCall/src/pyeval.jl:233 =#
    var"#20#ret"
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should've asked this before but why is insert_pyevals run at run-time? It looks like this doesn't depend on the run-time value of globals and locals (or maybe I'm missing something)? If we expand everything at macro-expansion time, perhaps esc of @py_str here and @prepare_for_pyjulia_call there can be removed? Then maybe _py_str doesn't have to be a macro?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
esc(:(PyCall.@_py_str($m, $m, $fname, $code, $(options...))))
esc(:($PyCall.@_py_str($m, $m, $fname, $code, $(options...))))

I think you need this?

I think we better test py"" with something like

baremodule TestPyStr
using PyCall: @py_str
pystr(x) = py"$$x"
end
@test TestPyStr.pystr("1+1") == 2

end

macro _py_str(pyglobals, pylocals, fname, code, options...)
T = length(options) == 1 && 'o' in options[1] ? PyObject : PyAny
code, locals = interpolate_pycode(code)
input_type = '\n' in code ? Py_file_input : Py_eval_input
fname = make_fname(@__FILE__)
assignlocals = Expr(:block, [(isa(v,String) ?
:(m[$v] = PyObject($(esc(ex)))) :
:(pylocals[$v] = PyObject($(esc(ex)))) :
nothing) for (v,ex) in locals]...)
code_expr = Expr(:call, esc(:(Base.string)))
code_expr = Expr(:call, Base.string)
i0 = firstindex(code)
for i in sort!(collect(filter(k -> isa(k,Integer), keys(locals))))
push!(code_expr.args, code[i0:prevind(code,i)], esc(locals[i]))
i0 = i
end
push!(code_expr.args, code[i0:lastindex(code)])
if input_type == Py_eval_input
removelocals = Expr(:block, [:(delete!(m, $v)) for v in keys(locals)]...)
removelocals = Expr(:block, [:(delete!(pylocals, $v)) for v in keys(locals)]...)
else
# if we are evaluating multi-line input, then it is not
# safe to remove the local variables, because they might be referred
# to in Python function definitions etc. that will be called later.
removelocals = nothing
end
quote
m = pynamespace($__module__)
pyglobals, pylocals = $pyglobals, $pylocals
$assignlocals
ret = $T(pyeval_($code_expr, m, m, $input_type, $fname))
ret = $T(pyeval_($code_expr, pyglobals, pylocals, $input_type, $fname))
$removelocals
ret
end
Expand Down