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.

Sign up for GitHub

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

Expand documentation of Set #45416

Merged
merged 9 commits into from
Jun 1, 2022
Merged

Expand documentation of Set #45416

Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
55bbdd2
Expand documentation of Set
jakobnissen May 22, 2022
ed1b91d
Amend docs for Set element uniqueness
jakobnissen May 22, 2022
b96715a
Address reviewer comments:
jakobnissen May 23, 2022
093176c
Fix doctest mistake
jakobnissen May 23, 2022
375fe43
Update Set example
jakobnissen May 24, 2022
f122d20
Add extended help
jakobnissen May 24, 2022
118b47f
Fix whitespace
jakobnissen May 24, 2022
aae98b1
Reduce Set docstring
jakobnissen May 24, 2022
38a8ff0
Remove mention of element hashing
jakobnissen May 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 28 additions & 10 deletions base/set.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,33 @@
# This file is a part of Julia. License is MIT: https://julialang.org/license

"""
Set{T} <: AbstractSet{T}

The default implementation of hash sets.

`Set`s are containers that provide fast lookup and membership testing.
Elements in a `Set` are unique, as determined by the elements' implementation of
`isequal` and `hash`.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

@petvana petvana May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@mcabbott mcabbott May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

The order of elements in a `Set` is an implementation detail and cannot be relied on.

See also: [`AbstractSet`](@ref), [`BitSet`](@ref), [`Dict`](@ref)

# Examples
```jldoctest filter = r"^Set{[A-Za-z0-9]+} with [0-9]+ element(s)?:"
julia> Set()
Set{Any}()

julia> Set(1)
Set{Int64} with 1 element:
1

julia> Set(['a', 'b', 'c'])
Set{Char} with 3 elements:
'a'
'c'
'b'
```
"""
struct Set{T} <: AbstractSet{T}
dict::Dict{T,Nothing}

Expand All @@ -19,17 +47,7 @@ function Set{T}(s::KeySet{T, <:Dict{T}}) where {T}
_Set(Dict{T,Nothing}(slots, keys, vals, d.ndel, d.count, d.age, d.idxfloor, d.maxprobe))
end

"""
Set([itr])

Construct a [`Set`](@ref) of the values generated by the given iterable object, or an
empty set. Should be used instead of [`BitSet`](@ref) for sparse integer sets, or
for sets of arbitrary objects.

See also: [`push!`](@ref), [`empty!`](@ref), [`union!`](@ref), [`in`](@ref).
"""
Set(itr) = _Set(itr, IteratorEltype(itr))

_Set(itr, ::HasEltype) = Set{eltype(itr)}(itr)

function _Set(itr, ::EltypeUnknown)
Expand Down