v0.6 precompile error: new UnionAll type breaks things

ghost commented 7 years ago

INFO: Precompiling module ColorTypes.
ERROR: LoadError: LoadError: MethodError: no method matching subtypes(::Type{ColorTypes.Color})
Closest candidates are:
  subtypes(::Module, ::DataType) at reflection.jl:316
  subtypes(::DataType) at reflection.jl:333
Stacktrace:
 [1] include_from_node1(::String) at ./loading.jl:532
 [2] include(::String) at ./sysimg.jl:14
 [3] include_from_node1(::String) at ./loading.jl:532
 [4] include(::String) at ./sysimg.jl:14
 [5] anonymous at ./<missing>:2
while loading /home/joe/.julia/v0.6/ColorTypes/src/types.jl, in expression starting on line 417
while loading /home/joe/.julia/v0.6/ColorTypes/src/ColorTypes.jl, in expression starting on line 56

can be fixed with:

const color3types = filter(x->(!x.abstract && length(fieldnames(x))>1), union(subtypes(Color{T,N}) where T where N, subtypes(AbstractRGB{T}) where T))

but then it fails again on

INFO: Precompiling module ColorTypes.
ERROR: LoadError: LoadError: type UnionAll has no field name
Stacktrace:
 [1] macro expansion; at /home/joe/.julia/v0.6/ColorTypes/src/types.jl:555 [inlined]
 [2] anonymous at ./<missing>:?
 [3] include_from_node1(::String) at ./loading.jl:532
 [4] include(::String) at ./sysimg.jl:14
 [5] include_from_node1(::String) at ./loading.jl:532
 [6] include(::String) at ./sysimg.jl:14
 [7] anonymous at ./<missing>:2
while loading /home/joe/.julia/v0.6/ColorTypes/src/types.jl, in expression starting on line 532
while loading /home/joe/.julia/v0.6/ColorTypes/src/ColorTypes.jl, in expression starting on line 56

ghost commented 7 years ago

can be fixed with

for (C, acol, cola) in [(DIN99d{T}, :ADIN99d, :DIN99dA),
                        (DIN99o{T}, :ADIN99o, :DIN99oA),
                        (DIN99{T}, :ADIN99, :DIN99A),
                        (HSI{T}, :AHSI, :HSIA),
                        (HSL{T}, :AHSL, :HSLA),
                        (HSV{T}, :AHSV, :HSVA),
                        (LCHab{T}, :ALCHab, :LCHabA),
                        (LCHuv{T}, :ALCHuv, :LCHuvA),
                        (LMS{T}, :ALMS, :LMSA),
                        (Lab{T}, :ALab, :LabA),
                        (Luv{T}, :ALuv, :LuvA),
                        (XYZ{T}, :AXYZ, :XYZA),
                        (YCbCr{T}, :AYCbCr, :YCbCrA),
                        (YIQ{T}, :AYIQ, :YIQA),
                        (xyY{T}, :AxyY, :xyYA),
                        (BGR{T}, :ABGR, :BGRA),
                        (RGB{T}, :ARGB, :RGBA),
                        (Gray{T}, :AGray, :GrayA)] where T

but then fails in color_type and base_colorant_type

ghost commented 7 years ago

Subtypes() should accept UnionAll, not sure about everything else :( https://github.com/JuliaLang/julia/pull/20084

JeffBezanson commented 7 years ago

subtypes(AbstractRGB{T}) where T

This should not work --- an array of types is not a valid type. I'll add an error for this.

timholy commented 7 years ago

ColorTypes is basically a poster child for the kind of package that could benefit from the type revisions (e.g., https://github.com/JuliaGraphics/ColorTypes.jl/blob/a0f98987261aa41bc188b5a7707e7acaa2f71da1/src/traits.jl#L177-L257 and https://github.com/JuliaGraphics/ColorTypes.jl/blob/master/src/conversions.jl). At some point it will be interesting to rewrite it for the new capabilities.

Keno commented 7 years ago

I have a work in progress for this package.

Keno commented 7 years ago

https://gist.github.com/Keno/4f963f43ec128b2e9aa6eb2598fbb7c0

Keno commented 7 years ago

The last status was that I hit https://github.com/JuliaLang/julia/pull/18457#issuecomment-272669414.

timholy commented 7 years ago

I also remember (I think) there being a number of places that could have benefited from triangular dispatch, which I think I worked around by expanding the number of arguments in several places. E.g.,

foo{C<:Colorant}(::Type{C}) = _foo(C, eltype(C))
# and now _foo can dispatch on both C and T where e.g. C = RGB{Float32}, T = Float32

tlnagy commented 7 years ago

Any progress on this? This breaks all dependent packages on 0.6 (including gadfly).

juliohm commented 7 years ago

This is also breaking ImageQuilting.jl on Julia v0.6.

SimonDanisch commented 7 years ago

this breaks basically everything :P

julia> Pkg.dependents("Colors")
50-element Array{AbstractString,1}:
 "Luxor"           
 "Images"          
 "ImageFiltering"  
 "VoronoiDelaunay" 
 "NamedColors"     
 "PlotUtils"       
 "VLFeat"          
 "Qwt"             
 "ProteinEnsembles"
 "Cairo"           
 ⋮                 
 "QuartzImageIO"   
 "GraphPlot"       
 "ImageView"       
 "Brim"            
 "ColorBrewer"     
 "Winston"         
 "Caesar"          
 "FaceDatasets"    
 "Bio"

Keno commented 7 years ago

I have a branch of julia/ColorTypes that passes here. However, I'll be traveling for the rest of the week, so I'm not gonna be able to clean this up. So here's the deal: I'll put up what I have in it's messy state (over the next hour or so), but with what I believe are all the tricky issues worked out and somebody can pick it up and finish it off. Otherwise, I'll get to it next week. I also have a PR for FixedPointNumbers, which this depends on. Same deal.

timholy commented 7 years ago

Sounds good to me. I can't promise to get to it very quickly, but I might, and of course there seem to be other volunteers in this thread who could tackle it.

Maybe easiest if you push as a branch rather than a gist?

Keno commented 7 years ago

Pushed as kf/06 here and as a PR on FixedPointNumbers. This also requires my kf/newreflection branch of Julia itself.

lobingera commented 7 years ago

@Keno Any idea what will happen with https://github.com/JuliaLang/julia/pull/20006? Simon is unfortunately right: ColorTypes.jl breaks an awful lot of testing on 0.6

timholy commented 7 years ago

Looks like it's actively being worked on. These type-system issues aren't simple. You could disable testing on nightly if the failures really bother you.

Keno commented 7 years ago

It's through review. I'll merge it as soon as it's green on CI. Looks like the latest set of changes broke sth.

timholy commented 7 years ago

That's awesome. Don't worry about the breakages, I can take a peek. Really appreciate your help on this.

Keno commented 7 years ago

Sorry, what I meant was that my latest changes on that branch broke something in base, which I'll have to investigate. I did already rebase the new ColorTypes on top of the changes in base (but haven't pushed that yet).

timholy commented 7 years ago

OK, just ping here when all is set (I am too swamped to follow what's happening with the type-system stuff).

SimonDanisch commented 7 years ago

just to better track the progress here, this is the PR we are waiting for: https://github.com/JuliaLang/julia/pull/20006

juliohm commented 7 years ago

It looks like the PR was merged, this means that the issue is solved in master?

tlnagy commented 7 years ago

i think we're still waiting for https://github.com/JuliaGraphics/ColorTypes.jl/pull/69 to merge

On Tue, Feb 7, 2017, at 20:00, Júlio Hoffimann wrote:

It looks like the PR was merged, this means that the issue is solved in master? — You are receiving this because you commented. Reply to this email directly, view it on GitHub[1], or mute the thread[2].

Links:

timholy commented 7 years ago

Closed by #68 and #69.

JuliaGraphics / ColorTypes.jl

v0.6 precompile error: new UnionAll type breaks things #65