-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Group import hints #56753
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
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 .BThen the hints for The problem here is that (unlike The same thing happens with 1.11. hints on 1.11I 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]:1julia> 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]:1There 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
trueThere 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))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've now done that.
Co-authored-by: Alex Arslan <[email protected]>
|
Friendly post-holiday bump :) I think the failures here were also on master and that this is ready modulo additional review. |
|
Ping :) Are the failing tests here my fault or just flaky? |
|
there was a merge conflict, so CI needs to re-run anyways, and I fixed an incorrect comment (this PR appears to suppress messages for modules where the binding exists but doesn't currently have a value assigned to them) which didn't get updated in f2885c6 after the code change there |
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. --------- Co-authored-by: Alex Arslan <[email protected]> Co-authored-by: Ian Butterworth <[email protected]> Co-authored-by: Jameson Nash <[email protected]> (cherry picked from commit 5f13cd2)
This is my attempt to resolve #53000.
I would have beefed up the test case, but can't follow how it works.