Skip to content

Commit

Permalink
Replace regex package module checks with actual code checks (#55824)
Browse files Browse the repository at this point in the history 
Fixes #55792
Replaces #55822
Improves what #51635 was trying
to do

i.e.
```
ERROR: LoadError: `using/import Printf` outside of a Module detected. Importing a package outside of a module is not allowed during package precompilation.
```
  • Loading branch information
@IanButterworth
IanButterworth authored Sep 23, 2024
1 parent f62a380 commit 0fade45
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 137 deletions.
49 changes: 18 additions & 31 deletions base/loading.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1524,6 +1524,7 @@ function _insert_extension_triggers(parent::PkgId, extensions::Dict{String, Any}
end
end

precompiling_package::Bool = false
loading_extension::Bool = false
precompiling_extension::Bool = false
function run_extension_callbacks(extid::ExtensionId)
Expand Down Expand Up @@ -2215,6 +2216,11 @@ For more details regarding code loading, see the manual sections on [modules](@r
[parallel computing](@ref code-availability).
"""
function require(into::Module, mod::Symbol)
if into === Base.__toplevel__ && precompiling_package
# this error type needs to match the error type compilecache throws for non-125 errors.
error("`using/import $mod` outside of a Module detected. Importing a package outside of a module \
is not allowed during package precompilation.")
end
if _require_world_age[] != typemax(UInt)
Base.invoke_in_world(_require_world_age[], __require, into, mod)
else
Expand Down Expand Up @@ -2792,41 +2798,10 @@ function load_path_setup_code(load_path::Bool=true)
return code
end

"""
check_src_module_wrap(srcpath::String)
Checks that a package entry file `srcpath` has a module declaration, and that it is before any using/import statements.
"""
function check_src_module_wrap(pkg::PkgId, srcpath::String)
module_rgx = r"^(|end |\"\"\" )\s*(?:@)*(?:bare)?module\s"
load_rgx = r"\b(?:using|import)\s"
load_seen = false
inside_string = false
for s in eachline(srcpath)
if count("\"\"\"", s) == 1
# ignore module docstrings
inside_string = !inside_string
end
inside_string && continue
if contains(s, module_rgx)
if load_seen
throw(ErrorException("Package $(repr("text/plain", pkg)) source file $srcpath has a using/import before a module declaration."))
end
return true
end
if startswith(s, load_rgx)
load_seen = true
end
end
throw(ErrorException("Package $(repr("text/plain", pkg)) source file $srcpath does not contain a module declaration."))
end

# this is called in the external process that generates precompiled package files
function include_package_for_output(pkg::PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String},
concrete_deps::typeof(_concrete_dependencies), source::Union{Nothing,String})

check_src_module_wrap(pkg, input)

append!(empty!(Base.DEPOT_PATH), depot_path)
append!(empty!(Base.DL_LOAD_PATH), dl_load_path)
append!(empty!(Base.LOAD_PATH), load_path)
Expand All @@ -2853,6 +2828,17 @@ function include_package_for_output(pkg::PkgId, input::String, depot_path::Vecto
finally
Core.Compiler.track_newly_inferred.x = false
end
# check that the package defined the expected module so we can give a nice error message if not
Base.check_package_module_loaded(pkg)
end

function check_package_module_loaded(pkg::PkgId)
if !haskey(Base.loaded_modules, pkg)
# match compilecache error type for non-125 errors
error("$(repr("text/plain", pkg)) did not define the expected module `$(pkg.name)`, \
check for typos in package module name")
end
return nothing
end

const PRECOMPILE_TRACE_COMPILE = Ref{String}()
Expand Down Expand Up @@ -2927,6 +2913,7 @@ function create_expr_cache(pkg::PkgId, input::String, output::String, output_o::
empty!(Base.EXT_DORMITORY) # If we have a custom sysimage with `EXT_DORMITORY` prepopulated
Base.track_nested_precomp($precomp_stack)
Base.precompiling_extension = $(loading_extension)
Base.precompiling_package = true
Base.include_package_for_output($(pkg_str(pkg)), $(repr(abspath(input))), $(repr(depot_path)), $(repr(dl_load_path)),
$(repr(load_path)), $deps, $(repr(source_path(nothing))))
""")
Expand Down
106 changes: 0 additions & 106 deletions test/loading.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -855,22 +855,6 @@ end
end
end

@testset "error message loading pkg bad module name" begin
mktempdir() do tmp
old_loadpath = copy(LOAD_PATH)
try
push!(LOAD_PATH, tmp)
write(joinpath(tmp, "BadCase.jl"), "module badcase end")
@test_logs (:warn, r"The call to compilecache failed.*") match_mode=:any begin
@test_throws ErrorException("package `BadCase` did not define the expected module `BadCase`, \
check for typos in package module name") (@eval using BadCase)
end
finally
copy!(LOAD_PATH, old_loadpath)
end
end
end

@testset "Preferences loading" begin
mktempdir() do dir
this_uuid = uuid4()
Expand Down Expand Up @@ -1268,96 +1252,6 @@ end
@test success(`$(Base.julia_cmd()) --startup-file=no -e 'using Statistics'`)
end

@testset "checking srcpath modules" begin
p = Base.PkgId("Dummy")
fpath, _ = mktemp()
@testset "valid" begin
write(fpath, """
module Foo
using Bar
end
""")
@test Base.check_src_module_wrap(p, fpath)

write(fpath, """
baremodule Foo
using Bar
end
""")
@test Base.check_src_module_wrap(p, fpath)

write(fpath, """
\"\"\"
Foo
using Foo
\"\"\"
module Foo
using Bar
end
""")
@test Base.check_src_module_wrap(p, fpath)

write(fpath, """
\"\"\" Foo \"\"\"
module Foo
using Bar
end
""")
@test Base.check_src_module_wrap(p, fpath)

write(fpath, """
\"\"\"
Foo
\"\"\" module Foo
using Bar
end
""")
@test Base.check_src_module_wrap(p, fpath)

write(fpath, """
@doc let x = 1
x
end module Foo
using Bar
end
""")
@test Base.check_src_module_wrap(p, fpath)

write(fpath, """
# using foo
module Foo
using Bar
end
""")
@test Base.check_src_module_wrap(p, fpath)
end
@testset "invalid" begin
write(fpath, """
# module Foo
using Bar
# end
""")
@test_throws ErrorException Base.check_src_module_wrap(p, fpath)

write(fpath, """
using Bar
module Foo
end
""")
@test_throws ErrorException Base.check_src_module_wrap(p, fpath)

write(fpath, """
using Bar
""")
@test_throws ErrorException Base.check_src_module_wrap(p, fpath)

write(fpath, """
x = 1
""")
@test_throws ErrorException Base.check_src_module_wrap(p, fpath)
end
end

@testset "relocatable upgrades #51989" begin
mktempdir() do depot
# realpath is needed because Pkg is used for one of the precompile paths below, and Pkg calls realpath on the
Expand Down
74 changes: 74 additions & 0 deletions test/precompile.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2093,4 +2093,78 @@ precompile_test_harness("Binding Unique") do load_path
@test UniqueBinding2.thebinding2 === ccall(:jl_get_module_binding, Ref{Core.Binding}, (Any, Any, Cint), UniqueBinding2, :thebinding, true)
end

precompile_test_harness("Detecting importing outside of a package module") do load_path
io = IOBuffer()
write(joinpath(load_path, "ImportBeforeMod.jl"),
"""
import Printf
module ImportBeforeMod
end #module
""")
@test_throws r"Failed to precompile ImportBeforeMod" Base.compilecache(Base.identify_package("ImportBeforeMod"), io, io)
@test occursin(
"`using/import Printf` outside of a Module detected. Importing a package outside of a module is not allowed during package precompilation.",
String(take!(io)))


write(joinpath(load_path, "HarmlessComments.jl"),
"""
# import Printf
#=
import Printf
=#
module HarmlessComments
end #module
# import Printf
#=
import Printf
=#
""")
Base.compilecache(Base.identify_package("HarmlessComments"))


write(joinpath(load_path, "ImportAfterMod.jl"), """
module ImportAfterMod
end #module
import Printf
""")
@test_throws r"Failed to precompile ImportAfterMod" Base.compilecache(Base.identify_package("ImportAfterMod"), io, io)
@test occursin(
"`using/import Printf` outside of a Module detected. Importing a package outside of a module is not allowed during package precompilation.",
String(take!(io)))
end

precompile_test_harness("No package module") do load_path
io = IOBuffer()
write(joinpath(load_path, "NoModule.jl"),
"""
1
""")
@test_throws r"Failed to precompile NoModule" Base.compilecache(Base.identify_package("NoModule"), io, io)
@test occursin(
"NoModule [top-level] did not define the expected module `NoModule`, check for typos in package module name",
String(take!(io)))


write(joinpath(load_path, "WrongModuleName.jl"),
"""
module DifferentName
x = 1
end #module
""")
@test_throws r"Failed to precompile WrongModuleName" Base.compilecache(Base.identify_package("WrongModuleName"), io, io)
@test occursin(
"WrongModuleName [top-level] did not define the expected module `WrongModuleName`, check for typos in package module name",
String(take!(io)))


write(joinpath(load_path, "NoModuleWithImport.jl"), """
import Printf
""")
@test_throws r"Failed to precompile NoModuleWithImport" Base.compilecache(Base.identify_package("NoModuleWithImport"), io, io)
@test occursin(
"`using/import Printf` outside of a Module detected. Importing a package outside of a module is not allowed during package precompilation.",
String(take!(io)))
end

finish_precompile_test!()

0 comments on commit 0fade45

Please sign in to comment.