Skip to content

Fix intersect error reported in #41096 #46350

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 8 commits into from
Aug 18, 2022
Merged

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Aug 15, 2022

MWE

julia> A = Val{<:Union{Nothing, Val{true},Val{false}}}
Val{<:Union{Val{true}, Val{false}, Nothing}}

julia> B = Val{Val{B}} where {B}
Val{Val{B}} where B

julia> (Base.typeintersect(B, A))
Val{<:Union{Val{true}, Val{false}}}  # Val{Val{true}} before this PR

This PR makes sure env is restored between every 2 adjacent intersects during intersect_all.
The valid env changes are tracked (and merged) with a new function merge_env.
Not sure this is the correct direction to fix the problem.

Close #41096.
Edit: looks like this PR also fixes #43082.

@N5N3 N5N3 added the types and dispatch Types, subtyping and method dispatch label Aug 15, 2022
@Keno Keno requested a review from JeffBezanson August 15, 2022 04:42
@Keno Keno added the needs tests Unit tests are required for this change label Aug 15, 2022
@N5N3 N5N3 removed the needs tests Unit tests are required for this change label Aug 15, 2022
@JeffBezanson
Copy link
Member

Wow, this is great. Seems right to me, and other than that I go by the tests. I see one test was changed, but it's ok because the results are more conservative, so I'm happy with that. Thank you, this code is very tricky!

@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.

N5N3 and others added 2 commits August 18, 2022 18:34
Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
@N5N3

This comment was marked as outdated.

@nanosoldier

This comment was marked as outdated.

@N5N3
Copy link
Member Author

N5N3 commented Aug 18, 2022

@nanosoldier runtests(["CairoMakie", "CountdownNumbers", "FlameGraphs", "Folds", "PProf", "Pidfile", "ReinforcementLearningExperiments", "RetroCap", "RoundAndSwap", "StrBase"], vs = ":master")

@nanosoldier
Copy link
Collaborator

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

@N5N3 N5N3 merged commit 9aabb4c into JuliaLang:master Aug 18, 2022
@N5N3 N5N3 deleted the intersect_fix2 branch August 18, 2022 18:48
@N5N3 N5N3 added backport 1.6 Change should be backported to release-1.6 backport 1.8 Change should be backported to release-1.8 labels Aug 18, 2022
KristofferC pushed a commit that referenced this pull request Aug 26, 2022
… tries a new Union decision (#46350)

* `intersect_all` should always `restore_env`. let `merge_env` track valid `env` change.

* Add test.

Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>
(cherry picked from commit 9aabb4c)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.6 Change should be backported to release-1.6 types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect inference in MTK type-intersection env may fail merging of multiple value matches
5 participants