deprecate fallback constructor

[JeffBezanson]

This resolves #15120. The approach used here is to define explicit constructors for all types (as a bonus, it's nice that more constructors can be defined in Core where the types themselves are defined), and then add convert methods to indicate which constructors are safe for use in conversion. This also has the nice side effect of removing an unsightly warning from the system image build, since Core.Inference can work without the fallback constructor.

@nanosoldier runbenchmarks(ALL, vs=":master")

[vtjnash]

in abstract_call_method, there's a heuristic that should be changed to reflect the new signature:

        # the construct-to-convert method is a bottleneck in inference,
        # so just assume that recursion will get prevented at some other point
       && !(method.sig == Tuple{Type, Any}))

that should address the inference failure in mmap

[vtjnash]

I thought we suggested that this should be:

convert(::Type{I}, x) where {I <: Number} = I(x)

[JeffBezanson]

Yes, it maybe should be eventually, I was just being conservative for now.

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

[JeffBezanson]

Ah, what happened here was that the type assertion in

(::Type{T})(arg) where {T} = convert(T, arg)::T

was very helpful to inference in some cases. The new constructors are all inferable, but there are too many of them, so we hit the method match limit (#23210).

[vtjnash]

Can we just remove that limit?

[JeffBezanson]

That would be a good experiment. I guess we can rely on the property that worse type information will tend to make the inferred return type diverge to Any quickly, so we can still skip many methods. The main thing to do is to make jl_matching_methods return some kind of iterator, so we don't need to build a full list of matches when we're only going to look at 2 of them. (That would probably be a good change anyway.)

[JeffBezanson]

Here's a small wrinkle. It's pretty popular to define highly generic conversions like these:

convert(::Type{T}, x::TwicePrecision) where {T<:Number} = convert(T, x.hi + x.lo)

convert(::Type{T}, x::Enum{T2}) where {T<:Integer,T2<:Integer} = convert(T, bitcast(T2, x))

Do we want these to be constructors? We could certainly move to a world where things like this are defined as constructors by default, with only a relatively small number of convert methods. I think that's the plan, just want to spell it out clearly.

[vtjnash]

That second one should never have existed as a method of convert, so +1 to continuing with the plan.

[JeffBezanson]

I looked into the couple slowdowns here. I can reproduce the revcomp one locally, but the LLVM IR is identical. It might be due to dispatch table rearrangement; the benchmark involves a Box. (It's also slower on master than on 0.6.)

[vtjnash]

We should also merge #22300

[JeffBezanson]

Updated to change all Number conversions to constructors, and deprecate a few more conversions that seem sketchy to me.

[StefanKarpinski]

Yes, it will. Jeff is on vacation for the week.

[JeffBezanson]

Getting this after rebase, if somebody wants to dig in:

julia: /home/jeff/src/julia/src/APInt-C.cpp:455: void LLVMTrunc(unsigned int, llvm::integerPart*, unsigned int, llvm::integerPart*): Assertion `inumbits > onumbits' failed.

signal (6): Aborted
in expression starting at loading.jl:335
raise at /build/buildd/eglibc-2.19/signal/../nptl/sysdeps/unix/sysv/linux/raise.c:56
abort at /build/buildd/eglibc-2.19/stdlib/abort.c:89
__assert_fail_base at /build/buildd/eglibc-2.19/assert/assert.c:92
__assert_fail at /build/buildd/eglibc-2.19/assert/assert.c:101
LLVMTrunc at /home/jeff/src/julia/src/APInt-C.cpp:455
jl_intrinsic_cvt at /home/jeff/src/julia/src/runtime_intrinsics.c:400
jl_call_fptr_internal at /home/jeff/src/julia/src/julia_internal.h:380 [inlined]
jl_call_method_internal at /home/jeff/src/julia/src/julia_internal.h:399 [inlined]
jl_apply_generic at /home/jeff/src/julia/src/gf.c:2011
jl_apply at /home/jeff/src/julia/src/julia.h:1474 [inlined]
jl_f__apply at /home/jeff/src/julia/src/builtins.c:556
builtin_tfunction at ./inference.jl:1774

[JeffBezanson]

Fixed that issue and rebased; let's see what happens now.

[Sacha0]

Beautiful! :) All factorization->array convert methods become array constructors in this pull request I imagine?

[JeffBezanson]

Yes, I think so.

[JeffBezanson]

This has gotten a bit out of hand with the large number of changes happening, but please bear with me. I think we really need this in 1.0 --- it can't be handled neatly by renaming or deprecations.

[StefanKarpinski]

100% agree; just take care of it when you're back!

[JeffBezanson]

Tests passing again! Anybody know if these CircleCI failures are new?

[ararslan]

Nope, not new.

[StefanKarpinski]

They're not new, but they're unrelated afaict. They're both remote worker exceptions and Linux builds pass on Travis, so I think you're in the clear here. Circle CI has been shot for the past week or more.

[StefanKarpinski]

@JeffBezanson, you should probably merge this when you're ready – it's passing CI and we don't want to let it get any new conflicts!

[JeffBezanson]

🎉