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
Open

Conversation

ajwheeler
Copy link

This is my attempt to resolve #53000.

julia> solve
ERROR: UndefVarError: `solve` not defined in `Main`
Suggestion: check for spelling errors or missing imports.
Hint: a global variable of this name also exists in CommonSolve.
    - Also exported by SciMLBase.
    - Also exported by DiffEqBase.
    - Also exported by JumpProcesses.
    - Also exported by LinearSolve.
    - Also exported by BracketingNonlinearSolve (loaded but not imported in Main).
    - Also exported by SimpleNonlinearSolve.
    - Also exported by NonlinearSolve.
    - Also exported by OrdinaryDiffEqLowStorageRK (loaded but not imported in Main).
    - Also exported by OrdinaryDiffEqSSPRK (loaded but not imported in Main).
    - Also exported by OrdinaryDiffEqVerner (loaded but not imported in Main).
    - Also exported by OrdinaryDiffEqBDF (loaded but not imported in Main).
    - Also exported by OrdinaryDiffEqTsit5 (loaded but not imported in Main).
    - Also exported by OrdinaryDiffEqRosenbrock (loaded but not imported in Main).
    - Also exported by OrdinaryDiffEqDefault (loaded but not imported in Main).
    - Also exported by Sundials.

I would have beefed up the test case, but can't follow how it works.

stdlib/REPL/src/REPL.jl Outdated Show resolved Hide resolved
@Keno
Copy link
Member

Keno commented Dec 4, 2024

Seems sensible overall, modulo comment.

@Keno
Copy link
Member

Keno commented Dec 4, 2024

Does need a test though.

@LilithHafner LilithHafner added the error messages Better, more actionable error messages label Dec 4, 2024
@LilithHafner
Copy link
Member

There's also some trailing whitespace that needs to be removed. You can see where on the "files changd" tab of this PR. (Some editors have an option to strip trailing whitespace automatically)

@LilithHafner LilithHafner added the needs tests Unit tests are required for this change label Dec 4, 2024
@ajwheeler
Copy link
Author

ajwheeler commented Dec 10, 2024

In thinking about how to test this, I've come across what may be a problem. If I do

module A
export f
f() = 0.0
end

module B
import ..A: f
export f
end

module C
import ..B: f
public f
end

module D
public f
f() = 1.0
end

append!(Base.loaded_modules_order, [A, B, C, D]) # is there a less gross way?
import .B

Then the hints for julia> f look like this

ERROR: UndefVarError: `f` not defined in `Main`
Suggestion: check for spelling errors or missing imports.
Hint: a global variable of this name also exists in Main.A.
    - Also exported by Main.B (loaded but not imported in Main).
    - Also made available as public by Main.C (loaded but not imported in Main).
Hint: a global variable of this name also exists in Main.D.

The problem here is that (unlike C), B is imported. edit: I now believe that neither B nor C should get this parenthetical, since they are defined in Main

The same thing happens with 1.11.

hints on 1.11
julia> f
ERROR: UndefVarError: `f` not defined in `Main`
Suggestion: check for spelling errors or missing imports.
Hint: a global variable of this name may be made accessible by importing Main.A in the current active module Main
Hint: a global variable of this name may be made accessible by importing Main.B in the current active module Main
Hint: a global variable of this name may be made accessible by importing Main.C in the current active module Main
Hint: a global variable of this name may be made accessible by importing Main.D in the current active module Main

I think the problem arises from calling Symbol(m), where m is a Module, but I might be confused.

I would welcome input here about whether or not this is out of scope for this PR, and whether there is a better way to test this.

@ararslan
Copy link
Member

I think the problem arises from calling Symbol(m), where m is a Module, but I might be confused.

I haven't looked closely enough to determine whether Symbol(m) is the problem but if you suspect it is then you could try nameof(m) instead. (However, note that nameof here isn't fully equivalent; consider e.g. Symbol(Base.Broadcast) vs. nameof(Base.Broadcast). To get the same behavior, you'd need Symbol(join(fullname(m), '.')).)

@ararslan
Copy link
Member

By the way, GitHub says this is your first contribution to the Julia repo, so welcome, and excellent first PR!

@ajwheeler
Copy link
Author

I've added @test fail in the catch block here, which seems like the right thing to do? Otoh, the other REPL tests don't do it.

@inkydragon inkydragon added REPL Julia's REPL (Read Eval Print Loop) and removed needs tests Unit tests are required for this change labels Dec 19, 2024
stdlib/REPL/src/REPL.jl Outdated Show resolved Hide resolved
stdlib/REPL/src/REPL.jl Outdated Show resolved Hide resolved
"made available as public by"
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.)

stdlib/REPL/test/repl.jl Outdated Show resolved Hide resolved
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))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Better, more actionable error messages REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Group import hint by bindings
5 participants