Closed galenlynch closed 4 years ago
Merging #259 into master will not change coverage. The diff coverage is
100.00%
.
@@ Coverage Diff @@
## master #259 +/- ##
=======================================
Coverage 76.83% 76.83%
=======================================
Files 14 14
Lines 600 600
=======================================
Hits 461 461
Misses 139 139
Impacted Files | Coverage Δ | |
---|---|---|
src/avio.jl | 79.15% <100.00%> (ø) |
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 e69f3ff...5071d4f. Read the comment docs.
Thanks again. Very much appreciate your careful review of all of this 🙏🏻
Thanks for the wonderful package, the only reason I'm finding so many of these things is because I've been using your package so much!
To explain: on Julia 1.5,
julia> using MethodAnalysis
[ Info: Precompiling MethodAnalysis [85b6ec6f-f7df-4429-9514-a64bcd9ee824]
julia> for m in methods(eof)
_, argtypes = call_type(m.sig)
println(m, " returns ", Base.return_types(eof, argtypes))
end
eof(pipe::Base64.Base64DecodePipe) in Base64 at /home/tim/src/julia-1/usr/share/julia/stdlib/v1.5/Base64/src/decode.jl:126 returns Any[Any]
eof(io::Base.SecretBuffer) in Base at secretbuffer.jl:155 returns Any[Bool]
eof(f::Base.Filesystem.File) in Base.Filesystem at filesystem.jl:200 returns Any[Bool]
eof(s::Base.BufferStream) in Base at stream.jl:1287 returns Any[Bool]
eof(s::IOStream) in Base at iostream.jl:223 returns Any[Bool]
eof(::Base.DevNull) in Base at coreio.jl:22 returns Any[Bool]
eof(io::Base.AbstractPipe) in Base at io.jl:390 returns Any[Any]
eof(io::Base.GenericIOBuffer) in Base at iobuffer.jl:335 returns Any[Bool]
eof(s::Base.LibuvStream) in Base at stream.jl:46 returns Any[Bool, Union{Missing, Bool, Base.var"#64#65"{_A} where _A}]
eof(p::Pkg.TOML.Parser) in Pkg.TOML at /home/tim/src/julia-1/usr/share/julia/stdlib/v1.5/Pkg/ext/TOML/src/parser.jl:54 returns Any[Any]
so whether its inferrable depends on how specific the I
type is in AVInput
. Note also:
julia> subtypes(IO)
12-element Array{Any,1}:
Base.AbstractPipe
Base.DevNull
Base.Filesystem.AbstractFile
Base.GenericIOBuffer
Base.LibuvStream
Base.SecretBuffer
Base64.Base64DecodePipe
Base64.Base64EncodePipe
Core.CoreSTDERR
Core.CoreSTDOUT
IOStream
Mmap.Anonymous
julia> for T in subtypes(IO)
println("eof(::$T) returns ", Base.return_types(eof, Tuple{T}))
end
eof(::Base.AbstractPipe) returns Any[Any]
eof(::Base.DevNull) returns Any[Bool]
eof(::Base.Filesystem.AbstractFile) returns Any[Bool]
eof(::Base.GenericIOBuffer) returns Any[Bool]
eof(::Base.LibuvStream) returns Any[Bool, Union{Missing, Bool, Base.var"#64#65"{_A} where _A}]
eof(::Base.SecretBuffer) returns Any[Bool]
eof(::Base64.Base64DecodePipe) returns Any[Any]
eof(::Base64.Base64EncodePipe) returns Any[]
eof(::Core.CoreSTDERR) returns Any[]
eof(::Core.CoreSTDOUT) returns Any[]
eof(::IOStream) returns Any[Bool]
eof(::Mmap.Anonymous) returns Any[]
julia> Base.return_types(eof, Tuple{IO})
9-element Array{Any,1}:
Any
Bool
Bool
Bool
Bool
Bool
Any
Bool
Union{Missing, Bool, Base.var"#64#65"{_A} where _A}
Julia 1.6 has been vastly improved as part of the invalidation work:
julia> Base.return_types(eof, Tuple{IO})
9-element Vector{Any}:
Bool
Bool
Bool
Bool
Bool
Bool
Bool
Bool
Bool
Nevertheless if I
is ever abstract, you still might need this simply because the number of applicable methods is higher than 3, the limit for which Julia will infer all methods and see if they all return the same type.
Consequently, when testing things like this it would be good to specify what specifc AVInput
type inference was failing on. For many options it shouldn't have failed.
Very interesting, thanks for the explanation @timholy, as well as for your many packages that I use daily. I didn't know about MethodAnalysis, I will have to check it out.
Do you think we should not use the type assertion, then? I assumed that in practice VideoIO
objects would wrap an IO
type that would have a Bool
return type, though perhaps that is not a justified assumption.
I think the type assertion is basically harmless, and very helpful for when inference fails. I'd recommend keeping it.
I was basically saying that when you make a fix like this it might be useful to report the specific circumstances in which you found it necessary, as there may be something deeper going on that would suggest an alternative fix.
Currently, the Julia compiler cannot infer the return type of
eof
. I am not 100% sure why it exhibits type instability, but we can at least assert that the return type will be aBool
, and limit the spread of that type instability.This type assertion will hopefully be removed at some point when the code is made type stable.