From 2ae3423dc3e5da67ea5eabd1ce2e13f58be51ba5 Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Mon, 3 Jun 2024 15:59:34 +0200 Subject: [PATCH 1/7] simplify and fix intermittent names import --- src/frompackage/helpers.jl | 90 +++++++++++++++++--------------- src/frompackage/input_parsing.jl | 15 +++--- src/frompackage/loading.jl | 33 +++++++----- src/frompackage/macro.jl | 2 + src/frompackage/types.jl | 4 +- 5 files changed, 80 insertions(+), 64 deletions(-) diff --git a/src/frompackage/helpers.jl b/src/frompackage/helpers.jl index 919e9b3..b56d3a1 100644 --- a/src/frompackage/helpers.jl +++ b/src/frompackage/helpers.jl @@ -1,13 +1,12 @@ import ..PlutoDevMacros: hide_this_log function get_temp_module() - @assert isassigned(fromparent_module) "You have to assing the parent module by calling `maybe_create_module` with a Pluto workspace module as input before you can use `get_temp_module`" - fromparent_module[] + isdefined(Main, TEMP_MODULE_NAME) || return nothing + return getproperty(Main, TEMP_MODULE_NAME)::Module end # Extract the module that is the target in dict -get_target_module(dict) = get_target_module(Symbol(dict["name"])) -get_target_module(mod_name::Symbol) = getfield(get_temp_module(), mod_name) +get_target_module(dict) = dict["Created Module"] function get_target_uuid(dict) uuid = get(dict, "uuid", nothing) @@ -118,7 +117,7 @@ function get_package_data(packagepath::AbstractString) return package_data end -## getfirst +# Get the first element in itr that satisfies predicate p, or nothing if itr is empty or no elements satisfy p function getfirst(p, itr) for el in itr p(el) && return el @@ -127,45 +126,46 @@ function getfirst(p, itr) end getfirst(itr) = getfirst(x -> true, itr) -## filterednames -function filterednames(m::Module, caller_module = nothing; all = true, imported = true, explicit_names = nothing, package_dict = nothing) +## Similar to names but allows to exclude names and add explicit ones. It also filter names based on whether they are defined already in the caller module +function filterednames(m::Module; all = true, imported = true, explicit_names = Set{Symbol}(), caller_module::Module) excluded = (:eval, :include, :_fromparent_dict_, Symbol("@bind")) - mod_names = names(m;all, imported) - filter_args = if explicit_names isa Set{Symbol} - for name in mod_names - push!(explicit_names, name) - end - collect(explicit_names) - else - mod_names - end - filter_func = filterednames_filter_func(m; excluded, caller_module, package_dict) + mod_names = names(m; all, imported) + filter_args = union(mod_names, explicit_names) + filter_func = filterednames_filter_func(;excluded, caller_module) filter(filter_func, filter_args) end -function filterednames_filter_func(m; excluded, caller_module, package_dict) - f(s) = let excluded = excluded, caller_module = caller_module, package_dict = package_dict +function has_ancestor_module(target::Module, ancestor_name::Symbol; previous = nothing) + has_ancestor_module(target, (ancestor_name,); previous) +end +function has_ancestor_module(target::Module, ancestor_names; previous = nothing) + nm = nameof(target) + nm in ancestor_names && return true # Ancestor found + nm === previous && return false # The target is the same as previous, so we reached a top-level module + return has_ancestor_module(parentmodule(target), ancestor_names; previous = nm) +end + +# This returns two flags: whether the name can be included and whether a warning should be generated +function can_import_in_caller(name::Symbol, caller::Module) + isdefined(caller, name) || return true, false # If is not defined we can surely import it + owner = which(caller, name) + # Skip (and do not warn) for things defined in Base or Core + has_ancestor_module(owner, (:Base, :Core, :Markdown, :InteractiveUtils)) && return false, false + # We check if the name is inside the list of symbols imported by the previous module + in_previous = name in PREVIOUSLY_IMPORTED_NAMES + return in_previous, !in_previous +end + +function filterednames_filter_func(;excluded, caller_module) + f(s) = let excluded = excluded, caller = caller_module Base.isgensym(s) && return false s in excluded && return false - if caller_module isa Module - previous_target_module = get_stored_module(package_dict) - # We check and avoid putting in scope symbols which are already in the caller module - isdefined(caller_module, s) || return true - # Here we have to extract the symbols to compare them - mod_val = getfield(m, s) - caller_val = getfield(caller_module, s) - if caller_val !== mod_val - if isdefined(previous_target_module, s) && caller_val === getfield(previous_target_module, s) - # We are just replacing the previous implementation of this call's target package, so we want to overwrite - return true - else - @warn "Symbol `:$s`, is already defined in the caller module and points to a different object. Skipping" - end - end - return false - else # We don't check for names clashes with a caller module - return true + should_include, should_warn = can_import_in_caller(s, caller) + if should_warn + owner = which(caller, s) + @warn "The name `$s`, defined in $owner, is already present in the caller module and will not be imported." end + return should_include end return f end @@ -280,7 +280,7 @@ end # This relies on Base internals (and even the C API) but will allow make the loaded module behave more like if we simply did `using TargetPackage` in the REPL function register_target_module_as_root(package_dict) name_str = package_dict["name"] - m = get_target_module(Symbol(name_str)) + m = get_target_module(package_dict) id = get_target_pkgid(package_dict) uuid = id.uuid entry_point = package_dict["file"] @@ -317,12 +317,16 @@ function try_load_extensions(package_dict::Dict) end # This function will get the module stored in the created_modules dict based on the entry point -get_stored_module(package_dict) = get_stored_module(package_dict["uuid"]) -get_stored_module(key::String) = get(created_modules, key, nothing) +get_stored_module() = STORED_MODULE[] # This will store in it -update_stored_module(key::String, m::Module) = created_modules[key] = m +update_stored_module(m::Module) = STORED_MODULE[] = m function update_stored_module(package_dict::Dict) - uuid = package_dict["uuid"] m = get_target_module(package_dict) - update_stored_module(uuid, m) + update_stored_module(m) +end + +function overwrite_imported_symbols(package_dict) + empty!(PREVIOUSLY_IMPORTED_NAMES) + union!(PREVIOUSLY_IMPORTED_NAMES, package_dict["Imported Symbols"]) + nothing end \ No newline at end of file diff --git a/src/frompackage/input_parsing.jl b/src/frompackage/input_parsing.jl index 171d7e5..1c6e3c8 100644 --- a/src/frompackage/input_parsing.jl +++ b/src/frompackage/input_parsing.jl @@ -259,12 +259,12 @@ function process_imported_nameargs!(args, dict) end ## Per-type versions function process_imported_nameargs!(args, dict, t::FromPackageImport) - name_init = modname_path(fromparent_module[]) + name_init = modname_path(get_temp_module()) args[1] = t.mod_name prepend!(args, name_init) end function process_imported_nameargs!(args, dict, t::Union{FromParentImport, RelativeImport}) - name_init = modname_path(fromparent_module[]) + name_init = modname_path(get_temp_module()) # Here transform the relative module name to the one based on the full loaded module path target_path = get(dict, "Target Path", []) |> reverse isempty(target_path) && error("The current file was not found included in the loaded module $(t.mod_name), so you can't use relative path imports") @@ -285,7 +285,7 @@ function process_imported_nameargs!(args, dict, t::FromDepsImport) maybe_add_loaded_module(t.id) deps_module_name = :_LoadedModules_ args[1] = deps_module_name # We replace `>` with _LoadedModules_ - name_init = modname_path(fromparent_module[]) + name_init = modname_path(get_temp_module()) prepend!(args, name_init) return nothing end @@ -317,7 +317,7 @@ function should_include_using_names!(ex) end ## parseinput -function parseinput(ex, package_dict; caller_module = nothing) +function parseinput(ex, package_dict; caller_module) include_using = should_include_using_names!(ex) # We get the module modname_expr, importednames_exprs = extract_import_args(ex) @@ -346,10 +346,11 @@ function parseinput(ex, package_dict; caller_module = nothing) explicit_names = if include_using package_dict["Using Names"].explicit_names else - nothing + Set{Symbol}() end - # We extract the imported names either due to catchall or due to the standard using - imported_names = filterednames(_mod, caller_module; all = catchall, imported = catchall, explicit_names, package_dict) + imported_names = filterednames(_mod; all = catchall, imported = catchall, explicit_names, caller_module) + # We add the imported names to the set for tracking names imported by this macrocall + union!(get!(Set{Symbol}, package_dict, "Imported Symbols"), imported_names) # At this point we have all the names and we just have to create the final expression importednames_exprs = map(n -> Expr(:., n), imported_names) return reconstruct_import_expr(modname_expr, importednames_exprs) diff --git a/src/frompackage/loading.jl b/src/frompackage/loading.jl index f9a508b..b39f684 100644 --- a/src/frompackage/loading.jl +++ b/src/frompackage/loading.jl @@ -124,17 +124,16 @@ function eval_module_expr(parent_module, ex, dict) return out isa StopEval ? out : nothing end -function maybe_create_module(m::Module) - if !isassigned(fromparent_module) - fromparent_m = Core.eval(m, :(module $(gensym(:frompackage)) - end)) - # We create the dummy module where all the direct dependencies will be loaded - Core.eval(fromparent_m, :(module _DirectDeps_ end)) - # We also set a reference to LoadedModules for access from the notebook - Core.eval(fromparent_m, :(const _LoadedModules_ = $LoadedModules)) - fromparent_module[] = fromparent_m - end - return fromparent_module[] +function maybe_create_module() + m = get_temp_module() + isnothing(m) || return m + fromparent_m = Core.eval(Main, :(module $TEMP_MODULE_NAME + end)) + # We create the dummy module where all the direct dependencies will be loaded + Core.eval(fromparent_m, :(module _DirectDeps_ end)) + # We also set a reference to LoadedModules for access from the notebook + Core.eval(fromparent_m, :(const _LoadedModules_ = $LoadedModules)) + return fromparent_m end # This will explicitly import each direct dependency of the package inside the LoadedModules module. Loading all of the direct dependencies will help make every dependency available even if not directly loaded in the target source code. @@ -168,9 +167,15 @@ function load_module_in_caller(mod_exp::Expr, package_dict::Dict, caller_module) target_file = package_dict["target"] ecg = default_ecg() # If the module Reference inside fromparent_module is not assigned, we create the module in the calling workspace and assign it - _MODULE_ = maybe_create_module(caller_module) + _MODULE_ = maybe_create_module() # We reset the module path in case it was not cleaned mod_name = mod_exp.args[2] + # We reset the list of symbols if we loaded a different module + stored_module = get_stored_module() + if !isnothing(stored_module) && nameof(stored_module) !== mod_name + # We reset the list of previous symbols + empty!(PREVIOUSLY_IMPORTED_NAMES) + end # We inject the project in the LOAD_PATH if it is not present already add_loadpath(ecg; should_prepend = Settings.get_setting(package_dict, :SHOULD_PREPEND_LOAD_PATH)) # We start by loading each of the direct dependencies in the LoadedModules submodule @@ -184,7 +189,9 @@ function load_module_in_caller(mod_exp::Expr, package_dict::Dict, caller_module) rethrow(e) end # Get the moduleof the parent package - __module = getfield(_MODULE_, mod_name) + __module = getproperty(_MODULE_, mod_name)::Module + # We put the module in the dict + package_dict["Created Module"] = __module # We put the dict inside the loaded module Core.eval(__module, :(_fromparent_dict_ = $package_dict)) # Register this module as root module. diff --git a/src/frompackage/macro.jl b/src/frompackage/macro.jl index 26811c2..55269f7 100644 --- a/src/frompackage/macro.jl +++ b/src/frompackage/macro.jl @@ -83,6 +83,8 @@ function frompackage(ex, target_file, caller, caller_module; macroname) end # We update th stored root module update_stored_module(package_dict) + # We put the included names in PREVIOUSLY_IMPORTED_NAMES + overwrite_imported_symbols(package_dict) # We call at runtime the function to trigger extensions loading push!(args, :($try_load_extensions($package_dict))) # We wrap the import expressions inside a try-catch, as those also correctly work from there. diff --git a/src/frompackage/types.jl b/src/frompackage/types.jl index 6a99253..705b1ef 100644 --- a/src/frompackage/types.jl +++ b/src/frompackage/types.jl @@ -2,7 +2,9 @@ const _stdlibs = first.(values(Pkg.Types.stdlibs())) const default_pkg_io = Ref{IO}(devnull) -const fromparent_module = Ref{Module}() +const TEMP_MODULE_NAME = :_FromPackage_TempModule_ +const STORED_MODULE = Ref{Union{Module, Nothing}}(nothing) +const PREVIOUSLY_IMPORTED_NAMES = Set{Symbol}() const macro_cell = Ref("undefined") const manifest_names = ("JuliaManifest.toml", "Manifest.toml") From 56a95065a316854a0195a3752eb74d7444874cdf Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Mon, 3 Jun 2024 18:35:27 +0200 Subject: [PATCH 2/7] small fixes --- src/frompackage/code_parsing.jl | 2 +- src/frompackage/helpers.jl | 6 ++- test/frompackage/basics.jl | 68 +++++++++++++++++---------------- 3 files changed, 41 insertions(+), 35 deletions(-) diff --git a/src/frompackage/code_parsing.jl b/src/frompackage/code_parsing.jl index ca88ae3..ac703ee 100644 --- a/src/frompackage/code_parsing.jl +++ b/src/frompackage/code_parsing.jl @@ -67,7 +67,7 @@ function modify_package_using!(ex::Expr, loc, package_dict::Dict, eval_module::M extracted_package_name = first(package_expr_args) if extracted_package_name === package_name # We modify the specific using expression to point to the correct module path - prepend!(package_expr_args, modname_path(fromparent_module[])) + prepend!(package_expr_args, modname_path(get_temp_module())) end end return true diff --git a/src/frompackage/helpers.jl b/src/frompackage/helpers.jl index b56d3a1..1d6f8a1 100644 --- a/src/frompackage/helpers.jl +++ b/src/frompackage/helpers.jl @@ -325,8 +325,10 @@ function update_stored_module(package_dict::Dict) update_stored_module(m) end -function overwrite_imported_symbols(package_dict) +overwrite_imported_symbols(package_dict::Dict) = overwrite_imported_symbols(get(Set{Symbol}, package_dict, "Imported Symbols")) +# This overwrites the PREVIOUSLY_IMPORTED_SYMBOLS with the contents of new_symbols +function overwrite_imported_symbols(new_symbols) empty!(PREVIOUSLY_IMPORTED_NAMES) - union!(PREVIOUSLY_IMPORTED_NAMES, package_dict["Imported Symbols"]) + union!(PREVIOUSLY_IMPORTED_NAMES, new_symbols) nothing end \ No newline at end of file diff --git a/test/frompackage/basics.jl b/test/frompackage/basics.jl index 24d0abd..0099b51 100644 --- a/test/frompackage/basics.jl +++ b/test/frompackage/basics.jl @@ -1,4 +1,4 @@ -import PlutoDevMacros.FromPackage: process_outside_pluto!, load_module_in_caller, modname_path, fromparent_module, parseinput, get_package_data, @fromparent, _combined, process_skiplines!, get_temp_module, LineNumberRange, parse_skipline, extract_module_expression, _inrange, filterednames, reconstruct_import_expr, extract_import_args, extract_raw_str, @frompackage, update_stored_module, get_target_module, frompackage, FromPackage +import PlutoDevMacros.FromPackage: process_outside_pluto!, load_module_in_caller, modname_path, parseinput, get_package_data, @fromparent, _combined, process_skiplines!, get_temp_module, LineNumberRange, parse_skipline, extract_module_expression, _inrange, filterednames, reconstruct_import_expr, extract_import_args, extract_raw_str, @frompackage, update_stored_module, get_target_module, frompackage, FromPackage, can_import_in_caller, overwrite_imported_symbols using Test import Pkg @@ -102,11 +102,12 @@ end @testset "Include using names" begin target_dir = abspath(@__DIR__, "../TestUsingNames/") + caller_module = Core.eval(Main, :(module $(gensym(:TestUsingNames)) end)) function f(target) target_file = joinpath(target_dir, target * "#==#00000000-0000-0000-0000-000000000000") dict = get_package_data(target_file) # Load the module - load_module_in_caller(dict, Main) + load_module_in_caller(dict, caller_module) return dict end function has_symbol(symbol, ex::Expr) @@ -122,30 +123,30 @@ end dict = f(target) invalid(ex) = nothing === process_outside_pluto!(deepcopy(ex), dict) # Test that this only works with catchall - @test_throws "You can only use @exclude_using" parseinput(:(@exclude_using import Downloads), dict) + @test_throws "You can only use @exclude_using" parseinput(:(@exclude_using import Downloads), dict; caller_module) # Test that it throws with ill-formed expression - @test_throws "You can only use @exclude_using" parseinput(:(@exclude_using), dict) + @test_throws "You can only use @exclude_using" parseinput(:(@exclude_using), dict; caller_module) # Test the deprecation warning - @test_logs (:warn, r"Use `@exclude_using`") parseinput(:(@include_using import *), dict) + @test_logs (:warn, r"Use `@exclude_using`") parseinput(:(@include_using import *), dict; caller_module) # Test unsupported expression - @test_throws "The provided input expression is not supported" parseinput(:(@asd import *), dict) + @test_throws "The provided input expression is not supported" parseinput(:(@asd import *), dict; caller_module) # Test that even with the @exclude_using macro in front the expression is filtered out outside Pluo @test invalid(:(@exclude_using import *)) # Test that the names are extracted correctly - ex = parseinput(:(import *), dict) + ex = parseinput(:(import *), dict; caller_module) @test has_symbol(:domath, ex) - ex = parseinput(:(@exclude_using import *), dict) + ex = parseinput(:(@exclude_using import *), dict; caller_module) @test !has_symbol(:domath, ex) # Test2 - test2.jl target = "src/test2.jl" dict = f(target) # Test that the names are extracted correctly - ex = parseinput(:(import *), dict) + ex = parseinput(:(import *), dict; caller_module) @test has_symbol(:test1, ex) # test1 is exported by Module Test1 @test has_symbol(:base64encode, ex) # test1 is exported by Module Base64 - ex = parseinput(:(@exclude_using import *), dict) + ex = parseinput(:(@exclude_using import *), dict; caller_module) @test !has_symbol(:test1, ex) @test !has_symbol(:base64encode, ex) @@ -153,18 +154,18 @@ end target = "src/test3.jl" dict = f(target) # Test that the names are extracted correctly, :top_level_func is exported by TestUsingNames - ex = parseinput(:(import *), dict) + ex = parseinput(:(import *), dict; caller_module) @test has_symbol(:top_level_func, ex) - ex = parseinput(:(@exclude_using import *), dict) + ex = parseinput(:(@exclude_using import *), dict; caller_module) @test !has_symbol(:top_level_func, ex) # Test from a file outside the package target = "" dict = f(target) # Test that the names are extracted correctly, :base64encode is exported by Base64 - ex = parseinput(:(import *), dict) + ex = parseinput(:(import *), dict; caller_module) @test has_symbol(:base64encode, ex) - ex = parseinput(:(@exclude_using import *), dict) + ex = parseinput(:(@exclude_using import *), dict; caller_module) @test !has_symbol(:base64encode, ex) # We test the new skipping capabilities of `filterednames` @@ -172,26 +173,28 @@ end target_mod = update_stored_module(dict) m = Module(gensym()) m.top_level_func = target_mod.top_level_func - @test :top_level_func ∉ filterednames(target_mod, m; package_dict=dict) - # We now overwrite the module to mimic reloading the macro - package_dict = f(target) - new_mod = get_target_module(package_dict) - @test :top_level_func ∈ filterednames(new_mod, m; package_dict) + @test :top_level_func ∉ filterednames(target_mod; caller_module = m) + # We test that it will be imported in a new module without clash + @test :top_level_func ∈ filterednames(target_mod; caller_module = Module(gensym())) + # We test that it will also be imported with clash if the name was explicitly imported before. It will still throw as there is already a variable with that name defined in the caller but the filterins should not exclude it + overwrite_imported_symbols([:top_level_func]) + @test :top_level_func ∈ filterednames(target_mod; caller_module = m) + # We test the warning if we are trying to overwrite something we didn't put - Core.eval(m, :(voila() = 5)) - Core.eval(new_mod, :(voila() = 6)) - @test_logs (:warn, r"is already defined in the caller module") filterednames(new_mod, m; package_dict, explicit_names=Set([:voila])) + overwrite_imported_symbols([]) + @test_logs (:warn, r"is already present in the caller module") filterednames(target_mod; caller_module = m) end @testset "Inside Pluto" begin @testset "Input Parsing" begin @testset "target included in Package" begin - dict = load_module_in_caller(inpackage_target, Main) - f(ex) = parseinput(deepcopy(ex), dict) + caller_module = Module(gensym()) + dict = load_module_in_caller(inpackage_target, caller_module) + f(ex) = parseinput(deepcopy(ex), dict; caller_module) - parent_path = modname_path(fromparent_module[]) + parent_path = modname_path(get_temp_module()) # FromDeps imports ex = :(using >.BenchmarkTools) @@ -213,12 +216,12 @@ end # Test indirect import indirect_id = Base.PkgId(Base.UUID("682c06a0-de6a-54ab-a142-c8b1cf79cde6"), "JSON") # This will load Tricks inside _DepsImports_ - _ex = parseinput(:(using >.JSON), dict) + _ex = f(:(using >.JSON)) # We now test that Tricks is loaded in DepsImports @test LoadedModules.JSON === Base.maybe_root_module(indirect_id) # We test that trying to load a package that is not a dependency throws an error saying so - @test_throws "The package DataFrames was not" parseinput(:(using >.DataFrames), dict) + @test_throws "The package DataFrames was not" f(:(using >.DataFrames)) # FromPackage imports ex = :(import TestPackage) @@ -258,17 +261,18 @@ end @test expected == f(ex) end @testset "target not included in Package" begin - dict = load_module_in_caller(inpluto_caller, Main) - f(ex) = parseinput(deepcopy(ex), dict) - parent_path = modname_path(fromparent_module[]) + caller_module = Module(gensym()) + dict = load_module_in_caller(inpluto_caller, caller_module) + f(ex) = parseinput(deepcopy(ex), dict; caller_module) + parent_path = modname_path(get_temp_module()) ex = :(import PackageModule.Issue2) expected = :(import $(parent_path...).TestPackage.Issue2) @test expected == f(ex) ex = :(@exclude_using import *) - expected = let _mod = fromparent_module[].TestPackage - imported_names = filterednames(_mod; all=true, imported=true) + expected = let _mod = get_temp_module().TestPackage + imported_names = filterednames(_mod; all=true, imported=true, caller_module) importednames_exprs = map(n -> Expr(:., n), imported_names) modname_expr = Expr(:., vcat(parent_path, :TestPackage)...) reconstruct_import_expr(modname_expr, importednames_exprs) From 1a54e80060dcbbc849913defdfb63bf6de5f8a9f Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Tue, 4 Jun 2024 10:40:36 +0200 Subject: [PATCH 3/7] remove unused function --- src/frompackage/code_parsing.jl | 2 +- src/frompackage/input_parsing.jl | 22 +++++----------------- test/frompackage/basics.jl | 6 +++--- 3 files changed, 9 insertions(+), 21 deletions(-) diff --git a/src/frompackage/code_parsing.jl b/src/frompackage/code_parsing.jl index ac703ee..e2df39b 100644 --- a/src/frompackage/code_parsing.jl +++ b/src/frompackage/code_parsing.jl @@ -67,7 +67,7 @@ function modify_package_using!(ex::Expr, loc, package_dict::Dict, eval_module::M extracted_package_name = first(package_expr_args) if extracted_package_name === package_name # We modify the specific using expression to point to the correct module path - prepend!(package_expr_args, modname_path(get_temp_module())) + prepend!(package_expr_args, temp_module_path()) end end return true diff --git a/src/frompackage/input_parsing.jl b/src/frompackage/input_parsing.jl index 1c6e3c8..41b8fff 100644 --- a/src/frompackage/input_parsing.jl +++ b/src/frompackage/input_parsing.jl @@ -145,20 +145,8 @@ function import_type(args, dict) error("The provided import expression is not supported, please look at @frompackage documentation to see the supported imports") end -# Get the full path of the module as array of Symbols starting from Main -function modname_path(m::Module) - args = [nameof(m)] - m_old = m - _m = parentmodule(m) - while _m !== m_old && _m !== Main - m_old = _m - pushfirst!(args, nameof(_m)) - _m = parentmodule(_m) - end - _m === Main || error("modname_path did not reach Main, this is not expected") - pushfirst!(args, nameof(_m)) - return args -end +# This is the path of the temp module which will be prepended to the package name +temp_module_path() = (:Main, TEMP_MODULE_NAME) ## process outside pluto # We parse all the expressions in the block provided as input to @fromparent @@ -259,12 +247,12 @@ function process_imported_nameargs!(args, dict) end ## Per-type versions function process_imported_nameargs!(args, dict, t::FromPackageImport) - name_init = modname_path(get_temp_module()) + name_init = temp_module_path() args[1] = t.mod_name prepend!(args, name_init) end function process_imported_nameargs!(args, dict, t::Union{FromParentImport, RelativeImport}) - name_init = modname_path(get_temp_module()) + name_init = temp_module_path() # Here transform the relative module name to the one based on the full loaded module path target_path = get(dict, "Target Path", []) |> reverse isempty(target_path) && error("The current file was not found included in the loaded module $(t.mod_name), so you can't use relative path imports") @@ -285,7 +273,7 @@ function process_imported_nameargs!(args, dict, t::FromDepsImport) maybe_add_loaded_module(t.id) deps_module_name = :_LoadedModules_ args[1] = deps_module_name # We replace `>` with _LoadedModules_ - name_init = modname_path(get_temp_module()) + name_init = temp_module_path() prepend!(args, name_init) return nothing end diff --git a/test/frompackage/basics.jl b/test/frompackage/basics.jl index 0099b51..2233fde 100644 --- a/test/frompackage/basics.jl +++ b/test/frompackage/basics.jl @@ -1,4 +1,4 @@ -import PlutoDevMacros.FromPackage: process_outside_pluto!, load_module_in_caller, modname_path, parseinput, get_package_data, @fromparent, _combined, process_skiplines!, get_temp_module, LineNumberRange, parse_skipline, extract_module_expression, _inrange, filterednames, reconstruct_import_expr, extract_import_args, extract_raw_str, @frompackage, update_stored_module, get_target_module, frompackage, FromPackage, can_import_in_caller, overwrite_imported_symbols +import PlutoDevMacros.FromPackage: process_outside_pluto!, load_module_in_caller, temp_module_path, parseinput, get_package_data, @fromparent, _combined, process_skiplines!, get_temp_module, LineNumberRange, parse_skipline, extract_module_expression, _inrange, filterednames, reconstruct_import_expr, extract_import_args, extract_raw_str, @frompackage, update_stored_module, get_target_module, frompackage, FromPackage, can_import_in_caller, overwrite_imported_symbols using Test import Pkg @@ -194,7 +194,7 @@ end f(ex) = parseinput(deepcopy(ex), dict; caller_module) - parent_path = modname_path(get_temp_module()) + parent_path = temp_module_path() # FromDeps imports ex = :(using >.BenchmarkTools) @@ -264,7 +264,7 @@ end caller_module = Module(gensym()) dict = load_module_in_caller(inpluto_caller, caller_module) f(ex) = parseinput(deepcopy(ex), dict; caller_module) - parent_path = modname_path(get_temp_module()) + parent_path = temp_module_path() ex = :(import PackageModule.Issue2) expected = :(import $(parent_path...).TestPackage.Issue2) From 3b53241f955f72ab44f2f455d52a3a34a48e3b3b Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Tue, 4 Jun 2024 10:40:56 +0200 Subject: [PATCH 4/7] fix error in has_ancestor_module update --- src/frompackage/helpers.jl | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/frompackage/helpers.jl b/src/frompackage/helpers.jl index 1d6f8a1..aa6cede 100644 --- a/src/frompackage/helpers.jl +++ b/src/frompackage/helpers.jl @@ -135,14 +135,15 @@ function filterednames(m::Module; all = true, imported = true, explicit_names = filter(filter_func, filter_args) end -function has_ancestor_module(target::Module, ancestor_name::Symbol; previous = nothing) - has_ancestor_module(target, (ancestor_name,); previous) +function has_ancestor_module(target::Module, ancestor_name::Symbol; previous = nothing, only_rootmodule = false) + has_ancestor_module(target, (ancestor_name,); previous, only_rootmodule) end -function has_ancestor_module(target::Module, ancestor_names; previous = nothing) +function has_ancestor_module(target::Module, ancestor_names; previous = nothing, only_rootmodule = false) nm = nameof(target) - nm in ancestor_names && return true # Ancestor found - nm === previous && return false # The target is the same as previous, so we reached a top-level module - return has_ancestor_module(parentmodule(target), ancestor_names; previous = nm) + ancestor_found = nm in ancestor_names + !only_rootmodule && ancestor_found && return true # Ancestor found, and no check on only_rootmodule + nm === previous && return ancestor_found # The target is the same as previous, so we reached a top-level module. We return whether the ancestor was found and is a parent of itself + return has_ancestor_module(parentmodule(target), ancestor_names; previous = nm, only_rootmodule) end # This returns two flags: whether the name can be included and whether a warning should be generated @@ -150,9 +151,10 @@ function can_import_in_caller(name::Symbol, caller::Module) isdefined(caller, name) || return true, false # If is not defined we can surely import it owner = which(caller, name) # Skip (and do not warn) for things defined in Base or Core - has_ancestor_module(owner, (:Base, :Core, :Markdown, :InteractiveUtils)) && return false, false + invalid_ancestor = has_ancestor_module(owner, (:Base, :Core, :Markdown, :InteractiveUtils)) + invalid_ancestor && return false, false # We check if the name is inside the list of symbols imported by the previous module - in_previous = name in PREVIOUSLY_IMPORTED_NAMES + in_previous = name in PREVIOUS_CATCHALL_NAMES return in_previous, !in_previous end @@ -325,10 +327,10 @@ function update_stored_module(package_dict::Dict) update_stored_module(m) end -overwrite_imported_symbols(package_dict::Dict) = overwrite_imported_symbols(get(Set{Symbol}, package_dict, "Imported Symbols")) +overwrite_imported_symbols(package_dict::Dict) = overwrite_imported_symbols(get(Set{Symbol}, package_dict, "Catchall Imported Symbols")) # This overwrites the PREVIOUSLY_IMPORTED_SYMBOLS with the contents of new_symbols function overwrite_imported_symbols(new_symbols) - empty!(PREVIOUSLY_IMPORTED_NAMES) - union!(PREVIOUSLY_IMPORTED_NAMES, new_symbols) + empty!(PREVIOUS_CATCHALL_NAMES) + union!(PREVIOUS_CATCHALL_NAMES, new_symbols) nothing end \ No newline at end of file From ef2dde8c21cbe9ccbda398ad4c27b02835b88fa1 Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Tue, 4 Jun 2024 10:41:15 +0200 Subject: [PATCH 5/7] change variable name --- src/frompackage/input_parsing.jl | 2 +- src/frompackage/loading.jl | 2 +- src/frompackage/macro.jl | 2 +- src/frompackage/types.jl | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/frompackage/input_parsing.jl b/src/frompackage/input_parsing.jl index 41b8fff..3597ead 100644 --- a/src/frompackage/input_parsing.jl +++ b/src/frompackage/input_parsing.jl @@ -338,7 +338,7 @@ function parseinput(ex, package_dict; caller_module) end imported_names = filterednames(_mod; all = catchall, imported = catchall, explicit_names, caller_module) # We add the imported names to the set for tracking names imported by this macrocall - union!(get!(Set{Symbol}, package_dict, "Imported Symbols"), imported_names) + union!(get!(Set{Symbol}, package_dict, "Catchall Imported Symbols"), imported_names) # At this point we have all the names and we just have to create the final expression importednames_exprs = map(n -> Expr(:., n), imported_names) return reconstruct_import_expr(modname_expr, importednames_exprs) diff --git a/src/frompackage/loading.jl b/src/frompackage/loading.jl index b39f684..df370c1 100644 --- a/src/frompackage/loading.jl +++ b/src/frompackage/loading.jl @@ -174,7 +174,7 @@ function load_module_in_caller(mod_exp::Expr, package_dict::Dict, caller_module) stored_module = get_stored_module() if !isnothing(stored_module) && nameof(stored_module) !== mod_name # We reset the list of previous symbols - empty!(PREVIOUSLY_IMPORTED_NAMES) + empty!(PREVIOUS_CATCHALL_NAMES) end # We inject the project in the LOAD_PATH if it is not present already add_loadpath(ecg; should_prepend = Settings.get_setting(package_dict, :SHOULD_PREPEND_LOAD_PATH)) diff --git a/src/frompackage/macro.jl b/src/frompackage/macro.jl index 55269f7..df6ed9b 100644 --- a/src/frompackage/macro.jl +++ b/src/frompackage/macro.jl @@ -83,7 +83,7 @@ function frompackage(ex, target_file, caller, caller_module; macroname) end # We update th stored root module update_stored_module(package_dict) - # We put the included names in PREVIOUSLY_IMPORTED_NAMES + # We put the included names in PREVIOUS_CATCHALL_NAMES overwrite_imported_symbols(package_dict) # We call at runtime the function to trigger extensions loading push!(args, :($try_load_extensions($package_dict))) diff --git a/src/frompackage/types.jl b/src/frompackage/types.jl index 705b1ef..a2a5775 100644 --- a/src/frompackage/types.jl +++ b/src/frompackage/types.jl @@ -4,7 +4,7 @@ const default_pkg_io = Ref{IO}(devnull) const TEMP_MODULE_NAME = :_FromPackage_TempModule_ const STORED_MODULE = Ref{Union{Module, Nothing}}(nothing) -const PREVIOUSLY_IMPORTED_NAMES = Set{Symbol}() +const PREVIOUS_CATCHALL_NAMES = Set{Symbol}() const macro_cell = Ref("undefined") const manifest_names = ("JuliaManifest.toml", "Manifest.toml") From 2d3f5049decdcfb02a842d3e21740b0a7a4e63ec Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Tue, 4 Jun 2024 11:05:37 +0200 Subject: [PATCH 6/7] improve tests --- test/TestUsingNames/src/TestUsingNames.jl | 1 + test/TestUsingNames/test_notebook2.jl | 6 ++++++ test/frompackage/basics.jl | 15 ++++++++++++++- test/frompackage/with_pluto_session.jl | 12 ++++++++++++ 4 files changed, 33 insertions(+), 1 deletion(-) diff --git a/test/TestUsingNames/src/TestUsingNames.jl b/test/TestUsingNames/src/TestUsingNames.jl index cf6a7f4..b849ea9 100644 --- a/test/TestUsingNames/src/TestUsingNames.jl +++ b/test/TestUsingNames/src/TestUsingNames.jl @@ -5,6 +5,7 @@ using Base64 export top_level_func top_level_func() = 1 clash_name = 5 +rand_variable = rand() module Test1 using Example diff --git a/test/TestUsingNames/test_notebook2.jl b/test/TestUsingNames/test_notebook2.jl index e920dd2..b7524cf 100644 --- a/test/TestUsingNames/test_notebook2.jl +++ b/test/TestUsingNames/test_notebook2.jl @@ -49,8 +49,14 @@ isdefined(@__MODULE__, :base64encode) || error("base64encode from Base64 should clash_name === 0 || error("The clashed name was not handled correctly") ╠═╡ =# +# ╔═╡ 4492d516-2b23-45b7-bf76-7458e7352fea +#=╠═╡ +rand_variable # This should change at every re-run + ╠═╡ =# + # ╔═╡ Cell order: # ╠═4f8def86-f90b-4f74-ac47-93fe6e437cee # ╠═ac3d261a-86c9-453f-9d86-23a8f30ca583 # ╠═dd3f662f-e2ce-422d-a91a-487a4da359cc # ╠═c72f2544-eb2e-4ed6-a89b-495ead20b5f6 +# ╠═4492d516-2b23-45b7-bf76-7458e7352fea diff --git a/test/frompackage/basics.jl b/test/frompackage/basics.jl index 2233fde..c825e35 100644 --- a/test/frompackage/basics.jl +++ b/test/frompackage/basics.jl @@ -1,4 +1,4 @@ -import PlutoDevMacros.FromPackage: process_outside_pluto!, load_module_in_caller, temp_module_path, parseinput, get_package_data, @fromparent, _combined, process_skiplines!, get_temp_module, LineNumberRange, parse_skipline, extract_module_expression, _inrange, filterednames, reconstruct_import_expr, extract_import_args, extract_raw_str, @frompackage, update_stored_module, get_target_module, frompackage, FromPackage, can_import_in_caller, overwrite_imported_symbols +import PlutoDevMacros.FromPackage: process_outside_pluto!, load_module_in_caller, temp_module_path, parseinput, get_package_data, @fromparent, _combined, process_skiplines!, get_temp_module, LineNumberRange, parse_skipline, extract_module_expression, _inrange, filterednames, reconstruct_import_expr, extract_import_args, extract_raw_str, @frompackage, update_stored_module, get_target_module, frompackage, FromPackage, can_import_in_caller, overwrite_imported_symbols, has_ancestor_module using Test import Pkg @@ -424,4 +424,17 @@ mktempdir() do tmpdir m = get_target_module(dict) @test isdefined(m, :a) end +end + +# We test has_ancestor_module +@testset "has_ancestor_module" begin + m = Main + for path_name in (:Test1, :Test2, :Test3) + m = Core.eval(m, :(module $path_name end)) + end + @test has_ancestor_module(m, :Test1) # It has an ancestor called :Test1 + @test !has_ancestor_module(m, :Test1; only_rootmodule = true) # It has an ancestor called :Test1, but it's not the root module (the one which is the parent of itself, i.e. Main in this example) + @test has_ancestor_module(m, :Main; only_rootmodule = true) # It has an ancestor Main which is the root + @test has_ancestor_module(m, (:Test2, :Test4)) # it works with either of the elements provided + @test !has_ancestor_module(m, (:Test5, :Test4)) # it works with either of the elements provided end \ No newline at end of file diff --git a/test/frompackage/with_pluto_session.jl b/test/frompackage/with_pluto_session.jl index 74b643b..683df22 100644 --- a/test/frompackage/with_pluto_session.jl +++ b/test/frompackage/with_pluto_session.jl @@ -108,9 +108,21 @@ srcdir = joinpath(@__DIR__, "../TestUsingNames/src/") for filename in ["test_notebook1.jl", "test_notebook2.jl"] path = abspath(srcdir, "..", filename) nb = SessionActions.open(ss, path; run_async=false) + # We test that no errors are present for cell in nb.cells @test noerror(cell) end + # We extract the rand_variable value + first_value = eval_in_nb((ss, nb), :rand_variable) + # We rerun the second cell, containing the `@fromparent` call + update_run!(ss, nb, nb.cells[2]) + # We check again that no errors arose + for cell in nb.cells + @test noerror(cell) + end + # We check that the rand_variable value changed + second_value = eval_in_nb((ss, nb), :rand_variable) + @test first_value != second_value SessionActions.shutdown(ss, nb) end end \ No newline at end of file From 31aee2ca99ccff581b4b07a8788cb3087eaf761d Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Tue, 4 Jun 2024 11:55:36 +0200 Subject: [PATCH 7/7] fix tests --- test/frompackage/basics.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/frompackage/basics.jl b/test/frompackage/basics.jl index c825e35..b24be96 100644 --- a/test/frompackage/basics.jl +++ b/test/frompackage/basics.jl @@ -194,7 +194,7 @@ end f(ex) = parseinput(deepcopy(ex), dict; caller_module) - parent_path = temp_module_path() + parent_path = temp_module_path() |> collect # FromDeps imports ex = :(using >.BenchmarkTools) @@ -264,7 +264,7 @@ end caller_module = Module(gensym()) dict = load_module_in_caller(inpluto_caller, caller_module) f(ex) = parseinput(deepcopy(ex), dict; caller_module) - parent_path = temp_module_path() + parent_path = temp_module_path() |> collect ex = :(import PackageModule.Issue2) expected = :(import $(parent_path...).TestPackage.Issue2)