Confusing test failure display for KeySet

[fonsp]

Julia 1.6.0-rc3

runtests.jl:

using Test

d = Dict("hello" => "world")
@test keys(d) == ["hello"]

Test output:

     Testing Running tests...
Test Failed at /Users/fons/Documents/Pluto.jl/test/runtests.jl:4
  Expression: keys(d) == ["hello"]
   Evaluated: ["hello"] == ["hello"]
ERROR: LoadError: There was an error during testing
in expression starting at /Users/fons/Documents/Pluto.jl/test/runtests.jl:4

[vtjnash]

Huh, it appears we define function show(io::IO, s::Set) instead of for AbstactSet, so the printing is quite sub-optimal here:

julia> @less show(stdout, keys(d))
show(io::IO, iter::Union{KeySet,ValueIterator}) = show_vector(io, iter)

[maxasauruswall]

Hi @vtjnash, @fonsp, I'd like to start contributing. Is this an open issue I could help with? Cheers, Max

[halfdan]

I think there's a lot more weirdness with the function @vtjnash mentioned.

julia> s = Set{Int64}()
Set{Int64}()

julia> push!(s, 12)
Set{Int64} with 1 element:
  12

julia> show(s)
Set([12])

For an empty set the type of T is shown whereas for non-empty sets it's omitted.

[simeonschaub]

Pretty sure that is intentional, because Set() returns an empty Set{Any} instead of Set{Int}.

[simeonschaub]

Hi @vtjnash, @fonsp, I'd like to start contributing. Is this an open issue I could help with? Cheers, Max

Yes, sure! Just take a look at the method @vtjnash pointed out. We probably just want to print AbstractSets MySet as MySet(...) instead of Set(...).

[halfdan]

I guess I'm not understanding the syntax then:

julia> s = Set()
Set{Any}()

julia> push!(s, 12)
Set{Any} with 1 element:
  12

julia> show(s)
Set(Any[12])

julia> push!(s, "cat")
Set{Any} with 2 elements:
  "cat"
  12

julia> show(s)
Set(Any["cat", 12])

It seems counterintuitive to go from showing Set{Any}() to Set(Any[...])

[ArunS-tack]

What's the expected test output? @fonsp

[NHDaly]

Please see the discussion here: https://github.com/JuliaLang/julia/pull/41751#pullrequestreview-720405034

But it seems to me that the only issue here is to remove the confusing custom show method for KeySet, and nothing else needs to change for AbstracSets.

[NHDaly]

@vtjnash - do you have any suggestions for how best to print this instead?

My best proposal so far is maybe:

julia> show(keys(d))
Base.KeySet(Dict("hello" => "world"))

https://github.com/JuliaLang/julia/pull/41751#issuecomment-906022108

But I don't love how it unnecessarily prints the values of the dict, since the only thing the KeySet is supposed to care about is the keys.

Anyone have a better suggestion? CC: @thazhemadam

[NHDaly]

We could maybe add a constructor for KeySets that constructs from a Set, and then display it using that? it's maybe weird that it'd be a different value, but since they contain a reference to a non-isbits value, two == KeySets aren't ever going to be === anyway.