Skip to content

Commit

Permalink
improve errors for type application T{...}
Browse files Browse the repository at this point in the history 
replaces #12805

- add error for Tuple{x} where x isn't a Type
- add error for invalid Tuple and Union types constructed via a
  parameterized typealias
- improve error messages
  • Loading branch information
@JeffBezanson
JeffBezanson committed Sep 2, 2015
1 parent 4691d2b commit 85f4597
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 38 deletions.
22 changes: 2 additions & 20 deletions src/builtins.c
View file
Expand Up @@ -1028,26 +1028,8 @@ JL_CALLABLE(jl_trampoline)
JL_CALLABLE(jl_f_instantiate_type)
{
JL_NARGSV(instantiate_type, 1);
if (args[0] == (jl_value_t*)jl_uniontype_type) {
for(size_t i=1; i < nargs; i++) {
jl_value_t* ty = args[i];
if ((!jl_is_type(ty) && !jl_is_typevar(ty)) || jl_is_vararg_type(ty)) {
jl_type_error_rt("apply_type", "parameter of Union",
(jl_value_t*)jl_type_type, args[i]);
}
}
}
else if (args[0] == (jl_value_t*)jl_tuple_type) {
for(size_t i=1; i < nargs; i++) {
jl_value_t* ty = args[i];
if (jl_is_vararg_type(ty) && i != nargs-1) {
jl_type_error_rt("apply_type", "parameter of Tuple",
(jl_value_t*)jl_type_type, args[i]);
}
}
}
else if (!jl_is_datatype(args[0])) {
JL_TYPECHK(instantiate_type, typector, args[0]);
if (!jl_is_datatype(args[0]) && !jl_is_typector(args[0])) {
jl_type_error("Type{...} expression", (jl_value_t*)jl_type_type, args[0]);
}
return jl_apply_type_(args[0], &args[1], nargs-1);
}
Expand Down
8 changes: 7 additions & 1 deletion src/codegen.cpp
View file
Expand Up @@ -1347,7 +1347,13 @@ jl_value_t *jl_static_eval(jl_value_t *ex, void *ctx_, jl_module_t *mod,
return NULL;
}
}
jl_value_t *result = fptr(f, v, n);
jl_value_t *result;
JL_TRY {
result = fptr(f, v, n);
}
JL_CATCH {
result = NULL;
}
JL_GC_POP();
return result;
}
Expand Down
48 changes: 35 additions & 13 deletions src/jltypes.c
View file
Expand Up @@ -281,6 +281,12 @@ static jl_svec_t *jl_compute_type_union(jl_value_t **types, size_t ntypes)
jl_value_t *jl_type_union_v(jl_value_t **ts, size_t n)
{
if (n == 0) return (jl_value_t*)jl_bottom_type;
size_t i;
for(i=0; i < n; i++) {
jl_value_t *pi = ts[i];
if (!(jl_is_type(pi) || jl_is_typevar(pi)) || jl_is_vararg_type(pi))
jl_type_error_rt("Union", "parameter", (jl_value_t*)jl_type_type, pi);
}
if (n == 1) return ts[0];
jl_svec_t *types = jl_compute_type_union(ts, n);
if (jl_svec_len(types) == 0) return (jl_value_t*)jl_bottom_type;
Expand Down Expand Up @@ -821,6 +827,8 @@ static jl_value_t *approxify_type(jl_datatype_t *dt, jl_svec_t *pp)

static int has_ntuple_intersect_tuple = 0;

static jl_datatype_t *inst_tupletype_unchecked_uncached(jl_svec_t *p);

static jl_value_t *jl_type_intersect(jl_value_t *a, jl_value_t *b,
cenv_t *penv, cenv_t *eqc, variance_t var)
{
Expand Down Expand Up @@ -1054,8 +1062,8 @@ static jl_value_t *jl_type_intersect(jl_value_t *a, jl_value_t *b,
int prev_mim = match_intersection_mode;
match_intersection_mode = 1;
// TODO get rid of these intermediate tuple types
p = (jl_value_t*)jl_apply_tuple_type(super->parameters);
temp3 = (jl_value_t*)jl_apply_tuple_type(subs_sup_params);
p = (jl_value_t*)inst_tupletype_unchecked_uncached(super->parameters);
temp3 = (jl_value_t*)inst_tupletype_unchecked_uncached(subs_sup_params);
env = jl_type_match(p, temp3);
int sub_needs_parameters = 0;
if (env == jl_false) {
Expand Down Expand Up @@ -1689,7 +1697,7 @@ jl_value_t *jl_apply_type_(jl_value_t *tc, jl_value_t **params, size_t n)
jl_datatype_t *stprimary = NULL;
if (jl_is_typector(tc)) {
tp = ((jl_typector_t*)tc)->parameters;
tname = "alias";
tname = "typealias";
}
else {
assert(jl_is_datatype(tc));
Expand All @@ -1700,7 +1708,7 @@ jl_value_t *jl_apply_type_(jl_value_t *tc, jl_value_t **params, size_t n)
for(i=0; i < n; i++) {
jl_value_t *pi = params[i];
if (!valid_type_param(pi)) {
jl_type_error_rt("apply_type", tname,
jl_type_error_rt(tname, "parameter",
jl_subtype(pi, (jl_value_t*)jl_number_type, 1) ?
(jl_value_t*)jl_long_type : (jl_value_t*)jl_type_type,

This comment has been minimized.

Sign in to view

Copy link
@vtjnash

vtjnash Sep 3, 2015

Member

the jl_long_type here is a bit odd, since this doesn't really correspond to valid_type_param
pi);
Expand All @@ -1712,11 +1720,10 @@ jl_value_t *jl_apply_type_(jl_value_t *tc, jl_value_t **params, size_t n)
if (!jl_get_size(params[0], &nt)) {
// Only allow Int or TypeVar as the first parameter to
// NTuple. issue #9233
jl_type_error_rt("apply_type", "first parameter of NTuple",
jl_type_error_rt("NTuple", "parameter 1",
(jl_value_t*)jl_long_type, params[0]);
}
return jl_tupletype_fill(nt, (n==2) ? params[1] :
(jl_value_t*)jl_any_type);
return jl_tupletype_fill(nt, (n==2) ? params[1] : (jl_value_t*)jl_any_type);
}
}
size_t ntp = jl_svec_len(tp);
Expand Down Expand Up @@ -2116,16 +2123,24 @@ static jl_value_t *inst_datatype(jl_datatype_t *dt, jl_svec_t *p, jl_value_t **i
return (jl_value_t*)ndt;
}

static void check_tuple_parameter(jl_value_t *pi, size_t i, size_t np)
{
if (!(jl_is_type(pi) || jl_is_typevar(pi)))
jl_type_error_rt("Tuple", "parameter", (jl_value_t*)jl_type_type, pi);
if (i != np-1 && jl_is_vararg_type(pi))
jl_type_error_rt("Tuple", "non-final parameter", (jl_value_t*)jl_type_type, pi);
}

static jl_tupletype_t *jl_apply_tuple_type_v_(jl_value_t **p, size_t np, jl_svec_t *params)
{
int isabstract = 0, cacheable = 1;
for(size_t i=0; i < np; i++) {
if (!jl_is_leaf_type(p[i]))
jl_value_t *pi = p[i];
check_tuple_parameter(pi, i, np);
if (!jl_is_leaf_type(pi))
isabstract = 1;
if (jl_has_typevars_(p[i],0))
if (jl_has_typevars_(pi,0))
cacheable = 0;
if (i != np-1 && jl_is_vararg_type(p[i]))
jl_error("Vararg type in non-final position");
}
cacheable &= (!isabstract);
jl_datatype_t *ndt = (jl_datatype_t*)inst_datatype(jl_anytuple_type, params, p, np,
Expand Down Expand Up @@ -2153,6 +2168,11 @@ jl_datatype_t *jl_inst_concrete_tupletype_v(jl_value_t **p, size_t np)
return (jl_datatype_t*)inst_datatype(jl_anytuple_type, NULL, p, np, 1, 0, NULL, NULL, 0);
}

static jl_datatype_t *inst_tupletype_unchecked_uncached(jl_svec_t *p)
{
return (jl_datatype_t*)inst_datatype(jl_anytuple_type, p, jl_svec_data(p), jl_svec_len(p), 0, 1, NULL, NULL, 0);
}

static jl_svec_t *inst_all(jl_svec_t *p, jl_value_t **env, size_t n,
jl_typestack_t *stack, int check)
{
Expand Down Expand Up @@ -2190,10 +2210,12 @@ static jl_value_t *inst_tuple_w_(jl_value_t *t, jl_value_t **env, size_t n,
for(i=0; i < ntp; i++) {
jl_value_t *elt = jl_svecref(tp, i);
iparams[i] = (jl_value_t*)inst_type_w_(elt, env, n, stack, 0);
if (!isabstract && !jl_is_leaf_type(iparams[i])) {
jl_value_t *pi = iparams[i];
check_tuple_parameter(pi, i, ntp);
if (!isabstract && !jl_is_leaf_type(pi)) {
cacheable = 0; isabstract = 1;
}
if (cacheable && jl_has_typevars_(iparams[i],0))
if (cacheable && jl_has_typevars_(pi,0))
cacheable = 0;
}
jl_value_t *result = inst_datatype((jl_datatype_t*)tt, ip_heap, iparams, ntp, cacheable, isabstract,
Expand Down
13 changes: 11 additions & 2 deletions test/core.jl
View file
Expand Up @@ -3100,12 +3100,12 @@ Base.convert(::Type{Foo11874},x::Int) = float(x)
# issue #9233
let
err = @test_throws TypeError NTuple{Int, 1}
@test err.func == :apply_type
@test err.func == :NTuple
@test err.expected == Int
@test err.got == Int

err = @test_throws TypeError NTuple{0x1, Int}
@test err.func == :apply_type
@test err.func == :NTuple
@test err.expected == Int
@test err.got == 0x1
end
Expand Down Expand Up @@ -3245,6 +3245,15 @@ end
@test_throws TypeError Tuple{Vararg{Int32},Int64,Float64}
@test_throws TypeError Tuple{Int64,Vararg{Int32},Float64}

# don't allow non-types in Tuple or Union
@test_throws TypeError Tuple{1}
@test_throws TypeError Union{1}
@test_throws TypeError Union{Int,0}
typealias PossiblyInvalidUnion{T} Union{T,Int}
@test_throws TypeError PossiblyInvalidUnion{1}
typealias PossiblyInvalidTuple{T} Tuple{T,Int}
@test_throws TypeError PossiblyInvalidTuple{""}

# issue #12551 (make sure these don't throw in inference)
Base.return_types(unsafe_load, (Ptr{nothing},))
Base.return_types(getindex, (Vector{nothing},))
Expand Down
4 changes: 2 additions & 2 deletions test/replutil.jl
View file
Expand Up @@ -129,6 +129,6 @@ let
buff = IOBuffer()
showerror(buff, MethodError(convert, (3, 1.0)))
showerror(buff, MethodError(convert, (Int, 1.0)))
showerror(buff, MethodError(convert, Tuple{Type, 1.0}))
showerror(buff, MethodError(convert, Tuple{DataType, 1.0}))
showerror(buff, MethodError(convert, Tuple{Type, Float64}))
showerror(buff, MethodError(convert, Tuple{DataType, Float64}))
end

9 comments on commit 85f4597

@yuyichao
Copy link
Contributor

Choose a reason for hiding this comment

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

  • add error for Tuple{x} where x isn't a Type

I guess you mean Type{x} ? (doesn't matter much either way...)

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

No; general types allow non-type parameters like T{1}, but tuples shouldn't.

@SimonDanisch
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks my code and I'm not sure how to fix it:

abstract FixedArray{T, NDim, SIZE}
typealias FixedMatrix{Row, Column, T} FixedArray{T, 2, Tuple{Row, Column}}
immutable Mat{Row, Column, T} <: FixedMatrix{Row, Column, T}
    _::NTuple{Column, NTuple{Row, T}}
end
Mat(((1,2,3),)) # ERROR: TypeError: Tuple: in parameter, expected Type{T}, got Int64
abstract FixedArray{T, NDim, SIZE}
typealias FixedMatrix{Row, Column, T} FixedArray{T, 2, (Row, Column)}
ERROR: TypeError: FixedArray: in parameter, expected Type{T}, got Tuple{TypeVar,TypeVar}

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

Should we really allow Tuple{1, 2}?

Only solution I can think of so far is to use (Row, Col) to represent the size of both FixedArray and FixedMatrix.

@SimonDanisch
Copy link
Contributor

Choose a reason for hiding this comment

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

Well as I point out it does not work for the typealias, which is pretty substantial to do all kind of things...Also it fails for TypeVar, which makes these kind of things impossible:

row{R, T}(a::Mat{R, 1, T}, j::Int) = (a.(1)[1][j],)

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

This should work:

abstract D{x, rest}
abstract Nil

typealias FixedMatrix{Row, Column, T} FixedArray{T, 2, D{Row, D{Column, Nil}}}

Maybe I should allow Tuple{2} again to minimize breakage. I'm not sure it's a good idea though.

@SimonDanisch
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems verbose and makes it hard to work with arbitrary dimensioned arrays. Is it that bad to have Tuple{2} ?

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

Ok in 877b088 I have rolled back the most breaking part of this.

@SimonDanisch
Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thank you!

Please sign in to comment.