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

fix #40814, fix type inference regression introduced in #40379 #40987

Closed
wants to merge 1 commit into from

Conversation

aviatesk
Copy link
Member

Fixes the inference regression, while retaining the cases addressed by
#40379.

The same Type-name comparison pass (i.e. a branch of isa(c, DataType) && t.name === c.name)
can lead to more accurate result by recursive comparison than isType
check pass (i.e. a branch of isType(t)), and preferring the former
over the latter fixes the regression.

But just swapping the branch will lead to #40336,
and so this PR also implements additional check to make sure type_more_complex
still detects a single-level nesting correctly (especially, the tt === c parts).

@aviatesk aviatesk requested a review from vtjnash May 28, 2021 13:38
@oscardssmith oscardssmith added this to the 1.7 milestone May 28, 2021
Fixes the inference regression, while retaining the cases addressed by
<#40379>.

The same `Type`-name comparison pass (i.e. a branch of `isa(c, DataType) && t.name === c.name`)
can lead to more accurate result by recursive comparison than `isType`
check pass (i.e. a branch of `isType(t)`), and preferring the former
over the latter fixes the regression.

But just swapping the branch will lead to <#40336>,
and so this PR also implements additional check to make sure `type_more_complex`
still detects a single-level nesting correctly (especially, the `tt === c` parts).
@aviatesk

This comment has been minimized.

@nanosoldier

This comment has been minimized.

@aviatesk
Copy link
Member Author

aviatesk commented Jun 1, 2021

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG

@aviatesk
Copy link
Member Author

aviatesk commented Jun 1, 2021

The benchmark results look okay to me.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

This appears to get the check exactly backwards: while the test in this function is supposed to find a reason that the recursion is safe, this instead looks if the recursion matches a known reason it is unsafe. As such, this would seem to be neither total nor transitive.

@vtjnash vtjnash removed this from the 1.7 milestone Jun 2, 2021
aviatesk added a commit that referenced this pull request Jun 3, 2021
@aviatesk
Copy link
Member Author

aviatesk commented Jun 3, 2021

while the test in this function is supposed to find a reason that the recursion is safe, this instead looks if the recursion matches a known reason it is unsafe. As such, this would seem to be neither total nor transitive.

Hmm, I'm not quite sure what you mean by this. Why should inference always be conservative when seeing Type in a recursion ?

The comment of limit_type_size says:

# limit the complexity of type `t` to be simpler than the comparison type `compare`
# no new values may be introduced, so the parameter `source` encodes the set of all values already present
# the outermost tuple type is permitted to have up to `allowed_tuplelen` parameters
function limit_type_size(@nospecialize(t), @nospecialize(compare), @nospecialize(source), allowed_tupledepth::Int, allowed_tuplelen::Int)

and I'm not sure why we should give up the comparison pass (i.e. elseif isa(c, DataType) && t.name === c.name) when isType(t).
Even if we go with the comparison pass first, eventually a single-level complexity difference (e.g. Type{Type{Int}} vs. Type{Int} will be caught by newly introduced check tt === c and more level complexity difference will be caught by isType(tt) as before.
Maybe you mean the tt === c part is too ad-hoc to be safe in all the possible cases ?

@vtjnash
Copy link
Member

vtjnash commented Jun 3, 2021

We could delete the case for Type entirely, but we cannot move it without re-introducing the previous bug that PR fixed. The question is mainly which rule we like better. The implementation of _totuple currently uses the tuple_type_tail function, which we probably should deprecate, as it is causing _totuple to make inaccurate subtyping claims. I haven't yet been able to find a better formulation however, so we might need better compiler support first for Core._apply.

aviatesk added a commit that referenced this pull request Jun 6, 2021
shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
@aviatesk aviatesk closed this Jun 28, 2021
@aviatesk aviatesk deleted the avi/fix40814 branch June 28, 2021 04:39
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants