ImageCore warning needs fixing (by using XRGB and RGBX)

[ViralBShah]

We have this warning now which would be nice to fix.

┌ ImageMagick
│  WARNING: using deprecated binding Colors.RGB1 in ImageCore.
│  , use XRGB instead.
│  WARNING: using deprecated binding Colors.RGB4 in ImageCore.
│  , use RGBX instead.
└  

[dgleich]

I tracked this down to these lines in libmagicwand.jl

for AC in vcat(subtypes(AlphaColor), subtypes(ColorAlpha))
    Cstr = ColorTypes.colorant_string(color_type(AC))
    if haskey(IMColordict, Cstr)
        IMColordict[ColorTypes.colorant_string(AC)] = IMColordict[Cstr]
    end
end

It's the subtypes call that's throwing this deprecation error. That's in InteractiveUtils.jl

If I replace this by just listing out the types...

const colortypeslist = [
    ABGR
    ADIN99
    ADIN99d
    ADIN99o
    AGray
    AGray32
    AHSI
    AHSL
    AHSV
    ALCHab
    ALCHuv
    ALMS
    ALab
    ALuv
    AOklab
    AOklch
    ARGB
    ARGB32
    AXYZ
    AYCbCr
    AYIQ
    AxyY
    BGRA
 DIN99A
 DIN99dA
 DIN99oA
 GrayA
 HSIA
 HSLA
 HSVA
 LCHabA
 LCHuvA
 LMSA
 LabA
 LuvA
 OklabA
 OklchA
 RGBA
 XYZA
 YCbCrA
 YIQA
 xyYA
]
for AC in colortypeslist
Cstr = ColorTypes.colorant_string(color_type(AC))
    if haskey(IMColordict, Cstr)
        IMColordict[ColorTypes.colorant_string(AC)] = IMColordict[Cstr]
    end
end

Then it passes tests and doesn't throw this warning.

[ViralBShah]

Maybe @timholy or @johnnychen94 may need to chime in here.

[dgleich]

Here's the issue...

function mysubtypes(x::Type)
    mods = Base.loaded_modules_array()
    xt = Base.unwrap_unionall(x)
    if !isabstracttype(x) || !isa(xt, DataType)
        # Fast path
        return Type[]
    end
    sts = Vector{Any}()
    while !isempty(mods)
        m = pop!(mods)
        xt = xt::DataType
        for s in names(m, all = true)
            if m == ImageCore 
                println(stdout, "Checking $s in $m")
                if s == :RGB1 || s == :RGB4 
                    @show Base.isdeprecated(m, s)
                    @show isdefined(m, s)
                end
            end 

            if isdefined(m, s) && !Base.isdeprecated(m, s)
                t = getfield(m, s)
                dt = isa(t, UnionAll) ? Base.unwrap_unionall(t) : t
                if isa(dt, DataType)
                    if dt.name.name === s && dt.name.module == m && supertype(dt).name == xt.name
                        ti = typeintersect(t, x)
                        ti != Base.Bottom && push!(sts, ti)
                    end
                elseif isa(t, Module) && nameof(t) === s && parentmodule(t) === m && t !== m
                    t === Base || push!(mods, t) # exclude Base, since it also parented by Main
                end
            end
        end
    end
    return permute!(sts, sortperm(map(string, sts)))
end

This shows...

Checking RGArray in ImageCore
Checking RGB in ImageCore
Checking RGB1 in ImageCore
Base.isdeprecated(m, s) = false
WARNING: using deprecated binding Colors.RGB1 in ImageCore.
, use XRGB instead.
isdefined(m, s) = true
Checking RGB24 in ImageCore
Checking RGB4 in ImageCore
Base.isdeprecated(m, s) = false
WARNING: using deprecated binding Colors.RGB4 in ImageCore.
, use RGBX instead.
isdefined(m, s) = true
Checking RGBA in ImageCore
Checking RGBRGB in ImageCore

[dgleich]

So it seems like subtypes should check Base.isdeprecated before isdefined, but this might cause all sorts of other issues :)

[ViralBShah]

Is removing support for old versions of ImageCore sufficient to fix this?

[dgleich]

I'm getting this on the latest ImageCore (v0.10.2) with Julia 1.11... I think the only way to fix this is to avoid the subtypes call (i.e. hardcode the list of colors...) or adjust the subtypes function.

[ViralBShah]

Ah - so maybe leave this as is here and we can see if ImageCore can fix the problem? Running Pkg.test("ImageCore") (JuliaImages/ImageCore.jl#202):

     Testing Running tests...
WARNING: using deprecated binding Colors.RGB1 in ImageCore.
, use XRGB instead.
WARNING: ImageCore.RGB1 is deprecated, use ColorTypes.XRGB{T} where T<:Union{AbstractFloat, FixedPointNumbers.FixedPoint{T, f} where f where T<:Integer} instead.
  likely near none:8
WARNING: using deprecated binding Colors.RGB4 in ImageCore.
, use RGBX instead.
WARNING: ImageCore.RGB4 is deprecated, use ColorTypes.RGBX{T} where T<:Union{AbstractFloat, FixedPointNumbers.FixedPoint{T, f} where f where T<:Integer} instead.
  likely near none:8
WARNING: using deprecated binding Colors.RGB1 in ImageCore.
, use XRGB instead.
WARNING: ImageCore.RGB1 is deprecated, use ColorTypes.XRGB{T} where T<:Union{AbstractFloat, FixedPointNumbers.FixedPoint{T, f} where f where T<:Integer} instead.
  likely near /Users/viral/.julia/packages/ImageCore/DpvRI/test/runtests.jl:7
WARNING: using deprecated binding Colors.RGB4 in ImageCore.
, use RGBX instead.
WARNING: ImageCore.RGB4 is deprecated, use ColorTypes.RGBX{T} where T<:Union{AbstractFloat, FixedPointNumbers.FixedPoint{T, f} where f where T<:Integer} instead.
  likely near /Users/viral/.julia/packages/ImageCore/DpvRI/test/runtests.jl:7

[dgleich]

That will still cause this issue though. While I won't claim to get this exactly right, I believe...

Code in ColorTypes.jl

Base.@deprecate_binding RGB1 XRGB
Base.@deprecate_binding RGB4 RGBX

creates symbols/names for the deprecated bindings with an alias to their new type.

Whenever subtypes (or any other method) searches through the entire name space and tries isdefined on these symbols, that's when the deprecation warning gets thrown.

So I actually think the right fix is to switch the order of checking things in subtypes. (You can't be deprecated unless you are defined, so I conjecture it's fine to check them in the other order...)

So the ways to fix this are:

• remove the deprecated bindings in ColorTypes (these are years old now, so maybe that's the fix?)
• hard code the list of types (to avoid subtypes)
• switch the order of checks in subtypes

[ViralBShah]

So, if we remove the deprecated bindings, we then need to make sure the key packages work fine (and remove old compat bounds). That seems reasonable to do.

[dgleich]

Okay, there may be another culprit here. I was testing my hypothesis to try and isolate that this was really the issue. And I couldn't reproduce it ... until I threw Reexport into the mix.

So I think this is related -- simonster/Reexport.jl#42

After playing around with this a bunch, I think Reexporting a package with a deprecated binding is causing the issue here.

Here is package 1:

module ImageMagick

module InternalModule
    struct MyType
        x::Int
    end

    Base.@deprecate_binding MyOldType MyType

    export MyType 
end 

using .InternalModule: MyType

export MyType

end # module

and test case:

using ImageMagick
using InteractiveUtils: subtypes 
types = subtypes(Integer)
println.(types);

which outputs:

     Testing Running tests...
Bool
Signed
Unsigned
     Testing ImageMagick tests passed 

Now here's package 2

module ImageMagick

module InternalModule
    struct MyType
        x::Int
    end

    Base.@deprecate_binding MyOldType MyType

    export MyType 
end 

using Reexport 
@reexport using .InternalModule

export MyType

end # module

When I run the same test cases on that one:

     Testing Running tests...
WARNING: using deprecated binding InternalModule.MyOldType in ImageMagick.
, use MyType instead.
Bool
Signed
Unsigned
     Testing ImageMagick tests passed 

[dgleich]

Okay, here's the difference between the two scenarios, where I use my chatty-subtypes code above to look at what is trigging the deprecation...

Module 1: The one that doesn't use Reexport

Checking #eval in ImageMagick
Checking #include in ImageMagick
Checking ImageMagick in ImageMagick
Checking InternalModule in ImageMagick
Checking MyType in ImageMagick
Checking eval in ImageMagick
Checking include in ImageMagick
Bool
Signed
Unsigned

Module 2 with Reexport

     Testing Running tests...
Checking #eval in ImageMagick
Checking #include in ImageMagick
Checking ImageMagick in ImageMagick
Checking InternalModule in ImageMagick
Checking MyOldType in ImageMagick
Base.isdeprecated(m, s) = false
WARNING: using deprecated binding InternalModule.MyOldType in ImageMagick.
, use MyType instead.
isdefined(m, s) = true
Checking MyType in ImageMagick
Checking eval in ImageMagick
Checking include in ImageMagick
Bool
Signed
Unsigned

So the difference is that using Reexport will export the binding for the type in the deprecated binding, even though it isn't exported by the parent module.

[dgleich]

@ararslan Since you are involved with Reexport and the issue above, can you see if anything jumps out at you? I'm trying to see find any hints.

The short scenario is:

I would have expected names(ImageMagick) to not include MyOldType since it isn't exported by InternalModule. But it does when using Reexport. This holds even when I replace using .InternalModule: MyType with using .InternalModule

This occurs because Base.isexported(InternalModule, MyOldType) is true even though this isn't exported.

[dgleich]

In case you want to see the same thing without Reexport, the Reexport code for this case expands to:

using .InternalModule
export MyType, MyOldType 

This doesn't trigger a deprecation warning because you are apparently allowed to export non-defined symbols.

But this seems to create the MyOldType symbol in the ImageMagick module whereas just

using .InternalModule
export MyType

does not. (Symbols/names must be lazily created as they are used?)

[dgleich]

More testing... the MyOldType isn't even marked as deprecated in the ImageMagick space (in the examples above...)

Base.isdeprecated(ImageMagick, :MyOldType) = false
Base.isdeprecated(ImageMagick.InternalModule, :MyOldType) = true

[dgleich]

After carefully reading the discussion in

simonster/Reexport.jl#42

I think the best fix is actually to do this in subtypes()

             ... 
            if Base.isbindingresolved(m, s) && !Base.isdeprecated(m, s) && isdefined(m, s)
                t = getfield(m, s)
             ...

Everything else leaves the issue there and just patches around it.

[ararslan]

Great sleuthing! :) I agree, that seems like a good solution.

[timholy]

Great work @dgleich.

We should just remove those deprecated bindings, but the last time I pushed a breaking change to Color*.jl it was 3 days of tedious work to get JuliaImages updated with new releases (bump version, wait for CI, merge, register, and then go on to the next package in the dependency chains). I have plans to drastically reduce this by switching to a monorepo, but will need to find a block of dedicated time to do that.

[ViralBShah]

I'm happy to help out updating all the packages. Let's get it done! And perhaps we can even see if others can help.

[ViralBShah]

I think we should go ahead and register the new version of ImageMagick.jl first and then apply the deprecation fix discussed here and go through the ecosystem.

[dgleich]

Sounds good to register a new package, but wait for one more commit.

1. I think we should add one more commit with a version of subtypes that just fixes this (see my latest pull request...)

2. Then go through the exercise of actually removing RGB1/RGB4 & updating major versions.

3. (and also...) Contribute a patch for subtypes to InteractiveUtils.jl

[ViralBShah]

cc @aviatesk