remove empty namedtuple from key when no kwargs

[jwscook]

I was getting an error when I was trying to do something like below, where I want to constrain the key and value types (as well as put in a key-value pair on construction).

using Memoization

foo(x) = x
function bar(::T) where {T<:Integer}
  @memoize Dict{UInt64,T}(UInt64(0)=>T(0)) function baz(x); return foo(x); end
  return baz
end

qux = bar(1)
qux(1)

The error is:

ERROR: MethodError: Cannot `convert` an object of type Tuple{Tuple{Int64}, NamedTuple{(), Tuple{}}} to an object of type UInt64
Closest candidates are:
  convert(::Type{T}, ::Ptr) where T<:Integer at pointer.jl:23
  convert(::Type{T}, ::T) where T<:Number at number.jl:6
  convert(::Type{T}, ::Number) where T<:Number at number.jl:7
  ...
Stacktrace:
 [1] get!(default::Function, h::Dict{UInt64, Int64}, key0::Tuple{Tuple{Int64}, NamedTuple{(), Tuple{}}})
   @ Base ./dict.jl:452
 [2] _get!(::Function, ::Vararg{Any, N} where N)
   @ Memoization ~/.julia/dev/Memoization/src/Memoization.jl:131
 [3] (::var"#baz#3"{Int64})(x::Int64)
   @ Main ~/.julia/dev/Memoization/src/Memoization.jl:98
 [4] top-level scope
   @ REPL[5]:1

This appears to be because there is a NamedTuple in the key type to handle kwargs. This PR fixes this by findout out if the variable kwarg_signature signature is empty in an if statement and then removing that from the quoted expression. There may be a more elegant way of doing this - please advise.

With the fix:

julia> using Memoization
[ Info: Precompiling Memoization [6fafb56a-5788-4b4e-91ca-c0cea6611c73]

julia> foo(x) = x
foo (generic function with 1 method)

julia> function bar(::T) where {T<:Integer}
         @memoize Dict{UInt64,T}(UInt64(0)=>T(0)) function baz(x); return foo(x); end
         return baz
       end
bar (generic function with 1 method)

julia> qux = bar(1)
(::var"#baz#3"{Int64}) (generic function with 1 method)

julia> qux(1)
1

julia> Memoization.caches
IdDict{Any, Any} with 1 entry:
  baz => Dict{UInt64, Int64}(0x0000000000000000=>0, 0x0000000000000001=>1)

[marius311]

Hey, many thanks for this. I agree it would be nice to have something like this. I think we can shorten the code a bit, plus I'd like to think a bit more to make sure there's no unintended side-effects. Will get back to you later this week / merge.

[marius311]

Hey, sorry for the delay, thought more about this but I think I see an issue with approach. It seems to me with this PR, both of these call signatures would lead to the identical key:

foo( ((1,), (x=2,)) ) # per your new code, the cache key would be just `((1,), (x=2,))` itself
foo(1, x=2) # following the existing rules, the cache key would also be `((1,), (x=2,))`

I think to make this work, we would need special code to signify when there are kwargs, vs. a single arg which is itself a tuple.

[jwscook]

Oh... doesn't my PR fail that case completely?

julia> @memoize foo(a, x=2) = 1
foo (generic function with 2 methods)

julia> foo(1)
ERROR: MethodError: no method matching get!(::var"##getter#258#2", ::IdDict{Any, Any}, ::Int64, ::Int64)
Closest candidates are:
  get!(::Union{Function, Type}, ::IdDict{K, V}, ::Any) where {K, V} at iddict.jl:160
  get!(::Union{Function, Type}, ::AbstractDict{K, V}, ::Any) where {V, K} at abstractdict.jl:508
  get!(::IdDict{K, V}, ::Any, ::Any) where {K, V} at iddict.jl:140
  ...
Stacktrace:
 [1] _get!(::Function, ::Vararg{Any, N} where N)
   @ Memoization ~/.julia/dev/Memoization/src/Memoization.jl:140
 [2] foo(a::Int64, x::Int64)
   @ Main ~/.julia/dev/Memoization/src/Memoization.jl:99
 [3] foo(a::Int64)
   @ Main ~/.julia/dev/Memoization/src/Memoization.jl:97
 [4] top-level scope
   @ REPL[6]:1

[marius311]

Seems so, I didn't actually check it though. Thats probably a fixable thing, but I think the fundamental issue above is the bigger thing that needs thought. Tbh I was into the original idea since I definitely see the value of being able to pass in a cache preinitialized in the simple way you have above, but I'm less sure now its worth it if it requires alot of added complexity under the hood. Open to clever solutions though.

[jwscook]

The only thing I can think of right now is to dispatch on two different _get! functions: one for kwargless functions; and the other for kwargfull ones. It's a bloaty solution though. Not ideal.