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

Normalize (simplify) UnionAlls when used as type parameter #36211

Merged
merged 1 commit into from
Apr 6, 2021

Conversation

martinholters
Copy link
Member

A stab at #35130 although I'm not totally convinced this is a good idea, see #35130 (comment).

This normalizes UnionAlls (and Unions thereof) when they are used as type parameters. Normalization here means that the typevar is replaced with its upper bound if it does not appear in invariant position and is not subject to the diagonal rule (or in the trivial case where lower and upper bound are identical). Parameters to Type are excluded from this normalization to prevent the change in dispatch also mentioned in #35130 (comment). This is sufficient to cover the cases from #35130:

julia> Val{Tuple{T} where T}
Val{Tuple{Any}}

julia> Val{Tuple{Float64,Vararg{T,2}} where T<:Int64}
Val{Tuple{Float64,Int64,Int64}}

julia> Val{Tuple{Float64,T,T} where T<:Int64}
Val{Tuple{Float64,Int64,Int64}}

julia> Val{Union{Nothing, T} where T}
Val{Any}

This mostly also makes the tests from #34272 pass (except for where the Type exclusion is relevant), but I'm not convinced there aren't other cases where type-equal concrete types are not normalized to identical types, so #34272 might still be necessary in all its glory.

Closes #35130.

@JeffBezanson
Copy link
Member

I updated and rebased this. I think we should probably merge it, since it fixes some crashes. Seems to be a strict improvement.

@JeffBezanson JeffBezanson added this to the 1.7 milestone Mar 29, 2021
@JeffBezanson JeffBezanson added the backport 1.6 Change should be backported to release-1.6 label Mar 29, 2021
@JeffBezanson JeffBezanson removed this from the 1.7 milestone Mar 29, 2021
@JeffBezanson JeffBezanson removed the needs tests Unit tests are required for this change label Mar 31, 2021
@JeffBezanson JeffBezanson self-assigned this Mar 31, 2021
@JeffBezanson JeffBezanson changed the title RFC: Normalize (simplify) UnionAlls when used as type parameter Normalize (simplify) UnionAlls when used as type parameter Apr 6, 2021
@JeffBezanson JeffBezanson merged commit fedefe9 into master Apr 6, 2021
@JeffBezanson JeffBezanson deleted the mh/normalize_unionalls branch April 6, 2021 03:03
@KristofferC
Copy link
Member

@JeffBezanson, this needs a manual backport to #40209

@Sacha0
Copy link
Member

Sacha0 commented Apr 11, 2021

Thanks for shepherding this along Jeff, and for having an eye on the backport Kristoffer! :)

JeffBezanson pushed a commit that referenced this pull request Apr 14, 2021
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label May 4, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
vtjnash added a commit that referenced this pull request Aug 19, 2021
Need to compute `cacheable` after normalization, since the purpose of
the normalization was to turn these into normal cacheable objects,
when applicable.

Brokenness exposed by #36211
Fixes #41503
vtjnash added a commit that referenced this pull request Aug 23, 2021
Need to compute `cacheable` after normalization, since the purpose of
the normalization was to turn these into normal cacheable objects,
when applicable.

Brokenness exposed by #36211
Fixes #41503
KristofferC pushed a commit that referenced this pull request Aug 27, 2021
Need to compute `cacheable` after normalization, since the purpose of
the normalization was to turn these into normal cacheable objects,
when applicable.

Brokenness exposed by #36211
Fixes #41503

(cherry picked from commit 292f1a9)
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
Need to compute `cacheable` after normalization, since the purpose of
the normalization was to turn these into normal cacheable objects,
when applicable.

Brokenness exposed by JuliaLang#36211
Fixes JuliaLang#41503
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Need to compute `cacheable` after normalization, since the purpose of
the normalization was to turn these into normal cacheable objects,
when applicable.

Brokenness exposed by JuliaLang#36211
Fixes JuliaLang#41503
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
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.

datatype cache normalization bugs
4 participants