From 2224a0e6897ce72514d1dc3482ab0eaf70f109d4 Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Tue, 3 Dec 2024 17:12:49 -0500 Subject: [PATCH 01/11] group UndefVarError hints by parent module --- stdlib/REPL/src/REPL.jl | 74 +++++++++++++++++++++++++++++------------ 1 file changed, 52 insertions(+), 22 deletions(-) diff --git a/stdlib/REPL/src/REPL.jl b/stdlib/REPL/src/REPL.jl index 6c3f4bd4ba73a..24f10c5b756ee 100644 --- a/stdlib/REPL/src/REPL.jl +++ b/stdlib/REPL/src/REPL.jl @@ -55,37 +55,67 @@ function UndefVarError_hint(io::IO, ex::UndefVarError) else scope = undef end - if scope !== Base && !_UndefVarError_warnfor(io, Base, var) - warned = false - for m in Base.loaded_modules_order - m === Core && continue - m === Base && continue - m === Main && continue - m === scope && continue - warned |= _UndefVarError_warnfor(io, m, var) + if scope !== Base + warned = _UndefVarError_warnfor(io, [Base], var) + + if !warned + modules_to_check = (m for m in Base.loaded_modules_order + if m !== Core && m !== Base && m !== Main && m !== scope) + warned |= _UndefVarError_warnfor(io, modules_to_check, var) end - warned || - _UndefVarError_warnfor(io, Core, var) || - _UndefVarError_warnfor(io, Main, var) + + warned || _UndefVarError_warnfor(io, [Core, Main], var) end return nothing end -function _UndefVarError_warnfor(io::IO, m::Module, var::Symbol) - Base.isbindingresolved(m, var) || return false - (Base.isexported(m, var) || Base.ispublic(m, var)) || return false +function _UndefVarError_warnfor(io::IO, modules, var::Symbol) active_mod = Base.active_module() - print(io, "\nHint: ") - if isdefined(active_mod, Symbol(m)) - print(io, "a global variable of this name also exists in $m.") - else - if Symbol(m) == var - print(io, "$m is loaded but not imported in the active module $active_mod.") + + warned = false + # collect modules which export or make public the variable by + # the parentmodule in which the variable is defined + to_warn_about = Dict{Module, Vector{Module}}() + for m in modules + # only warn if exported or public TODO why did this used to check isbindingresolved? + if !Base.isexported(m, var) && !Base.ispublic(m, var) + continue + end + warned = true + + # handle case where the undefined variable is the name of a loaded module + if Symbol(m) == var && !isdefined(active_mod, var) + print(io, "\nHint: $m is loaded but not imported in the active module $active_mod.") + continue + end + + parent_m = parentmodule(getproperty(m, var)) + if !haskey(to_warn_about, parent_m) + to_warn_about[parent_m] = [m] else - print(io, "a global variable of this name may be made accessible by importing $m in the current active module $active_mod") + push!(to_warn_about[parent_m], m) + end + end + + if !isempty(to_warn_about) + for (parent_m, modules) in pairs(to_warn_about) + print(io, "\nHint: a global variable of this name also exists in ", parent_m) + for m in modules + m == parent_m && continue + how_available = if Base.isexported(m, var) + "exported by" + elseif Base.ispublic(m, var) + "made available as public by" + end + print(io, "\n - Also $how_available $m") + if !isdefined(active_mod, Symbol(m)) + print(io, ", which would need to be imported in $active_mod") + end + print(io, ".") + end end end - return true + return warned end function __init__() From 00d518121952d40af379da26e5c26fbe39acff2c Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Wed, 4 Dec 2024 15:42:32 -0500 Subject: [PATCH 02/11] also filter on isbindingresolved --- stdlib/REPL/src/REPL.jl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stdlib/REPL/src/REPL.jl b/stdlib/REPL/src/REPL.jl index 24f10c5b756ee..4e8b44c6ff374 100644 --- a/stdlib/REPL/src/REPL.jl +++ b/stdlib/REPL/src/REPL.jl @@ -77,8 +77,8 @@ function _UndefVarError_warnfor(io::IO, modules, var::Symbol) # the parentmodule in which the variable is defined to_warn_about = Dict{Module, Vector{Module}}() for m in modules - # only warn if exported or public TODO why did this used to check isbindingresolved? - if !Base.isexported(m, var) && !Base.ispublic(m, var) + # only warn if binding is resolved and exported or public + if !Base.isbindingresolved(m, var) || (!Base.isexported(m, var) && !Base.ispublic(m, var)) continue end warned = true @@ -109,7 +109,7 @@ function _UndefVarError_warnfor(io::IO, modules, var::Symbol) end print(io, "\n - Also $how_available $m") if !isdefined(active_mod, Symbol(m)) - print(io, ", which would need to be imported in $active_mod") + print(io, " (loaded but not imported in $active_mod)") end print(io, ".") end From 819b8b16f176e6e510b41d4f1621c5f08916d146 Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Thu, 5 Dec 2024 10:23:12 -0500 Subject: [PATCH 03/11] use binding_module; remove whitespace --- stdlib/REPL/src/REPL.jl | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/stdlib/REPL/src/REPL.jl b/stdlib/REPL/src/REPL.jl index 4e8b44c6ff374..0a5446ef6a3b2 100644 --- a/stdlib/REPL/src/REPL.jl +++ b/stdlib/REPL/src/REPL.jl @@ -55,7 +55,7 @@ function UndefVarError_hint(io::IO, ex::UndefVarError) else scope = undef end - if scope !== Base + if scope !== Base warned = _UndefVarError_warnfor(io, [Base], var) if !warned @@ -73,11 +73,11 @@ function _UndefVarError_warnfor(io::IO, modules, var::Symbol) active_mod = Base.active_module() warned = false - # collect modules which export or make public the variable by - # the parentmodule in which the variable is defined + # collect modules which export or make public the variable by + # the module in which the variable is defined to_warn_about = Dict{Module, Vector{Module}}() for m in modules - # only warn if binding is resolved and exported or public + # only warn if binding is resolved and exported or public if !Base.isbindingresolved(m, var) || (!Base.isexported(m, var) && !Base.ispublic(m, var)) continue end @@ -89,19 +89,19 @@ function _UndefVarError_warnfor(io::IO, modules, var::Symbol) continue end - parent_m = parentmodule(getproperty(m, var)) - if !haskey(to_warn_about, parent_m) - to_warn_about[parent_m] = [m] + binding_m = Base.binding_module(m, var) + if !haskey(to_warn_about, binding_m) + to_warn_about[binding_m] = [m] else - push!(to_warn_about[parent_m], m) + push!(to_warn_about[binding_m], m) end end if !isempty(to_warn_about) - for (parent_m, modules) in pairs(to_warn_about) - print(io, "\nHint: a global variable of this name also exists in ", parent_m) + for (binding_m, modules) in pairs(to_warn_about) + print(io, "\nHint: a global variable of this name also exists in ", binding_m, ".") for m in modules - m == parent_m && continue + m == binding_m && continue how_available = if Base.isexported(m, var) "exported by" elseif Base.ispublic(m, var) From 6ff6c0ce1876525161e414dfcbc39731a438a1d9 Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Thu, 12 Dec 2024 15:02:14 -0500 Subject: [PATCH 04/11] use nameof to get module name, not Symbol --- stdlib/REPL/src/REPL.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stdlib/REPL/src/REPL.jl b/stdlib/REPL/src/REPL.jl index 0a5446ef6a3b2..784164e414e16 100644 --- a/stdlib/REPL/src/REPL.jl +++ b/stdlib/REPL/src/REPL.jl @@ -108,7 +108,7 @@ function _UndefVarError_warnfor(io::IO, modules, var::Symbol) "made available as public by" end print(io, "\n - Also $how_available $m") - if !isdefined(active_mod, Symbol(m)) + if !isdefined(active_mod, nameof(m)) print(io, " (loaded but not imported in $active_mod)") end print(io, ".") From 87b96e2fef3f3f4c4c1131735a48c6fc0236d239 Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Wed, 18 Dec 2024 13:42:51 -0500 Subject: [PATCH 05/11] new test for grouped import hints --- stdlib/REPL/test/repl.jl | 41 +++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/stdlib/REPL/test/repl.jl b/stdlib/REPL/test/repl.jl index 809913502c3d7..fb8f4b894d66b 100644 --- a/stdlib/REPL/test/repl.jl +++ b/stdlib/REPL/test/repl.jl @@ -1833,23 +1833,50 @@ fake_repl() do stdin_write, stdout_read, repl @test contains(txt, "Some type information was truncated. Use `show(err)` to see complete types.") end -try # test the functionality of `UndefVarError_hint` against `Base.remove_linenums!` +try # test the functionality of `UndefVarError_hint` @assert isempty(Base.Experimental._hint_handlers) Base.Experimental.register_error_hint(REPL.UndefVarError_hint, UndefVarError) - # check the requirement to trigger the hint via `UndefVarError_hint` - @test !isdefined(Main, :remove_linenums!) && Base.ispublic(Base, :remove_linenums!) - fake_repl() do stdin_write, stdout_read, repl backend = REPL.REPLBackend() repltask = @async REPL.run_repl(repl; backend) - write(stdin_write, - "remove_linenums!\n\"ZZZZZ\"\n") + write(stdin_write, """ + module AAAA # can't clash with anything else (module A defined elsewhere) + export f + f() = 0.0 + end + + module C_outer + import ..AAAA: f + public f + + module C_inner + import ..C_outer: f + export f + end # C_inner + end # C_outer + + module D + public f + f() = 1.0 + end + + append!(Base.loaded_modules_order, [AAAA, C_outer, C_outer.C_inner, D]) + f + """ + ) + write(stdin_write, "\nZZZZZ\n") txt = readuntil(stdout_read, "ZZZZZ") write(stdin_write, '\x04') wait(repltask) - @test occursin("Hint: a global variable of this name also exists in Base.", txt) + @test occursin("Hint: a global variable of this name also exists in Main.AAAA.", txt) + @test occursin("Hint: a global variable of this name also exists in Main.D.", txt) + @test occursin("- Also made available as public by Main.C_outer.", txt) + @test occursin("- Also exported by Main.C_outer.C_inner (loaded but not imported in Main).", txt) end +catch e + # fail test if error + @test false finally empty!(Base.Experimental._hint_handlers) end From f5d17ec3b6d96f78f3c9df32222fbbbcf0018a54 Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Thu, 19 Dec 2024 14:02:24 -0500 Subject: [PATCH 06/11] Update stdlib/REPL/src/REPL.jl Co-authored-by: Alex Arslan --- stdlib/REPL/src/REPL.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stdlib/REPL/src/REPL.jl b/stdlib/REPL/src/REPL.jl index 784164e414e16..f081e12f6ffa1 100644 --- a/stdlib/REPL/src/REPL.jl +++ b/stdlib/REPL/src/REPL.jl @@ -105,7 +105,7 @@ function _UndefVarError_warnfor(io::IO, modules, var::Symbol) how_available = if Base.isexported(m, var) "exported by" elseif Base.ispublic(m, var) - "made available as public by" + "declared public in" end print(io, "\n - Also $how_available $m") if !isdefined(active_mod, nameof(m)) From 91c62a97cdec8a0c7834381c3a38ddc242673d45 Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Thu, 19 Dec 2024 14:18:05 -0500 Subject: [PATCH 07/11] remove unnecessary if --- stdlib/REPL/src/REPL.jl | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/stdlib/REPL/src/REPL.jl b/stdlib/REPL/src/REPL.jl index f081e12f6ffa1..8c154b96e799f 100644 --- a/stdlib/REPL/src/REPL.jl +++ b/stdlib/REPL/src/REPL.jl @@ -97,22 +97,20 @@ function _UndefVarError_warnfor(io::IO, modules, var::Symbol) end end - if !isempty(to_warn_about) - for (binding_m, modules) in pairs(to_warn_about) - print(io, "\nHint: a global variable of this name also exists in ", binding_m, ".") - for m in modules - m == binding_m && continue - how_available = if Base.isexported(m, var) - "exported by" - elseif Base.ispublic(m, var) - "declared public in" - end - print(io, "\n - Also $how_available $m") - if !isdefined(active_mod, nameof(m)) - print(io, " (loaded but not imported in $active_mod)") - end - print(io, ".") + for (binding_m, modules) in pairs(to_warn_about) + print(io, "\nHint: a global variable of this name also exists in ", binding_m, ".") + for m in modules + m == binding_m && continue + how_available = if Base.isexported(m, var) + "exported by" + elseif Base.ispublic(m, var) + "made available as public by" + end + print(io, "\n - Also $how_available $m") + if !isdefined(active_mod, nameof(m)) + print(io, " (loaded but not imported in $active_mod)") end + print(io, ".") end end return warned From 736d17b38d00b062b12d8393250bc15686e6da83 Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Thu, 19 Dec 2024 14:18:49 -0500 Subject: [PATCH 08/11] name modules in test with issue number to avoid conflicts --- stdlib/REPL/test/repl.jl | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/stdlib/REPL/test/repl.jl b/stdlib/REPL/test/repl.jl index fb8f4b894d66b..ca6a073d0f910 100644 --- a/stdlib/REPL/test/repl.jl +++ b/stdlib/REPL/test/repl.jl @@ -1841,27 +1841,27 @@ try # test the functionality of `UndefVarError_hint` backend = REPL.REPLBackend() repltask = @async REPL.run_repl(repl; backend) write(stdin_write, """ - module AAAA # can't clash with anything else (module A defined elsewhere) + module A53000 export f f() = 0.0 end - module C_outer - import ..AAAA: f + module C_outer_53000 + import ..A53000: f public f - module C_inner - import ..C_outer: f + module C_inner_53000 + import ..C_outer_53000: f export f - end # C_inner - end # C_outer + end + end - module D + module D_53000 public f f() = 1.0 end - append!(Base.loaded_modules_order, [AAAA, C_outer, C_outer.C_inner, D]) + append!(Base.loaded_modules_order, [A53000, C_outer_53000, C_outer_53000.C_inner_53000, D_53000]) f """ ) @@ -1869,10 +1869,10 @@ try # test the functionality of `UndefVarError_hint` txt = readuntil(stdout_read, "ZZZZZ") write(stdin_write, '\x04') wait(repltask) - @test occursin("Hint: a global variable of this name also exists in Main.AAAA.", txt) - @test occursin("Hint: a global variable of this name also exists in Main.D.", txt) - @test occursin("- Also made available as public by Main.C_outer.", txt) - @test occursin("- Also exported by Main.C_outer.C_inner (loaded but not imported in Main).", txt) + @test occursin("Hint: a global variable of this name also exists in Main.A53000.", txt) + @test occursin("Hint: a global variable of this name also exists in Main.D_53000.", txt) + @test occursin("- Also made available as public by Main.C_outer_53000.", txt) + @test occursin("- Also exported by Main.C_outer_53000.C_inner_53000 (loaded but not imported in Main).", txt) end catch e # fail test if error From dca0c03c19c93620c41e872766a01464ec008e2d Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Thu, 19 Dec 2024 15:39:07 -0500 Subject: [PATCH 09/11] oops, re-correct language for public --- stdlib/REPL/src/REPL.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stdlib/REPL/src/REPL.jl b/stdlib/REPL/src/REPL.jl index 8c154b96e799f..198d3ccbc967e 100644 --- a/stdlib/REPL/src/REPL.jl +++ b/stdlib/REPL/src/REPL.jl @@ -104,7 +104,7 @@ function _UndefVarError_warnfor(io::IO, modules, var::Symbol) how_available = if Base.isexported(m, var) "exported by" elseif Base.ispublic(m, var) - "made available as public by" + "declared public in" end print(io, "\n - Also $how_available $m") if !isdefined(active_mod, nameof(m)) From 6f8cd1e1f529c6f5ed08dae5f3f508f55a8207df Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Thu, 19 Dec 2024 16:20:01 -0500 Subject: [PATCH 10/11] correct test for new wording --- stdlib/REPL/test/repl.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stdlib/REPL/test/repl.jl b/stdlib/REPL/test/repl.jl index ca6a073d0f910..2d3d8a6e00258 100644 --- a/stdlib/REPL/test/repl.jl +++ b/stdlib/REPL/test/repl.jl @@ -1871,7 +1871,7 @@ try # test the functionality of `UndefVarError_hint` wait(repltask) @test occursin("Hint: a global variable of this name also exists in Main.A53000.", txt) @test occursin("Hint: a global variable of this name also exists in Main.D_53000.", txt) - @test occursin("- Also made available as public by Main.C_outer_53000.", txt) + @test occursin("- Also declared public in Main.C_outer_53000.", txt) @test occursin("- Also exported by Main.C_outer_53000.C_inner_53000 (loaded but not imported in Main).", txt) end catch e From 90b2fcac5b6ca17cbc106509ada7531fe123e5cf Mon Sep 17 00:00:00 2001 From: Adam Wheeler Date: Thu, 19 Dec 2024 16:20:56 -0500 Subject: [PATCH 11/11] handle case where active module has binding with same name as module --- stdlib/REPL/src/REPL.jl | 2 +- stdlib/REPL/test/repl.jl | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/stdlib/REPL/src/REPL.jl b/stdlib/REPL/src/REPL.jl index 198d3ccbc967e..a44c0b49b7327 100644 --- a/stdlib/REPL/src/REPL.jl +++ b/stdlib/REPL/src/REPL.jl @@ -107,7 +107,7 @@ function _UndefVarError_warnfor(io::IO, modules, var::Symbol) "declared public in" end print(io, "\n - Also $how_available $m") - if !isdefined(active_mod, nameof(m)) + if !isdefined(active_mod, nameof(m)) || (getproperty(active_mod, nameof(m)) !== m) print(io, " (loaded but not imported in $active_mod)") end print(io, ".") diff --git a/stdlib/REPL/test/repl.jl b/stdlib/REPL/test/repl.jl index 2d3d8a6e00258..a67b792f5a133 100644 --- a/stdlib/REPL/test/repl.jl +++ b/stdlib/REPL/test/repl.jl @@ -1861,6 +1861,8 @@ try # test the functionality of `UndefVarError_hint` f() = 1.0 end + C_inner_53000 = "I'm a decoy with the same name as C_inner_53000!" + append!(Base.loaded_modules_order, [A53000, C_outer_53000, C_outer_53000.C_inner_53000, D_53000]) f """