Fix inference for fix(::Type, ...) #7
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| @@ -48,6 +48,8 @@ struct Fix{F, A, K} <: Function | |||
| k::K | |||
| end | |||
|
|
|||
| Fix(::Type{T}, a, k) where {T} = Fix{Type{T}, typeof(a), typeof(k)}(T, a, k) | |||
There was a problem hiding this comment.
Oh nice, I was not aware, that one can do this.
| f = @inferred fix(CartesianIndex, nothing, Some(1)) | ||
| @test @inferred(f(2)) === CartesianIndex(2, 1) | ||
| end | ||
|
|
There was a problem hiding this comment.
I guess if a type occurs in the fixed arguments, inference will fail? We could add a broken test for that (not sure whats the best way to spell out a broken @inferred though.)
There was a problem hiding this comment.
Positional arguments are inferrable after JuliaLang/julia#35980 (Julia >= 1.6) because the field type of Some(::Type{T}) is now Type{T}. If you want to fix it in earlier Julia, I think we can put the specialization for Some(::Type) in Compat.jl.
I think the only remaining problem is when a type is specified through a keyword argument.
There was a problem hiding this comment.
It's great that this works for args on 1.6. I think it's not worth fixing it for older versions, but it would be nice to have a broken test that reminds us about it. Especially the kw case, which is still open.
There was a problem hiding this comment.
Okay. I added a broken test for keyword argumets in c63b9e7.
| @@ -22,6 +22,11 @@ using Test | |||
| @inferred a1(1.0) | |||
| end | |||
|
|
|||
| @testset "fix(::Type, ...)" begin | |||
| f = @inferred fix(CartesianIndex, nothing, Some(1)) | |||
There was a problem hiding this comment.
This @inferred passes even without this change. Is it worth clarifying what it's trying to test?
There was a problem hiding this comment.
I think its good to have this test. While this does not go wrong in master it is something that one can imagine going wrong. So this test makes future refactoring easier.
There was a problem hiding this comment.
I was wondering what could go wrong that would make this test fail. I agree the test should stay.
|
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 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 AnyTo 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 Anyjulia> @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: JuliaLang/julia#35980 (comment) |
|
Yeah I think you understood it correctly. |
|
@goretkin Yeah, I think your understanding is accurate. I think it'll be explained in the manual after JuliaLang/julia#36591 |
This PR fixes inference for, e.g.,
fix(CartesianIndex, nothing, Some(1))(2).