Skip to content

Commit

Permalink
improve checking of Vararg parameters (#32056)
Browse files Browse the repository at this point in the history 
- make sure length var has full bounds
- check for negative length earlier (inside Vararg, not just Tuple{Vararg{}})
  • Loading branch information
@JeffBezanson
JeffBezanson authored May 21, 2019
1 parent 62a0a3f commit 516067b
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 4 deletions.
16 changes: 12 additions & 4 deletions src/jltypes.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1116,8 +1116,17 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value

if (!istuple) {
if (jl_is_vararg_type((jl_value_t*)dt) && ntp == 2) {
if (!jl_is_long(iparams[1]) && !jl_is_typevar(iparams[1])) {
jl_type_error_rt("Vararg", "count", (jl_value_t*)jl_long_type, iparams[1]);
jl_value_t *lenparam = iparams[1];
if (jl_is_typevar(lenparam)) {
jl_tvar_t *N = (jl_tvar_t*)lenparam;
if (!(N->lb == jl_bottom_type && N->ub == (jl_value_t*)jl_any_type))
jl_error("TypeVar in Vararg length must have bounds Union{} and Any");
}
else if (!jl_is_long(lenparam)) {
jl_type_error_rt("Vararg", "count", (jl_value_t*)jl_long_type, lenparam);
}
else if (jl_unbox_long(lenparam) < 0) {
jl_errorf("Vararg length is negative: %zd", jl_unbox_long(lenparam));
}
}
// check parameters against bounds in type definition
Expand Down Expand Up @@ -1159,8 +1168,7 @@ static jl_value_t *inst_datatype_inner(jl_datatype_t *dt, jl_svec_t *p, jl_value
}
if (jl_is_long(va1)) {
ssize_t nt = jl_unbox_long(va1);
if (nt < 0)
jl_errorf("apply_type: Vararg length N is negative: %zd", nt);
assert(nt >= 0);
if (nt == 0 || !jl_has_free_typevars(va0)) {
if (cacheable) JL_UNLOCK(&typecache_lock); // Might GC
if (ntp == 1) {
Expand Down
5 changes: 5 additions & 0 deletions test/core.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3488,6 +3488,11 @@ end
@test_throws ErrorException NTuple{-1, Int}
@test_throws TypeError Union{Int, 1}

@test_throws ErrorException Vararg{Any,-2}
@test_throws ErrorException Vararg{Int, N} where N<:T where T
@test_throws ErrorException Vararg{Int, N} where N<:Integer
@test_throws ErrorException Vararg{Int, N} where N>:Integer

mutable struct FooNTuple{N}
z::Tuple{Integer, Vararg{Int, N}}
end
Expand Down

4 comments on commit 516067b

@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)

@chriselrod
Copy link
Contributor

Choose a reason for hiding this comment

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

Before this PR:

julia> using AxisAlgorithms
[ Info: Precompiling AxisAlgorithms [13072b0f-2c55-5437-9ae7-d433b7a33950]

julia> versioninfo()
Julia Version 1.3.0-DEV.262
Commit 62a0a3f6cb (2019-05-21 19:50 UTC)

After:

julia> using AxisAlgorithms
[ Info: Recompiling stale cache file /home/chriselrod/.julia/compiled/v1.3/AxisAlgorithms/lW7vi.ji for AxisAlgorithms [13072b0f-2c55-5437-9ae7-d433b7a33950]
ERROR: LoadError: LoadError: TypeVar in Vararg length must have bounds Union{} and Any
Stacktrace:
 [1] top-level scope at /home/chriselrod/.julia/packages/AxisAlgorithms/m2HBJ/src/tridiag.jl:21
 [2] include at ./boot.jl:328 [inlined]
 [3] include_relative(::Module, ::String) at ./loading.jl:1094
 [4] include at ./Base.jl:31 [inlined]
 [5] include(::String) at /home/chriselrod/.julia/packages/AxisAlgorithms/m2HBJ/src/AxisAlgorithms.jl:3
 [6] top-level scope at /home/chriselrod/.julia/packages/AxisAlgorithms/m2HBJ/src/AxisAlgorithms.jl:46
 [7] include at ./boot.jl:328 [inlined]
 [8] include_relative(::Module, ::String) at ./loading.jl:1094
 [9] include(::Module, ::String) at ./Base.jl:31
 [10] top-level scope at none:2
 [11] eval at ./boot.jl:330 [inlined]
 [12] eval(::Expr) at ./client.jl:433
 [13] top-level scope at ./none:3
in expression starting at /home/chriselrod/.julia/packages/AxisAlgorithms/m2HBJ/src/tridiag.jl:21
in expression starting at /home/chriselrod/.julia/packages/AxisAlgorithms/m2HBJ/src/AxisAlgorithms.jl:46
ERROR: Failed to precompile AxisAlgorithms [13072b0f-2c55-5437-9ae7-d433b7a33950] to /home/chriselrod/.julia/compiled/v1.3/AxisAlgorithms/lW7vi.ji.
Stacktrace:
 [1] error(::String) at ./error.jl:33
 [2] compilecache(::Base.PkgId, ::String) at ./loading.jl:1253
 [3] _require(::Base.PkgId) at ./loading.jl:1013
 [4] require(::Base.PkgId) at ./loading.jl:911
 [5] require(::Module, ::Symbol) at ./loading.jl:906

julia> versioninfo()
Julia Version 1.3.0-DEV.263
Commit 516067b452 (2019-05-21 19:52 UTC)

Here is the line in AxisAlgorithms.jl:

function _A_ldiv_B_md!(dest, F::LU{T,<:Tridiagonal{T}}, src, R1::CartesianIndices{<:CartesianIndex{0}}, R2::CartesianIndices) where {T}

Is that the sort of line that should be caught by this PR (ie, should I file an issue there), or is this an unintended regression?

@JeffBezanson
Copy link
Member Author

Choose a reason for hiding this comment

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

@kshyatt reported that on slack. Indeed the new error is correct and useful; CartesianIndices{<:CartesianIndex{0}} is no longer valid. I think it should be CartesianIndices{0}.

@chriselrod
Copy link
Contributor

Choose a reason for hiding this comment

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

That fixed it! timholy/AxisAlgorithms.jl#12 (comment)

Please sign in to comment.