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

More specific characterization of mutable collections with can_setindex and can_change_size #46500

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Tokazama
Copy link
Contributor

@Tokazama Tokazama commented Aug 26, 2022

This implements can_setindex and can_change_size, which provide more clear information that is related to ismutable but isn't necessarily the same.
Knowing that the length can change may invalidate bounds checking assumptions and collections that can use setindex! aren't threadsafe (#41214, #40096).
Helpful for making optionally mutating operations generic (#30407).
Sometimes we have a type where we want to disallow these mutating behaviors even if there is somehow generic support for them.

@mcabbott
Copy link
Contributor

mcabbott commented Nov 8, 2022

I think that some tests / traits for such properties would be valuable. And the reason to have them in Base is to make extending them frictionless. But they need to be handled more carefully than this PR does.

    can_change_size(::Type{T}) -> Bool

Returns `true` if instances of `T` may change size through operations such as `pop!` and
`popfirst!`.

The rough idea is clear, it's to distinguish things like Vector from things like SVector & UnitRange. But what exactly does "may change size" mean?

true could be offering a guarantee that operations like pop! will work. Then it must fail to false on unknown types. However, this cannot be known from the type alone:

julia> v = [1,2,3,4]; m = reshape(v, 2,2); pop!(v)  # Vector{Int} is not enough
ERROR: cannot resize array with shared data

Alternatively, false could be offering a guarantee that pop! will not work, and hence that an instance cannot be changed by someone else while you weren't looking. Then it must default to true. That's what you hint at above:

Knowing that the length can change may invalidate bounds checking

Returning Bool is limiting, maybe it should return a trait, like how IteratorSize can return SizeUnknown? But acting on a type means Vector is unknown. Can it act on an instance instead, and return Union{Bool, Nothing}? What's best may depend on what this is to be used for. What is this to be used for?

(And why say "change size" not length, given that it accepts non-array collections, and size(Dict(1=>2)) is an error?)

    can_setindex(::Type{T}) -> Bool

Query whether a type can use `setindex!`.

Again, the rough idea is clear, but what exactly does true or false promise?

collections that can use setindex! aren't threadsafe

Some are safe, some aren't.

What's envisaged for CuArrays (which allow mutation, but not one element at a time).

Can what's desired be known from the type or does it needs an instance, and the proposed indices? It might want to be a close cousin of checkbounds except for writing not reading.

julia> x = Bidiagonal([1,2,3], [4,5], :L)
3×3 Bidiagonal{Int64, Vector{Int64}}:
 1    
 4  2  
   5  3

julia> x[2,1] = 10;  # this is OK, but typeof(x) doesn't know about :L

julia> x[1,2] = 10
ERROR: ArgumentError: cannot set entry (1, 2) off the lower bidiagonal band to a nonzero value (10)

julia> checkbounds(Bool, x, 1, 3)  # tells you about getindex
true

julia> d = Dict(1=>:A, 2=>:B);

julia> d[1:2] = [:C, :D]  # only some kinds of `setindex!` work here
ERROR: MethodError: Cannot `convert` an object of type UnitRange{Int64} to an object of type Int64

julia> Base.setindex((:a, :b, :c), :Z, 3)  # can setindex, just not setindex!
(:a, :b, :Z)

@aplavin
Copy link
Contributor

aplavin commented Nov 8, 2022

    can_setindex(::Type{T}) -> Bool

Query whether a type can use `setindex!`.

Note that the name suggests the question is about setindex(x, v, i), not setindex!(x, v, i).

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 8, 2022

can_change_length is probably a better name than cant_change_size. can_setindex!(x) seems more like it's mutating x so that you can set its index. I'm open to whatever name everyone converges on though.

@mcabbott, it might be of interest to see the original issue that preceded these being implemented in ArrayInterface (JuliaArrays/ArrayInterface.jl#22). The original intention was to pass types so that this information would be known at compile time. It seems there are very few situations where this could return true with type only info. Perhaps it would be better to have something like fixed_length_type(T::Type). This would allow Bool to be accurately determined without instances.

@jariji
Copy link
Contributor

jariji commented Dec 21, 2023

I'd propose Try.jl-style trysetindex!()::Union{Ok,Err} instead - if you want to set an index, try doing so, and if it doesn't work try something else. That way there's no need for detailed parsing of semantics about "what does can_set_index! mean, how does it differ from setindex!", "is can_ an upper bound or lower bound" -- just try and if it works it works.

@Tokazama
Copy link
Contributor Author

Even TryExperimental.jl still uses traits like Base.IteratorSize to avoid the try-catch block where possible. Mutating values at a specific index is a pretty fundamental part of any collection types interface and is already has corollaries in base such as ismutabletype.

@jariji
Copy link
Contributor

jariji commented Dec 21, 2023

The point of Try.jl is to redesign the APIs so that there is no try/catch at all: failure doesn't throw, it returns Err{MethodError(...)}.

@Tokazama
Copy link
Contributor Author

The point of Try.jl is to redesign the APIs so that there is no try/catch at all: failure doesn't throw, it returns Err{MethodError(...)}.

That still requires doing a try catch block internally on methods that don't have relevant trait. If anything these methods would support creating the type of API you want.

@jariji
Copy link
Contributor

jariji commented Dec 21, 2023

My worry is that can_set_index! is going to be a not-required part of collections that can set index, so it will become impossible to implement the no-catch design: If can_setindex! isn't required by the collection interface, then returning false is not a reliable indicator that a collection is unable to setindex! and people will get a false, not trust it, and try setindex! catch, which undermines the whole no-catch program.

@adienes
Copy link
Contributor

adienes commented Dec 21, 2023

isassignable as parallel to isassigned ?

@Tokazama
Copy link
Contributor Author

My worry is that can_set_index! is going to be a not-required part of collections that can set index, so it will become impossible to implement the no-catch design: If can_setindex! isn't required by the collection interface, then returning false is not a reliable indicator that a collection is unable to setindex! and people will get a false, not trust it, and try setindex! catch, which undermines the whole no-catch program.

I may be missing something because I don't see how this is any different from adding any trait to base that might be a branch point. We try to choose the safest default and the ability of downstream methods working correctly depends on adherence to the trait method. The alternative is to delay improving the situation at all until Julia v2 where we can redefine indexing interfaces. All interfaces that accomplish what you're aiming for require some degree of additional traits (internally or externally). BangBang.jl also has to implement its own variant of checking index mutability traits to accomplish this.

isassignable as parallel to isassigned ?

Sounds good to me. We could also imitate the syntax for ismutabletype and have is_mutable_index_type/is_mutable_size_type. I'm pretty sure I'll be fine with whatever choice people come up with here. I just want the functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants