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

WIP: towards transitive specificity #30171

Closed
wants to merge 2 commits into from
Closed

WIP: towards transitive specificity #30171

wants to merge 2 commits into from

Conversation

JeffBezanson
Copy link
Member

This is an experiment to see what it would take to make specificity transitive (at least far more than it is now), fully fixing #29594. It's possible, but would be pretty disruptive.

The main source of non-transitivity in the current system is the following example (< is more specific, !< is not-more-specific, and { } is short for a tuple type):

1. {Array, Real}    < {AbstractArray}
2. {AbstractArray}  < {Any, Integer}
3. {Array, Real}   !< {Any, Integer}

To consider how to fix this, we can consider changing each relation (which also sheds light on why it works this way currently).

If we change (3), dispatch is asymmetric (we'd prefer the leftmost argument), which of course is a design that's been used but a huge change for us.

If we change (2), we would have

{AbstractArray}  <  {AbstractArray, Int...}  !<  {Any, Integer}

(varargs of course will be the theme here). The second relation is not desirable, since it should mean a lot that Int is more specific than Integer in the second slot even though the last signature is more specific in argument number.

If we change (1), we would have

{Array, Real}  <  {Array, Any...}  !<  {AbstractArray}

which seems to be the best option, although of course still not great since it would be nice if Array in the first slot could take precedence over AbstractArray. This is the option I implemented here. Now at least it is clear why we can't have absolutely everything we might want from specificity.

The other big breaking change this implies is that

{Array{<:Any,N}, Vararg{Int,N}} where N  <  {Array, Int}

no longer holds. That actually simplifies a lot of the specificity code, but adds new ambiguities of course. Some array methods will need to be added or updated, but it shouldn't be too bad.

This pattern seems to be quite common, unfortunately:

BitArray(::UndefInitializer, dims::Integer...) = method 1
BitArray(itr) = method 2

previously the first method was more specific, but it can't be anymore. A similar pattern occurs with iterate methods that use varargs to make the state optional. An interesting fix for this could be to insert two method signatures for vararg methods: the full signature as now, and a copy with the Vararg removed (i.e. add both Tuple{X, Vararg{Y}} and Tuple{X}). That would keep these methods working without the need to manually add those disambiguating methods. Basically, implementing the behavior we have now with a transitive specificity relation requires occupying two spots in the order instead of just one. It's quite likely that's a better way to implement the behavior.

Of course, this might be a 2.0 thing at this point.

In the current state of this PR, #29594 is fixed and the specificity tests pass, but many other things fail due to ambiguities. I'll try implementing the 2-slots-per-vararg-method trick and see how far it gets.

@JeffBezanson JeffBezanson added breaking This change will break code types and dispatch Types, subtyping and method dispatch DO NOT MERGE Do not merge this PR! labels Nov 27, 2018
Fixes basically all transitivity failures in specificity.
At the moment, adds lots of ambiguities.

[ci skip]
@JeffBezanson
Copy link
Member Author

I should mention that one reason a few of us are interested in transitivity is that it will probably let us merge method tables more efficiently, cutting down package load times.

@vtjnash
Copy link
Member

vtjnash commented Nov 28, 2018

I wonder if one way to look at this is that example is to observe that the absence of a value can also be represented by Bottom, so we can have the equivalence that {} == {Vararg{Union{}}}. This we can then use that to pad out the types to the same length, by following the equivalences that {AbstractArray} == {AbstractArray, Vararg{Union{}}} == {AbstractArray, Union{}}. Then I think it becomes apparent why the first rule is problematic for varargs (yes, continuing the theme):

1. {Array, Real}    < {AbstractArray, Union{}}  =implies=> Real < Union{}
2. {AbstractArray, Union{}}  < {Any, Integer}   =implies=> Union{} < Integer

But we also have that Integer < Real, violating transitivity, so we may thus expect that this rule will be problematic for transitivity.

@StefanKarpinski
Copy link
Member

{AbstractArray, Vararg{Union{}}} == {AbstractArray, Union{}} does not seem true, but the argument goes through if you replace Union{} below with Vararg{Union{}}:

1. {Array, Real}   < {AbstractArray, Vararg{Union{}}}  ==> Real < Vararg{Union{}}
2. {AbstractArray, Vararg{Union{}}}  < {Any, Integer}  ==> Vararg{Union{}} < Integer

It also seems like we don't need transitivity to conclude that Real < Vararg{Union{}} is wrong.

@timholy
Copy link
Member

timholy commented Nov 28, 2018

I'm generally in favor of more formal properties and fewer heuristics when it comes to specificity, see #16276.

@timholy
Copy link
Member

timholy commented Nov 28, 2018

In case it's useful, here are type-diagrams that help visualize why both 1 and 3 should be !< and 2 should be < (each row represents the corresponding comparison in the OP):

cmp1

cmp2

cmp3

gist here

@vtjnash
Copy link
Member

vtjnash commented Nov 28, 2018

If we change (3), dispatch is asymmetric (we'd prefer the leftmost argument), which of course is a design that's been used but a huge change for us.

Isn't this equivalently a statement that we should change to resolving these ambiguities in favor of the first argument? Deprecating this method ambiguity error would definitely be a huge philosophical change, but changing an error to a non-error seems like potentially one of the least-breaking options practically.

@JeffBezanson
Copy link
Member Author

Yes, that's a good point and that option might as well be on the table. It would be interesting to survey disambiguation methods we've added and see how often left-to-right would do the right thing.

@vtjnash
Copy link
Member

vtjnash commented Jan 6, 2021

No longer relevant, since we no longer assume or expect it to be transitive

@DilumAluthge DilumAluthge deleted the jb/fix29594 branch March 25, 2021 21:59
@DilumAluthge DilumAluthge removed the DO NOT MERGE Do not merge this PR! label Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants