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

Intersect: try normal+reverse+existential subtyping during intersection #57476

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Feb 20, 2025

Typevars are all existential (in the sense of variable lb/ub) during intersection. This fix is somehow costly as we have to perform 3 times check to prove a false result. But a single existential <: seems too dangerous as it cause many circular env in the past.
fix #57429
fix #41561

@N5N3 N5N3 added types and dispatch Types, subtyping and method dispatch bugfix This change fixes an existing bug needs pkgeval Tests for all registered packages should be run with this change labels Feb 20, 2025
@N5N3 N5N3 marked this pull request as draft February 20, 2025 13:58
fix JuliaLang#57429 JuliaLang#41561
Typevars are all existential (in the sense of variable lb/ub) during intersection. This fix is somehow costly as we have to perform 3 times check to prove a false result. But a single existential <: seems too dangerous as it cause many circular env in the past.
@N5N3 N5N3 marked this pull request as ready for review February 20, 2025 14:30
@adienes
Copy link
Contributor

adienes commented Feb 20, 2025

This fix is somehow costly

would it make sense to include an example snippet benchmarking this cost? even if it is a regression it could be tracked for later improvement

@N5N3
Copy link
Member Author

N5N3 commented Feb 21, 2025

I tried some packages, e.g.

julia> @time using DifferentialEquations
10.825558 seconds (19.42 M allocations: 1.040 GiB, 9.68% gc time, 6.35% compilation time)       # before
10.814457 seconds (19.40 M allocations: 1005.084 MiB, 6.78% gc time, 4.93% compilation time) # after

The influence seems negligible here, but not sure if it's representative.

@N5N3
Copy link
Member Author

N5N3 commented Feb 21, 2025

@nanosoldier runtests()

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 needs pkgeval Tests for all registered packages should be run with this change types and dispatch Types, subtyping and method dispatch
Projects
None yet
2 participants