Skip to content

Commit

Permalink
fix an invariance bug in limit_type_depth. part of #23786 (#23800)
Browse files Browse the repository at this point in the history
  • Loading branch information
@JeffBezanson
JeffBezanson committed Sep 21, 2017
1 parent 896d599 commit ff9fb48
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 3 deletions.
11 changes: 8 additions & 3 deletions base/inference.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,7 @@ function limit_type_depth(@nospecialize(t), d::Int, cov::Bool, vars::Vector{Type
end
ub = Any
else
ub = limit_type_depth(v.ub, d - 1, true)
ub = limit_type_depth(v.ub, d - 1, cov, vars)
end
if v.lb === Bottom || type_depth(v.lb) > d
# note: lower bounds need to be widened by making them lower
Expand All @@ -855,7 +855,8 @@ function limit_type_depth(@nospecialize(t), d::Int, cov::Bool, vars::Vector{Type
if d < 0
if isvarargtype(t)
# never replace Vararg with non-Vararg
return Vararg{limit_type_depth(P[1], d, cov, vars), P[2]}
# passing depth=0 avoids putting a bare typevar here, for the diagonal rule
return Vararg{limit_type_depth(P[1], 0, cov, vars), P[2]}
end
widert = t.name.wrapper
if !(t <: widert)
Expand All @@ -870,7 +871,11 @@ function limit_type_depth(@nospecialize(t), d::Int, cov::Bool, vars::Vector{Type
return var
end
stillcov = cov && (t.name === Tuple.name)
Q = map(x -> limit_type_depth(x, d - 1, stillcov, vars), P)
newdepth = d - 1
if isvarargtype(t)
newdepth = max(newdepth, 0)
end
Q = map(x -> limit_type_depth(x, newdepth, stillcov, vars), P)
R = t.name.wrapper{Q...}
if cov && !stillcov
for var in vars
Expand Down
7 changes: 7 additions & 0 deletions test/inference.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1218,3 +1218,10 @@ let c(::Type{T}, x) where {T<:Array} = T,
f() = c(Vector{Any[Int][1]}, [1])
@test f() === Vector{Int}
end

# issue #23786
struct T23786{D<:Tuple{Vararg{Vector{T} where T}}, N}
end
let t = Tuple{Type{T23786{D, N} where N where D<:Tuple{Vararg{Array{T, 1} where T, N} where N}}}
@test Core.Inference.limit_type_depth(t, 4) >: t
end

9 comments on commit ff9fb48

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @ararslan

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

ERROR: LoadError: must be a toplevel module
Stacktrace:
 [1] addrequire(::JLD.JldFile, ::Module) at /home/nanosoldier/.julia/v0.7/JLD/src/JLD.jl:1250
 [2] (::getfield(BenchmarkTools, Symbol("##25#26")))(::JLD.JldFile) at /home/nanosoldier/.julia/v0.7/BenchmarkTools/src/serialization.jl:38
 [3] #jldopen#11(::Array{Any,1}, ::Function, ::getfield(BenchmarkTools, Symbol("##25#26")), ::String, ::Vararg{Any,N} where N) at /home/nanosoldier/.julia/v0.7/JLD/src/JLD.jl:243
 [4] save(::String, ::String, ::Vararg{Any,N} where N) at /home/nanosoldier/.julia/v0.7/BenchmarkTools/src/serialization.jl:37
 [5] include_relative(::Module, ::String) at ./loading.jl:526
 [6] include(::Module, ::String) at ./sysimg.jl:14
 [7] process_options(::Base.JLOptions) at ./client.jl:315
 [8] _start() at ./client.jl:383
in expression starting at /home/nanosoldier/workdir/tmpMuc2Bn/benchscript.jl:28

@ararslan
Copy link
Member

Choose a reason for hiding this comment

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

It's being triggered by

module_parent(mod) == Main || error("must be a toplevel module")

I wonder if this is because packages are no longer loaded into Main by default.

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

Yep, probably: #23579 (comment)

@ararslan
Copy link
Member

Choose a reason for hiding this comment

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

Looks like the check should probably be

module_parent(mod) == Main || Base.is_root_module(mod) || error("...")

The breaking change was that the parent module of a loaded package is itself, not Main anymore.

@ararslan
Copy link
Member

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, isdaily=true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@ararslan
Copy link
Member

Choose a reason for hiding this comment

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

He is risen

Please sign in to comment.