Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redefining show for Type{Foo{T}} breaks display of any MethodError #13306

Closed
tbreloff opened this issue Sep 24, 2015 · 26 comments
Closed

Redefining show for Type{Foo{T}} breaks display of any MethodError #13306

tbreloff opened this issue Sep 24, 2015 · 26 comments
Labels
bug Indicates an unexpected problem or unintended behavior types and dispatch Types, subtyping and method dispatch

Comments

@tbreloff
Copy link

I think the issue is on this line

as it doesn't actually show any useful information.

@ihnorton
Copy link
Member

@tbreloff: please provide a demonstrative code sample.

@ihnorton
Copy link
Member

(if you are hitting that line: Julia tried to print the backtrace, but failed... there is nothing else to be printed at that point)

@tbreloff
Copy link
Author

So then the real problem is that printing the backtrace failed, in which case the bug is somewhere else?

Here's the REPL output when try to show the object (which, in this case, is probably failing somewhere within Gadfly's draw method). As you can see it never shows a backtrace, so I'm not sure exactly where the problem is. Do you have an idea of where the backtrace might fail? I can try to debug myself.

julia> p
Error showing value of type Plots.Plot{Plots.ImmersePackage}:
ERROR: MethodError: `convert` has no method matching convert(::Type{ColorTypes.RGBA{Float32}}, ::Array{ColorTypes.RGB{T<:Union{AbstractFloat,FixedPointNumbers.FixedPoint}},1})
This may have arisen from a call to the constructor ColorTypes.RGBA{Float32}(...),
since type constructors fall back to convert methods.SYSTEM: show(lasterr) caused an error

@ihnorton
Copy link
Member

Do you have an idea of where the backtrace might fail?

From that error message, it is probably in a show method. You could try printing each of the fields of that type until you find which one can't be shown. Then look at the overloads using @which, etc.

@tbreloff
Copy link
Author

I'm going to close this until I can pinpoint if the issue lies within base julia. Thanks.

@yuyichao
Copy link
Contributor

It would be helpful if you could at least provide a complete procedure to reproduce the issue.

@tbreloff
Copy link
Author

So I peppered the Gadfly codebase with println's and show's in order to pinpoint where the error gets thrown, and now the backtraces are printing (within one of Gadfly's render methods). I wonder if there's some weird thing with STDERR flushing, and that maybe there's multiple writers hitting a race condition, and writing output removes the race? I'm not really sure how to investigate further.

@timholy
Copy link
Member

timholy commented Sep 24, 2015

That SYSTEM: show(lasterr) caused an error is worrisome. I've seen that one too but never pinned down something reproducible, except to say that I only see it after using Gtk. (This could be a red herring; it could be the fault of some other package, I just remember that Gtk was loaded.)

@tbreloff, I'll echo the requests of others: a full reproducible test case might help find it, even if it involves lots of packages.

@tbreloff
Copy link
Author

Yes it could be related to Gtk! I can't remember if I have only had the
problems after loading Immerse... It's possible the problem went away
because I restarted and switched to Gadfly only. I'll investigate when I
have time.

On Thursday, September 24, 2015, Tim Holy [email protected] wrote:

That SYSTEM: show(lasterr) caused an error is worrisome. I've seen that
one too but never pinned down something reproducible, except to say that I
only see it after using Gtk. (This could be a red herring; It could be
the fault of some other package, I just remember that Gtk was loaded.)

@tbreloff https://github.com/tbreloff, I'll echo the requests of
others: a full reproducible test case might help find it, even if it
involves lots of packages.


Reply to this email directly or view it on GitHub
#13306 (comment).

@tbreloff
Copy link
Author

It seems like the backtraces are not shown when Immerse (and hence Gtk) are loaded. The first pass will import Gadfly, Compose, and the second pass will import Immerse, Gadfly, Compose, Gtk. If I can figure out more detailed info I'll post it.

_
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.4.0-rc1+78 (2015-09-17 01:30 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 1af871b* (7 days old release-0.4)
|__/                   |  x86_64-redhat-linux

julia> using Plots; gadfly!()
[Plots.jl] Default backend: immerse
[Plots.jl] Switched to backend: gadfly
Plots.GadflyPackage()

julia> p = plot(rand(10), c=Any[distinguishable_colors(10)])
[Plots.jl] Initializing backend: gadfly
INFO: Recompiling stale cache file /home/tom/.julia/lib/v0.4/Gadfly.ji for module Gadfly.
INFO: Recompiling stale cache file /home/tom/.julia/lib/v0.4/Compose.ji for module Compose.
WARNING: `require` is deprecated, use `using` or `import` instead
 in depwarn at deprecated.jl:73
 [inlined code] from deprecated.jl:693
 in require at no file:0
 in isinstalled at /home/tom/.julia/v0.4/Compose/src/Compose.jl:36
 in include at ./boot.jl:261
 in include_from_node1 at ./loading.jl:304
 [inlined code] from none:2
 in anonymous at no file:0
 in process_options at ./client.jl:284
 in _start at ./client.jl:411
while loading /home/tom/.julia/v0.4/Compose/src/Compose.jl, in expression starting on line 155
WARNING: `require` is deprecated, use `using` or `import` instead
 in depwarn at deprecated.jl:73
 [inlined code] from deprecated.jl:693
 in require at no file:0
 in isinstalled at /home/tom/.julia/v0.4/Compose/src/Compose.jl:36
 in include at ./boot.jl:261
 in include_from_node1 at ./loading.jl:304
 [inlined code] from none:2
 in anonymous at no file:0
 in process_options at ./client.jl:284
 in _start at ./client.jl:411
while loading /home/tom/.julia/v0.4/Gadfly/src/Gadfly.jl, in expression starting on line 961
WARNING: Base.None is deprecated, use Union{} instead.
in aesthetics_type at /home/tom/.julia/v0.4/Gadfly/src/coord.jl:112
WARNING: Base.FloatingPoint is deprecated, use AbstractFloat instead.
in concrete_minimum at /home/tom/.julia/v0.4/Showoff/src/Showoff.jl:50
WARNING: Base.FloatingPoint is deprecated, use AbstractFloat instead.
in concrete_minimum at /home/tom/.julia/v0.4/Showoff/src/Showoff.jl:50
WARNING: Base.FloatingPoint is deprecated, use AbstractFloat instead.
in concrete_maximum at /home/tom/.julia/v0.4/Showoff/src/Showoff.jl:72
WARNING: Base.FloatingPoint is deprecated, use AbstractFloat instead.
in concrete_maximum at /home/tom/.julia/v0.4/Showoff/src/Showoff.jl:72
WARNING: Base.String is deprecated, use AbstractString instead.
in showoff at /home/tom/.julia/v0.4/Showoff/src/Showoff.jl:112
WARNING: Base.String is deprecated, use AbstractString instead.
in showoff at /home/tom/.julia/v0.4/Showoff/src/Showoff.jl:112
WARNING: Base.String is deprecated, use AbstractString instead.
in showoff at /home/tom/.julia/v0.4/Showoff/src/Showoff.jl:112
WARNING: Base.String is deprecated, use AbstractString instead.
in showoff at /home/tom/.julia/v0.4/Showoff/src/Showoff.jl:280
WARNING: Base.String is deprecated, use AbstractString instead.
in showoff at /home/tom/.julia/v0.4/Showoff/src/Showoff.jl:36
here: text/html
about to try_display 1
WARNING: Base.None is deprecated, use Union{} instead.
in aesthetics_type at /home/tom/.julia/v0.4/Gadfly/src/coord.jl:112
CT = ColorTypes.RGBA{Float32}
Error showing value of type Plots.Plot{Plots.GadflyPackage}:
ERROR: MethodError: `append!` has no method matching append!(::Array{ColorTypes.RGBA{Float32},1}, ::ColorTypes.RGBA{Float32})
Closest candidates are:
  append!{T}(::Array{T,1}, ::AbstractArray{T,1})
 in render at /home/tom/.julia/v0.4/Gadfly/src/geom/line.jl:196
 in render at /home/tom/.julia/v0.4/Gadfly/src/geometry.jl:47
 in render_prepared at /home/tom/.julia/v0.4/Gadfly/src/Gadfly.jl:863
 in render at /home/tom/.julia/v0.4/Gadfly/src/Gadfly.jl:802
 in display at /home/tom/.julia/v0.4/Gadfly/src/Gadfly.jl:1061
 [inlined code] from multimedia.jl:151
 in display at /home/tom/.julia/v0.4/Gadfly/src/Gadfly.jl:1002
 in display at /home/tom/.julia/v0.4/Plots/src/Plots.jl:129
 in display at REPL.jl:118
 [inlined code] from multimedia.jl:151
 in display at multimedia.jl:162
 in print_response at REPL.jl:135
 in print_response at REPL.jl:122
 in anonymous at REPL.jl:627
 in run_interface at ./LineEdit.jl:1610
 in run_frontend at ./REPL.jl:866
 in run_repl at ./REPL.jl:170
 in _start at ./client.jl:453

julia> 
[tom@tomoffice ~]$ julia
               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "?help" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.4.0-rc1+78 (2015-09-17 01:30 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit 1af871b* (7 days old release-0.4)
|__/                   |  x86_64-redhat-linux

julia> using Plots; immerse!()
[Plots.jl] Default backend: immerse
[Plots.jl] Switched to backend: immerse
Plots.ImmersePackage()

julia> p = plot(rand(10), c=Any[distinguishable_colors(10)])
[Plots.jl] Initializing backend: immerse
INFO: Recompiling stale cache file /home/tom/.julia/lib/v0.4/Immerse.ji for module Immerse.
WARNING: Method definition draw(Compose.Backend, Compose.Context) in module Compose at /home/tom/.julia/v0.4/Compose/src/container.jl:404 overwritten in module Immerse at /home/tom/.julia/v0.4/Immerse/src/compose.jl:33.
WARNING: Method definition display(Base.REPL.REPLDisplay, Base.Multimedia.MIME{:text/html}, Gadfly.Plot) in module Gadfly at /home/tom/.julia/v0.4/Gadfly/src/Gadfly.jl:1057 overwritten in module Immerse at /home/tom/.julia/v0.4/Immerse/src/display_gadfly.jl:144.
WARNING: Method definition display(Base.REPL.REPLDisplay{R<:Base.REPL.AbstractREPL}, Base.Multimedia.MIME{:text/html}, Gadfly.Plot) in module Gadfly at /home/tom/.julia/v0.4/Gadfly/src/Gadfly.jl:1057 overwritten in module Immerse at /home/tom/.julia/v0.4/Immerse/src/display_gadfly.jl:144.
WARNING: Method definition draw(Compose.Backend, Compose.Context) in module Compose at /home/tom/.julia/v0.4/Compose/src/container.jl:404 overwritten in module Immerse at /home/tom/.julia/v0.4/Immerse/src/compose.jl:33.
about to display
here: text/html
about to try_display 1
here
about to try_display 2
WARNING: Base.None is deprecated, use Union{} instead.
in aesthetics_type at /home/tom/.julia/v0.4/Gadfly/src/coord.jl:112
CT = ColorTypes.RGBA{Float32}
Error showing value of type Plots.Plot{Plots.ImmersePackage}:
ERROR: MethodError: `append!` has no method matching append!(::Array{ColorTypes.RGBA{Float32},1}, ::ColorTypes.RGBA{Float32})
Closest candidates are:
  append!{T}(::Array{T,1}, ::AbstractArray{T,1})
  append!(::Gtk.GtkContainer, ::Any)

julia> 

@timholy
Copy link
Member

timholy commented Nov 5, 2015

I hit this myself, and for @tbreloff's benefit I'll explain my debugging procedure. A couple of minutes of playing around led me to realize that

julia> using Gtk

julia> 5+"hello"
ERROR: MethodError: `+` has no method matching +(::Int64, ::ASCIIString)SYSTEM: show(lasterr) caused an error

was a way to consistently reproduce the bug. There is nothing particular about that choice of call, you just have to trigger a MethodError.

From there, I started commenting out various include statements in the Gtk package. This quickly traced it down to src/GLib/glist.jl. Then a "binary search" (commenting out half of the file and testing, then subdividing the responsible half in half again, etc.) narrowed it down to a single line.

From there, I was able to come up with a standalone way to reproduce this. Here it is:

julia> type Foo{T}
           data::T
       end

julia> Base.show{T}(io::IO, ::Type{Foo{T}}) = print(io, "This is Foo{T}")
show (generic function with 98 methods)

julia> 5+"hello"
ERROR: MethodError: `+` has no method matching +(::Int64, ::ASCIIString)SYSTEM: show(lasterr) caused an error

While @JeffBezanson has in the past said "don't alter the printing of types", this is still a julia bug.

@timholy timholy added the bug Indicates an unexpected problem or unintended behavior label Nov 5, 2015
@timholy timholy changed the title Backtrace not printed on REPL display errors Redefining show for Type{Foo{T}} breaks display of any MethodError Nov 5, 2015
@timholy
Copy link
Member

timholy commented Nov 6, 2015

Gotta switch to other things now, but here's further progress:

julia> ex = MethodError(+, (5, "hello"))
MethodError(+,(5,"hello"))

julia> type Foo{T} end

julia> Base.show{T}(io::IO, ::Type{Foo{T}}) = print(io, "This is Foo")
show (generic function with 98 methods)

julia> Base.show_method_candidates(STDOUT, ex)
ERROR: type TypeConstructor has no field name
 in show at show.jl:105
 in show at expr.jl:56
 in show_type_parameter at ./show.jl:100
 in show at show.jl:110
 in print at strings/io.jl:8
 in print_to_string at strings/io.jl:36
 in string at strings/io.jl:43
 in show_method_candidates at replutil.jl:258

In contrast (and in a fresh julia session),

julia> ex = MethodError(+, (5, "hello"))
MethodError(+,(5,"hello"))

julia> type Foo{T} end

julia> Base.show_method_candidates(STDOUT, ex)

Closest candidates are:
  +(::Any, ::Any, ::Any, ::Any...)
  +(::Int64, ::Int64)
  +(::Integer, ::Ptr{T})
  ...

@yuyichao
Copy link
Contributor

yuyichao commented Nov 6, 2015

FYI #9471

Edit: Yeah, and I remember exactly the same process of bisecting the code I loaded in my .juliarc file....

@yuyichao
Copy link
Contributor

yuyichao commented Nov 6, 2015

This looks exactly like #9471, i.e. error accessing .name of TypeConstructor when printing the method table after overriding the show method of a type.

@yuyichao
Copy link
Contributor

yuyichao commented Nov 6, 2015

So just pasting the only thing from my previous investigation in #9471 from one year ago that might help. #9471 (comment) (in particular, point 3.)

@timholy
Copy link
Member

timholy commented Nov 6, 2015

I've spent a little more time debugging this. For the test case in #13306 (comment), it turns out that the show(::IO, ::DataType) method gets (wrongly) called with a TypeConstructor 156 times (if you prevent it from erroring when it does); each time that TypeConstructor is:

parameters: svec(T)
body: AbstractArray{T,2}

This is definitely strange. Might be a job for valgrind.

@yuyichao
Copy link
Contributor

yuyichao commented Nov 7, 2015

A even shorter repro

type Foo{T}
    data::T
end

Base.show{T}(io::IO, ::Type{Foo{T}}) = print(io, "This is Foo{T}")

typealias TC{N} Array{Bool,N}

io = IOBuffer()
show(io, Array{Bool})
show(io, TC)

(constructed by patching jl_apply_generic to see what have been shown before the failure)

@yuyichao
Copy link
Contributor

yuyichao commented Nov 7, 2015

And finally a show free repro!!

show2(x::DataType) = x.name
show2(x::TypeConstructor) = x.body

type Foo{T}
    data::T
end

show2{T}(::Type{Foo{T}}) = nothing

typealias TC{N} Array{Bool,N}

show2(Array{Bool})
show2(TC)

This seems to be a bad interaction between TypeConstructor, type with free parameter and defining a parametrized method for a specific type.

Note that the problem disappears if:

  1. the TypeConstructor (TC) and the type (Array{Bool}) are not equivalent.
  2. or Foo doesn't have type parameters.

@yuyichao
Copy link
Contributor

yuyichao commented Nov 7, 2015

I'm guessing this has something to do with type widening and the following interesting relation?

julia> typealias TC{N} Array{Bool,N}
Array{Bool,N}

julia> TC
Array{Bool,N}

julia> TC <: Array{Bool}
true

julia> Array{Bool} <: TC
true

julia> Array{Bool} === TC
false

julia> Array{Bool} == TC
true

julia> typeof(Array{Bool})
DataType

julia> typeof(TC)
TypeConstructor

yuyichao added a commit to yuyichao/explore that referenced this issue Nov 7, 2015
@timholy
Copy link
Member

timholy commented Nov 7, 2015

Very nice! Just before going to bed last night I thought to myself, "you know, it would help a lot of if we can find a way to reproduce this that doesn't involve show." And you've done it. Also a nice diagnosis in terms of type equivalences.

I am also in the middle of trying to get Immerse and Gadfly playing nicely again, but I'll see what I can do. Meanwhile, worth pinging @JeffBezanson over this, too, just in case something obvious occurs to him.

@timholy timholy added the types and dispatch Types, subtyping and method dispatch label Nov 7, 2015
@yuyichao
Copy link
Contributor

yuyichao commented Nov 7, 2015

The reason seems to be that without the ::Type{Foo{T}} method, after the first call of show2(Array{Bool}), the cache entry populated is cache_arg1 and the cached/specialized signature is Tuple{DataType}.

(gdb) p jl_(mt->cache_arg1)
Array{Any, 1}[#<null>, #<null>, #<null>, #<null>, Method(sig=Tuple{DataType}, va=false, isstaged=false, tvars=svec(), func=#<function Main.show2(DataType)>, invokes=nothing, next=nothing), #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>, #<null>]

However, when the (::Type{Foo{T}}) method is defned, the cache populated is cache and the signature specialized/cached is Tuple{Type{Array{Bool}}} instead.

(gdb) p jl_(mt->cache)
Method(sig=Tuple{Type{Array{Bool, N<:Any}}}, va=false, isstaged=false, tvars=svec(), func=#<function Main.show2(Type{Array{Bool, N<:Any}})>, invokes=nothing, next=nothing)

Apparently the first one does not match any TypeConstructor while the second one could match the TypeConstructor of a equivalent type.

I guess the issue is in cache_method. There's also some fairly complex heuristic in cache_method related to TypeConstructor.

@vtjnash
Copy link
Member

vtjnash commented Nov 7, 2015

related to that heuristic, there might need to be an additional addition to the condition at

julia/src/gf.c

Line 560 in a664281

else if (jl_is_type_type(elt) && jl_is_typector(jl_tparam0(elt)) &&
that adds the appropriate guard entries to the method cache

@timholy
Copy link
Member

timholy commented Apr 22, 2016

This seems to be fixed.

@timholy timholy closed this as completed Apr 22, 2016
@vtjnash
Copy link
Member

vtjnash commented Apr 22, 2016

this should be covered by the test added in c28ef78

@timholy
Copy link
Member

timholy commented Apr 22, 2016

Good. This was a nasty one that episodically led to several packages corrupting julia. Many thanks for fixing it (even if inadvertently).

@vtjnash
Copy link
Member

vtjnash commented Apr 22, 2016

I made the fix explicitly, not inadvertently, I just didn't know there was a bug open for it. The problem was that cache_method could decide to reject the condition we had constructed if it required creating a guard entry with too much complexity. To handle that case, the TypeMapEntries have a second simple-signature that it uses so that it can store both the requested and the widened signatures, in lieu of creating the guard entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

No branches or pull requests

5 participants