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.

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

more predictable global binding resolution #22984

Merged
merged 1 commit into from
Aug 7, 2017
Merged

more predictable global binding resolution #22984

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
9 changes: 9 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ Language changes

+ `bits_type` => `primitive_type`

* The `global` keyword now only introduces a new binding if one doesn't already exist
in the module.
This means that assignment to a global (`global sin = 3`) may now throw the error:
"cannot assign variable Base.sin from module Main", rather than emitting a warning.
Additionally, the new bindings are now created before the statement is executed.
For example, `f() = (global sin = "gluttony"; nothing)` will now resolve which module
contains `sin` eagerly, rather than delaying that decision until `f` is run. ([#22984]).


Breaking changes
----------------

Expand Down
3 changes: 1 addition & 2 deletions base/poll.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,7 @@ mutable struct _FDWatcher
end
end

global uvfinalize
function uvfinalize(t::_FDWatcher)
function Base.uvfinalize(t::_FDWatcher)
if t.handle != C_NULL
disassociate_julia_struct(t)
ccall(:jl_close_uv, Void, (Ptr{Void},), t.handle)
Expand Down
6 changes: 3 additions & 3 deletions doc/src/manual/modules.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,8 @@ in other modules can be invoked as `Mod.@mac` or `@Mod.mac`.
The syntax `M.x = y` does not work to assign a global in another module; global assignment is
always module-local.

A variable can be "reserved" for the current module without assigning to it by declaring it as
`global x` at the top level. This can be used to prevent name conflicts for globals initialized
after load time.
A variable name can be "reserved" without assigning to it by declaring it as `global x`.
This prevents name conflicts for globals initialized after load time.

### Module initialization and precompilation

Expand Down Expand Up @@ -283,6 +282,7 @@ const foo_data_ptr = Ref{Ptr{Void}}(0)
function __init__()
ccall((:foo_init, :libfoo), Void, ())
foo_data_ptr[] = ccall((:foo_data, :libfoo), Ptr{Void}, ())
nothing
end
```

Expand Down
22 changes: 4 additions & 18 deletions doc/src/manual/variables-and-scoping.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,19 +174,6 @@ julia> x
12
```

Within soft scopes, the *global* keyword is never necessary, although allowed. The only case
when it would change the semantics is (currently) a syntax error:

```jldoctest
julia> let
local j = 2
let
global j = 3
end
end
ERROR: syntax: `global j`: j is local variable in the enclosing scope
```

### Hard Local Scope

Hard local scopes are introduced by function definitions (in all their forms), struct type definition blocks,
Expand Down Expand Up @@ -336,8 +323,7 @@ constructing [closures](https://en.wikipedia.org/wiki/Closure_%28computer_progra
have a private state, for instance the `state` variable in the following example:

```jldoctest
julia> let
state = 0
julia> let state = 0
global counter
counter() = state += 1
end;
Expand Down Expand Up @@ -483,13 +469,13 @@ julia> const e = 2.71828182845904523536;
julia> const pi = 3.14159265358979323846;
```

The `const` declaration is allowed on both global and local variables, but is especially useful
for globals. It is difficult for the compiler to optimize code involving global variables, since
The `const` declaration should only be used in global scope on globals.
It is difficult for the compiler to optimize code involving global variables, since
their values (or even their types) might change at almost any time. If a global variable will
not change, adding a `const` declaration solves this performance problem.

Local constants are quite different. The compiler is able to determine automatically when a local
variable is constant, so local constant declarations are not necessary for performance purposes.
variable is constant, so local constant declarations are not necessary, and are currently just ignored.

Special top-level assignments, such as those performed by the `function` and `struct` keywords,
are constant by default.
Expand Down
11 changes: 9 additions & 2 deletions src/codegen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3012,8 +3012,15 @@ static Value *global_binding_pointer(jl_codectx_t &ctx, jl_module_t *m, jl_sym_t
{
jl_binding_t *b = NULL;
if (assign) {
b = jl_get_binding_wr(m, s);
b = jl_get_binding_wr(m, s, 0);
assert(b != NULL);
if (b->owner != m) {
char *msg;
asprintf(&msg, "cannot assign variable %s.%s from module %s",
jl_symbol_name(b->owner->name), jl_symbol_name(s), jl_symbol_name(m->name));
emit_error(ctx, msg);
free(msg);
}
}
else {
b = jl_get_binding(m, s);
Expand Down Expand Up @@ -3748,7 +3755,7 @@ static jl_cgval_t emit_expr(jl_codectx_t &ctx, jl_value_t *expr)
jl_ptls_t ptls = jl_get_ptls_states();
jl_value_t *e = ptls->exception_in_transit;
// errors. boo. root it somehow :(
bnd = jl_get_binding_wr(ctx.module, (jl_sym_t*)jl_gensym());
bnd = jl_get_binding_wr(ctx.module, (jl_sym_t*)jl_gensym(), 1);
bnd->value = e;
bnd->constp = 1;
raise_exception(ctx, literal_pointer_val(ctx, e));
Expand Down
4 changes: 2 additions & 2 deletions src/dump.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1515,7 +1515,7 @@ static jl_value_t *jl_deserialize_value_module(jl_serializer_state *s)
jl_sym_t *name = (jl_sym_t*)jl_deserialize_value(s, NULL);
if (name == NULL)
break;
jl_binding_t *b = jl_get_binding_wr(m, name);
jl_binding_t *b = jl_get_binding_wr(m, name, 1);
b->value = jl_deserialize_value(s, &b->value);
jl_gc_wb_buf(m, b, sizeof(jl_binding_t));
if (b->value != NULL) jl_gc_wb(m, b->value);
Expand Down Expand Up @@ -1981,7 +1981,7 @@ static void jl_reinit_item(jl_value_t *v, int how, arraylist_t *tracee_list)
}
case 2: { // reinsert module v into parent (const)
jl_module_t *mod = (jl_module_t*)v;
jl_binding_t *b = jl_get_binding_wr(mod->parent, mod->name);
jl_binding_t *b = jl_get_binding_wr(mod->parent, mod->name, 1);
jl_declare_constant(b); // this can throw
if (b->value != NULL) {
if (!jl_is_module(b->value)) {
Expand Down
48 changes: 12 additions & 36 deletions src/interpreter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,20 +302,15 @@ static jl_value_t *eval(jl_value_t *e, interpreter_state *s)
assert(jl_expr_nargs(ex) != 1 || jl_is_symbol(fname));

if (jl_is_symbol(fname)) {
jl_value_t **bp=NULL;
jl_value_t *bp_owner=NULL;
jl_binding_t *b=NULL;
if (bp == NULL) {
b = jl_get_binding_for_method_def(modu, fname);
bp = &b->value;
bp_owner = (jl_value_t*)modu;
}
jl_value_t *gf = jl_generic_function_def(fname, modu, bp, bp_owner, b);
jl_value_t *bp_owner = (jl_value_t*)modu;
jl_binding_t *b = jl_get_binding_for_method_def(modu, fname);
jl_value_t **bp = &b->value;
jl_value_t *gf = jl_generic_function_def(b->name, b->owner, bp, bp_owner, b);
if (jl_expr_nargs(ex) == 1)
return gf;
}

jl_value_t *atypes=NULL, *meth=NULL;
jl_value_t *atypes = NULL, *meth = NULL;
JL_GC_PUSH2(&atypes, &meth);
atypes = eval(args[1], s);
meth = eval(args[2], s);
Expand All @@ -330,26 +325,10 @@ static jl_value_t *eval(jl_value_t *e, interpreter_state *s)
sym = jl_globalref_name(sym);
}
assert(jl_is_symbol(sym));
jl_binding_t *b = jl_get_binding_wr(modu, sym);
jl_binding_t *b = jl_get_binding_wr(modu, sym, 1);
jl_declare_constant(b);
return (jl_value_t*)jl_nothing;
}
else if (ex->head == global_sym) {
// create uninitialized mutable binding for "global x" decl
// TODO: handle type decls
size_t i, l = jl_array_len(ex->args);
for (i = 0; i < l; i++) {
jl_sym_t *gsym = (jl_sym_t*)args[i];
jl_module_t *gmodu = modu;
if (jl_is_globalref(gsym)) {
gmodu = jl_globalref_mod(gsym);
gsym = jl_globalref_name(gsym);
}
assert(jl_is_symbol(gsym));
jl_get_binding_wr(gmodu, gsym);
}
return (jl_value_t*)jl_nothing;
}
else if (ex->head == abstracttype_sym) {
if (inside_typedef)
jl_error("cannot eval a new abstract type definition while defining another type");
Expand All @@ -368,7 +347,7 @@ static jl_value_t *eval(jl_value_t *e, interpreter_state *s)
assert(jl_is_symbol(name));
dt = jl_new_abstracttype(name, modu, NULL, (jl_svec_t*)para);
w = dt->name->wrapper;
jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name);
jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1);
temp = b->value;
check_can_assign_type(b, w);
b->value = w;
Expand Down Expand Up @@ -416,7 +395,7 @@ static jl_value_t *eval(jl_value_t *e, interpreter_state *s)
jl_symbol_name((jl_sym_t*)name));
dt = jl_new_primitivetype(name, modu, NULL, (jl_svec_t*)para, nb);
w = dt->name->wrapper;
jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name);
jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1);
temp = b->value;
check_can_assign_type(b, w);
b->value = w;
Expand Down Expand Up @@ -461,7 +440,7 @@ static jl_value_t *eval(jl_value_t *e, interpreter_state *s)
0, args[5]==jl_true ? 1 : 0, jl_unbox_long(args[6]));
w = dt->name->wrapper;

jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name);
jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)name, 1);
temp = b->value; // save old value
// temporarily assign so binding is available for field types
check_can_assign_type(b, w);
Expand Down Expand Up @@ -573,17 +552,14 @@ static jl_value_t *eval_body(jl_array_t *stmts, interpreter_state *s, int start,
s->locals[n-1] = rhs;
}
else {
jl_module_t *m;
jl_module_t *modu = s->module;
if (jl_is_globalref(sym)) {
m = jl_globalref_mod(sym);
modu = jl_globalref_mod(sym);
sym = (jl_value_t*)jl_globalref_name(sym);
}
else {
m = s->module;
}
assert(jl_is_symbol(sym));
JL_GC_PUSH1(&rhs);
jl_binding_t *b = jl_get_binding_wr(m, (jl_sym_t*)sym);
jl_binding_t *b = jl_get_binding_wr(modu, (jl_sym_t*)sym, 1);
jl_checked_assignment(b, rhs);
JL_GC_POP();
}
Expand Down
91 changes: 49 additions & 42 deletions src/julia-syntax.scm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2717,8 +2717,8 @@
(let ((vi (var-info-for (cadr e) env)))
(vinfo:set-never-undef! vi #t)))
((=)
(let ((vi (var-info-for (cadr e) env)))
(if vi
(let ((vi (and (symbol? (cadr e)) (var-info-for (cadr e) env))))
(if vi ; if local or captured
(begin (if (vinfo:asgn vi)
(vinfo:set-sa! vi #f)
(vinfo:set-sa! vi #t))
Expand Down Expand Up @@ -2861,35 +2861,45 @@ f(x) = yt(x)
;; when doing this, the original value needs to be preserved, to
;; ensure the expression `a=b` always returns exactly `b`.
(define (convert-assignment var rhs0 fname lam interp)
(let* ((vi (assq var (car (lam:vinfo lam))))
(cv (assq var (cadr (lam:vinfo lam))))
(vt (or (and vi (vinfo:type vi))
(and cv (vinfo:type cv))
'(core Any)))
(closed (and cv (vinfo:asgn cv) (vinfo:capt cv)))
(capt (and vi (vinfo:asgn vi) (vinfo:capt vi))))
(if (and (not closed) (not capt) (equal? vt '(core Any)))
`(= ,var ,rhs0)
(let* ((rhs1 (if (or (ssavalue? rhs0) (simple-atom? rhs0)
(equal? rhs0 '(the_exception)))
rhs0
(make-ssavalue)))
(rhs (if (equal? vt '(core Any))
rhs1
(convert-for-type-decl rhs1 (cl-convert vt fname lam #f #f interp))))
(ex (cond (closed `(call (core setfield!)
,(if interp
`($ ,var)
`(call (core getfield) ,fname (inert ,var)))
(inert contents)
,rhs))
(capt `(call (core setfield!) ,var (inert contents) ,rhs))
(else `(= ,var ,rhs)))))
(if (eq? rhs1 rhs0)
`(block ,ex ,rhs0)
`(block (= ,rhs1 ,rhs0)
,ex
,rhs1))))))
(cond
((symbol? var)
(let* ((vi (assq var (car (lam:vinfo lam))))
(cv (assq var (cadr (lam:vinfo lam))))
(vt (or (and vi (vinfo:type vi))
(and cv (vinfo:type cv))
'(core Any)))
(closed (and cv (vinfo:asgn cv) (vinfo:capt cv)))
(capt (and vi (vinfo:asgn vi) (vinfo:capt vi))))
(if (and (not closed) (not capt) (equal? vt '(core Any)))
`(= ,var ,rhs0)
(let* ((rhs1 (if (or (ssavalue? rhs0) (simple-atom? rhs0)
(equal? rhs0 '(the_exception)))
rhs0
(make-ssavalue)))
(rhs (if (equal? vt '(core Any))
rhs1
(convert-for-type-decl rhs1 (cl-convert vt fname lam #f #f interp))))
(ex (cond (closed `(call (core setfield!)
,(if interp
`($ ,var)
`(call (core getfield) ,fname (inert ,var)))
(inert contents)
,rhs))
(capt `(call (core setfield!) ,var (inert contents) ,rhs))
(else `(= ,var ,rhs)))))
(if (eq? rhs1 rhs0)
`(block ,ex ,rhs0)
`(block (= ,rhs1 ,rhs0)
,ex
,rhs1))))))
((and (pair? var) (or (eq? (car var) 'outerref)
(eq? (car var) 'globalref)))

`(= ,var ,rhs0))
((ssavalue? var)
`(= ,var ,rhs0))
(else
(error (string "invalid assignment location \"" (deparse var) "\"")))))
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a good refactoring. Am I right that there's no functional change except for this error?

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. I was thinking of using this code-path to test for globals, but ended up deciding to put it elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

maybe put this in a separate PR, especially if it's a completely independent change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's largely just a re-indentation to accommodate printing the error message. It seems that lisp just usually makes trivial edits require re-indenting the entire function :(


;; replace leading (function) argument type with `typ`
(define (fix-function-arg-type te typ iskw namemap type-sp)
Expand Down Expand Up @@ -3076,9 +3086,7 @@ f(x) = yt(x)
((=)
(let ((var (cadr e))
(rhs (cl-convert (caddr e) fname lam namemap toplevel interp)))
(if (ssavalue? var)
`(= ,var ,rhs)
(convert-assignment var rhs fname lam interp))))
(convert-assignment var rhs fname lam interp)))
((local-def) ;; make new Box for local declaration of defined variable
(let ((vi (assq (cadr e) (car (lam:vinfo lam)))))
(if (and vi (vinfo:asgn vi) (vinfo:capt vi))
Expand Down Expand Up @@ -3120,10 +3128,10 @@ f(x) = yt(x)
(lam2 (if short #f (cadddr e)))
(vis (if short '(() () ()) (lam:vinfo lam2)))
(cvs (map car (cadr vis)))
(local? (lambda (s) (and (symbol? s)
(local? (lambda (s) (and lam (symbol? s)
(or (assq s (car (lam:vinfo lam)))
(assq s (cadr (lam:vinfo lam)))))))
(local (and lam (local? name)))
(local (local? name))
(sig (and (not short) (caddr e)))
(sp-inits (if (or short (not (eq? (car sig) 'block)))
'()
Expand Down Expand Up @@ -3200,7 +3208,7 @@ f(x) = yt(x)
(and (symbol? s)
(not (eq? name s))
(not (memq s capt-sp))
(or ;(local? s) ; TODO: make this work for local variables too?
(or ;(local? s) ; TODO: error for local variables
(memq s (lam:sp lam)))))))
(caddr methdef)
(lambda (e) (cadr e)))))
Expand Down Expand Up @@ -3326,7 +3334,7 @@ f(x) = yt(x)
;; numbered slots (or be simple immediate values), and then those will be the
;; only possible returned values.
(define (compile-body e vi lam)
(let ((code '())
(let ((code '()) ;; statements (emitted in reverse order)
(filename 'none)
(first-line #t)
(current-loc #f)
Expand Down Expand Up @@ -3628,13 +3636,12 @@ f(x) = yt(x)
(if (not (and (pair? code) (equal? (car code) e)))
(emit e)
#f))
((global) ; remove global declarations
((global) ; keep global declarations as statements
(if value (error "misplaced \"global\" declaration"))
(let ((vname (cadr e)))
(if (var-info-for vname vi)
;; issue #7264
(if (var-info-for vname vi) ;; issue #7264
(error (string "`global " vname "`: " vname " is local variable in the enclosing scope"))
#f)))
(emit e))))
((local-def) #f)
((local) #f)
((implicit-global) #f)
Expand Down
2 changes: 1 addition & 1 deletion src/julia.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,7 +1236,7 @@ JL_DLLEXPORT jl_binding_t *jl_get_binding(jl_module_t *m, jl_sym_t *var);
JL_DLLEXPORT jl_binding_t *jl_get_binding_or_error(jl_module_t *m, jl_sym_t *var);
JL_DLLEXPORT jl_value_t *jl_module_globalref(jl_module_t *m, jl_sym_t *var);
// get binding for assignment
JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m, jl_sym_t *var);
JL_DLLEXPORT jl_binding_t *jl_get_binding_wr(jl_module_t *m, jl_sym_t *var, int error);
JL_DLLEXPORT jl_binding_t *jl_get_binding_for_method_def(jl_module_t *m,
jl_sym_t *var);
JL_DLLEXPORT int jl_boundp(jl_module_t *m, jl_sym_t *var);
Expand Down
Loading