Skip to content

Avoid piracy #1248

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

Open
aplavin opened this issue Mar 9, 2024 · 3 comments · May be fixed by #1302
Open

Avoid piracy #1248

aplavin opened this issue Mar 9, 2024 · 3 comments · May be fixed by #1302

Comments

@aplavin
Copy link
Member

aplavin commented Mar 9, 2024

StaticArrays does some piracy, at least in similar():

const HeterogeneousBaseShape = Union{Integer, Base.OneTo}
const HeterogeneousShape = Union{HeterogeneousBaseShape, SOneTo}
const HeterogeneousShapeTuple = Tuple{Vararg{HeterogeneousShape}}
similar(A::AbstractArray, ::Type{T}, shape::HeterogeneousShapeTuple) where {T} = similar(A, T, homogenize_shape(shape))
similar(::Type{A}, shape::HeterogeneousShapeTuple) where {A<:AbstractArray} = similar(A, homogenize_shape(shape))

Here,

HeterogeneousShapeTuple = Tuple{Vararg{Union{Integer, Base.OneTo, SOneTo} }} 

and it fits eg Tuple{}, Tuple{Int}, Tuple{Int, Int}.

It's generally a good thing to avoid piracy in foundational packages. Maybe, also related to ambiguities like JuliaArrays/StructArrays.jl#279?..

@jishnub
Copy link
Member

jishnub commented Mar 10, 2024

Strictly speaking, this isn't piracy, as the narrower method is defined in Base. It would be good to remove this, though, if possible

@mateuszbaran
Copy link
Collaborator

It would be great if we actually had non-ambiguous documentation about what is and what isn't type piracy so that we can be sure what is and what is not wrong, ref. JuliaLang/julia#51669 . Currently there seems to be no consensus nor clear communication from core Julia devs regarding this "wider union" case.

@jishnub
Copy link
Member

jishnub commented Dec 27, 2024

The generic methods with mixed axis types should not be necessary after JuliaLang/julia#56902 if SOneTo subtypes AbstractOneTo. In that case, only the method with SOneTo axes would need to be defined. This should resolve the piracy issue, although not the ambiguity.

jishnub added a commit to JuliaLang/julia that referenced this issue Apr 1, 2025
Currently, `Base` defines `similar` for `Base.OneTo`, with the
understanding that offset axes will be handled elsewhere. However,
`Base.OneTo` is just one possible one-based range, and there are others
such as `StaticArrays.SOneTo` that exist in the ecosystem. `Base`
doesn't provide a method to handle a combination of different one-based
ranges in `similar`, which leaves the packages in an awkward position:
they need to define methods like
```julia
similar(A::AbstractArray, ::Type{T}, shape::HeterogeneousShapeTuple) where {T} = similar(A, T, homogenize_shape(shape))
```
where `HeterogeneousShapeTuple` is defined as
```julia
Tuple{Vararg{Union{Integer, Base.OneTo, SOneTo}}}
```

https://github.com/JuliaArrays/StaticArrays.jl/blob/07c12450d1b3481dda4b503564ae4a5cb4e27ce4/src/abstractarray.jl#L141-L146
Unfortunately, such methods are borderline type-piracy, as noted in
JuliaArrays/StaticArrays.jl#1248. In
particular, if the narrower `Base` method that handles `Union{Integer,
OneTo}` is removed, then this method explicitly becomes pirating.

A solution to this situation is to have `Base` handle all one-based
ranges, such that arbitrary combinations of one-based ranges hit
fallback methods in `Base`. This PR is a first step in this direction.
We add the abstract type `AbstractOneTo`, and have `OneTo` be its
subtype. We also add methods to `similar` and `reshape` that accept
`AbstractOneTo` arguments. This makes it unnecessary for packages to
dispatch on awkward combinations of `Union{Integer, OneTo}` and custom
one-based axes, as the base implementation would handle such cases
already.

There may be other methods that accept an `AbstractOneTo` instead of a
`OneTo`, but these may be addressed in separate PRs. Also, there may be
one-based ranges that can't subtype `AbstractOneTo`, and a full solution
that accepts such ranges as well needs to be implemented through a
trait. This may also be handled in a separate PR.

---------

Co-authored-by: Tim Holy <[email protected]>
@jishnub jishnub linked a pull request Apr 2, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants