-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
base: master
Are you sure you want to change the base?
Group import hints #56753
Conversation
Seems sensible overall, modulo comment. |
Does need a test though. |
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) |
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
The problem here is that (unlike The same thing happens with 1.11. hints on 1.11
I think the problem arises from calling 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. |
I haven't looked closely enough to determine whether |
By the way, GitHub says this is your first contribution to the Julia repo, so welcome, and excellent first PR! |
I've added |
stdlib/REPL/src/REPL.jl
Outdated
"made available as public by" | ||
end | ||
print(io, "\n - Also $how_available $m") | ||
if !isdefined(active_mod, nameof(m)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
end | ||
|
||
module C_outer | ||
import ..AAAA: f |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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))
Co-authored-by: Alex Arslan <[email protected]>
This is my attempt to resolve #53000.
I would have beefed up the test case, but can't follow how it works.