Skip to content

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

Closed
vchuravy wants to merge 1 commit intomasterfrom
vc/test_ambiguity
Closed

[RFC] Fix potential seqfault in testing for ambiguity#24706
vchuravy wants to merge 1 commit intomasterfrom
vc/test_ambiguity

Conversation

@vchuravy
Copy link
Copy Markdown
Member

@vchuravy vchuravy commented Nov 22, 2017

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
Copy Markdown
Member Author

vchuravy commented Nov 22, 2017

@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
Copy Markdown
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
@timholy timholy mentioned this pull request Nov 24, 2017
Merged
2 tasks
@JeffBezanson
Copy link
Copy Markdown
Member

JeffBezanson commented Jan 10, 2018

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

@JeffBezanson
Copy link
Copy Markdown
Member

JeffBezanson commented Jan 10, 2018

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

Reviewers

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees

No one assigned

Labels

test This change adds or pertains to unit tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Assertion failure in jl_gf_invoke_lookup (detect_unbound_args-related)

4 participants

@vchuravy @timholy @JeffBezanson @kshyatt