goretkin / FixArgs.jl

Other
19 stars 3 forks source link

Fix inference for fix(::Type, ...) #7

Closed tkf closed 4 years ago

tkf commented 4 years ago

This PR fixes inference for, e.g., fix(CartesianIndex, nothing, Some(1))(2).

codecov-commenter commented 4 years ago

Codecov Report

Merging #7 into master will increase coverage by 0.75%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
+ Coverage   90.90%   91.66%   +0.75%     
==========================================
  Files           1        1              
  Lines          11       12       +1     
==========================================
+ Hits           10       11       +1     
  Misses          1        1              
Impacted Files Coverage Δ
src/Curry.jl 91.66% <100.00%> (+0.75%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e20fa8b...c63b9e7. Read the comment docs.

goretkin commented 4 years ago

Thanks for this improvement @tkf! It looks good to merge to me. I don't profoundly understand the issue, so let me write down what I do know. If anyone has some explanations, I would appreciate it.

First I thought this was related to CartesianIndex being a parametric type. But I see that it's not. Before this commit:

julia> struct A
       x::Int
       y::Int
       end

julia> fix(A, nothing, 2)
(::Fix{DataType,Tuple{Nothing,Int64},Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}}) (generic function with 1 method)

julia> fix(A, nothing, 2)(1)
A(1, 2)

julia> @inferred fix(A, nothing, 2)(1)
ERROR: return type A does not match inferred return type Any

To highlight the issue

julia> @inferred Fix{DataType, Tuple{Nothing,Int64}, NamedTuple{(),Tuple{}}}(A, (nothing, 2), NamedTuple())(1)
ERROR: return type A does not match inferred return type Any
julia> @inferred Fix{Type{A}, Tuple{Nothing,Int64}, NamedTuple{(),Tuple{}}}(A, (nothing, 2), NamedTuple())(1)
A(1, 2)

And then I believe the issue that @jw3126 alluded to might have to do with

julia> typeof((A,3))
Tuple{DataType,Int64}

which seems to be what @tkf mentions here: https://github.com/JuliaLang/julia/pull/35980#issuecomment-637082293

jw3126 commented 4 years ago

Yeah I think you understood it correctly.

tkf commented 4 years ago

@goretkin Yeah, I think your understanding is accurate. I think it'll be explained in the manual after https://github.com/JuliaLang/julia/pull/36591