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.

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

fix #17997, don't load packages in Main #23579

Merged
merged 2 commits into from
Sep 20, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,10 @@ This section lists changes that do not have deprecation warnings.
* The `openspecfun` library is no longer built and shipped with Julia, as it is no longer
used internally ([#22390]).

* All loaded packges used to have bindings in `Main` (e.g. `Main.Package`). This is no
longer the case; now bindings will only exist for packages brought into scope by
typing `using Package` or `import Package` ([#17997]).

* `slicedim(b::BitVector, 1, x)` now consistently returns the same thing that `b[x]` would,
consistent with its documentation. Previously it would return a `BitArray{0}` for scalar
`x` ([#20233]).
Expand Down
23 changes: 10 additions & 13 deletions base/interactiveutil.jl
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ end
Return an array of methods with an argument of type `typ`.

The optional second argument restricts the search to a particular module or function
(the default is all modules, starting from Main).
(the default is all top-level modules).

If optional `showparents` is `true`, also return arguments with a parent type of `typ`,
excluding type `Any`.
Expand All @@ -588,7 +588,7 @@ function methodswith(t::Type, f::Function, showparents::Bool=false, meths = Meth
return meths
end

function methodswith(t::Type, m::Module, showparents::Bool=false)
function _methodswith(t::Type, m::Module, showparents::Bool)
meths = Method[]
for nm in names(m)
if isdefined(m, nm)
Expand All @@ -601,17 +601,12 @@ function methodswith(t::Type, m::Module, showparents::Bool=false)
return unique(meths)
end

methodswith(t::Type, m::Module, showparents::Bool=false) = _methodswith(t, m, showparents)

function methodswith(t::Type, showparents::Bool=false)
meths = Method[]
mainmod = Main
# find modules in Main
for nm in names(mainmod)
if isdefined(mainmod, nm)
mod = getfield(mainmod, nm)
if isa(mod, Module)
append!(meths, methodswith(t, mod, showparents))
end
end
for mod in loaded_modules_array()
append!(meths, _methodswith(t, mod, showparents))
end
return unique(meths)
end
Expand Down Expand Up @@ -678,8 +673,10 @@ download(url, filename)
workspace()

Replace the top-level module (`Main`) with a new one, providing a clean workspace. The
previous `Main` module is made available as `LastMain`. A previously-loaded package can be
accessed using a statement such as `using LastMain.Package`.
Copy link
Member

Choose a reason for hiding this comment

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

How does one do this now? / What does workspace() do? / Should we add empty!(Base. loaded_modules), and (re-)register the new modules with register_root_module? / Should we add a new function to do that (Base.reset_require() or argument to workspace) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll update the doc string. I think workspace() should just give you a new Main module. So using X in the new Main module will point to the already-loaded copy. To re-load packages there is reload.

previous `Main` module is made available as `LastMain`.

If `Package` was previously loaded, `using Package` in the new `Main` will re-use the
loaded copy. Run `reload("Package")` first to load a fresh copy.

This function should only be used interactively.
"""
Expand Down
106 changes: 81 additions & 25 deletions base/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ function reload(name::AbstractString)
error("use `include` instead of `reload` to load source files")
else
# reload("Package") is ok
unreference_module(Symbol(name))
Copy link
Member

Choose a reason for hiding this comment

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

should be unnecessary

require(Symbol(name))
end
end
Expand Down Expand Up @@ -315,21 +316,78 @@ all platforms, including those with case-insensitive filesystems like macOS and
Windows.
"""
function require(mod::Symbol)
_require(mod)
# After successfully loading, notify downstream consumers
if toplevel_load[] && myid() == 1 && nprocs() > 1
# broadcast top-level import/using from node 1 (only)
@sync for p in procs()
p == 1 && continue
@async remotecall_wait(p) do
if !isbindingresolved(Main, mod) || !isdefined(Main, mod)
_require(mod)
if !root_module_exists(mod)
_require(mod)
# After successfully loading, notify downstream consumers
if toplevel_load[] && myid() == 1 && nprocs() > 1
# broadcast top-level import/using from node 1 (only)
@sync for p in procs()
p == 1 && continue
@async remotecall_wait(p) do
require(mod)
nothing
end
end
end
for callback in package_callbacks
invokelatest(callback, mod)
end
end
for callback in package_callbacks
invokelatest(callback, mod)
return root_module(mod)
end

const loaded_modules = ObjectIdDict()
const module_keys = ObjectIdDict()

function register_root_module(key, m::Module)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a bad idea. Maybe require should set a global flag (haskey(package_locks, m) or perhaps better would be isempty(package_locks)) that disables rooting of the object in Main.m?

Copy link
Member Author

Choose a reason for hiding this comment

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

This function is really only meant for internal use. Nobody but the loading code should call it. Does that address your concern, or is there something else?

Copy link
Member

@vtjnash vtjnash Sep 4, 2017

Choose a reason for hiding this comment

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

This design appears to assume there will be a 1-to-1 correlation between require and module definition, which usually (but doesn't always) happen now. I think a simpler design would be handle that decision point during construction, rather than post-factum mutation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we'll be able to handle it either way. If loading something defines 0 modules, we can store nothing here instead of a module (to provide "load once" semantics). If a file defines multiple modules, you won't be able to reference all of them without a distinct using statement for each one you want anyway. But we should definitely discourage or disallow that. @StefanKarpinski even briefly floated the idea of removing the module keyword (or making it optional), and having the loader make modules instead (thus enforcing 1-to-1 file-to-module by construction).

if haskey(loaded_modules, key)
oldm = loaded_modules[key]
if oldm !== m
name = module_name(oldm)
warn("replacing module $name.")
Copy link
Member

Choose a reason for hiding this comment

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

Are we doing Uppercase messages? I don't precisely remember what style we've started to settle on.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't seem to be totally consistent yet. On a quick search it's close to 50/50. There are some other occurrences of this same message that are lowercase.

end
end
loaded_modules[key] = m
module_keys[m] = key
nothing
end

register_root_module(:Core, Core)
register_root_module(:Base, Base)
register_root_module(:Main, Main)

is_root_module(m::Module) = haskey(module_keys, m)

root_module_key(m::Module) = module_keys[m]

# This is used as the current module when loading top-level modules.
# It has the special behavior that modules evaluated in it get added
# to the loaded_modules table instead of getting bindings.
baremodule __toplevel__
using Base
end

# get a top-level Module from the given key
# for now keys can only be Symbols, but that will change
root_module(key::Symbol) = loaded_modules[key]

root_module_exists(key::Symbol) = haskey(loaded_modules, key)

loaded_modules_array() = collect(values(loaded_modules))

function unreference_module(key)
if haskey(loaded_modules, key)
m = pop!(loaded_modules, key)
# need to ensure all modules are GC rooted; will still be referenced
# in module_keys
end
end

function register_all(a)
for m in a
if module_parent(m) === m
register_root_module(module_name(m), m)
end
end
end

Expand Down Expand Up @@ -364,7 +422,8 @@ function _require(mod::Symbol)
if JLOptions().use_compiled_modules != 0
doneprecompile = _require_search_from_serialized(mod, path)
if !isa(doneprecompile, Bool)
return # success
register_all(doneprecompile)
return
end
end

Expand All @@ -391,14 +450,16 @@ function _require(mod::Symbol)
warn(m, prefix="WARNING: ")
# fall-through, TODO: disable __precompile__(true) error so that the normal include will succeed
else
return # success
register_all(m)
return
end
end

# just load the file normally via include
# for unknown dependencies
try
Base.include_relative(Main, path)
Base.include_relative(__toplevel__, path)
return
catch ex
if doneprecompile === true || JLOptions().use_compiled_modules == 0 || !precompilableerror(ex, true)
rethrow() # rethrow non-precompilable=true errors
Expand All @@ -411,6 +472,7 @@ function _require(mod::Symbol)
# TODO: disable __precompile__(true) error and do normal include instead of error
error("Module $mod declares __precompile__(true) but require failed to create a usable precompiled cache file.")
end
register_all(m)
end
finally
toplevel_load[] = last
Expand Down Expand Up @@ -532,7 +594,7 @@ function create_expr_cache(input::String, output::String, concrete_deps::Vector{
task_local_storage()[:SOURCE_PATH] = $(source)
end)
end
serialize(in, :(Base.include(Main, $(abspath(input)))))
serialize(in, :(Base.include(Base.__toplevel__, $(abspath(input)))))
if source !== nothing
serialize(in, :(delete!(task_local_storage(), :SOURCE_PATH)))
end
Expand Down Expand Up @@ -570,15 +632,9 @@ function compilecache(name::String)
cachefile::String = abspath(cachepath, name*".ji")
# build up the list of modules that we want the precompile process to preserve
concrete_deps = copy(_concrete_dependencies)
for existing in names(Main)
if isdefined(Main, existing)
mod = getfield(Main, existing)
if isa(mod, Module) && !(mod === Main || mod === Core || mod === Base)
mod = mod::Module
if module_parent(mod) === Main && module_name(mod) === existing
push!(concrete_deps, (existing, module_uuid(mod)))
end
end
for (key,mod) in loaded_modules
if !(mod === Main || mod === Core || mod === Base)
push!(concrete_deps, (key, module_uuid(mod)))
end
end
# run the expression and cache the result
Expand Down Expand Up @@ -675,7 +731,7 @@ function stale_cachefile(modpath::String, cachefile::String)
if mod == :Main || mod == :Core || mod == :Base
continue
# Module is already loaded
elseif isbindingresolved(Main, mod)
elseif root_module_exists(mod)
continue
end
name = string(mod)
Expand Down
2 changes: 1 addition & 1 deletion base/pkg/entry.jl
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ function resolve(
info("$(up)grading $pkg: v$ver1 => v$ver2")
Write.update(pkg, Read.sha1(pkg,ver2))
pkgsym = Symbol(pkg)
if Base.isbindingresolved(Main, pkgsym) && isa(getfield(Main, pkgsym), Module)
if Base.root_module_exists(pkgsym)
push!(imported, "- $pkg")
end
end
Expand Down
31 changes: 21 additions & 10 deletions base/reflection.jl
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,17 @@ julia> fullname(Main)
```
"""
function fullname(m::Module)
m === Main && return ()
m === Base && return (:Base,) # issue #10653
mn = module_name(m)
if m === Main || m === Base || m === Core
return (mn,)
end
mp = module_parent(m)
if mp === m
# not Main, but is its own parent, means a prior Main module
n = ()
if mn !== :Main
return (mn,)
end
# top-level module, not Main, called :Main => prior Main module
n = (:Main,)
this = Main
while this !== m
if isdefined(this, :LastMain)
Expand Down Expand Up @@ -530,15 +534,22 @@ function _subtypes(m::Module, x::Union{DataType,UnionAll},
end
return sts
end
function subtypes(m::Module, x::Union{DataType,UnionAll})
if isabstract(x)
sort!(collect(_subtypes(m, x)), by=string)
else

function _subtypes_in(mods::Array, x::Union{DataType,UnionAll})
if !isabstract(x)
# Fast path
Union{DataType,UnionAll}[]
return Union{DataType,UnionAll}[]
end
sts = Set{Union{DataType,UnionAll}}()
visited = Set{Module}()
for m in mods
_subtypes(m, x, sts, visited)
end
return sort!(collect(sts), by=string)
end

subtypes(m::Module, x::Union{DataType,UnionAll}) = _subtypes_in([m], x)

"""
subtypes(T::DataType)

Expand All @@ -555,7 +566,7 @@ julia> subtypes(Integer)
Unsigned
```
"""
subtypes(x::Union{DataType,UnionAll}) = subtypes(Main, x)
subtypes(x::Union{DataType,UnionAll}) = _subtypes_in(loaded_modules_array(), x)

function to_tuple_type(@nospecialize(t))
@_pure_meta
Expand Down
29 changes: 17 additions & 12 deletions base/serialize.jl
Original file line number Diff line number Diff line change
Expand Up @@ -347,9 +347,10 @@ function serialize(s::AbstractSerializer, d::Dict)
end

function serialize_mod_names(s::AbstractSerializer, m::Module)
p = module_parent(m)
if m !== p
serialize_mod_names(s, p)
if Base.is_root_module(m)
serialize(s, Base.root_module_key(m))
else
serialize_mod_names(s, module_parent(m))
serialize(s, module_name(m))
end
end
Expand Down Expand Up @@ -820,21 +821,25 @@ function deserialize_svec(s::AbstractSerializer)
end

function deserialize_module(s::AbstractSerializer)
path = deserialize(s)
m = Main
if isa(path,Tuple) && path !== ()
# old version
for mname in path
m = getfield(m,mname)::Module
mkey = deserialize(s)
if isa(mkey, Tuple)
# old version, TODO: remove
if mkey === ()
return Main
end
m = Base.root_module(mkey[1])
for i = 2:length(mkey)
m = getfield(m, mkey[i])::Module
end
else
mname = path
m = Base.root_module(mkey)
mname = deserialize(s)
while mname !== ()
m = getfield(m,mname)::Module
m = getfield(m, mname)::Module
mname = deserialize(s)
end
end
m
return m
end

function deserialize(s::AbstractSerializer, ::Type{Method})
Expand Down
4 changes: 2 additions & 2 deletions base/show.jl
Original file line number Diff line number Diff line change
Expand Up @@ -380,8 +380,8 @@ function show(io::IO, p::Pair)
end

function show(io::IO, m::Module)
if m === Main
print(io, "Main")
if is_root_module(m)
print(io, module_name(m))
else
print(io, join(fullname(m),"."))
end
Expand Down
10 changes: 1 addition & 9 deletions doc/src/devdocs/require.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,7 @@ The callback below is an example of how to do that:
```julia
# Get the fully-qualified name of a module.
function module_fqn(name::Symbol)
fqn = Symbol[name]
mod = getfield(Main, name)
parent = Base.module_parent(mod)
while parent !== Main
push!(fqn, Base.module_name(parent))
parent = Base.module_parent(parent)
end
fqn = reverse!(fqn)
fqn = fullname(Base.root_module(name))
return join(fqn, '.')
end
```

Loading