inference: fix recursion limit heuristic #28517
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There's several related issues here:
1. Covariant kinds provide no meaningful information for the type size
limit heuristic. We should be just ignoring those and walking through to
the actual structural information of any real substance.
This seems to also make type_more_complex, _limit_type_size, and
is_derived_type more similar, which is probably always preferable.
2. Allowing type intersection to revise our type later can
cause it to reintroduce complexity (constraints) according to our metric,
while we want to only move up the type-hierarchy,
which can allow us to accidentally escape from the limit.
So if we've limited the type, we don't want to use a different type
that was derived by type-intersection and may not have our limits
applied anymore.
3. Inside `is_derived_type`, the var field of a UnionAll
should not increase the nesting depth, since `T<:(Ref{S} where S<:Any)`,
should observe the same depth as `{S<:Any, T<:Ref{S}}`.
fix #26665
vtjnash
added
bugfix
|
Thanks! Will there be another release candidate for 0.7? If so, could this be backported? |
No, the final 0.7.0 has already been tagged. |
|
Obvious next question: Will there be 1.0-rc2? |
No, but this will go into 1.0 final. |
|
If you end up needing a version of 0.7 (to get deprecation warnings) that has this bug fix, we can release a 0.7.1 at some point. |
|
Indeed, I was thinking about the deprecation warnings. Thanks. |
|
PkgEval shows the same set of packages passing as on master. Merging. |
|
Thanks again for fixing this Jameson. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's several related issues here:
Covariant kinds provide no meaningful information for the type size
limit heuristic. We should be just ignoring those and walking through to
the actual structural information of any real substance.
This seems to also make type_more_complex, _limit_type_size, and
is_derived_type more similar, which is probably always preferable.
Allowing type intersection to revise our type later can
cause it to reintroduce complexity (constraints) according to our metric,
while we want to only move up the type-hierarchy,
which can allow us to accidentally escape from the limit.
So if we've limited the type, we don't want to use a different type
that was derived by type-intersection and may not have our limits
applied anymore.
Inside
is_derived_type, the var field of a UnionAllshould not increase the nesting depth, since
T<:(Ref{S} where S<:Any),should observe the same depth as
{S<:Any, T<:Ref{S}}.fix #26665