Skip to content

inference: revisit all methods in cycle#59974

Merged
vtjnash merged 2 commits intomasterfrom
jn/59943
Nov 1, 2025
Merged

inference: revisit all methods in cycle#59974
vtjnash merged 2 commits intomasterfrom
jn/59943

Conversation

@vtjnash
Copy link
Copy Markdown
Member

@vtjnash vtjnash commented Oct 27, 2025

When encountering complicated cycles within cycles, make sure to revisit all applicable methods explicitly, since not all methods within the cycle will necessarily change return type after resolving the cycle. This shows up as the possibility of frames that don't get revisited before being cached, probably just caching Union{} as the call type instead. I always assumed this code was probably wrong, but didn't have any way to construct the counter-example to have confidence that fixing it would not cause some other side-effect. But still keep the backedge work lazy, since we don't want to allocate unnecessarily for a rarely used feature (recursion).

Fix #59943

When encountering complicated cycles within cycles, make sure to revisit
all applicable methods explicitly, since not all methods within the
cycle will necessarily change return type after resolving the cycle.
This shows up as the possibility of frames that don't get revisited
before being cached, probably just caching `Union{}` as the call type
instead. I always assumed this code was probably wrong, but didn't have
any way to construct the counter-example to have confidence that fixing
it would not cause some other side-effect. But still keep the backedge
work lazy, since we don't want to allocate unnecessarily for a rarely
used feature (recursion).

Fix #59943
@vtjnash vtjnash added compiler:inference Type inference backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 labels Oct 27, 2025
@oscardssmith oscardssmith added the needs tests Unit tests are required for this change label Oct 27, 2025
@JeffBezanson
Copy link
Copy Markdown
Member

a rarely used feature (recursion)

This is true but this wording is just hilarious 😂

@oscardssmith
Copy link
Copy Markdown
Member

Tested and confirmed that this fixes the MWE from @AayushSabharwa. @vtjnash do you have a simpler mwe now that you know what the problem was?

@vtjnash vtjnash force-pushed the jn/59943 branch 2 times, most recently from 4f4b3c9 to 5fcbcf3 Compare October 29, 2025 20:38
@vtjnash vtjnash added merge me PR is reviewed. Merge when all tests are passing and removed needs tests Unit tests are required for this change labels Oct 29, 2025
@vtjnash
Copy link
Copy Markdown
Member Author

vtjnash commented Oct 29, 2025

You're welcome to write tests. I have no more idea how to write one now than when I did when I didn't write this code years ago.

@vtjnash vtjnash merged commit f4847bf into master Nov 1, 2025
5 of 7 checks passed
@vtjnash vtjnash deleted the jn/59943 branch November 1, 2025 02:52
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Nov 6, 2025
KristofferC pushed a commit that referenced this pull request Nov 7, 2025
When encountering complicated cycles within cycles, make sure to revisit
all applicable methods explicitly, since not all methods within the
cycle will necessarily change return type after resolving the cycle.
This shows up as the possibility of frames that don't get revisited
before being cached, probably just caching `Union{}` as the call type
instead. I always assumed this code was probably wrong, but didn't have
any way to construct the counter-example to have confidence that fixing
it would not cause some other side-effect. But still keep the backedge
work lazy, since we don't want to allocate unnecessarily for a rarely
used feature (recursion).

Fix #59943

(cherry picked from commit f4847bf)
@KristofferC KristofferC mentioned this pull request Nov 7, 2025
35 tasks
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Nov 21, 2025
kpamnany pushed a commit to RelationalAI/julia that referenced this pull request Jan 21, 2026
When encountering complicated cycles within cycles, make sure to revisit
all applicable methods explicitly, since not all methods within the
cycle will necessarily change return type after resolving the cycle.
This shows up as the possibility of frames that don't get revisited
before being cached, probably just caching `Union{}` as the call type
instead. I always assumed this code was probably wrong, but didn't have
any way to construct the counter-example to have confidence that fixing
it would not cause some other side-effect. But still keep the backedge
work lazy, since we don't want to allocate unnecessarily for a rarely
used feature (recursion).

Fix JuliaLang#59943

(cherry picked from commit f4847bf)
@KristofferC KristofferC mentioned this pull request Feb 20, 2026
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 compiler:inference Type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inference bug leading to segfault in Julia 1.10-1.12

5 participants