Skip to content

Commit

Permalink
Use a curried helper for module-local eval/include
Browse files Browse the repository at this point in the history
In #55466, the automatically added `include` method for non-bare modules
was adjusted to conform the signature to the version of those methods
in Main (defined in sysimg.jl, since `Main` is technically a bare module).
Unfortunately, this broke some downstream packages which overload Base.include
with additional functionality and got broken by the additional type restriction.
The motivation in #55466 was to give a slightly nicer MethodError. While I don't
think this is per-se a particularly strong justification, I do agree that it's
awkward for the `Main` version of these functions to have (marginally) different
behavior than the version of these functions that gets introduced automatically in
new modules (Which has been the case ever since [1], which added the AbstractString
restriction in `Main`, but not in the auto-generated versions). This is particularly
true, because we use the `Main` version to document the auto-introduction of these
methods, which has regularly been a point of confusion.

This PR tries to address this problem once and for all, but just not generating
special methods into every new module. Instead, there are curried helpers for
eval and include in Core and Base (respectively), which can be added to a module
simply by doing `const include = IncludeInto(MyModule)` (and similarly for `eval`).
As before, this happens automatically for non-bare modules. It thus conforms the
behavior of the `Main` version of these functions and the auto-generated versions
by construction. Additionally, it saves us having to generate all the additional
code/types/objects, etc associated with having extra generic functions in each
new module. The impact of this isn't huge, because there aren't that many modules,
but it feels conceptually nicer.

There is a little bit of extra work in this PR because we have special snowflake
backtrace printing code for the `include` machinery, which needs adjusting, but
other than that the change is straightforward.

[1] 957848b
  • Loading branch information
Keno committed Oct 22, 2024
1 parent 8bae3ee commit 7f238f9
Show file tree
Hide file tree
Showing 11 changed files with 61 additions and 45 deletions.
14 changes: 12 additions & 2 deletions base/Base.jl
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ function include(mod::Module, path::String)
end
include(path::String) = include(Base, path)

struct IncludeInto <: Function
m::Module
end
(this::IncludeInto)(fname::AbstractString) = include(this.m, fname)

# from now on, this is now a top-module for resolving syntax
const is_primary_base_module = ccall(:jl_module_parent, Ref{Module}, (Any,), Base) === Core.Main
ccall(:jl_set_istopmod, Cvoid, (Any, Bool), Base, is_primary_base_module)
Expand Down Expand Up @@ -572,15 +577,20 @@ include("precompilation.jl")
for m in methods(include)
delete_method(m)
end
for m in methods(IncludeInto(Base))
delete_method(m)
end

# This method is here only to be overwritten during the test suite to test
# various sysimg related invalidation scenarios.
a_method_to_overwrite_in_test() = inferencebarrier(1)

# These functions are duplicated in client.jl/include(::String) for
# nicer stacktraces. Modifications here have to be backported there
include(mod::Module, _path::AbstractString) = _include(identity, mod, _path)
include(mapexpr::Function, mod::Module, _path::AbstractString) = _include(mapexpr, mod, _path)
@noinline include(mod::Module, _path::AbstractString) = _include(identity, mod, _path)
@noinline include(mapexpr::Function, mod::Module, _path::AbstractString) = _include(mapexpr, mod, _path)
(this::IncludeInto)(fname::AbstractString) = include(identity, this.m, fname)
(this::IncludeInto)(mapexpr::Function, fname::AbstractString) = include(mapexpr, this.m, fname)

# External libraries vendored into Base
Core.println("JuliaSyntax/src/JuliaSyntax.jl")
Expand Down
8 changes: 6 additions & 2 deletions base/boot.jl
Original file line number Diff line number Diff line change
Expand Up @@ -454,9 +454,13 @@ Nothing() = nothing
# This should always be inlined
getptls() = ccall(:jl_get_ptls_states, Ptr{Cvoid}, ())

include(m::Module, fname::String) = ccall(:jl_load_, Any, (Any, Any), m, fname)
include(m::Module, fname::String) = (@noinline; ccall(:jl_load_, Any, (Any, Any), m, fname))
eval(m::Module, @nospecialize(e)) = (@noinline; ccall(:jl_toplevel_eval_in, Any, (Any, Any), m, e))

eval(m::Module, @nospecialize(e)) = ccall(:jl_toplevel_eval_in, Any, (Any, Any), m, e)
struct EvalInto <: Function
m::Module
end
(this::EvalInto)(@nospecialize(e)) = eval(this.m, e)

mutable struct Box
contents::Any
Expand Down
2 changes: 1 addition & 1 deletion base/docs/basedocs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2580,7 +2580,7 @@ cases.
See also [`setproperty!`](@ref Base.setproperty!) and [`getglobal`](@ref)
# Examples
```jldoctest; filter = r"Stacktrace:(\\n \\[[0-9]+\\].*)*"
```jldoctest; filter = r"Stacktrace:(\\n \\[[0-9]+\\].*\\n.*)*"
julia> module M; global a; end;
julia> M.a # same as `getglobal(M, :a)`
Expand Down
5 changes: 4 additions & 1 deletion base/errorshow.jl
Original file line number Diff line number Diff line change
Expand Up @@ -850,7 +850,10 @@ function _simplify_include_frames(trace)
for i in length(trace):-1:1
frame::StackFrame, _ = trace[i]
mod = parentmodule(frame)
if first_ignored === nothing
if mod === Base && frame.func === :IncludeInto ||
mod === Core && frame.func === :EvalInto
kept_frames[i] = false
elseif first_ignored === nothing
if mod === Base && frame.func === :_include
# Hide include() machinery by default
first_ignored = i
Expand Down
8 changes: 4 additions & 4 deletions base/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2890,12 +2890,12 @@ julia> rm("testfile.jl")
```
"""
function evalfile(path::AbstractString, args::Vector{String}=String[])
return Core.eval(Module(:__anon__),
m = Module(:__anon__)
return Core.eval(m,
Expr(:toplevel,
:(const ARGS = $args),
:(eval(x) = $(Expr(:core, :eval))(__anon__, x)),
:(include(x) = $(Expr(:top, :include))(__anon__, x)),
:(include(mapexpr::Function, x) = $(Expr(:top, :include))(mapexpr, __anon__, x)),
:(const include = $(Base.IncludeInto(m))),
:(const eval = $(Core.EvalInto(m))),
:(include($path))))
end
evalfile(path::AbstractString, args::Vector) = evalfile(path, String[args...])
Expand Down
19 changes: 19 additions & 0 deletions base/show.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3418,3 +3418,22 @@ function show(io::IO, ::MIME"text/plain", bnd::Core.Binding)
end
end
end

# Special pretty printing for EvalInto/IncludeInto
function show(io::IO, ii::IncludeInto)
if getglobal(ii.m, :include) === ii
print(io, ii.m)
print(io, ".include")
else
show_default(io, ii)
end
end

function show(io::IO, ei::Core.EvalInto)
if getglobal(ei.m, :eval) === ei
print(io, ei.m)
print(io, ".eval")
else
show_default(io, ei)
end
end
8 changes: 2 additions & 6 deletions base/sysimg.jl
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ Use [`Base.include`](@ref) to evaluate a file into another module.
!!! compat "Julia 1.5"
Julia 1.5 is required for passing the `mapexpr` argument.
"""
include(mapexpr::Function, fname::AbstractString) = Base._include(mapexpr, Main, fname)
function include(fname::AbstractString)
isa(fname, String) || (fname = Base.convert(String, fname)::String)
Base._include(identity, Main, fname)
end
const include = Base.IncludeInto(Main)

"""
eval(expr)
Expand All @@ -45,7 +41,7 @@ Evaluate an expression in the global scope of the containing module.
Every `Module` (except those defined with `baremodule`) has its own 1-argument
definition of `eval`, which evaluates expressions in that module.
"""
eval(x) = Core.eval(Main, x)
const eval = Core.EvalInto(Main)

# Ensure this file is also tracked
pushfirst!(Base._included_files, (@__MODULE__, abspath(@__FILE__)))
Expand Down
22 changes: 0 additions & 22 deletions src/jlfrontend.scm
Original file line number Diff line number Diff line change
Expand Up @@ -199,28 +199,6 @@
(error-wrap (lambda ()
(julia-expand-macroscope expr))))

;; construct default definitions of `eval` for non-bare modules
;; called by jl_eval_module_expr
(define (module-default-defs name file line)
(jl-expand-to-thunk
(let* ((loc (if (and (eq? file 'none) (eq? line 0)) '() `((line ,line ,file))))
(x (if (eq? name 'x) 'y 'x))
(mex (if (eq? name 'mapexpr) 'map_expr 'mapexpr)))
`(block
(= (call eval ,x)
(block
,@loc
(call (core eval) ,name ,x)))
(= (call include ,x)
(block
,@loc
(call (core _call_latest) (top include) ,name ,x)))
(= (call include (:: ,mex (top Function)) ,x)
(block
,@loc
(call (core _call_latest) (top include) ,mex ,name ,x)))))
file line))

; run whole frontend on a string. useful for testing.
(define (fe str)
(expand-toplevel-expr (julia-parse str) 'none 0))
Expand Down
14 changes: 10 additions & 4 deletions src/toplevel.c
Original file line number Diff line number Diff line change
Expand Up @@ -206,11 +206,17 @@ static jl_value_t *jl_eval_module_expr(jl_module_t *parent_module, jl_expr_t *ex
if (std_imports) {
if (jl_base_module != NULL) {
jl_add_standard_imports(newm);
jl_datatype_t *include_into = (jl_datatype_t *)jl_get_global(jl_base_module, jl_symbol("IncludeInto"));
if (include_into) {
form = jl_new_struct(include_into, newm);
jl_set_const(newm, jl_symbol("include"), form);
}
}
jl_datatype_t *eval_into = (jl_datatype_t *)jl_get_global(jl_core_module, jl_symbol("EvalInto"));
if (eval_into) {
form = jl_new_struct(eval_into, newm);
jl_set_const(newm, jl_symbol("eval"), form);
}
// add `eval` function
form = jl_call_scm_on_ast_and_loc("module-default-defs", (jl_value_t*)name, newm, filename, lineno);
jl_toplevel_eval_flex(newm, form, 0, 1, &filename, &lineno);
form = NULL;
}

newm->file = jl_symbol(filename);
Expand Down
2 changes: 1 addition & 1 deletion test/docs.jl
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ end

@test Docs.undocumented_names(_ModuleWithUndocumentedNames) == [Symbol("@foo"), :f, :]
@test isempty(Docs.undocumented_names(_ModuleWithSomeDocumentedNames))
@test Docs.undocumented_names(_ModuleWithSomeDocumentedNames; private=true) == [:eval, :g, :include]
@test Docs.undocumented_names(_ModuleWithSomeDocumentedNames; private=true) == [:g]


# issue #11548
Expand Down
4 changes: 2 additions & 2 deletions test/reflection.jl
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ let
@test Base.binding_module(TestMod7648.TestModSub9475, :b9475) == TestMod7648.TestModSub9475
defaultset = Set(Symbol[:Foo7648, :TestMod7648, :a9475, :c7648, :f9475, :foo7648, :foo7648_nomethods])
allset = defaultset Set(Symbol[
Symbol("#eval"), Symbol("#foo7648"), Symbol("#foo7648_nomethods"), Symbol("#include"),
Symbol("#foo7648"), Symbol("#foo7648_nomethods"),
:TestModSub9475, :d7648, :eval, :f7648, :include])
imported = Set(Symbol[:convert, :curmod_name, :curmod])
usings_from_Test = Set(Symbol[
Expand Down Expand Up @@ -265,7 +265,7 @@ let defaultset = Set((:A,))
imported = Set((:M2,))
usings_from_Base = delete!(Set(names(Module(); usings=true)), :anonymous) # the name of the anonymous module itself
usings = Set((:A, :f, :C, :y, :M1, :m1_x)) usings_from_Base
allset = Set((:A, :B, :C, :eval, :include, Symbol("#eval"), Symbol("#include")))
allset = Set((:A, :B, :C, :eval, :include))
@test Set(names(TestMod54609.A)) == defaultset
@test Set(names(TestMod54609.A, imported=true)) == defaultset imported
@test Set(names(TestMod54609.A, usings=true)) == defaultset usings
Expand Down

0 comments on commit 7f238f9

Please sign in to comment.