Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Group import hints #56753

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
74 changes: 52 additions & 22 deletions stdlib/REPL/src/REPL.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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 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
if !Base.isbindingresolved(m, var) || (!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

binding_m = Base.binding_module(m, var)
if !haskey(to_warn_about, binding_m)
to_warn_about[binding_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[binding_m], m)
end
end

if !isempty(to_warn_about)
ajwheeler marked this conversation as resolved.
Show resolved Hide resolved
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"
ajwheeler marked this conversation as resolved.
Show resolved Hide resolved
end
print(io, "\n - Also $how_available $m")
if !isdefined(active_mod, nameof(m))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this query might be inaccurate if active_mod has a binding of some kind with the same name as m but referring to something different.

That said, this probably won't be worse than the current behavior, which is rather silly (note the difference in hints here):

julia> using StatsBase

julia> StatsBase.StatsAPI
ERROR: UndefVarError: `StatsAPI` not defined in `StatsBase`
Suggestion: check for spelling errors or missing imports.
Hint: StatsAPI is loaded but not imported in the active module Main.
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)
   @ Base ./Base_compiler.jl:47
 [2] top-level scope
   @ REPL[2]:1
julia> using StatsBase

julia> StatsAPI = "🅰🅿ℹ";

julia> StatsBase.StatsAPI
ERROR: UndefVarError: `StatsAPI` not defined in `StatsBase`
Suggestion: check for spelling errors or missing imports.
Hint: a global variable of this name also exists in StatsAPI.
Stacktrace:
 [1] getproperty(x::Module, f::Symbol)
   @ Base ./Base_compiler.jl:47
 [2] top-level scope
   @ REPL[5]:1

Copy link
Author

@ajwheeler ajwheeler Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm now handing this case correctly. (See updated test.)

print(io, " (loaded but not imported in $active_mod)")
end
print(io, ".")
end
end
end
return true
return warned
end

function __init__()
Expand Down
41 changes: 34 additions & 7 deletions stdlib/REPL/test/repl.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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)
ajwheeler marked this conversation as resolved.
Show resolved Hide resolved
export f
f() = 0.0
end

module C_outer
import ..AAAA: f
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An additional important case to test is that of available but unresolved bindings, e.g.

julia> module AAAA
           export f
           f() = 0.0
       end
Main.AAAA

julia> module B
           using ..AAAA
           export g, f
           g(x) = 69x + 420
       end
Main.B

julia> Base.isbindingresolved(B, :f)
false

julia> isdefined(B, :f)  # note that `isdefined` forces binding resolution
true

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thank you. What is the best place to find docs on this? Is getting the desired behavior just a matter of changing Base.isbindingresolved to isdefined in line 81?

        if !Base.isbindingresolved(m, var) || (!Base.isexported(m, var) && !Base.ispublic(m, var))

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
Expand Down
Loading