From c81697a8e5479fd8cfa6eb500283e54ba3b4be1d Mon Sep 17 00:00:00 2001 From: Ian Butterworth Date: Fri, 20 Sep 2024 10:18:07 -0400 Subject: [PATCH 1/6] handle outside package module import check via require --- base/loading.jl | 38 ++++---------------- test/loading.jl | 90 ---------------------------------------------- test/precompile.jl | 17 +++++++++ 3 files changed, 24 insertions(+), 121 deletions(-) diff --git a/base/loading.jl b/base/loading.jl index 8d180845f942f..9e2055b5bd9c2 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -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) @@ -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 @@ -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) @@ -2927,6 +2902,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)))) """) diff --git a/test/loading.jl b/test/loading.jl index 8db8405ef2a83..f7fc30768eac1 100644 --- a/test/loading.jl +++ b/test/loading.jl @@ -1268,96 +1268,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 diff --git a/test/precompile.jl b/test/precompile.jl index bc738e557bb51..ca617bbbd7f9f 100644 --- a/test/precompile.jl +++ b/test/precompile.jl @@ -2093,4 +2093,21 @@ 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 + write(joinpath(load_path, "ImportBeforeMod.jl"), + """ + import Printf + module ImportBeforeMod + end #module + """) + @test_throws ErrorException Base.compilecache(Base.identify_package("ImportBeforeMod"), Base.DevNull()) + + write(joinpath(load_path, "ImportAfterMod.jl"), """ + module ImportAfterMod + end #module + import Printf + """) + @test_throws ErrorException Base.compilecache(Base.identify_package("ImportAfterMod"), Base.DevNull()) +end + finish_precompile_test!() From 06a3d015a191fa3e10381eff8492fe86dd331d3d Mon Sep 17 00:00:00 2001 From: Ian Butterworth Date: Fri, 20 Sep 2024 15:16:48 -0400 Subject: [PATCH 2/6] also detect when no package module is defined --- base/loading.jl | 10 ++++++++++ test/precompile.jl | 13 +++++++++++++ 2 files changed, 23 insertions(+) diff --git a/base/loading.jl b/base/loading.jl index 9e2055b5bd9c2..3c6a8064826b9 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -2828,6 +2828,16 @@ 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)`") + end + return nothing end const PRECOMPILE_TRACE_COMPILE = Ref{String}() diff --git a/test/precompile.jl b/test/precompile.jl index ca617bbbd7f9f..6c626ab4c78a2 100644 --- a/test/precompile.jl +++ b/test/precompile.jl @@ -2110,4 +2110,17 @@ precompile_test_harness("Detecting importing outside of a package module") do lo @test_throws ErrorException Base.compilecache(Base.identify_package("ImportAfterMod"), Base.DevNull()) end +precompile_test_harness("No package module") do load_path + write(joinpath(load_path, "NoModule.jl"), + """ + x = 1 + """) + @test_throws ErrorException Base.compilecache(Base.identify_package("NoModule"), Base.DevNull()) + + write(joinpath(load_path, "NoModuleWithImport.jl"), """ + import Printf + """) + @test_throws ErrorException Base.compilecache(Base.identify_package("NoModuleWithImport"), Base.DevNull()) +end + finish_precompile_test!() From 3c844c65d74fd4a544487adfe4ce44ee1ad41938 Mon Sep 17 00:00:00 2001 From: Ian Butterworth Date: Fri, 20 Sep 2024 15:53:59 -0400 Subject: [PATCH 3/6] add more tests --- test/precompile.jl | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/precompile.jl b/test/precompile.jl index 6c626ab4c78a2..f89bb20807281 100644 --- a/test/precompile.jl +++ b/test/precompile.jl @@ -2102,6 +2102,21 @@ precompile_test_harness("Detecting importing outside of a package module") do lo """) @test_throws ErrorException Base.compilecache(Base.identify_package("ImportBeforeMod"), Base.DevNull()) + write(joinpath(load_path, "HarmlessComments.jl"), + """ + # import Printf + #= + import Printf + =# + module HarmlessComment + end #module + # import Printf + #= + import Printf + =# + """) + Base.compilecache(Base.identify_package("HarmlessComments"), Base.DevNull()) + write(joinpath(load_path, "ImportAfterMod.jl"), """ module ImportAfterMod end #module @@ -2117,6 +2132,14 @@ precompile_test_harness("No package module") do load_path """) @test_throws ErrorException Base.compilecache(Base.identify_package("NoModule"), Base.DevNull()) + write(joinpath(load_path, "WrongModuleName.jl"), + """ + module DifferentName + x = 1 + end #module + """) + @test_throws ErrorException Base.compilecache(Base.identify_package("WrongModuleName")) + write(joinpath(load_path, "NoModuleWithImport.jl"), """ import Printf """) From 8486a4d6d281920c1c9bffd30425b823072d8a4f Mon Sep 17 00:00:00 2001 From: Ian Butterworth Date: Fri, 20 Sep 2024 17:15:00 -0400 Subject: [PATCH 4/6] add tip to error message --- base/loading.jl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/base/loading.jl b/base/loading.jl index 3c6a8064826b9..2c4a7a16ec7c0 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -2835,7 +2835,8 @@ 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)`") + error("$(repr("text/plain", pkg)) did not define the expected module `$(pkg.name)`, \ + check for typos in package module name") end return nothing end From 3a6262579693d9a3bb63fe4bbf532d48b5a2f7f0 Mon Sep 17 00:00:00 2001 From: Ian Butterworth Date: Sun, 22 Sep 2024 10:06:07 -0400 Subject: [PATCH 5/6] fix tests --- test/precompile.jl | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/test/precompile.jl b/test/precompile.jl index f89bb20807281..7a6e41061f9b1 100644 --- a/test/precompile.jl +++ b/test/precompile.jl @@ -2094,13 +2094,18 @@ precompile_test_harness("Binding Unique") do load_path 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 ErrorException Base.compilecache(Base.identify_package("ImportBeforeMod"), Base.DevNull()) + @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"), """ @@ -2108,29 +2113,38 @@ precompile_test_harness("Detecting importing outside of a package module") do lo #= import Printf =# - module HarmlessComment + module HarmlessComments end #module # import Printf #= import Printf =# """) - Base.compilecache(Base.identify_package("HarmlessComments"), Base.DevNull()) + Base.compilecache(Base.identify_package("HarmlessComments")) + write(joinpath(load_path, "ImportAfterMod.jl"), """ module ImportAfterMod end #module import Printf """) - @test_throws ErrorException Base.compilecache(Base.identify_package("ImportAfterMod"), Base.DevNull()) + @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"), """ - x = 1 + 1 """) - @test_throws ErrorException Base.compilecache(Base.identify_package("NoModule"), Base.DevNull()) + @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"), """ @@ -2138,12 +2152,19 @@ precompile_test_harness("No package module") do load_path x = 1 end #module """) - @test_throws ErrorException Base.compilecache(Base.identify_package("WrongModuleName")) + @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 ErrorException Base.compilecache(Base.identify_package("NoModuleWithImport"), Base.DevNull()) + @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!() From 01c87315b4e6ccfec2dc8156909a283d1b3f0335 Mon Sep 17 00:00:00 2001 From: Ian Butterworth Date: Sun, 22 Sep 2024 11:04:12 -0400 Subject: [PATCH 6/6] rm loading test_throws given we now fail earlier during precompilation --- test/loading.jl | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/test/loading.jl b/test/loading.jl index f7fc30768eac1..fb200bf7a0a93 100644 --- a/test/loading.jl +++ b/test/loading.jl @@ -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()