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

fix #29269, type intersection bug in union parameters with typevars #29406

Merged
merged 2 commits into from
Nov 27, 2018

Conversation

JeffBezanson
Copy link
Sponsor Member

@JeffBezanson JeffBezanson commented Sep 28, 2018

fixes #25752, fixes #29269, fixes #29430

This fixes some more cases where type intersection incorrectly returned Union{} (which is unsound and causes crashes). Unfortunately, more non-empty intersections means more ambiguities. This mostly affects definitions with types like Type{Union{Missing, T}} ... where T, common of course in missing-data-related code. This requires #29405 (commit included), and also more disambiguating definitions.

The timing of this and #29380 was unintentionally good, so we can try these on master from the beginning of the 1.0.2 and/or 1.1 cycle. Since this fixes a crash it's a candidate for 1.0.2, but a PkgEval run is certainly warranted. If packages hit the same cases Base did, this will have to wait for 1.x.

@JeffBezanson JeffBezanson added kind:breaking This change will break code domain:types and dispatch Types, subtyping and method dispatch kind:bugfix This change fixes an existing bug labels Sep 28, 2018
@quinnj quinnj mentioned this pull request Sep 28, 2018
@JeffBezanson
Copy link
Sponsor Member Author

This will now also require #29419.

@JeffBezanson
Copy link
Sponsor Member Author

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

@nanosoldier
Copy link
Collaborator

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

@nalimilan
Copy link
Member

@JeffBezanson Do you think this is ready to merge? There are lots of apparent regressions but it's hard for me to tell which ones could be real.

@KristofferC
Copy link
Sponsor Member

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

@JeffBezanson
Copy link
Sponsor Member Author

I think this is ready to merge but a PkgEval run would be good. It's hard to know how many packages will hit the same kinds of ambiguities Base and stdlib did.

@nanosoldier
Copy link
Collaborator

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

@ararslan ararslan added the needs pkgeval Tests for all registered packages should be run with this change label Oct 9, 2018
@nalimilan
Copy link
Member

Bump.

@JeffBezanson
Copy link
Sponsor Member Author

Bump for PkgEval.

@ararslan
Copy link
Member

I think right now @Keno and I are the only ones who know how to do a run of NewPkgEval on demand. Unfortunately it requires a fair bit of setup since we don't have the infrastructure in place to just "click go" as it were, and I haven't had time recently to devote to doing on-demand runs nor to improving the infrastructure. Maybe Keno can take care of this in the shorter term, but I won't be able to get to it for a while yet.

@KristofferC
Copy link
Sponsor Member

KristofferC commented Oct 24, 2018

Also fixes the following crash

@inline function partition_missing!(v::Vector{Union{T,Missing}}) where {T}
    i, j = 1, length(v)
    @inbounds while true
        while i < j && v[i] !== missing; i += 1; end
        while i < j && v[j] === missing; j -= 1; end
        i >= j && break
        v[i], v[j] = v[j], v[i]
        i += 1; j -= 1;
    end
    @inbounds return v[i] === missing ? i - 1 : i
end

function my_sort!(v::Vector{Union{T,Missing}}) where {T}
    m = partition_missing!(v)
    w = unsafe_wrap(Array, Ptr{T}(pointer(v)), m)
    @time sort!(w)
    v
end

using Random

function bench(T::Type = Int, n = 1_000_000)
    y = rand(T, n)
    vec = ifelse.(y .< 0.9, y, missing)
    z = copy(vec)
    bench_new = @time my_sort!(z)
end

@Keno
Copy link
Member

Keno commented Nov 22, 2018

Is this still waiting for PkgEval?

@nalimilan
Copy link
Member

Yes, but I think we should merge this, PkgEval is only needed for backporting (since we need to fix this bug anyway, even if it breaks a few packages).

@JeffBezanson
Copy link
Sponsor Member Author

Ok, this can be merged now if we're willing to defer deciding whether to backport it.

@nalimilan
Copy link
Member

I've rebased. Merge?

@nalimilan
Copy link
Member

I'll merge this tomorrow if nobody objects. I really don't want it to miss 1.1.

@JeffBezanson JeffBezanson merged commit 4f8a7b9 into master Nov 27, 2018
@JeffBezanson JeffBezanson deleted the jb/fix29269 branch November 27, 2018 23:21
nalimilan added a commit to JuliaData/CategoricalArrays.jl that referenced this pull request Nov 29, 2018
JuliaLang/julia#29406 changed ambiguity rules, which makes the tests fail.
nalimilan added a commit to JuliaData/CategoricalArrays.jl that referenced this pull request Nov 29, 2018
JuliaLang/julia#29406 changed ambiguity rules, which makes the tests fail.
@StefanKarpinski StefanKarpinski added status:triage This should be discussed on a triage call and removed status:triage This should be discussed on a triage call labels Jan 31, 2019
@JeffBezanson JeffBezanson removed status:triage This should be discussed on a triage call triage backport pending 1.0 labels Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:types and dispatch Types, subtyping and method dispatch kind:breaking This change will break code kind:bugfix This change fixes an existing bug needs pkgeval Tests for all registered packages should be run with this change
Projects
None yet
7 participants