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

allow concrete-eval for Union-type arguments #48205

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aviatesk
Copy link
Sponsor Member

By allowing hasuniquerep to analyze Union-types. Motivated by aviatesk/JET.jl#444.

@vtjnash do you think this extension on hasuniquerep is valid?

Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

I think this should be valid, because egal_types on union just defers to egal on the components and we always sort types on construction (and certainly anything that hasuniquerep has a well defined sort order under our predicate).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 10, 2023

I don't think we don't always sort Union types on construction, and datatype_name_cmp gives up after a couple attempts at ordering as well. I think we need to make both of those stronger before we can assume they are always normalized.

By allowing `hasuniquerep` to analyze `Union`-types.
Motivated by <aviatesk/JET.jl#444>.
@aviatesk
Copy link
Sponsor Member Author

I've exported union_sort_cmp and changed hasuniquerep to verify whether t.a/t.b of t::Union are sorted. This adjustment should allow for the correct use of hasuniquerep with Union, but does this seem like a correct?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 29, 2024

I don't think union_sort_cmp is capable of meeting that sort of requirement. For example, it is easy to construct equal types for which the union_sort_cmp would necessarily choose different orders for the components:

julia> Union{Tuple{String}, Tuple{Real}}
Union{Tuple{Real}, Tuple{String}}

julia> Union{Tuple{String}, Tuple{R} where R<:Real}
Union{Tuple{String}, Tuple{R} where R<:Real}

julia> Union{Tuple{String}, Tuple{Real}} == Union{Tuple{String}, Tuple{R} where R<:Real}
true

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.

None yet

3 participants