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

Add special case for Void in typejoin() to return Union{T, Void} #24332

Closed
wants to merge 3 commits into from

Conversation

nalimilan
Copy link
Member

@nalimilan nalimilan commented Oct 25, 2017

This allows for a more efficient code and array layout than using Any.
Other types like Null will be able to opt into this mechanism.

This is an alternative to #23940, though it doesn't address all issues since e.g. collect return types are not always inferred here even though they are correct thanks to the progressive broadening of the container type (compare tests with the other PR).

That's also the minimal change among multiple options:

  • use a special case for Void and Null (this PR)
  • apply this to all pairs of a concrete type with a singleton type (quite arbitary)
  • apply this to all pairs of concrete types

The last solution is less arbitrary, but things like [1, "a"] would return a Vector{Union{String, Int}}, and the fact that this applies only to Unions of two types is still somewhat arbitrary.

Finally, the name isspecial should really be improved but I couldn't find a good solution.

@ararslan ararslan added the types and dispatch Types, subtyping and method dispatch label Oct 25, 2017
@omus
Copy link
Member

omus commented Oct 26, 2017

Maybe call this typejoin_rule? It could work similarly to promote_rule.

@nalimilan
Copy link
Member Author

Except that it's not as general as a rule: it's very limited since it only allows returning Union rather than Any. You can't give a custom type to return. So it's actually closer to a binary trait like IndexStyle. Also it's not supposed to be a public API, only Nulls.jl should use it at this point, waiting for its possible inclusion in stdlib.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

There's nothing wrong with changing this function in Base, but we're currently using the same code in Core.Inference, and we don't want that usage to change. (it would be perfectly acceptable however to copy this function and have different versions in use, or do something like create a promote_join that is overloadable and calls typejoin as a fallback, and use it instead everywhere except inference)

@@ -2,6 +2,10 @@

## type join (closest common ancestor, or least upper bound) ##

isspecial(::Type) = false
isspecial(::Type{Void}) = true
isspecialpair(a, b) = isconcrete(a) && isconcrete(b) && (isspecial(a) || isspecial(b))
Copy link
Member

Choose a reason for hiding this comment

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

If you're intending for people to override this, typejoin won't be pure anymore (currently marked as such)

Copy link
Member Author

Choose a reason for hiding this comment

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

Even if people are only supposed to override this with isspecial(::Type{Null}) = true? @pure is still a mystery to me (is the problem due to the addition of methods after the fact?).

Copy link
Member

Choose a reason for hiding this comment

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

is the problem due to the addition of methods after the fact?

Yes.

@nalimilan
Copy link
Member Author

There's nothing wrong with changing this function in Base, but we're currently using the same code in Core.Inference, and we don't want that usage to change. (it would be perfectly acceptable however to copy this function and have different versions in use, or do something like create a promote_join that is overloadable and calls typejoin as a fallback, and use it instead everywhere except inference)

Thanks for the suggestions. I really don't have strong opinions about what's the best approach. At this point I'm not sure there's a use case for a complex promote-like mechanism, so I'd say we should rather go with the most limited changes which work for the Union{T, Void} case.

@nalimilan
Copy link
Member Author

@JeffBezanson Any suggestions regarding the best approach?

@JeffBezanson
Copy link
Member

I prefer doing the most conservative thing and only adding this for nothing and missing.

@nalimilan
Copy link
Member Author

Fine with me, will have a look once missing is in Base.

…, Void/Missing}

This allows for a more efficient code and array layout than using Any.
@nalimilan nalimilan force-pushed the nl/typejoin branch 2 times, most recently from 31f9407 to d5f0769 Compare December 8, 2017 18:26
@nalimilan nalimilan added the missing data Base.missing and related functionality label Dec 8, 2017
…priate

typejoin() is now used only for inference, and promote_join() everywhere else
to choose the appropriate element type for a collection.
@nalimilan
Copy link
Member Author

I've updated the PR. I now leave typejoin as-is, and introduce promote_join (unexported) instead, which falls back to typejoin but has special methods for Void and Missing. I'm not sure about the name, though, since promote evokes the promotion mechanism which may imply a conversion. Not a big deal since it's internal, but if somebody has better ideas...

@nalimilan
Copy link
Member Author

@JeffBezanson @vtjnash Is the new approach OK?

@quinnj
Copy link
Member

quinnj commented Jan 3, 2018

Looks like this needs a rebase; @JeffBezanson, mind taking a look?

@nalimilan
Copy link
Member Author

Closing in favor of the alternative promote_type_strict approach at #25423.

@nalimilan nalimilan closed this Jan 5, 2018
@nalimilan nalimilan deleted the nl/typejoin branch January 5, 2018 22:49
@nalimilan nalimilan restored the nl/typejoin branch January 13, 2018 19:03
@nalimilan
Copy link
Member Author

GitHub won't let me reopen this, so I've filed #25553 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing data Base.missing and related functionality types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants