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

[RFC] Fix potential seqfault in testing for ambiguity #24706

Closed
wants to merge 1 commit into from

Conversation

vchuravy
Copy link
Member

fixes #24460

jl_gf_invoke_lookup seqfaults on a UnionAll since that has no field parameters,
I think it was originally intended to just take a tuple type.
Base.rewrap_unionall only returns a UnionAll if m.sig is a UnionAll so I suspect
that this codepath was just never tested with a UnionAll

cc: @timholy

@vchuravy vchuravy requested a review from vtjnash November 22, 2017 15:18
@vchuravy
Copy link
Member Author

@timholy this turns your assertion error into:

detect_unbound_args in Base and Core: Test Failed at /home/vchuravy/src/julia/test/ambiguous.jl:283                                                                                                       
  Expression: need_to_handle_undef_sparam == Set()                                                                                                                                                        
   Evaluated: Set(Method[broadcast_similar(f, ::Base.Broadcast.MatrixStyle, ::Type{Bool}, inds::Tuple{AbstractUnitRange,AbstractUnitRange}, As...) where ElType in Base.Broadcast at broadcast.jl:217, bro
adcast_similar(f, ::Base.Broadcast.VectorStyle, ::Type{Bool}, inds::Tuple{AbstractUnitRange}, As...) where ElType in Base.Broadcast at broadcast.jl:215, broadcast_similar(f, ::Base.Broadcast.DefaultArra
yStyle{N}, ::Type{Bool}, inds::Tuple{Vararg{AbstractUnitRange,N}}, As...) where {N, ElType} in Base.Broadcast at broadcast.jl:202, broadcast_similar(f, ::Base.Broadcast.ArrayConflict, ::Type{Bool}, inds
::Tuple{Vararg{AbstractUnitRange,N}} where N, As...) where ElType in Base.Broadcast at broadcast.jl:207]) == Set{Any}()

@timholy
Copy link
Member

timholy commented Nov 22, 2017

I will have a look in the next few days. Many thanks for tackling this!

@kshyatt kshyatt added the test This change adds or pertains to unit tests label Nov 22, 2017
@JeffBezanson
Copy link
Member

I'll see if I can get a better fix working (making invoke fully support UnionAll types).

@JeffBezanson
Copy link
Member

replaced by #25495

@vchuravy vchuravy deleted the vc/test_ambiguity branch January 12, 2018 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion failure in jl_gf_invoke_lookup (detect_unbound_args-related)
4 participants