From 0fade450a183470b01c656a9001512ef2f1aae47 Mon Sep 17 00:00:00 2001 From: Ian Butterworth Date: Mon, 23 Sep 2024 11:39:00 -0400 Subject: [PATCH] Replace regex package module checks with actual code checks (#55824) Fixes https://github.com/JuliaLang/julia/issues/55792 Replaces https://github.com/JuliaLang/julia/pull/55822 Improves what https://github.com/JuliaLang/julia/pull/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. ``` --- base/loading.jl | 49 ++++++++------------- test/loading.jl | 106 --------------------------------------------- test/precompile.jl | 74 +++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 137 deletions(-) diff --git a/base/loading.jl b/base/loading.jl index 8d180845f942f..2c4a7a16ec7c0 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) @@ -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}() @@ -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)))) """) diff --git a/test/loading.jl b/test/loading.jl index 8db8405ef2a83..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() @@ -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 diff --git a/test/precompile.jl b/test/precompile.jl index bc738e557bb51..7a6e41061f9b1 100644 --- a/test/precompile.jl +++ b/test/precompile.jl @@ -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!()