re-exported deprecated symbols not marked deprecated

[t-bltg]

module Foo
  module Types
    export OldType, NewType
    const OldType = Union{Int, Nothing}
    const NewType = Union{Int, Missing}
  end
  using Reexport
  @reexport using .Types
  Base.@deprecate_binding OldType NewType
end
import .Foo

julia> Base.isdeprecated(Main, :OldType)
false  # expected Main.OldType to be also deprecated, since `Main.Foo.OldType` is
julia> Base.isdeprecated(Foo, :OldType)
true

Related to JuliaTesting/Aqua.jl#89, since this is the root cause of the UnicodePlots test suite warnings thrown when using Aqua.

Running the UnicodePlots test suite:

pkg> test UnicodePlots
[...]
tst_imageplot.jl |    2      2
WARNING: importing deprecated binding Colors.RGB1 into ImageCore.
WARNING: importing deprecated binding Colors.RGB1 into ImageBase.
WARNING: Colors.RGB1 is deprecated, use XRGB instead.
  likely near ~/.julia/packages/UnicodePlots/vtIuF/test/tst_quality.jl:1
WARNING: importing deprecated binding Colors.RGB4 into ImageCore.
WARNING: importing deprecated binding Colors.RGB4 into ImageBase.
WARNING: Colors.RGB4 is deprecated, use RGBX instead.
  likely near ~/.julia/packages/UnicodePlots/vtIuF/test/tst_quality.jl:1
WARNING: importing deprecated binding ImageCore.permuteddimsview into ImageBase.
WARNING: ImageCore.permuteddimsview is deprecated, use PermutedDimsArray instead.
  likely near ~/.julia/packages/UnicodePlots/vtIuF/test/tst_quality.jl:1
Test Summary:  | Pass  Total
tst_quality.jl |    7      7
[...]

These warnings are triggered in Aqua.jl/src/unbound_args.jl, which maps to the failing line in julia/stdlib/Test/src/Test.jl, because the re-exported symbol are not marked as deprecated in the @reexport calling module.

I tried patching Reexport to call Base.deprecate(mod, sym) in the @reexport scope but it current fails these @deprecate_binding lines with ERROR: LoadError: cannot assign a value to variable ColorTypes.RGB1 from module Colors (so this would be breaking I guess).

diff --git a/src/Reexport.jl b/src/Reexport.jl
index 58d17d8..b5d18f4 100644
--- a/src/Reexport.jl
+++ b/src/Reexport.jl
@@ -37,15 +37,27 @@ function reexport(m::Module, ex::Expr)
         modules = Any[e.args[end] for e in ex.args]
     end
 
-    names = GlobalRef(@__MODULE__, :exported_names)
+    exp_names = GlobalRef(@__MODULE__, :exported_names)
+    dep_names = GlobalRef(@__MODULE__, :deprecated_names)
+
     out = Expr(:toplevel, ex)
     for mod in modules
-        push!(out.args, :($eval($m, Expr(:export, $names($mod)...))))
+        push!(
+            out.args,
+            quote
+                $eval($m, Expr(:export, $exp_names($mod)...))
+                foreach(n -> Base.deprecate($m, n), $dep_names($mod))
+            end
+        )
     end
     return out
 end
 
-exported_names(m::Module) = filter!(x -> Base.isexported(m, x), names(m; all=true, imported=true))
+exported_names(m::Module) =
+    filter!(x -> Base.isexported(m, x), names(m; all=true, imported=true))
+
+deprecated_names(m::Module) =
+    filter!(x -> Base.isdeprecated(m, x), names(m; all=true, imported=true))
 
 export @reexport

Related: #28.

[ararslan]

There are a couple of things going on here.

First, reexported deprecated symbols actually are marked deprecated. In your example,

julia> Base.isdeprecated(Main, :OldType)
false  # expected Main.OldType to be also deprecated, since `Main.Foo.OldType` is

note that OldType is not defined in Main since you did import .Foo rather than using .Foo, but you would still get false with using because the OldType binding isn't yet resolved in Main. If you resolve it, you can see that it does have the deprecated flag set:

julia> module Foo
           module Types
               export OldType, NewType
               const OldType = Union{Int,Nothing}
               const NewType = Union{Int,Missing}
           end
           using Reexport
           @reexport using .Types
           Base.@deprecate_binding OldType NewType
       end
Main.Foo

julia> using .Foo

julia> Base.isdeprecated(Main, :OldType)
false

julia> OldType
Union{Missing, Int64}

julia> Base.isdeprecated(Main, :OldType)
true

Now, the issue with the proposed Base.deprecate solution is that Base.deprecate resolves bindings. In the Colors.jl example, ColorTypes.jl includes the exact same @deprecate_binding calls as Colors.jl does (which is kind of strange and it took me a bit to understand why that actually works as expected). Inside the Colors module, we have:

@reexport using ColorTypes
Base.@deprecate_binding RGB1 XRGB

Expanding the @deprecate_binding, we get:

@reexport using ColorTypes
export RGB1
const _dep_message_RGB1 = ", use XRGB instead."
const RGB1 = XRGB
Base.deprecate(Colors, :RGB1)

The bindings RGB1 and XRGB are brought into scope from ColorTypes unresolved on the first line. On the fourth line, XRGB is resolved as ColorTypes.XRGB and RGB1 is assigned. That's the crucial part here: since RGB1 hadn't already been resolved, this is a new binding that shadows the corresponding one from ColorTypes. Colors.RGB1 === ColorTypes.XRGB by definition but also ColorTypes.RGB1 === ColorTypes.XRGB, so Colors.RGB1 === ColorTypes.RGB1, which means everything works just fine. But if the @reexport on the first line were to expand to include a call to deprecate, RGB1 would be resolved by the time we go to assign it, resulting in the error you observed.

I don't think the cited warnings in the UnicodePlots' tests are related to Reexport.

[t-bltg]

Thanks for the detailed analysis and pointing out the resolution of bindings (a low level mechanism which was unknown to me: TIL).

This explains why getproperty in JuliaTesting/Aqua.jl#89 trigger deprecation warnings, since it triggers binding resolution.

I'm a bit puzzled here (these warnings also occur in Plots ci, basically packages using Aqua and having Colors loaded, so not only UnicodePlots).

Removing the

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

lines of Colors.jl now triggers the warning from ColorTypes instead (this was expected):

== start: testing with 24bit colormode ==

WARNING: importing deprecated binding ColorTypes.RGB1 into Colors.
WARNING: importing deprecated binding ColorTypes.RGB1 into ImageCore.
WARNING: importing deprecated binding ColorTypes.RGB1 into ImageBase.
WARNING: ColorTypes.RGB1 is deprecated, use XRGB instead.
  likely near [...]/UnicodePlots.jl/test/tst_quality.jl:1
WARNING: importing deprecated binding ColorTypes.RGB4 into Colors.
WARNING: importing deprecated binding ColorTypes.RGB4 into ImageCore.
WARNING: importing deprecated binding ColorTypes.RGB4 into ImageBase.
WARNING: ColorTypes.RGB4 is deprecated, use RGBX instead.
  likely near [...]/UnicodePlots.jl/test/tst_quality.jl:1
WARNING: importing deprecated binding ImageCore.permuteddimsview into ImageBase.
WARNING: ImageCore.permuteddimsview is deprecated, use PermutedDimsArray instead.
  likely near [...]/UnicodePlots.jl/test/tst_quality.jl:1
WARNING: importing deprecated binding ColorTypes.RGB1 into Colors.
WARNING: ColorTypes.RGB1 is deprecated, use XRGB instead.
  likely near none:8
WARNING: importing deprecated binding ColorTypes.RGB4 into Colors.
WARNING: ColorTypes.RGB4 is deprecated, use RGBX instead.
  likely near none:8
Test Summary:  | Pass  Total   Time
tst_quality.jl |    7      7  18.2s

Then, using the proposed Base.deprecate fix in conjunction with removing the @deprecate_binding for Colors.jl does remove the warnings triggered by Aqua:

== start: testing with 24bit colormode ==

Test Summary:  | Pass  Total   Time
tst_quality.jl |    7      7  18.4s

This tends to indicate that there is a relation to Reexport, although admittedly a better solution might exist ?

[ararslan]

I still think Reexport might be a red herring here, especially given

basically packages using Aqua and having Colors loaded, so not only UnicodePlots

It sounds to me like the deprecation warnings are occurring because Aqua is resolving the deprecated bindings. I'm not familiar with Aqua so perhaps that's incorrect but I'm not seeing a clear connection to Reexport.

[t-bltg]

It sounds to me like the deprecation warnings are occurring because Aqua is resolving the deprecated bindings.

That is exactly what is described in the mentioned Aqua PR JuliaTesting/Aqua.jl#89 (addition of !Base.isdeprecated(x, n)), but it only fixes e.g. GeometryBasics tests, not the UnicodePlots ones, hence the opening of this issue.

[ararslan]

There may be other places where Aqua is resolving bindings, e.g. https://github.com/JuliaTesting/Aqua.jl/blob/master/src/exports.jl#L20. Perhaps that also needs a isdeprecated guard?

[t-bltg]

Unfortunately, it does not help.

Only getproperty seems to trigger deprecation warnings:

$ julia --depwarn=yes
julia> using Colors
julia> isdefined(Colors, :RGB1)
true
julia> getproperty(Colors, :RGB1)
WARNING: Colors.RGB1 is deprecated, use XRGB instead.
  likely near REPL[6]:1
XRGB

And I've identified only one single location in Aqua (only test_unbound_args throws these warnings).

[ararslan]

And I've identified only one single location in Aqua

Sorry, the location of what? If you mean a call to getproperty on a module, it seems this also does it: https://github.com/JuliaTesting/Aqua.jl/blob/master/src/ambiguities.jl#L67-L71.

[t-bltg]

I should've said a single location for the unbounds_args tests.

Yeah I've seen that one before and added an @assert false before the foldl to ensure that it doesn't get there ;)
This code path is not concerned ...
Anyway, surrounding Aqua tests with prints everywhere, it is still this block that is concerned https://github.com/JuliaTesting/Aqua.jl/blob/7da693b36868d1b36337deec3959a45d65e42b6d/src/unbound_args.jl#L13-L19, with the explanation in #42 (comment) still valid.

[ararslan]

So actually, TIL that isdefined also resolves bindings. (Though in retrospect I guess that should have been obvious; how else can you tell whether a binding is defined other than trying to resolve it.) Something you could try is to amend your Aqua PR to do

--- a/src/exports.jl
+++ b/src/exports.jl
@@ -1,7 +1,8 @@
 function walkmodules(f, x::Module)
     f(x)
-    for n in names(x; all=true)
-        if isdefined(x, n)
+    for n in names(x; all=true, imported=true)
+        # `isdefined` and `getproperty` can trigger deprecation warnings
+        if Base.isbindingresolved(x, n) && !Base.isdeprecated(x, n) && isdefined(x, n)
             y = getproperty(x, n)
             if y isa Module && y !== x && parentmodule(y) === x
                 walkmodules(f, y)

This ensures that the binding is only accessed if it's already been resolved, which AFAICT is the desired behavior here.

[t-bltg]

Thanks for sharing.
I've added the suggestion to the Aqua PR.
Unfortunately, deprecation warnings still appear in the UnicodePlots quality test.

[ararslan]

Hmm. I'm still not sure what exactly is triggering these warnings then... I wish warnings could produce stack traces the same way errors do.

[dgleich]

Just a quick nudge that there's a note on where this is coming from in the linked thread that'd be useful to have some additional input on.