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

@SArray namespace hygiene #691

Closed
goretkin opened this issue Nov 19, 2019 · 6 comments
Closed

@SArray namespace hygiene #691

goretkin opened this issue Nov 19, 2019 · 6 comments
Labels

Comments

@goretkin
Copy link

using StaticArrays: @SArray
@SArray Float32[1, 2]

gives
ERROR: UndefVarError: SArray not defined

indeed:

julia> macroexpand(Main, :(@SArray [1, 2]))
:((StaticArrays.SArray{Tuple{2},T,N,L} where L where N where T)((1, 2)))

julia> macroexpand(Main, :(@SArray Float32[1, 2]))
:(SArray{Tuple{2}, Float32}((1, 2)))

The definition

macro SArray(ex)
if !isa(ex, Expr)
error("Bad input for @SArray")
end
if ex.head == :vect # vector
return esc(Expr(:call, SArray{Tuple{length(ex.args)}}, Expr(:tuple, ex.args...)))
elseif ex.head == :ref # typed, vector
return esc(Expr(:call, Expr(:curly, :SArray, Tuple{length(ex.args)-1}, ex.args[1]), Expr(:tuple, ex.args[2:end]...)))
elseif ex.head == :hcat # 1 x n
s1 = 1
s2 = length(ex.args)
return esc(Expr(:call, SArray{Tuple{s1, s2}}, Expr(:tuple, ex.args...)))
elseif ex.head == :typed_hcat # typed, 1 x n
s1 = 1
s2 = length(ex.args) - 1
return esc(Expr(:call, Expr(:curly, :SArray, Tuple{s1, s2}, ex.args[1]), Expr(:tuple, ex.args[2:end]...)))
elseif ex.head == :vcat
if isa(ex.args[1], Expr) && ex.args[1].head == :row # n x m
# Validate
s1 = length(ex.args)
s2s = map(i -> ((isa(ex.args[i], Expr) && ex.args[i].head == :row) ? length(ex.args[i].args) : 1), 1:s1)
s2 = minimum(s2s)
if maximum(s2s) != s2
error("Rows must be of matching lengths")
end
exprs = [ex.args[i].args[j] for i = 1:s1, j = 1:s2]
return esc(Expr(:call, SArray{Tuple{s1, s2}}, Expr(:tuple, exprs...)))
else # n x 1
return esc(Expr(:call, SArray{Tuple{length(ex.args), 1}}, Expr(:tuple, ex.args...)))
end
elseif ex.head == :typed_vcat
if isa(ex.args[2], Expr) && ex.args[2].head == :row # typed, n x m
# Validate
s1 = length(ex.args) - 1
s2s = map(i -> ((isa(ex.args[i+1], Expr) && ex.args[i+1].head == :row) ? length(ex.args[i+1].args) : 1), 1:s1)
s2 = minimum(s2s)
if maximum(s2s) != s2
error("Rows must be of matching lengths")
end
exprs = [ex.args[i+1].args[j] for i = 1:s1, j = 1:s2]
return esc(Expr(:call, Expr(:curly, :SArray, Tuple{s1, s2}, ex.args[1]), Expr(:tuple, exprs...)))
else # typed, n x 1
return esc(Expr(:call, Expr(:curly, :SArray, Tuple{length(ex.args)-1, 1}, ex.args[1]), Expr(:tuple, ex.args[2:end]...)))
end
elseif isa(ex, Expr) && ex.head == :comprehension
if length(ex.args) != 1 || !isa(ex.args[1], Expr) || ex.args[1].head != :generator
error("Expected generator in comprehension, e.g. [f(i,j) for i = 1:3, j = 1:3]")
end
ex = ex.args[1]
n_rng = length(ex.args) - 1
rng_args = [ex.args[i+1].args[1] for i = 1:n_rng]
rngs = Any[Core.eval(__module__, ex.args[i+1].args[2]) for i = 1:n_rng]
rng_lengths = map(length, rngs)
f = gensym()
f_expr = :($f = ($(Expr(:tuple, rng_args...)) -> $(ex.args[1])))
# TODO figure out a generic way of doing this...
if n_rng == 1
exprs = [:($f($j1)) for j1 in rngs[1]]
elseif n_rng == 2
exprs = [:($f($j1, $j2)) for j1 in rngs[1], j2 in rngs[2]]
elseif n_rng == 3
exprs = [:($f($j1, $j2, $j3)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3]]
elseif n_rng == 4
exprs = [:($f($j1, $j2, $j3, $j4)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3], j4 in rngs[4]]
elseif n_rng == 5
exprs = [:($f($j1, $j2, $j3, $j4, $j5)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3], j4 in rngs[4], j5 in rngs[5]]
elseif n_rng == 6
exprs = [:($f($j1, $j2, $j3, $j4, $j5, $j6)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3], j4 in rngs[4], j5 in rngs[5], j6 in rngs[6]]
elseif n_rng == 7
exprs = [:($f($j1, $j2, $j3, $j4, $j5, $j6, $j7)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3], j4 in rngs[4], j5 in rngs[5], j6 in rngs[6], j7 in rngs[7]]
elseif n_rng == 8
exprs = [:($f($j1, $j2, $j3, $j4, $j5, $j6, $j7, $j8)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3], j4 in rngs[4], j5 in rngs[5], j6 in rngs[6], j7 in rngs[7], j8 in rngs[8]]
else
error("@SArray only supports up to 8-dimensional comprehensions")
end
return quote
$(esc(f_expr))
$(esc(Expr(:call, Expr(:curly, :SArray, Tuple{rng_lengths...}), Expr(:tuple, exprs...))))
end
elseif isa(ex, Expr) && ex.head == :typed_comprehension
if length(ex.args) != 2 || !isa(ex.args[2], Expr) || ex.args[2].head != :generator
error("Expected generator in typed comprehension, e.g. Float64[f(i,j) for i = 1:3, j = 1:3]")
end
T = ex.args[1]
ex = ex.args[2]
n_rng = length(ex.args) - 1
rng_args = [ex.args[i+1].args[1] for i = 1:n_rng]
rngs = [Core.eval(__module__, ex.args[i+1].args[2]) for i = 1:n_rng]
rng_lengths = map(length, rngs)
f = gensym()
f_expr = :($f = ($(Expr(:tuple, rng_args...)) -> $(ex.args[1])))
# TODO figure out a generic way of doing this...
if n_rng == 1
exprs = [:($f($j1)) for j1 in rngs[1]]
elseif n_rng == 2
exprs = [:($f($j1, $j2)) for j1 in rngs[1], j2 in rngs[2]]
elseif n_rng == 3
exprs = [:($f($j1, $j2, $j3)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3]]
elseif n_rng == 4
exprs = [:($f($j1, $j2, $j3, $j4)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3], j4 in rngs[4]]
elseif n_rng == 5
exprs = [:($f($j1, $j2, $j3, $j4, $j5)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3], j4 in rngs[4], j5 in rngs[5]]
elseif n_rng == 6
exprs = [:($f($j1, $j2, $j3, $j4, $j5, $j6)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3], j4 in rngs[4], j5 in rngs[5], j6 in rngs[6]]
elseif n_rng == 7
exprs = [:($f($j1, $j2, $j3, $j4, $j5, $j6, $j7)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3], j4 in rngs[4], j5 in rngs[5], j6 in rngs[6], j7 in rngs[7]]
elseif n_rng == 8
exprs = [:($f($j1, $j2, $j3, $j4, $j5, $j6, $j7, $j8)) for j1 in rngs[1], j2 in rngs[2], j3 in rngs[3], j4 in rngs[4], j5 in rngs[5], j6 in rngs[6], j7 in rngs[7], j8 in rngs[8]]
else
error("@SArray only supports up to 8-dimensional comprehensions")
end
return quote
$(esc(f_expr))
$(esc(Expr(:call, Expr(:curly, :SArray, Tuple{rng_lengths...}, T), Expr(:tuple, exprs...))))
end
elseif isa(ex, Expr) && ex.head == :call
if ex.args[1] == :zeros || ex.args[1] == :ones || ex.args[1] == :rand || ex.args[1] == :randn || ex.args[1] == :randexp
if length(ex.args) == 1
error("@SArray got bad expression: $(ex.args[1])()")
else
return quote
if isa($(esc(ex.args[2])), DataType)
$(ex.args[1])($(esc(Expr(:curly, SArray, Expr(:curly, Tuple, ex.args[3:end]...), ex.args[2]))))
else
$(ex.args[1])($(esc(Expr(:curly, SArray, Expr(:curly, Tuple, ex.args[2:end]...)))))
end
end
end
elseif ex.args[1] == :fill
if length(ex.args) == 1
error("@SArray got bad expression: $(ex.args[1])()")
elseif length(ex.args) == 2
error("@SArray got bad expression: $(ex.args[1])($(ex.args[2]))")
else
return quote
$(esc(ex.args[1]))($(esc(ex.args[2])), SArray{$(esc(Expr(:curly, Tuple, ex.args[3:end]...)))})
end
end
else
error("@SArray only supports the zeros(), ones(), rand(), randn(), and randexp() functions.")
end
else
error("Bad input for @SArray")
end
end

@c42f c42f added the bug label Nov 19, 2019
@c42f
Copy link
Member

c42f commented Nov 19, 2019

Yep this is a bug, thanks for the report.

It's probably not too hard to fix, but I'm wishing we could remove these macros, or at least reduce their number (eg, removing @SMatrix, @SVector).

With that in mind, what you think about the following alternative which was recently implemented in version 0.12?

julia> using StaticArrays: SA_F64

julia> SA_F64[1,2]
2-element StaticArrays.SArray{Tuple{2},Float64,1,2} with indices SOneTo(2):
 1.0
 2.0

It's a syntax pun of course, though not particularly different from the existing getindex / typed vector initialization pun that Base already uses.

@goretkin
Copy link
Author

Thanks for asking what I think. My first reaction is that I did not like that alternative, since it's not orthogonal. What if I wanted a StaticArray of MyQuaternion? I admit not trying the @SArray macro in more complicated settings, but at least there's a syntactical way for it to work.

But I see that you could do SA{MyQuaternion}, so that concern is not valid.

I wonder if there would be some generic code that assumes eltype(T[...])===T, which would break under this pun. I think I prefer the macro over the pun, even though I've run into a few pain points with @SArray (related to not having enough parens, and not understanding the syntax issue) which would be fixed by this pun.

Are there other reasons you don't like the macro?

@c42f
Copy link
Member

c42f commented Nov 19, 2019

I wonder if there would be some generic code that assumes eltype(T[...])===T, which would break under this pun

Only if there's some unexpected way for T to become SA which seems unlikely because SA is designed to be used as syntax. As I alluded to, this syntax pun already exists in julia:

julia> foo(T) = T[1,2]  # This is typed array construction, right?

julia> foo(rand(2,2))
0.11408848854832776  # wait, what :-D

We did discuss having an improved @SA macro vs the SA syntax. See the discussion at #633 and #636. It was super hard to decide but I felt that for literals it's desirable to have the shortest syntax, especially if we intend to do composition via conversion such as the syntax MMatrix(SA[1 2; 3 4]) for MMatrix literals.

I don't think the macros will be going away any time soon, I think that would be quite unncessarily breaking for 1.0. But I'm still wondering about a viable transition plan for reducing the number of "constructor macros" and hoping people will be happy enough with the new syntax that we could go ahead with that eventually.

@goretkin
Copy link
Author

As I alluded to, this syntax pun already exists in julia

Yes, right. But this is a pun on top of that pun, I'd think. You demonstrate that Base already breaks eltype(T[...])===T. What about eltype(getindex(::Type{T}, ...))===T?

@c42f
Copy link
Member

c42f commented Nov 19, 2019

Indeed, it adds a third meaning to the syntax which could be regarded as a good or bad move, depending on how often you happen to be writing short SVector and SMatrix literals.

However I can't think of a situation where SA would be likely to be used and where violating eltype(getindex(::Type{T}, ...))===T would actually matter. (It would matter if one might pass the SA type around as data, but using it directly in literal syntax seems harmless.)

@hyrodium
Copy link
Collaborator

The original issue was already solved.

julia> using StaticArrays: @SArray

julia> @SArray Float32[1, 2]
2-element StaticArraysCore.SVector{2, Float32} with indices SOneTo(2):
 1.0
 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants