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

add back wordaround for Slot objects should not occur in an AST in Ipython mode #47878

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Dec 12, 2022

This workaround was removed in #47178 (cc @vtjnash).

Ref #46451

On master:

julia> using REPL

julia> REPL.ipython_mode!()

In [4]: x = range(-1; stop = 1)
ERROR: syntax: Slot objects should not occur in an AST
Stacktrace:
 [1] top-level scope
   @ REPL[4]:1
 [2] top-level scope
   @ ~/julia/usr/share/julia/stdlib/v1.10/REPL/src/REPL.jl:1407

@KristofferC KristofferC added REPL Julia's REPL (Read Eval Print Loop) backport 1.9 Change should be backported to release-1.9 labels Dec 12, 2022
Comment on lines +1407 to 1410
let __temp_val_a72df459 = $x
$capture_result($n, __temp_val_a72df459)
__temp_val_a72df459
end
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Alternatively, I had toyed with making capture_result return the value too, so that it could be written as:

Suggested change
let __temp_val_a72df459 = $x
$capture_result($n, __temp_val_a72df459)
__temp_val_a72df459
end
$capture_result($n, $x)

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Dec 13, 2022

@vtjnash, any idea how this could have broken the REPL completion tests? If I comment out the new tests, they pass...

@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 13, 2022

Strange. Those seem like they should be isolated

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Dec 14, 2022

So what happens is that in the new test I define:

x = range(-1; stop = 1)

which makes x get defined in Main from the REPL evaluating it. In the REPL completion test, there is a check to complete

"CompletionFoo.kwtest4(x23=18, x; "

which ends up in

push!(args_ex, get_type(get_type(ex, context_module)..., default_any))

where the type of x is looked up, which now gets a different result:

julia> REPLCompletions.get_type(:(x), Main)
(UnitRange{Int64}, true)
# (Any, false) if test is not run

and then we get:

julia> t_in = Tuple{typeof(Main.CompletionFoo.kwtest4), UnitRange{Int64}};

julia> max_method_completions = -2;

julia> Base._methods_by_ftype(t_in, nothing, max_method_completions, Base.get_world_counter(),
               #=ambig=# true, Ref(typemin(UInt)), Ref(typemax(UInt)), Ptr{Int32}(C_NULL))
Any[]

instead of (if the test is not run and x is not defined).

julia> t_in = Tuple{typeof(Main.CompletionFoo.kwtest4), Any};

julia> max_method_completions = -2;

julia> Base._methods_by_ftype(t_in, nothing, max_method_completions, Base.get_world_counter(),
               #=ambig=# true, Ref(typemin(UInt)), Ref(typemax(UInt)), Ptr{Int32}(C_NULL))
3-element Vector{Any}:
 Core.MethodMatch(Tuple{typeof(Main.CompletionFoo.kwtest4), SubString}, svec(), kwtest4(a::SubString; x23, _something) @ Main.CompletionFoo ~/julia/stdlib/REPL/test/replcompletions.jl:113, false)
 Core.MethodMatch(Tuple{typeof(Main.CompletionFoo.kwtest4), String}, svec(), kwtest4(a::String; _a1b, xαβγ) @ Main.CompletionFoo ~/julia/stdlib/REPL/test/replcompletions.jl:112, false)
 Core.MethodMatch(Tuple{typeof(Main.CompletionFoo.kwtest4), AbstractString}, svec(), kwtest4(a::AbstractString; _a1b, x23) @ Main.CompletionFoo ~/julia/stdlib/REPL/test/replcompletions.jl:111, false)

We can replicate this in the REPL:

julia> begin
           kwtest4(a::AbstractString; _a1b, x23) = pass
           kwtest4(a::String; _a1b, xαβγ) = pass
           kwtest4(a::SubString; x23, _something) = pass
       end
kwtest5 (generic function with 1 method)

julia> kwtest4(x23=18, x; <TAB>
kwtest4(a::SubString; x23, _something) @ Main REPL[1]:4
kwtest4(a::String; _a1b, xαβγ) @ Main REPL[1]:3
kwtest4(a::AbstractString; _a1b, x23) @ Main REPL[1]:2

julia> x = 3
3

julia> kwtest4(x23=18, x; <TAB>

where the last tab complete no longer works. So the "solution" to fix the test here is to use another variable name than x but this REPL completion behavior feels a bit strange to me.

cc @Liozou who I think implemented the keyword completion stuff.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Dec 14, 2022

Ah, that is right. CompletionFoo.kwtest4 should be doing something like x_notdefined though

@Liozou
Copy link
Member

Liozou commented Dec 14, 2022

Apologies for having made you track such an ugly test issue! I should not have used a simple x like that, sorry for not thinking about it.

The REPL completion behavior may be slightly convoluted but it is indeed valid:

  • when x is undefined, its type is considered to be Any for REPL completion. Thus, the call starting with kwtest4(x23=18, x; could be completed into a valid call, for instance kwtest4(x23=18, x; _a1b=13) with x = lazy"foo", say.
  • when x is a UnitRange{Int64} (or any other type not subtyping AbstractString in this case), then no call starting with kwtest4(x23=18, x; can avoid a MethodError so there is no REPL completion.

The solution is indeed to rename the x into something else. You can do so on my test to avoid any further issue like that (or I can open another PR for this):

-let s = "CompletionFoo.kwtest4(x23=18, x; "
+let s = "CompletionFoo.kwtest4(x23=18, undefined43536; "
     c, r, res = test_complete(s)
     @test !res
     @test length(c) == 3 # TODO: remove "kwtest4(a::String; _a1b, xαβγ)"
     @test any(str->occursin("kwtest4(a::SubString", str), c)
     @test any(str->occursin("kwtest4(a::AbstractString", str), c)
-    @test (c, r, res) == test_complete("CompletionFoo.kwtest4(x23=18, x, ")
+    @test (c, r, res) == test_complete("CompletionFoo.kwtest4(x23=18, undefined43536, ")
     @test (c, r, res) == test_complete("CompletionFoo.kwtest4(x23=18, ")
 end

There is another test further down which might possibly be affected by having an x in Main:

let s = "log(log.(x),"

@KristofferC KristofferC merged commit 7b10d5f into master Dec 14, 2022
@KristofferC KristofferC deleted the kc/ipython_slot branch December 14, 2022 19:15
KristofferC added a commit that referenced this pull request Dec 16, 2022
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants