Skip to content

fix unsoundness in fieldtype of Tuple with non-Type params #40067

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

Merged
merged 2 commits into from
May 28, 2021

Conversation

JeffBezanson
Copy link
Member

This is one way to fix #39988. The alternative is to make fieldtype(Tuple{:x}, 1) return Union{}, which would be more consistent with other types. But, I'm assuming the default right thing is to make inference correct rather than change the value we return, so I'm trying this first. This version would be backportable for instance. But, if it causes problems (e.g. maybe too many new invalidations) we could just make the change to fieldtype instead and give up on fixing it for <1.7.

@JeffBezanson JeffBezanson added the compiler:inference Type inference label Mar 16, 2021
@vtjnash
Copy link
Member

vtjnash commented Mar 16, 2021

Regardless of what we decide here for now, I believe fieldtype and first-parameter may eventually need to diverge somewhat to fix this mistake with the diagonal rule / covariance:

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

julia> fieldtype(ans, 1)
Tuple{T, T} where T # wrong: should be `Tuple{Any, Any}`

julia> ((1, 2.), Val{Any}()) isa Tuple{Tuple{T, T}, Val{T}} where T
true

julia> getfield(((1, 2.), Val{Any}()), 1) isa fieldtype(Tuple{Tuple{T, T}, Val{T}} where T, 1)
false

@JeffBezanson JeffBezanson added the needs pkgeval Tests for all registered packages should be run with this change label Mar 18, 2021
@JeffBezanson

This comment has been minimized.

@nanosoldier

This comment has been minimized.

@maleadt

This comment has been minimized.

@nanosoldier

This comment has been minimized.

@maleadt
Copy link
Member

maleadt commented Apr 14, 2021

The server has been rebooted, so let's try once more:

@nanosoldier runtests(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@JeffBezanson
Copy link
Member Author

Looks like all the failures are numeric issues, benchmark noise, etc.

@JeffBezanson JeffBezanson added backport 1.6 Change should be backported to release-1.6 bugfix This change fixes an existing bug and removed needs pkgeval Tests for all registered packages should be run with this change labels May 19, 2021
@JeffBezanson JeffBezanson changed the title WIP/RFC: fix unsoundness in fieldtype of Tuple with non-Type params fix unsoundness in fieldtype of Tuple with non-Type params May 20, 2021
@JeffBezanson JeffBezanson force-pushed the jb/nonstdtupleinf branch 2 times, most recently from b115b2c to 19e5060 Compare May 25, 2021 19:11
@vtjnash vtjnash merged commit 18df4b5 into master May 28, 2021
@vtjnash vtjnash deleted the jb/nonstdtupleinf branch May 28, 2021 19:55
@KristofferC KristofferC mentioned this pull request Jun 4, 2021
45 tasks
@KristofferC
Copy link
Member

Could this be rebased on #40702? I am not sure what the replacements for jl_is_vararg and jl_vararg_t are.

@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

regression bug in Base.tuple_type_head (1.6.0-RC1 vs 1.5.3)
5 participants