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

Functional non-mutating methods. #46453

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
207 changes: 207 additions & 0 deletions base/array.jl
Original file line number Diff line number Diff line change
Expand Up @@ -995,6 +995,64 @@ function setindex!(A::Array{T}, X::Array{T}, c::Colon) where T
return A
end

"""
setindex(collection, value, key...)

Create a new collection with the element/elements of the location specified by `key`
replaced with `value`(s).

# Implementation

`setindex(collection, value, key...)` must have the property such that

```julia
y1 = setindex(x, value, key...)

y2 = copy′(x)
@assert convert.(eltype(y2), x) == y2

y2[key...] = value
@assert convert.(eltype(y2), y1) == y2
```

with a suitable definition of `copy′` such that `y2[key...] = value` succeeds.

`setindex` should support more combinations of arguments by widening collection
type as required.
"""
function setindex end

function setindex(x::AbstractArray, v, i...)
@_propagate_inbounds_meta
inds = to_indices(x, i)
if inds isa Tuple{Vararg{Integer}}
T = promote_type(eltype(x), typeof(v))
else
T = promote_type(eltype(x), eltype(v))
end
y = similar(x, T)
copy!(y, x)
y[inds...] = v
return y
end
function setindex(t::Tuple, v, inds::AbstractUnitRange{<:Integer})
Tokazama marked this conversation as resolved.
Show resolved Hide resolved
start = first(inds)
stop = last(inds)
offset = start - firstindex(v)
ntuple(Val{nfields(t)}()) do i
if i < start || i > stop
getfield(t, i)
else
v[i - offset]
end
end
end
function setindex(t::Tuple, v, inds::AbstractVector{<:Integer})
Copy link
Contributor

Choose a reason for hiding this comment

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

For vectors with statically known size (eg SVectors), the following alternative performs better and without allocations:

	function setindex(t::Tuple, v, inds::AbstractVector{<:Integer})
	    Base.@_propagate_inbounds_meta
		foldl(ntuple(identity, length(inds)); init=t) do acc, i
			Base.setindex(acc, v[i], inds[i])
		end
	end

Maybe, this should be used instead? And/or, add a similar method with inds::NTuple{Integer}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the snippet! it now replaces the old commented code.

Copy link
Contributor

@aplavin aplavin Mar 2, 2023

Choose a reason for hiding this comment

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

Sorry, I must've gone crazy with that fold!
This works the same for static arrays, better for regular arrays, and even seems efficient for tuples:

function setindex(t::Tuple, v, inds::AbstractVector{<:Integer})
    ntuple(length(t)) do i
        ix = findfirst(==(i), inds)
        isnothing(ix) ? t[i] : v[ix]
    end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

and for unitranges

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did we want to allow having tuples of indices be permitted as inds here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah maybe best without tuples.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that's correct.

julia> let x = zeros(3)
         setindex!(x, [4,5,6], [1,2,1])
         x
       end
3-element Vector{Float64}:
 6.0
 5.0
 0.0

julia> function _setindex(t::Tuple, v, inds::AbstractVector{<:Integer})
           ntuple(length(t)) do i
               ix = findfirst(==(i), inds)
               isnothing(ix) ? t[i] : v[ix]
           end
       end
_setindex (generic function with 1 method)

julia> let x = Tuple(zeros(3))
         _setindex(x, [4,5,6], [1,2,1])
       end
(4, 5, 0.0)

Copy link
Contributor

@aplavin aplavin Mar 2, 2023

Choose a reason for hiding this comment

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

So, findlast instead of findfirst? Or throw?
Not clear which is the best semantics.

@_propagate_inbounds_meta
(setindex!(Any[t...], v, inds)...,)
end


# efficiently grow an array

_growbeg!(a::Vector, delta::Integer) =
Expand Down Expand Up @@ -1133,6 +1191,63 @@ function _append!(a, ::IteratorSize, iter)
a
end

"""
insert(a::AbstractVector, index::Integer, item)

Returns a copy of `a` with `item` inserted at `index`.

See also: [`insert!`](@ref), [`deleteat`](@ref), [`delete`](@ref)

# Examples
```jldoctest
julia> A = Any[1, 2, 3]
3-element Vector{Any}:
1
2
3

julia> insert(A, 3, "here") == [1, 2, "here", 3]
true

julia> A == [1, 2, 3]
true
````
"""
function insert(src::AbstractVector, index, item)
Tokazama marked this conversation as resolved.
Show resolved Hide resolved
src_idx = firstindex(src)
src_end = lastindex(src)
if index == (src_end + 1)
dst = similar(src, promote_type(eltype(src), typeof(item)), length(src) + 1)
for dst_idx in eachindex(dst)
@inbounds if isassigned(src, src_idx)
@inbounds dst[dst_idx] = src[src_idx]
end
src_idx += 1
end
@inbounds dst[end] = item
else
@boundscheck (src_end < index || index < src_idx) && throw(BoundsError(src, index))
dst = similar(src, promote_type(eltype(src), typeof(item)), length(src) + 1)
dst_idx = firstindex(dst)
while src_idx < index
@inbounds if isassigned(src, src_idx)
@inbounds dst[dst_idx] = src[src_idx]
end
dst_idx += 1
src_idx += 1
end
setindex!(dst, item, dst_idx)
dst_idx += 1
for i in src_idx:src_end
@inbounds if isassigned(src, i)
@inbounds dst[dst_idx] = src[i]
end
dst_idx += 1
end
end
dst
end

"""
prepend!(a::Vector, collections...) -> collection

Expand Down Expand Up @@ -1616,6 +1731,98 @@ function deleteat!(a::Vector, inds::AbstractVector{Bool})
return a
end

"""
deleteat(a::Vector, i::Integer)

Remove the item at the given `i` and return the modified `a`. Subsequent items
are shifted to fill the resulting gap.

See also: [`deleteat!`](@ref), [`delete`](@ref), [`insert`](@ref)

# Examples
```jldoctest
julia> x = [6, 5, 4]
3-element Vector{Int64}:
6
5
4

julia> deleteat(x, 2) == [6, 4]
true

julia> deleteat(x, [2, 3]) == [6]
true

julia> x == [6, 5, 4]
true
```
"""
function deleteat(src::AbstractVector, i::Integer)
Copy link
Contributor

Choose a reason for hiding this comment

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

deleteat(src::AbstractVector, i) = deleteat!(copy(src), i)

?
Instead of all these methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see - it may work with setindex-able, but not resizeable arrays as of now...
Unfortunate that so much duplication is needed.

Copy link
Contributor Author

@Tokazama Tokazama Mar 2, 2023

Choose a reason for hiding this comment

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

Yeah, it's a bit messy. I thought about putting # TODO refactor when immutable arrays are in base but I don't think we know how that's going to play out still. Also, I use "TODO" as a keyword in searches for my code bases, so I try not to pollute community projects with it unless it's clearly helpful for everyone.

src_idx = firstindex(src)
src_end = lastindex(src)
src_idx <= i <= src_end || throw(BoundsError(src, i))
dst = similar(src, length(src) - 1)
dst_idx = firstindex(dst)
while src_idx <= src_end
if src_idx != i
@inbounds isassigned(src, src_idx) && setindex!(dst, src[src_idx], dst_idx)
dst_idx += 1
end
src_idx += 1
end
dst
end
function deleteat(src::AbstractVector, inds::AbstractVector{Bool})
n = length(src)
length(inds) == n || throw(BoundsError(src, inds))
dst = similar(src, n - count(inds))
dst_idx = firstindex(dst)
src_idx = firstindex(src)
for index in inds
if !index
@inbounds isassigned(src, src_idx) && setindex!(dst, src[src_idx], dst_idx)
dst_idx += 1
end
src_idx += 1
end
dst
end
function deleteat(src::AbstractVector, inds)
itr = iterate(inds)
if itr === nothing
copy(src)
else
len = length(src) - length(inds)
# `length(inds) > length(src)` then `inds` has repeated or out of bounds indices
len > 0 || throw(BoundsError(src, inds))
index, state = itr
dst = similar(src, len)
dst_idx = firstindex(dst)
src_idx = firstindex(src)
src_end = lastindex(src)
src_idx > index && throw(BoundsError(src, inds))
while true
(src_end < index || index < src_idx) && throw(BoundsError(src, inds))
while src_idx < index
@inbounds isassigned(src, src_idx) && setindex!(dst, src[src_idx], dst_idx)
dst_idx += 1
src_idx += 1
end
src_idx += 1
itr = iterate(inds, state)
itr === nothing && break
itr[1] <= index && throw(ArgumentError("indices must be unique and sorted"))
index, state = itr
end
while src_idx <= src_end
@inbounds isassigned(src, src_idx) && setindex!(dst, src[src_idx], dst_idx)
dst_idx += 1
src_idx += 1
end
dst
end
end

const _default_splice = []

"""
Expand Down
63 changes: 63 additions & 0 deletions base/dict.jl
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,15 @@ function setindex!(h::Dict{K,Any}, v, key::K) where K
return h
end

function setindex(d0::Dict, v, k)
K = promote_type(keytype(d0), typeof(k))
V = promote_type(valtype(d0), typeof(v))
d = Dict{K, V}()
copy!(d, d0)
d[k] = v
return d
end


"""
get!(collection, key, default)
Expand Down Expand Up @@ -669,6 +678,27 @@ function delete!(h::Dict, key)
return h
end

"""
delete(collection, key)

Create and return a new collection containing all the elements from `collection` except for
the mapping corresponding to `key`.

See also: [`delete!`](@ref).

# Examples
```jldoctest
julia> d = Dict("a"=>1, "b"=>2);

julia> delete(d, "b") == Dict("a"=>1)
true

julia> d == Dict("a"=>1, "b"=>2)
true
```
"""
delete(collection, k) = delete!(copy(collection), k)

function skip_deleted(h::Dict, i)
L = length(h.slots)
for i = i:L
Expand Down Expand Up @@ -821,6 +851,39 @@ function get(default::Callable, dict::ImmutableDict, key)
return default()
end

function delete(d::ImmutableDict{K,V}, key) where {K,V}
if isdefined(d, :parent)
if isequal(d.key, key)
d.parent
else
ImmutableDict{K,V}(delete(d.parent, key), d.key, d.value)
end
else
d
end
end
Comment on lines +847 to +857
Copy link
Member

Choose a reason for hiding this comment

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

This implementation is not quite correct since I can current;y construct an ImmutableDict where there are multiple entries for key each shadowed.

Copy link
Sponsor Member

@vtjnash vtjnash Sep 11, 2023

Choose a reason for hiding this comment

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

I suspect delete would be more easily done with adding a new key as a tombstone (with the same key, but val being #undef), rather than a bulk copy like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect delete would be more easily done with adding a new key as a tombstone (with the same key, but val being #undef), rather than a bulk copy like this.

I really like that idea, but I think we would need to have a new type since we can't rely on #undef for bits types.


function setindex(d::ImmutableDict, v, k)
if isdefined(d, :parent)
if isequal(d.key, k)
d0 = d.parent
v0 = v
k0 = k
else
d0 = setindex(d.parent, v, k)
v0 = d.value
k0 = d.key
end
else
d0 = d
v0 = v
k0 = k
end
K = promote_type(keytype(d0), typeof(k0))
V = promote_type(valtype(d0), typeof(v0))
ImmutableDict{K,V}(d0, k0, v0)
end

# this actually defines reverse iteration (e.g. it should not be used for merge/copy/filter type operations)
function iterate(d::ImmutableDict{K,V}, t=d) where {K, V}
!isdefined(t, :parent) && return nothing
Expand Down
3 changes: 3 additions & 0 deletions base/exports.jl
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@ export
# dequeues
append!,
insert!,
insert,
pop!,
popat!,
prepend!,
Expand All @@ -512,7 +513,9 @@ export
count!,
count,
delete!,
delete,
deleteat!,
deleteat,
keepat!,
eltype,
empty!,
Expand Down
39 changes: 37 additions & 2 deletions base/namedtuple.jl
Original file line number Diff line number Diff line change
Expand Up @@ -375,8 +375,43 @@ julia> Base.setindex(nt, "a", :a)
(a = "a",)
```
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

setindex(x::NamedTuple{names}, v, i::Integer) where {names} = NamedTuple{names}(setindex(values(x), v, i))

function setindex(nt::NamedTuple, v, idx::Symbol)
merge(nt, (; idx => v))
setindex(nt::NamedTuple, v, idx::Symbol) = merge(nt, (; idx => v))
function setindex(nt::NamedTuple, vs, idxs::AbstractVector{Symbol})
length(vs) == length(idxs) || throw_setindex_mismatch(v, idxs)
merge(nt, (; [i => v for (i,v) in zip(idxs, vs)]...))
Copy link
Contributor

Choose a reason for hiding this comment

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

There might be a little footgun hidden here, as zip does not check lengths:

julia> zip([:a,:c], [1,2,3])
zip([:a, :c], [1, 2, 3])

julia> collect(zip([:a,:c], [1,2,3]))
2-element Vector{Tuple{Symbol, Int64}}:
 (:a, 1)
 (:c, 2)

So I think there should be a length check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also allows duplicates, which getindex does not. Is this indended?

julia> _setindex((x=1, y=2, z=3), [4, 5, 6, 7], [:x, :y, :x])
(x = 6, y = 5, z = 3)

julia> (x=1, y=2, z=3)[[:x, :y, :x]]
ERROR: duplicate field name

Only one test https://github.com/JuliaLang/julia/pull/46453/files#diff-a4ae685ea77cc80977dc4214e7dbb9d96fed55bdb918396d3453f840da0a695bR308

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured this was more like the behavior of setindex! where a value can be overwritten multiple times.

Copy link
Contributor

@mcabbott mcabbott Sep 3, 2022

Choose a reason for hiding this comment

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

But get/set both allow repeats for arrays. I'm not sure whether getindex disallowing repeats here is a feature or a bug, either. Do you know?

For arrays, the types must match: setindex!(rand(10), (3, 4), 5:6) is an error, rather than itererating the tuple, because getindex(rand(10), 5:6) is a vector. Is there an argument for why this method should accept & iterate anything?

julia> rand(10)[5:6]
2-element Vector{Float64}:
 0.46482934129714726
 0.06656695848727301

julia> (x=1, y=2, z=3)[[:x, :y]]
(x = 1, y = 2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But get/set both allow repeats for arrays. I'm not sure whether getindex disallowing repeats here is a feature or a bug, either. Do you know?

I think it's just because you can't have repeated fields in a NamedTuple, so (x=1, y=2, z=3)[[:x, :y, :x]] would have to make up a new symbol for the last :x in order for it to work.

For arrays, the types must match: setindex!(rand(10), (3, 4), 5:6) is an error, rather than itererating the tuple, because getindex(rand(10), 5:6) is a vector. Is there an argument for why this method should accept & iterate anything?

I'm not completely sure what rules should carry over. You also can't index arrays with anything but integers and arrays of integers. I don't know if this means that anytime setindex sets multiple values the collection should be an array, or if the type should match the destination.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I guess the lack of repeats in getindex is indeed forced on you by the return type.

The others I don't know, I just think they need careful thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An alternative definition could be

function setindex(nt::NamedTuple, vs, idxs::Union{AbstractVector{Symbol},Tuple{Vararg{Symbol}}})
    merge(nt, NamedTuple{Tuple(idxs)}(vs))
end

which requires vs have a conversion to Tuple and matches the definition of getindex more.

end

"""
delete(nt::NamedTuple, key::Symbol)
delete(nt::NamedTuple, key::Integer)

Constructs a new `NamedTuple` with the field corresponding to `key` removed.
If no field corresponds to `key`, `nt` is returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

This differs from the error for tuples with out-of-bounds index. The logic is that setindex lets you add arbitrary keys? But not for integers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following the behavior of delete which doesn't throw an error if key isn't found. deleteat! and deleteat do throw errors if the field/index isn't found.


See also: [`delete!`](@ref), [`deleteat`](@ref)

# Examples
```jldoctest
julia> nt = (a = 1, b = 2, c = 3)
(a = 1, b = 2, c = 3)

julia> delete(nt, :b)
(a = 1, c = 3)

julia> delete(nt, 2)
(a = 1, c = 3)

julia> delete(nt, :z)
(a = 1, b = 2, c = 3)
```
"""
function delete(nt::NamedTuple, i::Symbol)
fidx = fieldindex(typeof(nt), i, false)
fidx === 0 ? nt : unsafe_delete(nt, fidx)
end
delete(nt::NamedTuple, i::Integer) = (i < 1 || i > length(nt)) ? nt : unsafe_delete(nt, i)
function unsafe_delete(nt::NamedTuple{names}, i::Integer) where {names}
NamedTuple{_deleteat(names, i)}(_deleteat(Tuple(nt), i))
end

"""
Expand Down
Loading