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

some fixes to method ambiguity reflection #16220

Merged
merged 1 commit into from
May 6, 2016
Merged

some fixes to method ambiguity reflection #16220

merged 1 commit into from
May 6, 2016

Conversation

JeffBezanson
Copy link
Sponsor Member

@JeffBezanson JeffBezanson commented May 5, 2016

Putting #16125 through its paces, I discovered the following case where methods returned duplicates:

julia> amb_2(::Int, y) = 1

julia> amb_2(x, ::Int) = 2

julia> amb_2(::Int8, y) = 3

julia> methods(amb_2)
#4 methods for generic function "amb_2":
amb_2(::Int64, y) at REPL[1]:1
amb_2(x, ::Int64) at REPL[2]:1
amb_2(::Int8, y) at REPL[3]:1
amb_2(x, ::Int64) at REPL[2]:1

I then decided that the default behavior of methods(f, t) should be to return only methods that could actually be called, so ambiguous matches should be excluded. This PR implements that and fully avoids returning duplicate entries.

I also found this evil case:

amb_3(::Int8, ::Int8) = 1
amb_3(::Int16, ::Int16) = 2
amb_3(::Integer, ::Integer) = 3
amb_3(::Integer, x) = 4
amb_3(x, ::Integer) = 5

Here, the last two methods appear ambiguous but aren't, because their intersection is fully covered by prior methods. isambiguous did not handle this.

I have added tests for these cases.

`methods` now doesn't return ambiguous matches by default

some fixes to `isambiguous` and more tests
@@ -749,6 +749,7 @@ static int check_ambiguous_visitor(jl_typemap_entry_t *oldentry, struct typemap_
struct ambiguous_matches_env *closure = container_of(closure0, struct ambiguous_matches_env, match);
if (oldentry == closure->newentry)
return 0; // finished once it finds the method that was just inserted
// TODO: instead, maybe stop once we hit something newentry is definitely more specific than
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

alternatively, I've been wondering if we can use the work done here to create a fast-path for invalidate_conflicting where we only test lambdas that were derived from methods that check_ambiguous deemed to have an intersection

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was motivated by the following example:

f(::Int8, x)
f(::Integer, x)
f(x, ::Int)

Here we fail to detect that Int8, Int is ambiguous.

Yes, that sounds promising.

@timholy
Copy link
Sponsor Member

timholy commented May 5, 2016

Wow, thanks for noticing those problems and fixing them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants