Skip to content

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

Closed
nalimilan wants to merge 3 commits intomasterfrom
nl/typejoin
Closed

Add special case for Void in typejoin() to return Union{T, Void}#24332
nalimilan wants to merge 3 commits intomasterfrom
nl/typejoin

Conversation

@nalimilan
Copy link
Copy Markdown
Member

@nalimilan nalimilan commented Oct 25, 2017 •
edited
Loading

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 requested a review from JeffBezanson October 25, 2017 20:12
@ararslan ararslan added the types and dispatch Types, subtyping and method dispatch label Oct 25, 2017
@omus
Copy link
Copy Markdown
Member

omus commented Oct 26, 2017

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

@nalimilan
Copy link
Copy Markdown
Member Author

nalimilan commented Oct 26, 2017

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.

vtjnash
vtjnash requested changes Oct 26, 2017
View reviewed changes
Copy link
Copy Markdown
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)

base/promotion.jl Outdated

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

@vtjnash vtjnash Oct 26, 2017

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

@nalimilan nalimilan Oct 26, 2017

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

@JeffBezanson JeffBezanson Nov 14, 2017

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

nalimilan commented Oct 26, 2017

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

nalimilan commented Nov 14, 2017

@JeffBezanson Any suggestions regarding the best approach?

@JeffBezanson
Copy link
Copy Markdown
Member

JeffBezanson commented Nov 14, 2017

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

@nalimilan
Copy link
Copy Markdown
Member Author

nalimilan commented Nov 14, 2017

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

This was referenced Nov 28, 2017
Closed
Closed
@bkamins bkamins mentioned this pull request Dec 2, 2017
Open
@nalimilan
Add special case for Void and Missing in typejoin() to return Union{T…
1c0e5b7 
…, 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
@nalimilan
Introduce promote_join() and use it instead of typejoin() where appro…
9094694 
…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
Copy Markdown
Member Author

nalimilan commented Dec 8, 2017

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 nalimilan mentioned this pull request Dec 9, 2017
Closed
@nalimilan nalimilan requested a review from vtjnash December 13, 2017 09:27
@nalimilan
Copy link
Copy Markdown
Member Author

nalimilan commented Dec 22, 2017

@JeffBezanson @vtjnash Is the new approach OK?

@quinnj
Copy link
Copy Markdown
Member

quinnj commented Jan 3, 2018

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

@nalimilan nalimilan mentioned this pull request Jan 5, 2018
Closed
@nalimilan
Copy link
Copy Markdown
Member Author

nalimilan commented Jan 5, 2018

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 nalimilan mentioned this pull request Jan 14, 2018
Closed
@nalimilan nalimilan mentioned this pull request Jan 14, 2018
Merged
@nalimilan
Copy link
Copy Markdown
Member Author

nalimilan commented Jan 14, 2018

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

Reviewers

@JeffBezanson JeffBezanson Awaiting requested review from JeffBezanson

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees

No one assigned

Labels

missing data Base.missing and related functionality types and dispatch Types, subtyping and method dispatch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nalimilan @omus @JeffBezanson @quinnj @vtjnash @ararslan