Skip to content

fix #33370, avoid collisions between gensyms and anon function names #33426

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 3, 2019
Merged

fix #33370, avoid collisions between gensyms and anon function names #33426

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion src/datatype.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ extern "C" {

// allocating TypeNames -----------------------------------------------------------

static int is10digit(char c) JL_NOTSAFEPOINT
{
return (c >= '0' && c <= '9');
}

jl_sym_t *jl_demangle_typename(jl_sym_t *s) JL_NOTSAFEPOINT
{
char *n = jl_symbol_name(s);
Expand All @@ -30,6 +35,8 @@ jl_sym_t *jl_demangle_typename(jl_sym_t *s) JL_NOTSAFEPOINT
len = strlen(n) - 1;
else
len = (end-n) - 1; // extract `f` from `#f#...`
if (is10digit(n[1]))
return jl_symbol_n(n, len+1);
return jl_symbol_n(&n[1], len);
}

Expand Down Expand Up @@ -448,6 +455,14 @@ void jl_compute_field_offsets(jl_datatype_t *st)
jl_errorf("type %s has field offset %d that exceeds the page size", jl_symbol_name(st->name->name), descsz);
}

static int is_anonfn_typename(char *name)
{
if (name[0] != '#')
return 0;
char *other = strrchr(name, '#');
return (name[1] != '#' && other > &name[1] && is10digit(other[1]));
}

JL_DLLEXPORT jl_datatype_t *jl_new_datatype(
jl_sym_t *name,
jl_module_t *module,
Expand Down Expand Up @@ -487,7 +502,7 @@ JL_DLLEXPORT jl_datatype_t *jl_new_datatype(
}
else {
tn = jl_new_typename_in((jl_sym_t*)name, module);
if (super == jl_function_type || super == jl_builtin_type || jl_symbol_name(name)[0] == '#') {
if (super == jl_function_type || super == jl_builtin_type || is_anonfn_typename(jl_symbol_name(name))) {
// Callable objects (including compiler-generated closures) get independent method tables
// as an optimization
tn->mt = jl_new_method_table(name, module);
Expand Down
13 changes: 10 additions & 3 deletions src/julia-syntax.scm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,8 +432,12 @@
sparams)))
(let ((kw (gensy))
(rkw (if (null? restkw) (make-ssavalue) (symbol (string (car restkw) "..."))))
(mangled (symbol (string (if name (undot-name name) '_) "#"
(current-julia-module-counter)))))
(mangled (let ((und (and name (undot-name name))))
(symbol (string (if (and name (= (string.char (string name) 0) #\#))
""
"#")
(or und '_) "#"
(string (current-julia-module-counter)))))))
;; this is a hack: nest these statements inside a call so they get closure
;; converted together, allowing all needed types to be defined before any methods.
`(call (core ifelse) false false (block
Expand Down Expand Up @@ -3269,7 +3273,10 @@ f(x) = yt(x)
(let* ((exists (get defined name #f))
(type-name (or (get namemap name #f)
(and name
(symbol (string "#" name "#" (current-julia-module-counter))))))
(symbol (string (if (= (string.char (string name) 0) #\#)
""
"#")
name "#" (current-julia-module-counter))))))
(alldefs (expr-find-all
(lambda (ex) (and (length> ex 2) (eq? (car ex) 'method)
(not (eq? ex e))
Expand Down
14 changes: 14 additions & 0 deletions test/core.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2404,6 +2404,20 @@ for f in (:(Core.arrayref), :((::typeof(Core.arrayref))), :((::Core.IntrinsicFun
@test_throws ErrorException("cannot add methods to a builtin function") @eval $f() = 1
end

# issue #33370
abstract type B33370 end

let n = gensym(), c(x) = B33370[x][1]()
Copy link
Member

Choose a reason for hiding this comment

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

For some reason I found the intent of this typed array very confusing at first sight. I think I would have been less confused by c(x) = Ref{B33370}(x)[](). Especially with a comment saying that this is to prevent type inference from determining the concrete type of x and to force a runtime method table lookup. (at least I assume this is roughly the idea.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the idea is to force inference to use the abstract type in its method lookup. At some point we should change all such things in our tests to something like infer_as(x, T).

@eval begin
struct $n <: B33370
end

function (::$n)()
end
end
@test c(eval(n)()) === nothing
end

# issue #8798
let
npy_typestrs = Dict("b1"=>Bool,
Expand Down