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

type inference regression in Base._totuple #40814

Closed
simeonschaub opened this issue May 13, 2021 · 4 comments
Closed

type inference regression in Base._totuple #40814

simeonschaub opened this issue May 13, 2021 · 4 comments
Assignees
Labels
compiler:inference Type inference regression Regression in behavior compared to a previous version
Milestone

Comments

@simeonschaub
Copy link
Member

simeonschaub commented May 13, 2021

This infers correctly in 1.6.1, but doesn't anymore in 1.7:

julia> @code_warntype NTuple{3, Int}(rand(Int, 3))
MethodInstance for Tuple{Int64, Int64, Int64}(::Vector{Int64})
  from (::Type{T})(itr) where T<:Tuple in Base at tuple.jl:314
Static Parameters
  T = Tuple{Int64, Int64, Int64}
Arguments
  #self#::Core.Const(Tuple{Int64, Int64, Int64})
  itr::Vector{Int64}
Body::Tuple{Int64, Int64, Any, Vararg{Any}}
1%1 = Base._totuple($(Expr(:static_parameter, 1)), itr)::Tuple{Int64, Int64, Any, Vararg{Any}}
└──      return %1

#40561 also doesn't seem to fix this. It would be easy enough to work around this with an explicit type annotation, but it would probably be good to figure out which changes in inference caused this.
Discovered in FiniteDifferences.jl's tests.

@simeonschaub simeonschaub added the compiler:inference Type inference label May 13, 2021
@simeonschaub simeonschaub added this to the 1.7 milestone May 13, 2021
@aviatesk aviatesk self-assigned this May 13, 2021
@simeonschaub
Copy link
Member Author

bisected to c2ec70c

@aviatesk
Copy link
Member

The following diff seems to fix this issue, while keeping the cases addressed by the commit. Not too sure this is the right way to go, though.

diff --git a/base/compiler/typelimits.jl b/base/compiler/typelimits.jl
index 2eb2d6f542..87154839db 100644
--- a/base/compiler/typelimits.jl
+++ b/base/compiler/typelimits.jl
@@ -233,10 +233,6 @@ function type_more_complex(@nospecialize(t), @nospecialize(c), sources::SimpleVe
         tP = t.parameters
         if isa(c, Core.TypeofVararg)
             return type_more_complex(t, unwrapva(c), sources, depth, tupledepth, 0)
-        elseif isType(t) # allow taking typeof any source type anywhere as Type{...}, as long as it isn't nesting Type{Type{...}}
-            tt = unwrap_unionall(t.parameters[1])
-            (!isa(tt, DataType) || isType(tt)) && (depth += 1)
-            return !is_derived_type_from_any(tt, sources, depth)
         elseif isa(c, DataType) && t.name === c.name
             cP = c.parameters
             length(cP) < length(tP) && return true
@@ -267,6 +263,10 @@ function type_more_complex(@nospecialize(t), @nospecialize(c), sources::SimpleVe
                 type_more_complex(tPi, cPi, sources, depth + 1, tupledepth, 0) && return true
             end
             return false
+        elseif isType(t) # allow taking typeof any source type anywhere as Type{...}, as long as it isn't nesting Type{Type{...}}
+            tt = unwrap_unionall(t.parameters[1])
+            (!isa(tt, DataType) || isType(tt) || tt === c) && (depth += 1)
+            return !is_derived_type_from_any(tt, sources, depth)
         end
     end
     return true

@JeffBezanson JeffBezanson added the regression Regression in behavior compared to a previous version label May 17, 2021
aviatesk added a commit that referenced this issue 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
Copy link
Member

#40987 will fix this.

aviatesk added a commit that referenced this issue May 31, 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).
@vtjnash
Copy link
Member

vtjnash commented Jun 2, 2021

It looks like _totuple needs to be rewritten. The rewrite of convert for Tuple from last year may supply some guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference regression Regression in behavior compared to a previous version
Projects
None yet
Development

No branches or pull requests

4 participants