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

WIP: fix #52385 by supporting Union{} inside Tuple{} #54792

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

Conversation

JeffBezanson
Copy link
Member

@JeffBezanson JeffBezanson commented Jun 13, 2024

This fixes the helpful example from #52385 (comment). More examples would be great.

This changes the "core" intersection algorithm to return e.g. Tuple{Union{}} for disjoint tuple elements; however there are wrappers around it that still give Union{} since that is the result wanted for most use cases, e.g. argument types and types of values generally. The subtype.jl tests call the "core" algorithm as _type_intersect which I have disabled for now. It will need separate tests.

fix #52385

@JeffBezanson JeffBezanson added types and dispatch Types, subtyping and method dispatch needs tests Unit tests are required for this change labels Jun 13, 2024
@JeffBezanson
Copy link
Member Author

Note most of the changes in 35e4a1f are generally good, so it doesn't need to be reverted.

@@ -4386,6 +4381,24 @@ static int might_intersect_concrete(jl_value_t *a)
return 0;
}

static int is_uninhabited_tuple_type(jl_value_t *t)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we memoize this property in the tuple type?

@JeffBezanson
Copy link
Member Author

I think this is the way we want to go, but messing with type intersection is pretty risky so I don't think we can merge this into 1.11 this late in the cycle.

@vtjnash
Copy link
Member

vtjnash commented Jun 23, 2024

The optimization in #49393 needs to be reverted, as the TODO comment from before that PR is not a legal outcome if this PR is merged.

The usages of the predicates for isconcretetype, isleaftype, isdispatchtuple, and the maybe_subtype_of_cache computations need to be looked at for correctness, as they no longer can be used to refine inference / typemap / subtyping results for Tuple (via the assumption that a leaf-type result implies there is no subtypes of that to examine). It doesn't look like we fully took advantage of the error existence yet for some of those, so some of them may not require many tweaks.

The methods lookup code also may need some work, as it may presuppose that the above restriction was true of all inputs. In particular, code may have assumed that S <: T implies that methods(S) ⊆ methods(T), which I think should imply that there needs to be methods(S) == ∅ if S is a Tuple with a Union{} element. This does not agree with the normal subtyping/intersection definition of method table lookup for S, so it may need to be a special case of some sort.

Could you supply some numbers, once those are fixed, how much this affects the size and performance of OmniPackage and system images?

@JeffBezanson
Copy link
Member Author

should imply that there needs to be methods(S) == ∅ if S is a Tuple with a Union{} element

That seems totally fine to me; in the context of methods we know an argument being Union{} is never actually possible so there ought to be many places where we can prohibit and don't need to handle such types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Unit tests are required for this change types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Tuple field type cannot be Union{}" error in all 1.10 and 1.11 versions
3 participants