inference: remove hacks around type-system incorrectness #21892

vtjnash · 2017-05-15T22:24:35Z

infers a missing method (MethodError) as returning Union{}
and fix tests that were wrong, but not detected previously!

martinholters · 2017-05-16T06:42:41Z

base/inference.jl

@@ -1397,10 +1395,6 @@ function abstract_call_gf_by_type(f::ANY, atype::ANY, sv::InferenceState)
        add_mt_backedge(ftname.mt, argtype, sv)
        update_valid_age!(min_valid[1], max_valid[1], sv)
    end
-    if isempty(applicable)
-        # TODO: this is needed because type intersection is wrong in some cases
-        return Any


Are we sure we are ready for this? Admittedly, of the typeintersect bugs I have seen recently, none was erroneously giving Bottom, but since proposing this in #20866, I have lost some confidence in typeintersect... Anyway, we can try, and if there are problems and we can't straighten it out in subype.c in time, we can just add this back.

martinholters · 2017-05-16T06:44:51Z

base/inference.jl

@@ -3546,7 +3533,7 @@ function invoke_NF(argexprs, etype::ANY, atypes, sv, atype_unlimited::ANY,
                            all = false
                        end
                    end
-                    if UNION_SPLIT_MISMATCH_ERROR && all


Just to be sure: This was also about wrongly missing a method due to a wrongly empty type intersection?

martinholters · 2017-05-16T06:54:00Z

test/arrayops.jl

 @testset "Exactness of cumsum # 21666" begin
    # test that cumsum uses more stable algorithm
    # for types with unknown/rounding arithmetic
-    Base.TypeArithmetic(::Type{F21666{T}}) where {T} = T
-    Base.:+(x::F, y::F) where {F <: F21666} = F(x.x + y.x)
-    Base.convert(::Type{Float64}, x::F21666) = Float64(x.x)


This change is fine, of course, but...
Is everything in a @testset always happening at the same world age? Somehow, I was under the impression that some method definitions took effect immediately, but I fail to reproduce now, so I was probably wrong. Could you drop a comment at #21769?

Thinking about it again, the + and convert definitions must have been available inside the @testset for the tests to have worked, or am I missing something here?

Inference is willing to get everything wrong if you write method definitions like this. (ref #21570)

martinholters · 2017-05-16T07:01:18Z

Travis on OSX timed out because it was building LLVM. Was that to be expected?

tkelman · 2017-05-16T10:05:32Z

that can happen if it fails to download the llvm homebrew bottle. hopefully transient.

alyst · 2017-05-16T14:25:26Z

test/arrayops.jl

@@ -2059,12 +2059,12 @@ struct F21666{T <: Base.TypeArithmetic}
    x::Float32
 end

+Base.TypeArithmetic(::Type{F21666{T}}) where {T} = T()


Would it make sense to leave a note for the posterity why struct F21666 and these definitions should be outside of @testset?

Might make more sense to expand the Methods/Redefining Methods manual section a bit to explain how things behave inside let/try/... blocks at top level and add a warning note to the Unit Testing/Working with Test Sets section. If we just add a comment here, that won't stop the same mistake from being made elsewhere. (Assuming proper documentation will is pretty optimistic, I know, but still...)

martinholters · 2017-05-19T09:12:13Z

Current thinking: Let's go with this, hoping that we will weed out some more typeintersect bugs (uncovered by this or otherwise) during the release cycle. Merge before I change my mind once again?

KristofferC · 2017-05-19T09:15:22Z

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

martinholters · 2017-05-19T09:19:40Z

Worth a PkgEval run, too? (I haven't set it up here and I'm afraid I don't have the time to do so at the moment, but maybe someone else could run it?)

nanosoldier · 2017-05-19T09:40:34Z

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(132)) [132]

Logs and partial data can be found here
cc @jrevels

KristofferC · 2017-05-19T09:43:52Z

https://github.com/JuliaCI/BaseBenchmarkReports/blob/b5c7db227cca2c6e74253a1307daf5395ecd0be9/2d56182_vs_31f798e/logs/2d56182650f6fe68e1db062d256c1bae8cad4436_primary.err#L347 :

signal (4): Illegal instruction
while loading /home/nanosoldier/workdir/tmpeFa97x/benchscript.jl, in expression starting on line 16

martinholters · 2017-05-19T09:58:04Z

Hah, I guess we've already found another bug we definitely should fix before merging this.

martinholters · 2017-05-19T13:37:50Z

I cannot reproduce exactly the crash seen on nanosoldier, but load!("misc") crashes for me, which I believe is related to this:

julia> methods(string, Tuple{Char})
# 1 method for generic function "string":
string(a::Union{Char, String}...) in Base at strings/string.jl:351

julia> using LegacyStrings;

julia> methods(string, Tuple{Char})
# 0 methods for generic function "string":

julia> string('0')
"0"

Now, LegacyStrings indeed introduces an ambiguity here:

julia> methods(string)
# 17 methods for generic function "string":
# ...
string(a::Union{Char, String}...) in Base at strings/string.jl:351
#...
string(a::Union{Char, LegacyStrings.ASCIIString, LegacyStrings.UTF8String}...) in LegacyStrings at LegacyStrings/src/utf8.jl:161
#...

So string('0') should raise a MethodError and that is what inference assumes. Looks like this time, the bug is that string('0') is sill callable when really it shouldn't be.

martinholters · 2017-05-19T15:17:08Z

Fixing the ambiguity in LegacyStrings lets the benchmarks finish successfully, at least locally.

infers a missing method (MethodError) as returning `Union{}` and fix tests that were wrong, but not detected previously!

martinholters · 2017-05-30T06:26:32Z

Let's see if it works now: @nanosoldier runbenchmarks(ALL, vs = ":master")

nanosoldier · 2017-05-30T09:23:56Z

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

vtjnash requested a review from martinholters May 15, 2017 22:24

ararslan added the compiler:inference Type inference label May 16, 2017

martinholters approved these changes May 16, 2017

View reviewed changes

alyst reviewed May 16, 2017

View reviewed changes

martinholters mentioned this pull request May 19, 2017

Infer a missing method as returning Union{} #20866

Closed

martinholters mentioned this pull request May 19, 2017

Ambiguous method callable #21963

Closed

This was referenced May 26, 2017

Remove string(::Union{ByteString,Char}...) method JuliaStrings/LegacyStrings.jl#20

Merged

Method overwriting by an ambiguity should also invalidate the method cache #21965

Merged

inference: remove hacks around type-system incorrectness

753fbef

infers a missing method (MethodError) as returning `Union{}` and fix tests that were wrong, but not detected previously!

vtjnash force-pushed the jn/typeinf-more branch from b17b3b1 to 753fbef Compare May 29, 2017 18:14

vtjnash merged commit e3794ee into master May 30, 2017

vtjnash deleted the jn/typeinf-more branch May 30, 2017 17:07

vtjnash mentioned this pull request Jun 9, 2017

Inference substitutes Any for MethodError #14336

Closed

tkelman mentioned this pull request Aug 8, 2017

Inference disabled by a world age issue (0.6.0-rc3) #22355

Closed

Uh oh!

inference: remove hacks around type-system incorrectness #21892

inference: remove hacks around type-system incorrectness #21892

Uh oh!

Conversation

vtjnash commented May 15, 2017

Uh oh!

martinholters May 16, 2017

Choose a reason for hiding this comment

Uh oh!

martinholters May 16, 2017

Choose a reason for hiding this comment

Uh oh!

martinholters May 16, 2017

Choose a reason for hiding this comment

Uh oh!

martinholters May 17, 2017

Choose a reason for hiding this comment

Uh oh!

vtjnash May 17, 2017

Choose a reason for hiding this comment

Uh oh!

martinholters commented May 16, 2017

Uh oh!

tkelman commented May 16, 2017

Uh oh!

alyst May 16, 2017

Choose a reason for hiding this comment

Uh oh!

martinholters May 18, 2017

Choose a reason for hiding this comment

Uh oh!

martinholters commented May 19, 2017

Uh oh!

KristofferC commented May 19, 2017

Uh oh!

martinholters commented May 19, 2017

Uh oh!

nanosoldier commented May 19, 2017

Uh oh!

KristofferC commented May 19, 2017

Uh oh!

martinholters commented May 19, 2017

Uh oh!

martinholters commented May 19, 2017

Uh oh!

martinholters commented May 19, 2017

Uh oh!

martinholters commented May 30, 2017

Uh oh!

nanosoldier commented May 30, 2017

Uh oh!

Uh oh!