From c78022c4b684b9d0fa95a5eb97c69611e2cf1b37 Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Sun, 26 May 2024 14:31:47 +0200 Subject: [PATCH 1/5] cleanup --- src/PlutoDevMacros.jl | 1 - 1 file changed, 1 deletion(-) diff --git a/src/PlutoDevMacros.jl b/src/PlutoDevMacros.jl index e8bb951..af7beb7 100644 --- a/src/PlutoDevMacros.jl +++ b/src/PlutoDevMacros.jl @@ -37,7 +37,6 @@ include("../notebooks/plutoinclude_macro.jl") # hasexpr, default_exprlist, inclu # logs.parentElement.style.display = "none" # } # """) -# @info "GESU" # else # end From b73aab057a21edf70e8f5a8cbe1d7c93872a6e37 Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Sun, 26 May 2024 14:32:37 +0200 Subject: [PATCH 2/5] make include_using default and add filtering of imported names --- src/frompackage/helpers.jl | 43 ++++++++++++++++++++++++++------ src/frompackage/input_parsing.jl | 29 +++++++++++++-------- src/frompackage/macro.jl | 12 +++++---- src/frompackage/types.jl | 2 ++ 4 files changed, 64 insertions(+), 22 deletions(-) diff --git a/src/frompackage/helpers.jl b/src/frompackage/helpers.jl index a35217b..bf7876f 100644 --- a/src/frompackage/helpers.jl +++ b/src/frompackage/helpers.jl @@ -188,7 +188,7 @@ end getfirst(itr) = getfirst(x -> true, itr) ## filterednames -function filterednames(m::Module; all = true, imported = true, explicit_names = nothing) +function filterednames(m::Module, caller_module = nothing; all = true, imported = true, explicit_names = nothing, package_dict = nothing) excluded = (:eval, :include, :_fromparent_dict_, Symbol("@bind")) mod_names = names(m;all, imported) filter_args = if explicit_names isa Set{Symbol} @@ -199,11 +199,35 @@ function filterednames(m::Module; all = true, imported = true, explicit_names = else mod_names end - filter(filter_args) do s - Base.isgensym(s) && return false - s in excluded && return false - return true - end + filter_func = filterednames_filter_func(m; excluded, caller_module, package_dict) + 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 + 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 + end + end + return f end @@ -315,4 +339,9 @@ function register_target_module_as_root(dict) # Set the path of the module to the actual package Base.set_pkgorigin_version_path(id, entry_point) end -end \ No newline at end of file +end + +# This function will get the module stored in the created_modules dict based on the entry point +get_stored_module(dict) = get(created_modules, dict["file"], nothing) +# This will store in it +update_stored_module(dict) = created_modules[dict["file"]] = get_target_module(dict) \ No newline at end of file diff --git a/src/frompackage/input_parsing.jl b/src/frompackage/input_parsing.jl index 913d78d..a9ad643 100644 --- a/src/frompackage/input_parsing.jl +++ b/src/frompackage/input_parsing.jl @@ -256,9 +256,12 @@ function process_imported_nameargs!(args, dict, t::FromDepsImport) end # Check if the input expression has the `@include_using` macro. This function will also modify the expression to remove the `@include_using` macro -function has_include_using!(ex) - Meta.isexpr(ex, :macrocall) || return false - ex.args[1] === Symbol("@include_using") || return false +function should_include_using_names!(ex) + Meta.isexpr(ex, :macrocall) || return contains_catchall(ex) + macro_name = ex.args[1] + deprecated_name = Symbol("@include_using") + exclude_name = Symbol("@exclude_using") + @assert macro_name in (deprecated_name, exclude_name) "The provided input expression is not supported.\nExpressions should be only import statements, at most prepended by the `@exclude_using` macro in combination with catch-all import statements." # If we reach here, we have the include usings. We just extract the underlying expression actual_ex = ex.args[end] # Check if this contains catchall @@ -266,15 +269,21 @@ function has_include_using!(ex) contains_catchall(actual_ex) catch false - end "You can only use @include_using with the catchall import statement (e.g. `@include_using import *`).\nInstead you passed [$(ex)] as input." + end "You can only use @exclude_using with the catchall import statement (e.g. `@exclude_using import *`).\nInstead you passed [$(ex)] as input." + should_include = if macro_name === deprecated_name + @warn "The use of `@include_using` is deprecated and it's assumed at default. Use `@exclude_using` if you want to avoid including names from `using`" + true + elseif macro_name === exclude_name + false + end ex.head = actual_ex.head ex.args = actual_ex.args - return true + return should_include end ## parseinput -function parseinput(ex, dict) - include_using = has_include_using!(ex) +function parseinput(ex, package_dict; caller_module = nothing) + include_using = should_include_using_names!(ex) # We get the module modname_expr, importednames_exprs = extract_import_args(ex) # Check if we have a catchall @@ -283,7 +292,7 @@ function parseinput(ex, dict) # which names to eventually import, but all statements are converted into # `import` if they are not of type FromDepsImport is_using = ex.head === :using - args, type = process_imported_nameargs!(modname_expr.args, dict) + args, type = process_imported_nameargs!(modname_expr.args, package_dict) # Check that we don't catchall with imported dependencies if catchall && type isa FromDepsImport error("You can't use the catch-all name identifier (*) while importing dependencies of the Package Environment") @@ -300,12 +309,12 @@ function parseinput(ex, dict) return ex end explicit_names = if include_using - dict["Using Names"].explicit_names + package_dict["Using Names"].explicit_names else nothing end # We extract the imported names either due to catchall or due to the standard using - imported_names = filterednames(_mod; all = catchall, imported = catchall, explicit_names) + imported_names = filterednames(_mod, caller_module; all = catchall, imported = catchall, explicit_names, package_dict) # 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/macro.jl b/src/frompackage/macro.jl index c5d3de5..dbea906 100644 --- a/src/frompackage/macro.jl +++ b/src/frompackage/macro.jl @@ -54,7 +54,7 @@ function frompackage(ex, target_file, caller, caller_module; macroname) id_name = _id_name(cell_id) ex isa Expr || error("You have to call this macro with an import statement or a begin-end block of import statements") # Try to load the module of the target package in the calling workspace and return the dict with extracted paramteres - dict = if is_call_unique(cell_id, caller_module) + package_dict = if is_call_unique(cell_id, caller_module) dict = get_package_data(target_file) # We try to extract eventual lines to skip process_skiplines!(ex, dict) @@ -66,7 +66,7 @@ function frompackage(ex, target_file, caller, caller_module; macroname) # We extract the parse dict ex_args = if Meta.isexpr(ex, [:import, :using]) [ex] - elseif Meta.isexpr(ex, :macrocall) && ex.args[1] === Symbol("@include_using") + elseif Meta.isexpr(ex, :macrocall) && ex.args[1] in (Symbol("@include_using"), Symbol("@exclude_using")) # This is @include_using [ex] elseif Meta.isexpr(ex, :block) @@ -77,12 +77,14 @@ function frompackage(ex, target_file, caller, caller_module; macroname) # We now process/parse all the import/using statements for arg in ex_args arg isa LineNumberNode && continue - push!(args, parseinput(arg, dict)) + push!(args, parseinput(arg, package_dict; caller_module)) end + # We add this module to the created_modules dict + created_modules[package_dict["file"]] = get_target_module(package_dict) # Now we add the call to maybe load the package extensions - push!(args, :($load_package_extensions($dict, @__MODULE__))) + push!(args, :($load_package_extensions($package_dict, @__MODULE__))) # Register this module as root module. We need to this after trying to load the extensions - push!(args, :($register_target_module_as_root($dict))) + push!(args, :($register_target_module_as_root($package_dict))) # We wrap the import expressions inside a try-catch, as those also correctly work from there. # This also allow us to be able to catch the error in case something happens during loading and be able to gracefully clean the work space text = "Reload $macroname" diff --git a/src/frompackage/types.jl b/src/frompackage/types.jl index fae8e64..2d46a9a 100644 --- a/src/frompackage/types.jl +++ b/src/frompackage/types.jl @@ -4,6 +4,8 @@ const fromparent_module = Ref{Module}() const macro_cell = Ref("undefined") const manifest_names = ("JuliaManifest.toml", "Manifest.toml") +const created_modules = Dict{String, Module}() + struct PkgInfo name::Union{Nothing, String} uuid::Base.UUID From 4d155a6c13748d068b19991a92caedf7c8681d78 Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Sun, 26 May 2024 14:32:48 +0200 Subject: [PATCH 3/5] update tests --- test/TestPackage/src/inner_notebook2.jl | 2 +- test/TestUsingNames/test_notebook1.jl | 2 +- test/TestUsingNames/test_notebook2.jl | 2 +- test/frompackage/basics.jl | 57 ++++++++++++++++--------- test/frompackage/with_pluto_session.jl | 2 +- 5 files changed, 42 insertions(+), 23 deletions(-) diff --git a/test/TestPackage/src/inner_notebook2.jl b/test/TestPackage/src/inner_notebook2.jl index e5a0e2c..083016d 100644 --- a/test/TestPackage/src/inner_notebook2.jl +++ b/test/TestPackage/src/inner_notebook2.jl @@ -1,5 +1,5 @@ ### A Pluto.jl notebook ### -# v0.19.25 +# v0.19.42 using Markdown using InteractiveUtils diff --git a/test/TestUsingNames/test_notebook1.jl b/test/TestUsingNames/test_notebook1.jl index 01577bb..b648ccc 100644 --- a/test/TestUsingNames/test_notebook1.jl +++ b/test/TestUsingNames/test_notebook1.jl @@ -33,7 +33,7 @@ end # ╔═╡ 23a1bdef-5c31-4c90-a7f5-1f5a806d3d2e #=╠═╡ -@fromparent import * +@fromparent @exclude_using import * ╠═╡ =# # ╔═╡ e4f436ed-27e9-4d19-98bd-c2b3021cf8bd diff --git a/test/TestUsingNames/test_notebook2.jl b/test/TestUsingNames/test_notebook2.jl index fec0a65..e48ae23 100644 --- a/test/TestUsingNames/test_notebook2.jl +++ b/test/TestUsingNames/test_notebook2.jl @@ -33,7 +33,7 @@ end # ╔═╡ ac3d261a-86c9-453f-9d86-23a8f30ca583 #=╠═╡ -@fromparent @include_using import * +@fromparent import * ╠═╡ =# # ╔═╡ dd3f662f-e2ce-422d-a91a-487a4da359cc diff --git a/test/frompackage/basics.jl b/test/frompackage/basics.jl index ffa23b9..9887cf6 100644 --- a/test/frompackage/basics.jl +++ b/test/frompackage/basics.jl @@ -1,8 +1,8 @@ -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 -import Pkg - +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 using Test +import Pkg + # The LOAD_PATH hack is required because if we add ./TestPackage as a test dependency we get the error in https://github.com/JuliaLang/Pkg.jl/issues/1585 push!(LOAD_PATH, normpath(@__DIR__, "../TestPackage")) import TestPackage @@ -116,26 +116,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 @include_using" parseinput(:(@include_using import Downloads), dict) + @test_throws "You can only use @exclude_using" parseinput(:(@exclude_using import Downloads), dict) # Test that it throws with ill-formed expression - @test_throws "You can only use @include_using" parseinput(:(@include_using), dict) - - # Test that even with the @include_using macro in front the expression is filtered out outside Pluo - @test invalid(:(@include_using import *)) + @test_throws "You can only use @exclude_using" parseinput(:(@exclude_using), dict) + # Test the deprecation warning + @test_logs (:warn, r"Use `@exclude_using`") parseinput(:(@include_using import *), dict) + # Test unsupported expression + @test_throws "The provided input expression is not supported" parseinput(:(@asd import *), dict) + + # 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(:(@include_using import *), dict) - @test has_symbol(:domath, ex) ex = parseinput(:(import *), dict) + @test has_symbol(:domath, ex) + ex = parseinput(:(@exclude_using import *), dict) @test !has_symbol(:domath, ex) # Test2 - test2.jl target = "src/test2.jl" dict = f(target) # Test that the names are extracted correctly - ex = parseinput(:(@include_using import *), dict) + ex = parseinput(:(import *), dict) @test has_symbol(:test1, ex) # test1 is exported by Module Test1 @test has_symbol(:base64encode, ex) # test1 is exported by Module Base64 - ex = parseinput(:(import *), dict) + ex = parseinput(:(@exclude_using import *), dict) @test !has_symbol(:test1, ex) @test !has_symbol(:base64encode, ex) @@ -143,19 +147,34 @@ end target = "src/test3.jl" dict = f(target) # Test that the names are extracted correctly, :top_level_func is exported by TestUsingNames - ex = parseinput(:(@include_using import *), dict) - @test has_symbol(:top_level_func, ex) ex = parseinput(:(import *), dict) + @test has_symbol(:top_level_func, ex) + ex = parseinput(:(@exclude_using import *), dict) @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(:(@include_using import *), dict) - @test has_symbol(:base64encode, ex) ex = parseinput(:(import *), dict) + @test has_symbol(:base64encode, ex) + ex = parseinput(:(@exclude_using import *), dict) @test !has_symbol(:base64encode, ex) + + # We test the new skipping capabilities of `filterednames` + # We save the loaded module in the created_modules variable + 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) + # 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])) end @@ -216,11 +235,11 @@ end @test_throws UndefVarError f(ex) # It can't find the module # FromParent import - ex = :(import *) + ex = :(@exclude_using import *) expected = :(import $(parent_path...).PlutoDevMacros.FromPackage: @addmethod, @frompackage, @fromparent, FromPackage, Pkg, TOML, _cell_data) @test expected == f(ex) - ex = :(import ParentModule: *) + ex = :(@exclude_using import ParentModule: *) @test expected == f(ex) ex = :(import ParentModule: _cell_data) @@ -240,7 +259,7 @@ end expected = :(import $(parent_path...).PlutoDevMacros.PlutoCombineHTL) @test expected == f(ex) - ex = :(import *) + ex = :(@exclude_using import *) expected = let _mod = fromparent_module[].PlutoDevMacros imported_names = filterednames(_mod; all = true, imported = true) importednames_exprs = map(n -> Expr(:., n), imported_names) diff --git a/test/frompackage/with_pluto_session.jl b/test/frompackage/with_pluto_session.jl index 06d3fa9..74b643b 100644 --- a/test/frompackage/with_pluto_session.jl +++ b/test/frompackage/with_pluto_session.jl @@ -101,7 +101,7 @@ srcdir = joinpath(@__DIR__, "../TestDevDependency/src/") SessionActions.shutdown(ss, nb) end -# We test @include_using (issue 11) +# We test @exclude_using (issue 11) srcdir = joinpath(@__DIR__, "../TestUsingNames/src/") @testset "Using Names" begin ss = ServerSession(; options) From 599317181820441ed4c9536e2547be205de81d6d Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Sun, 26 May 2024 14:40:14 +0200 Subject: [PATCH 4/5] add small test --- src/frompackage/helpers.jl | 2 +- test/TestUsingNames/src/TestUsingNames.jl | 1 + test/TestUsingNames/test_notebook2.jl | 8 ++++++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/frompackage/helpers.jl b/src/frompackage/helpers.jl index bf7876f..56b4859 100644 --- a/src/frompackage/helpers.jl +++ b/src/frompackage/helpers.jl @@ -219,7 +219,7 @@ function filterednames_filter_func(m; excluded, caller_module, package_dict) # 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" + @warn "Symbol `:$s`, is already defined in the caller module and points to a different object. Skipping" end end return false diff --git a/test/TestUsingNames/src/TestUsingNames.jl b/test/TestUsingNames/src/TestUsingNames.jl index d622cd7..cf6a7f4 100644 --- a/test/TestUsingNames/src/TestUsingNames.jl +++ b/test/TestUsingNames/src/TestUsingNames.jl @@ -4,6 +4,7 @@ using Base64 export top_level_func top_level_func() = 1 +clash_name = 5 module Test1 using Example diff --git a/test/TestUsingNames/test_notebook2.jl b/test/TestUsingNames/test_notebook2.jl index e48ae23..e920dd2 100644 --- a/test/TestUsingNames/test_notebook2.jl +++ b/test/TestUsingNames/test_notebook2.jl @@ -28,6 +28,8 @@ begin end using Main.Revise using Main.PlutoDevMacros + # We defined a variable here that clashes with something in the target to show the warning + clash_name = 0 end ╠═╡ =# @@ -42,7 +44,13 @@ end isdefined(@__MODULE__, :base64encode) || error("base64encode from Base64 should be defined") ╠═╡ =# +# ╔═╡ c72f2544-eb2e-4ed6-a89b-495ead20b5f6 +#=╠═╡ +clash_name === 0 || error("The clashed name was not handled correctly") + ╠═╡ =# + # ╔═╡ Cell order: # ╠═4f8def86-f90b-4f74-ac47-93fe6e437cee # ╠═ac3d261a-86c9-453f-9d86-23a8f30ca583 # ╠═dd3f662f-e2ce-422d-a91a-487a4da359cc +# ╠═c72f2544-eb2e-4ed6-a89b-495ead20b5f6 From 7db98e40b041c85a974c444597d91367dc0e9f6e Mon Sep 17 00:00:00 2001 From: Alberto Mengali Date: Sun, 26 May 2024 15:07:14 +0200 Subject: [PATCH 5/5] update docs --- docs/src/frompackage/import_statements.md | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/docs/src/frompackage/import_statements.md b/docs/src/frompackage/import_statements.md index 2011ff1..ed698e0 100644 --- a/docs/src/frompackage/import_statements.md +++ b/docs/src/frompackage/import_statements.md @@ -106,14 +106,9 @@ If for example you had `using Base64` at the top of your module, to effectively using >.Base64 end ``` -This is now not strictly necessary anymore, as `@frompackage`/`@fromparent` now recognize the special macro `@include_using` during expansion. This macro is not actually defined in the package but tells the `@fromparent` call to also re-export names exposed by `using` statements when paired with a [Catch-all statement](#Catch-All). -Since v0.7.3, you can re-export everything including names from `using` statements with the following: +Since v0.8.0 of PlutoDevMacros, the automatic inclusion of names exposed via `using` statements is the default behavior when doing `catch-all` imports. +If one wants to revert back to the previous version where only names effectively defined within the target module (or explicitly imported with `import OtherPackage: x`) would be brought into the Pluto module's scope, it is sufficient to prepend the `@exclude_using` macro to the _catch-all_ import statement like so: ```julia -@fromparent @include_using import * +@fromparent @exclude_using import * ``` -This statement can be used alone or coupled with other valid import statements within a `begin ... end` block. -!!! note - Only a catch-all statement is supported after the `@include_using` macro. - \ - \ - For the moment this behavior is opt-in (via `@include_using`) to avoid a breaking change, but it will likely become the default catch-all behavior at the next breaking release. \ No newline at end of file +This statement can be used alone or coupled with other valid import statements within a `begin ... end` block. \ No newline at end of file