minimal hook into loading needed for Pkg3 (take 2)

[StefanKarpinski]

Arguably, the behavior that this can cause loading via using and import from the current working directory is a bug which is somewhat mitigated by this additional patch:

diff --git a/base/loading.jl b/base/loading.jl
index 0e62463bc..ad23e1b4f 100644
--- a/base/loading.jl
+++ b/base/loading.jl
@@ -99,7 +99,7 @@ _str(x) = x
 
 # `wd` is a working directory to search. defaults to current working directory.
 # if `wd === nothing`, no extra path is searched.
-function find_in_path(name::String, wd::Union{Void,String})
+function find_in_path(name::String, wd::String)
     isabspath(name) && return name
     base = name
     if endswith(name,".jl")
@@ -110,6 +110,10 @@ function find_in_path(name::String, wd::Union{Void,String})
     if wd !== nothing
         isfile_casesensitive(joinpath(wd,name)) && return joinpath(wd,name)
     end
+    return find_in_path(base)
+end
+
+function find_in_path(base::String)
     path = nothing
     path = _str(load_hook(_str(Pkg.dir()), base, path))
     for dir in LOAD_PATH
@@ -117,7 +121,10 @@ function find_in_path(name::String, wd::Union{Void,String})
     end
     return path
 end
-find_in_path(name::AbstractString, wd::AbstractString = pwd()) =
+
+find_in_path(name::AbstractString) = find_in_path(String(name))
+find_in_path(name::AbstractString, ::Void) = find_in_path(String(name))
+find_in_path(name::AbstractString, wd::AbstractString) =
     find_in_path(String(name), String(wd))
 
 function find_in_node_path(name::String, srcpath, node::Int=1)
@@ -130,7 +137,7 @@ end
 
 function find_source_file(file::String)
     (isabspath(file) || isfile(file)) && return file
-    file2 = find_in_path(file)
+    file2 = find_in_path(file, pwd())
     file2 !== nothing && return file2
     file2 = joinpath(JULIA_HOME, DATAROOTDIR, "julia", "base", file)
     return isfile(file2) ? file2 : nothing
@@ -639,7 +646,7 @@ for important notes.
 function compilecache(name::String)
     myid() == 1 || error("can only precompile from node 1")
     # decide where to get the source file from
-    path = find_in_path(name, nothing)
+    path = find_in_path(name)
     path === nothing && throw(ArgumentError("$name not found in path"))
     path = String(path)
     # decide where to put the resulting cache file
diff --git a/test/loading.jl b/test/loading.jl
index 48348e455..ee1f3f63d 100644
--- a/test/loading.jl
+++ b/test/loading.jl
@@ -44,7 +44,6 @@ SAVED_LOAD_PATH = copy(LOAD_PATH)
 empty!(LOAD_PATH)
 dir = abspath(@__DIR__)
 push!(LOAD_PATH, dir)
-cd("..")
 
 let paddedname = "Atest_sourcepathZ"
     filename = SubString(paddedname, 2, length(paddedname)-1)
@@ -66,7 +65,6 @@ end
 Base.load_hook(prefix::CustomLoader, name::String, found::String) = found
 @test Base.find_in_path("test_sourcepath") == joinpath(dir, "test_sourcepath.jl")
 
-pop!(LOAD_PATH)
-cd(pop!(LOAD_PATH))
+empty!(LOAD_PATH)
 append!(LOAD_PATH, SAVED_LOAD_PATH)
 @test LOAD_PATH == SAVED_LOAD_PATH

However, I'm not sure if that's a safe change so I'm leaving it out of this. Note that the cd to .. and back in test/loading.jl is to avoid what I think is a pre-existing bug in find_in_path. If packages and/or edit are relying on this, they should stop doing so.

edit by tkelman: closes #8059

[tkelman]

the behavior that this can cause loading via using and import from the current working directory is a bug

Could you elaborate? That's something that this change makes happen but wasn't happening before? Are we going to want to keep that behavior, or would it be a temporary only-during-0.6 accident?

[StefanKarpinski]

It seems like the way using and import call find_in_path doesn't trigger the local path-finding logic:

julia> using ModInts
find_in_path("ModInts", nothing)
ERROR: ArgumentError: Module ModInts not found in current path.
Run `Pkg.add("ModInts")` to install the ModInts package.
Stacktrace:
 [1] require(::Symbol) at ./loading.jl:403

julia> Base.find_in_path("ModInts")
find_in_path("ModInts", "/Users/stefan/projects/julia/examples")
"/Users/stefan/projects/julia/examples/ModInts.jl"

However, there's a theoretical problem that doesn't happen in practice because of syntax limitations on what you can actually write after using, but you could trigger it with generated code:

julia> eval(:(using $(Symbol(abspath("ModInts.jl")))))
find_in_path("/Users/stefan/projects/julia/examples/ModInts.jl", nothing)
WARNING: requiring "/Users/stefan/projects/julia/examples/ModInts.jl" in module "Main" did not define a corresponding module.

That's a pretty contrived situation, but the point is that there are two kinds of code lookup that should be cleanly separated but are currently entangled: using/import lookup, to load global, shared resourced, versus finding files relative to the current location as in include.

[StefanKarpinski]

I made the test more realistic by having it call find_in_path(name, nothing) since that's what using/import call, rather than find_in_path(name).

[tkelman]

Thanks, that does sound mostly contrived, guess we'll see one way or another once pkgeval actually works correctly on nightly again. I think for the sake of schedules we should go for the imperfect but "good enough to do the job for now" version.

Do we want to document this or is it going to be a "for Stefan's use only" feature until we do a proper cleaner version that decouples everything, post-0.6?

[StefanKarpinski]

Others are welcome to use this, with the caveat that we might still change it, which will break stuff :)

[StefanKarpinski]

I should note that custom loaders do not work when you define the custom loader in the REPL, but they do work if defined in a package that is loaded via using or import, which is what we need.

[StefanKarpinski]

Example usage via a dummy Pkg3 package:

shell> cat tmp/Pkg3.jl
module Pkg3
immutable Loader; path::String; end
import Base.load_hook
load_hook(loader::Loader, name::String, found) = load_hook(loader.path, name, found)
end

julia> push!(LOAD_PATH, abspath("tmp"))
3-element Array{Any,1}:
 "/Users/stefan/projects/julia/usr/local/share/julia/site/v0.6"
 "/Users/stefan/projects/julia/usr/share/julia/site/v0.6"
 "/Users/stefan/projects/julia/tmp"

julia> using Pkg3

julia> using ModInts
ERROR: ArgumentError: Module ModInts not found in current path.
Run `Pkg.add("ModInts")` to install the ModInts package.
Stacktrace:
 [1] require(::Symbol) at ./loading.jl:403

julia> push!(LOAD_PATH, Pkg3.Loader("examples"))
4-element Array{Any,1}:
 "/Users/stefan/projects/julia/usr/local/share/julia/site/v0.6"
 "/Users/stefan/projects/julia/usr/share/julia/site/v0.6"
 "/Users/stefan/projects/julia/tmp"
 Pkg3.Loader("examples")

julia> using ModInts

[tkelman]

(as strings, or ... )

[StefanKarpinski]

We seem to have a bug randomly affecting test/core.jl (among other things).

[StefanKarpinski]

Tests all pass. Unless there are objections from anyone (@vtjnash?), I'll merge tomorrow.

[tkelman]

looks like this should work

[tkelman]

objects

[vtjnash]

lgtm. there's a comment on https://github.com/JuliaLang/julia/pull/20120/files#diff-a03d5ce5729a81495000698e643fce27R322 (require_modname) that it should be deleted when base/require.jl is deleted.

[tkelman]

I think the comment's a little bit wrong, since require_modname is still called a few lines later by reload ?

(wrong button sorry)

[StefanKarpinski]

Yeah, I'm just going to ignore that comment for now – I've undeleted that change.

[StefanKarpinski]

Unfortunately, I've now wiggled this around enough that I can't just trust a merge – I'll have to wait until the tests pass again.