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

NamedTuples changed iteration behavior for vararg keywords #25177

Closed
rdeits opened this issue Dec 19, 2017 · 11 comments
Closed

NamedTuples changed iteration behavior for vararg keywords #25177

rdeits opened this issue Dec 19, 2017 · 11 comments

Comments

@rdeits
Copy link
Contributor

rdeits commented Dec 19, 2017

This seems to be a consequence of #24795

In previous Julia versions, we could iterate through vararg keywords as key-value tuples:

julia> function foo(;kw...)
         for (k, v) in kw
           @show (k, v)
         end
       end
foo (generic function with 1 method)

julia> foo(x=1, y=2)
(k, v) = (:x, 1)
(k, v) = (:y, 2)

but after #24795, iterating through vararg keywords only gives the values:

julia> function foo(;kw...)
         for (k, v) in kw
           @show (k, v)
         end
       end
foo (generic function with 1 method)

julia> foo(x=1, y=2)
ERROR: BoundsError: attempt to access 1
  at index [2]
Stacktrace:
 [1] indexed_next at ./tuple.jl:60 [inlined]
 [2] #foo#2(::NamedTuple{(:x, :y),Tuple{Int64,Int64}}, ::Function) at ./REPL[3]:2
 [3] (::getfield(, Symbol("#kw##foo")))(::NamedTuple{(:x, :y),Tuple{Int64,Int64}}, ::typeof(foo)) at ./<missing>:0
 [4] top-level scope

If this is an intended change, then it should probably at least be mentioned in NEWS.md. I realize that the changelog mentions that "name-value pairs can be iterated using pairs(kw)" but I think it's important to clarify that this is a breaking change.

@rdeits
Copy link
Contributor Author

rdeits commented Dec 19, 2017

It looks like the correct way to do this in a cross-version way is:

using Compat: pairs

function foo(;kw...)
  for (k, v) in pairs(kw)
    @show (k, v)
  end
end

@vtjnash
Copy link
Member

vtjnash commented Dec 19, 2017

Perhaps we should call the inner function with the object kw = pairs(kw'), so that there's no change to the API, just faster kwargs?

@rdeits
Copy link
Contributor Author

rdeits commented Dec 19, 2017

That would definitely make adapting existing code easier.

@andyferris
Copy link
Member

I think this was known, but it's annoying as a breaking change.

Wrapping in pairs would destroy the ability to getfield the kwargs, no?

@vtjnash
Copy link
Member

vtjnash commented Dec 19, 2017

no, it'll still maintain it's new abilities from now being a NamedTuple

@StefanKarpinski
Copy link
Member

That does not seem to be true at all, @vtjnash:

julia> f(; kws...) = pairs(kws)
f (generic function with 1 method)

julia> kws = f(a = 1, b = 2)
Base.Generator{Base.Iterators.Zip2{Tuple{Symbol,Symbol},Tuple{Int64,Int64}},getfield(Base, Symbol("##7#8")){Pair}}(getfield(Base, Symbol("##7#8")){Pair}(), Base.Iterators.Zip2{Tuple{Symbol,Symbol},Tuple{Int64,Int64}}((:a, :b), (1, 2)))

julia> kws.a
ERROR: type Generator has no field a

@vtjnash
Copy link
Member

vtjnash commented Dec 19, 2017

You don’t seem to be on master

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Dec 19, 2017

I'm on a day-old version of master so unless this is more recent than that.

@StefanKarpinski
Copy link
Member

Fresh master has the same error, although it does print the key-value map in the pairs object:

julia> f(; kws...) = pairs(kws)
f (generic function with 1 method)

julia> kws = f(a = 1, b = 2)
Base.Iterators.IndexValue{Symbol,Int64,Tuple{Symbol,Symbol},NamedTuple{(:a, :b),Tuple{Int64,Int64}}} with 2 entries:
  :a => 1
  :b => 2

julia> kws.a
ERROR: type IndexValue has no field a
Stacktrace:
 [1] getproperty(::Base.Iterators.IndexValue{Symbol,Int64,Tuple{Symbol,Symbol},NamedTuple{(:a, :b),Tuple{Int64,Int64}}}, ::Symbol) at ./sysimg.jl:8
 [2] top-level scope

@vtjnash
Copy link
Member

vtjnash commented Dec 19, 2017

kws[:a]getproperty overloading only just merged yesterday, so we haven't yet had time to implement that API

@c42f
Copy link
Member

c42f commented Sep 27, 2019

This was fixed by #25290

@c42f c42f closed this as completed Sep 27, 2019
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

No branches or pull requests

5 participants