Make @include_using the default catch-all behavior

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.
+This statement can be used alone or coupled with other valid import statements within a `begin ... end` block.

src/PlutoDevMacros.jl

@@ -37,7 +37,6 @@ include("../notebooks/plutoinclude_macro.jl") # hasexpr, default_exprlist, inclu
 #                 logs.parentElement.style.display = "none"
 #             }
 #         </script>""")
-#         @info "GESU"
 #     else
 
 #     end

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
+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)

src/frompackage/input_parsing.jl

@@ -256,25 +256,34 @@ 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
     @assert try 
         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)

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"

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

test/TestPackage/src/inner_notebook2.jl

@@ -1,5 +1,5 @@
 ### A Pluto.jl notebook ###
-# v0.19.25
+# v0.19.42
 
 using Markdown
 using InteractiveUtils

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

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

test/TestUsingNames/test_notebook2.jl

@@ -28,12 +28,14 @@ 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
   ╠═╡ =#
 
 # ╔═╡ ac3d261a-86c9-453f-9d86-23a8f30ca583
 #=╠═╡
-@fromparent @include_using import *
+@fromparent import *
   ╠═╡ =#
 
 # ╔═╡ dd3f662f-e2ce-422d-a91a-487a4da359cc
@@ -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

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,46 +116,65 @@ 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)
 
     # Test3 - test3.jl
     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)

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)