-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Pair construction can bork convert #8631
Comments
That's just a copy-paste of the |
Actually, even if you use the new syntax, the error remains. |
An even more minimal case: julia> immutable RGB
r::Float32
g::Float32
b::Float32
end
julia> const colormaps_sequential = Dict(
"blues" => (255, 0.3, 0.25, 0.88, 0.6, 0.75, RGB(1,1,0), RGB(0,0,1)))
Dict{ASCIIString,(Int64,Float64,Float64,Float64,Float64,Float64,RGB,RGB)} with 1 entry:
"blues" => (255,0.3,0.25,0.88,0.6,0.75,RGB(1.0f0,1.0f0,0.0f0),RGB(0.0f0,0.0f0,1.0f0))
julia> convert((Int...), (uint(1),))
ERROR: `convert` has no method matching convert(::Type{Int64...}, ::Uint64)
in convert at base.jl:9
in convert at base.jl:17 |
Even more minimal: julia> 1 => (1,1,1,1,1,1,1,1,1.,false);
julia> convert((Int...), (uint(1),))
ERROR: `convert` has no method matching convert(::Type{Int64...}, ::Uint64)
in convert at base.jl:9
in convert at base.jl:17 |
Good, this is the level of breakage I was hoping to get to in 0.4-devel :) On the other hand, oh f*** there goes my week. |
Playing around with this julia> 1=>(1,1,1,1,false)
1=>(1,1,1,1,false)
julia> convert((Int...), (uint(1),))
(1,) julia> 1=>(1,1,1,1,1,false)
1=>(1,1,1,1,1,false)
julia> convert((Int...), (uint(1),))
ERROR: `convert` has no method matching convert(::Type{Int64...}, ::Uint64) julia> 1=>(1,1,1,1,2.0)
1=>(1,1,1,1,2.0)
julia> convert((Int...), (uint(1),))
(1,) julia> 1=>(1,1,1,1,1,2.0)
1=>(1,1,1,1,1,2.0)
julia> convert((Int...), (uint(1),))
ERROR: `convert` has no method matching convert(::Type{Int64...}, ::Uint64) julia> 1=>(1,1,1,1,1,2.0,1)
1=>(1,1,1,1,1,2.0,1)
julia> convert((Int...), (uint(1),))
(1,) julia> 1=>(1,1,1,1,1,1,2.0,1)
1=>(1,1,1,1,1,1,2.0,1)
julia> convert((Int...), (uint(1),))
ERROR: `convert` has no method matching convert(::Type{Int64...}, ::Uint64) julia> 1=>(1.0,2,3,4, false)
1=>(1.0,2,3,4,false)
julia> convert((Int...), (uint(1),))
(1,) julia> 1=>(1.0,2,3,4,5, false)
1=>(1.0,2,3,4,5,false)
julia> convert((Int...), (uint(1),))
ERROR: `convert` has no method matching convert(::Type{Int64...}, ::Uint64) julia> 1=>(false,1.0,2,3,4)
1=>(false,1.0,2,3,4)
julia> convert((Int...), (uint(1),))
(1,) julia> 1=>(false,1.0,2,3,4,5)
1=>(false,1.0,2,3,4,5)
julia> convert((Int...), (uint(1),))
ERROR: `convert` has no method matching convert(::Type{Int64...}, ::Uint64) julia> 1 => (false,1,2,3,4,5)
1=>(false,1,2,3,4,5)
julia> convert((Int...), (uint(1),))
(1,) So it seems like it is potentially related to these magic numbers although I don't offhand know why it starts at N+1. |
Actually changing those numbers does not seem to impact behavior. |
In case it's not old news, convert is breaking for me on 0.3: julia> aa = convert(Array{Ufixed8}, a)
ERROR: `convert` has no method matching convert(::Type{Array{UfixedBase{Uint8,8},N}}, ::Array{Float64,2})
in convert at base.jl:13
julia> Pkg.status()
ERROR: `convert` has no method matching convert(::Type{UTF8String}, ::ASCIIString)
in convert at base.jl:13
in dir at pkg/git.jl:10
in git at pkg/git.jl:16
in success at pkg/git.jl:27
in iscommit at pkg/git.jl:44
in installed_version at ./pkg/read.jl:65
in installed at ./pkg/read.jl:122
in status at pkg/entry.jl:107
in anonymous at pkg/dir.jl:28
in cd at ./file.jl:20
in cd at pkg/dir.jl:28
in status at pkg.jl:28 (repeats 2 times)
julia> versioninfo()
Julia Version 0.3.2-pre+48
Commit bf35ce6 (2014-10-08 14:44 UTC)
Platform Info:
System: Darwin (x86_64-apple-darwin13.4.0)
CPU: Intel(R) Core(TM) i5-4258U CPU @ 2.40GHz
WORD_SIZE: 64
BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
LAPACK: libopenblas
LIBM: libopenlibm
LLVM: libLLVM-3.3 |
@bjarthur noted that this can be triggered on 0.3, too, although the minimal testcase above is not sufficient to do it. |
I'm getting this on my 3.3 install when I try to return a tuple of
|
I needed to take a short break from more tedious stuff, so I got started here; only a tiny step forward, but I thought I'd throw it out there since I still have deadlines that will occupy most of my attention. I began by inserting a print statement: diff --git a/src/gf.c b/src/gf.c
index ee226d9..0b21779 100644
--- a/src/gf.c
+++ b/src/gf.c
@@ -1841,6 +1843,8 @@ static jl_value_t *ml_matches(jl_methlist_t *ml, jl_value_t *type,
ti = lookup_match(type, (jl_value_t*)ml->sig, &env, ml->tvars);
if (ti != (jl_value_t*)jl_bottom_type) {
assert(ml->func->linfo); // no builtin methods
+ if (strcmp(ml->func->linfo->name->name, "convert") == 0)
+ jl_printf(JL_STDOUT, "match function %s, file %s, line %d\n", ml->func->linfo->name->name, ml->func->linfo->file->name, ml->func->linfo->line);
assert(jl_is_tuple(env));
int skip = 0; and then used the following debug file: 1 => (1,2) # A non-breaking warmup
# 1 => (1,1,1,1,1,1,1,1,1.,false);
println("Starting")
convert((Int...), (uint(1),)) If I leave the comment in place (so
but if I uncomment that line, then it looks like this:
Basically, the two start out identically except for the presence of a bunch of |
Surprisingly, "convertalypse" (or at least, this specific manifestation) turns out to be a previously undetected consequence of @vtjnash's inlining patches. The problem lies somewhere in this range of commits: I can't narrow it down any better than this, because julia won't build within this range. Here was my test script: type Pair{A,B}
first::A
second::B
end
Pair(1, (1,1,1,1,1,1,1,1,1.,false));
convert((Int...), (uint(1),)) |
In the inlining work, I assume that the subtype relationship must hold between the input arguments and the function type signature, however, for Tuples of typevars, Julia computes the wrong answer for subtype. Perhaps that is at issue here? I already opened an issue for it. |
Not sure I follow entirely. Would that explain why it works in a fresh julia session, but that once you construct a The |
Unfortunately, there seems to be more than one problem underlying "convertalypse." Disabling all inlining by building julia with diff --git a/base/inference.jl b/base/inference.jl
index 0ecb096..ed6765e 100644
--- a/base/inference.jl
+++ b/base/inference.jl
@@ -2032,6 +2032,7 @@ end
# static parameters are ok if all the static parameter values are leaf types,
# meaning they are fully known.
function inlineable(f, e::Expr, atypes, sv, enclosing_ast)
+ return NF
if !(isa(f,Function) || isstructtype(f) || isa(f,IntrinsicFunction))
return NF
end "fixes" the problem here but does not fix #8818. So your suggestion that this may be limited to tuple-types is probably well-founded. |
Switching the colormaps_sequential and colormaps_diverging values from tuples to vectors works around the Julia bug JuliaLang/julia#8631, solving JuliaAttic#68 and JuliaIO/HDF5.jl#160.
the issue may be that in the call to Line 781 in fb730f3
in particular, the following call:
is missing the following function definition:
therefore, when we specialize |
Nice detective work. I assume you don't yet have an idea why that method isn't showing up? |
no, usually i just hope jeff will chime in at some point. although, looking back at it, i'm not sure if my statement was entirely correct either. It's not exactly missing the match for the empty list so much as missing the match for when but it does let us make a more reduced test case:
and sticking a breakpoint in tracing through the code execution, we see that which means that we can "fix" it (and the original bug) with one liner (kids, don't try this at home):
so, now that you got me started again, it seems I do now have some idea why this isn't showing up |
@JeffBezanson can you verify the validity of the following patch (since you can't make a tuple containing fwiw, the following change doesn't seem to break things (and it fixes this bug): diff --git a/src/jltypes.c b/src/jltypes.c
index 7d4d53c..50a838e 100644
--- a/src/jltypes.c
+++ b/src/jltypes.c
@@ -751,10 +751,12 @@ static int tuple_to_Type(jl_tuple_t *a, jl_tuple_t **ptemp)
for(i=0; i < alen; i++) {
jl_value_t *el = jl_tupleref(a, i);
if (jl_is_type_type(el)) {
- jl_tupleset(*ptemp, i, jl_tparam0(el));
+ jl_value_t *T = jl_type_intersection(jl_tparam0(el), (jl_value_t*)jl_any_type); // removes Undef
+ jl_tupleset(*ptemp, i, T);
}
else if (i==alen-1 && jl_is_vararg_type(el) && jl_is_type_type(jl_tparam0(el))) {
- jl_tupleset(*ptemp, i, jl_wrap_vararg(jl_tparam0(jl_tparam0(el))));
+ jl_value_t *T = jl_type_intersection(jl_tparam0(jl_tparam0(el)), (jl_value_t*)jl_any_type); // removes Undef
+ jl_tupleset(*ptemp, i, jl_wrap_vararg(T));
}
else {
*ptemp = NULL; with the following test case:
i'm not sure if this is entirely complete, however, or if the following answer is also wrong:
|
That patch looks safe. In the second part |
ok. however, i don't feel like that patch is complete. i can't quite decide if the following are correct:
it seems to be a covariance/invariance complication arising from the so that leads to this alternative patch, which also fixes the convert bug, but in a very different way: diff --git a/src/jltypes.c b/src/jltypes.c
index 7d4d53c..69982b1 100644
--- a/src/jltypes.c
+++ b/src/jltypes.c
@@ -539,6 +539,14 @@ static jl_value_t *intersect_tag(jl_datatype_t *a, jl_datatype_t *b,
continue;
}
}
+ else if (jl_is_tuple(ap)) {
+ if (!jl_is_tuple(bp)) {
+ JL_GC_POP();
+ return jl_bottom_type;
+ }
+ ti = intersect_tuple((jl_tuple_t*)ap, (jl_tuple_t*)bp, penv,eqc,invariant);
+ jl_tupleset(p, i, ti);
+ }
else {
int tva = jl_has_typevars_(ap,0);
int tvb = jl_has_typevars_(bp,0); and now we get:
(although now this fails other tests) additionally, 98a1165 seems closely related to this issue. |
or instead:
although that can mucks things up pretty badly, since it turns
|
No comments here in over a month, so I'll ask: What is the current status of this issue with |
🍰 🎆 |
👏 @ivarne did you intend to reopen this? [edit: saw the revert, sorry for the noise] |
Nice work @JeffBezanson! How much different would this look like on release-0.3? We're probably tagging 0.3.6 soon, might be good to do this as one of the first commits for 0.3.7 (assuming it doesn't introduce any new issues on master). |
Thanks! This is a tricky one. Since this removes the |
Right, don't want to rush this, but wanted to get the question out there since this has been bothering people for a while. Let's see who can break it first :) |
Great to have this closed! Many thanks. |
Is this supposed to be working in 0.4.3?
|
|
A minimal testcase for the issue reported here:
And now:
The text was updated successfully, but these errors were encountered: