wip: REPLCompletions: allow completions for using-ed names

[NHDaly]

Add parameter to names(m::Module) to return the bindings that come from using statements inside that module.

This was actually a bit trickier than I expected it to be, because there are a few oddities to consider:

• Bindings that are exported from Foo, which come from using Foo, are added to m's bindings table lazily, whenever they're first referenced.
  • This means that it's not enough to allow returning names that have a different parent module, but weren't ->imported, because it means the names returned from names(m::Module, usings=true) would change over time as functions are called that perform the binding lookup!
  • So instead, I added logic to walk over all the modules in the m->usings list, and fetch all of their exported names.
• We have to take special care not to return names exported from Base and Core, to keep consistent with how names() currently works.
  • Though we could consider allowing these if all=true? i.e. should names(m, all=true, usings=true) return names that come from Base and Core? We did write all=true, and we're not returning all the names... but also, it's very noisy / probably useless to return all of them.
• We don't want to print submodules that are also usings twice

Here's a nice example that I think showcases all the edgecases:

julia> module M1
           const m1_x = 1
           export m1_x
       end
Main.M1

julia> module A
           module B
               f(x) = 1
               secret = 1
               module Inner2 end
           end
           module C
               x = 1
               y = 2
               export y
           end
           using .B: f
           using .C
           using ..M1
       end
Main.A

julia> names(A)
1-element Vector{Symbol}:
 :A

julia> names(A, imported=true)
1-element Vector{Symbol}:
 :A

julia> names(A, usings=true)
6-element Vector{Symbol}:
 :A
 :C
 :M1
 :f
 :m1_x
 :y

julia> names(A, all=true)
7-element Vector{Symbol}:
 Symbol("#eval")
 Symbol("#include")
 :A
 :B
 :C
 :eval
 :include

julia> names(A, all=true, usings=true)
11-element Vector{Symbol}:
 Symbol("#eval")
 Symbol("#include")
 :A
 :B
 :C
 :M1
 :eval
 :f
 :include
 :m1_x
 :y

Fixes #36529.

---

• closes REPL shell completion fail to detect imported symbol #49109
• closes Autocompletion after selective using #53524

[NHDaly]

Open questions for reviewers:

1. I think the other alternative would have been to change the behavior of imported=true to also return these, but i think that's bad because it's a breaking change? I'm not super sure if there's anyone who would want just the usings or just the imports though...? Maybe?
2. Any preference on the name of the parameter? Maybe usinged would be more consistent with imported, but that's also not a real word. Can anyone suggest a better alternative?

Thanks! 😊

[DilumAluthge]

2. Any preference on the name of the parameter? Maybe usinged would be more consistent with imported, but that's also not a real word. Can anyone suggest a better alternative?

• from_using
• via_using

[aviatesk]

• Bindings that are exported from Foo, which come from using Foo, are added to m's bindings table lazily, whenever they're first referenced.

yeah, I think the manual binding lookup is valid here in terms of the functionality of names.
No matter how lazily the actual binding is resolved, there are the same name and names can just return it ?

• We have to take special care not to return names exported from Base and Core, to keep consistent with how names() currently works.

I think we can add bindings that are explicitly usinged at least ? I found this special casing leads to a completion failure for Base bindings, e.g.

julia> using Base: @aggressive_constantprop

julia> @aggressive_ # tab completion doesn't work w/ this PR

Here is the corresponding test case I added in #42094:

julia/stdlib/REPL/test/replcompletions.jl

Lines 1194 to 1200 in 66b9235

	let # should for `Base` binding
	M = Module()
	@assert count(==("@aggressive_constprop"), test_complete_context1("@aggressive_", M)) ≠ 1
	@eval M using Base: @aggressive_constprop
	@test count(==("@aggressive_constprop"), test_complete_context1("@aggressive_", M)) == 1
	end

I'm not sure what is the most consistent behavior, but at least I want to remove this special casing for every case ?

julia/src/module.c

Lines 815 to 817 in e0ac286

	// Modules implicitly get all exported names from Base and Core as if they had
	// written `using Base`, but we don't show those via `names()`.
	b->owner != jl_base_module && b->owner != jl_core_module);

2. Any preference on the name of the parameter? Maybe usinged would be more consistent with imported, but that's also not a real word. Can anyone suggest a better alternative?

I don't have much opinion on naming in general, but usings sounds okay to me :p

---

/cc @pfitzseb The LS might also be interested in this ?

[pfitzseb]

The LS might also be interested in this ?

Yeah, for sure. We have a bunch of code that hacks around this issue in the SymbolServer.

[pfitzseb]

Any chance we can rebase and merge this? Just ran into another issue which needs this PR or something like it to fix.

[lgoettgens]

What is the state of this? It would be great to get this into 1.12.

[NHDaly]

I haven't looked at this in a couple of years. :( I'm sorry I let it linger.

@pfitzseb: would you be able to clean this up and bring it to merge? I think the naming question seems to have an answer above, so someone just needs to apply that and then rebase and merge?

@aviatesk i don't remember everything we were talking about before, but do you have any remaining unaddressed concerns?

[DilumAluthge]

@NHDaly What's the current status here? Are we waiting on action from @pfitzseb and @aviatesk?

[pfitzseb]

This should fix the tests (and behaviour). Basically, unresolved bindings should never show up in the output, which the additional check ensures.

I don't have write permissions for this repo, so I'll just post the diff here.

cc @aviatesk for a sanity check :)

diff --git a/src/module.c b/src/module.c
index e8ea5c7375..1292ed7cf9 100644
--- a/src/module.c
+++ b/src/module.c
@@ -992,7 +992,7 @@ JL_DLLEXPORT jl_value_t *jl_module_usings(jl_module_t *m)
 }
 
 uint8_t _binding_is_from_explicit_using(jl_binding_t *b) {
-    return (b->owner != b && !b->imported &&
+    return (jl_atomic_load_relaxed(&b->owner) != NULL && b->owner != b && !b->imported &&
             // Modules implicitly get all exported names from Base and Core as if they had
             // written `using Base`, but we don't show those via `names()`.
             // b->owner->value != (jl_value_t*)jl_base_module && b->owner->value != (jl_value_t*)jl_core_module);
diff --git a/test/reflection.jl b/test/reflection.jl
index 0ecf817ddd..e9e35b1ff4 100644
--- a/test/reflection.jl
+++ b/test/reflection.jl
@@ -188,14 +188,16 @@ let
                                         Symbol("@test_deprecated"), Symbol("@test_logs"), Symbol("@test_nowarn"), Symbol("@test_skip"),
                                         Symbol("@test_throws"), Symbol("@test_warn"), Symbol("@testset"), :GenericArray, :GenericDict,
                                         :GenericOrder, :GenericSet, :GenericString, :Test, :TestSetException, :detect_ambiguities, :detect_unbound_args,
-                                        :TestMod7648, :TestModSub9475, :TestMod7648, :a9475, :foo9475, :c7648, :foo7648, :foo7648_nomethods, :Foo7648])
+                                        :TestMod7648, :TestModSub9475, :TestMod7648, :a9475, :foo9475, :c7648, :foo7648, :foo7648_nomethods,
+                                        :Foo7648, Symbol("@__MODULE__"), :Base, :LogRecord, :(==), :(===), :TestLogger])
     @test Set(names(TestMod7648, all = true, usings = true)) == Set([:x36529, :Test,  Symbol("@inferred"), Symbol("@test"), Symbol("@test_broken"),
                                         Symbol("@test_deprecated"), Symbol("@test_logs"), Symbol("@test_nowarn"), Symbol("@test_skip"),
                                         Symbol("@test_throws"), Symbol("@test_warn"), Symbol("@testset"), :GenericArray, :GenericDict,
                                         :GenericOrder, :GenericSet, :GenericString, :Test, :TestSetException, :detect_ambiguities, :detect_unbound_args,
                                         :TestMod7648, :TestModSub9475, :a9475, :foo9475, :c7648, :d7648, :f7648,
                                         :foo7648, Symbol("#foo7648"), :foo7648_nomethods, Symbol("#foo7648_nomethods"),
-                                        :Foo7648, :eval, Symbol("#eval"), :include, Symbol("#include")])
+                                        :Foo7648, :eval, Symbol("#eval"), :include, Symbol("#include"), Symbol("@__MODULE__"), :Base,
+                                        :LogRecord, :(==), :(===), :TestLogger])
     @test isconst(TestMod7648, :c7648)
     @test !isconst(TestMod7648, :d7648)
 end

[aviatesk]

This PR should probably be merged along with the enhancement for REPL completions, so I'll merge #42094 into this one for now.

[aviatesk]

I'd like to replace this with #54609.