Expand documentation of Set

[jakobnissen]

Hash sets have properties that can be counter-intuitive to people not familiar
with computer science: They provide constant-time membership testing, are unordered,
and deduplicate their elements as determined by isequal and hash.

This commit expands the documentation of Set to mention the above properties,
and also that it is a subtype of AbstractSet.

[jakobnissen]

I've addressed your comments with the following changes

• Remove redundant first line
• Re-add removed "See also" references
• Removed redundant empty Set instantiation example
• Have example show mutability
• Have example show deduplication
• Have example show behaviour under isequal

[petvana]

What about pushing 0 to better highlight the mutability?

Suggested change

	julia> push!(s, 'a'); push!(s, 0.0)
	julia> push!(s, 'a'); push!(s, 0)

... and than 0.0 in s

[jakobnissen]

The issue with that is that I think the current behaviour where 0 overwrites 0.0 is an implementation detail (i.e. I think keeping 0.0 would be documented behaviour, too), so it's better to not have it in the doc example.

[Moelf]

can we finally close #23269 while at it?

[jakobnissen]

I added more examples by recommendation - if this docstring is too long let me know.

[petvana]

Suggested change

	julia> s = Set("aaBca")
	julia> s = Set("aaBca"); push!(s, 'b')

If you are worried it to be too long, I guess you can connect these together without losing any vital information.

[KristofferC]

I don't really get the addition of hash here. You could have a set with a bunch of elements that all hash to 1, it would just be inefficient.

[jakobnissen]

The idea is to prevent a user overloading isequal without overloading hash (e,g, having a bunch of not-equal elements that hash to 1 is an implementation error). This is not legal according to isequal docs though, so it might be redundant to also state it here. I'll remove it.

[petvana]

Here, the problem is kind of the opposite, i.e., having multiple isequal elements with different hashes. So, omitting hash may easily lead to hard-to-debug bugs, like

julia> import Base: isequal, hash

julia> struct X; x; end

julia> isequal(X(0.0), X(0))
false

julia> isequal(a::X, b::X) = a.x == b.x;

julia> isequal(X(0.0), X(0))
true

julia> s = Set([X(0.0), X(0)]) # Incorrect
Set{X} with 2 elements:
  X(0)
  X(0.0)

julia> hash.([X(0.0), X(0)])
2-element Vector{UInt64}:
 0x29f6c96c4b9e8bb8
 0x0792f7046dd5cf45

julia> hash(x::X) = hash(x.x);

julia> s = Set([X(0.0), X(0)]) # Correct
Set{X} with 1 element:
  X(0)

Btw, hash is usually mention for other languages too. Just to name few: C++, Rust, Java ... unless the set is BST-based, of course.

[mcabbott]

as determined by the elements' definitions of isequal and hash

This "and" doesn't seem quite accurate. I think it only calls hash, although that should always match isequal. Maybe we can just say that? The goal is always to agree with isequal, but the mechanism is hash. Making it sound like the mechanism is isequal seems misleading.

I think the logic is the same as unique, whose description does not mention hash, but (IMO) probably should. Mentioning that they agree, and linking to unique here, might also be a good idea.

help?> unique
search: unique unique! allunique

  unique(itr)

  Return an array containing only the unique elements of collection itr, as determined by isequal,
  in the order that the first of each set of equivalent elements originally appears. The element
  type of the input is preserved.

[petvana]

isequal is actually called to check hash collisions for Dict (which implements Set internally) in

julia/base/dict.jl

Line 273 in 0062c26

if (key === k || isequal(key, k))

How about writing the assumption explicitly, like in Rust?

isequal(a, b) -> hash(a) == hash(b)

[mcabbott]

Ah ok. So it does depend on both.

Maybe the description should first say what the contract is (unique according to isequal, just like unique) and later say that the fast implementation needs hash? Uniqueness in the 1st paragraph, what's efficient in the 2nd?

[jakobnissen]

Is that really necessary for the docstring, though? To me, mentioning hash seem like an optimization based on the implementation detail of Set, and that is usually outside the scope of a docstring.

[petvana]

IMHO, yes! These two functions are entangled tightly. So, mentioning only one seems dangerous to me because it motivates the user to re-define isequal only, and it may lead to an incorrect/undefined behavior.

[jakobnissen]

OK so it's clear to me that we don't all exactly agree on what should and shouldn't go in the docstring. But let's not let the perfect be the enemy of the good and let's make sure this PR doesn't stall out and get nowhere due to bikeshedding.

[aviatesk]

LGTM. Any objections to this? Otherwise I'd like to merge this tmrw.