more predictable global binding resolution

This changes the meaning of `global` from being a directive when used at toplevel, which forces the introduction of a new global when used in certain contexts, to always being just an scope annotation that there should exist a global binding for the given name. fix #18933 fix #17387 (for the syntactic case)
JuliaLang · Aug 3, 2017 · d3e52ba · d3e52ba
1 parent 3e37997
commit d3e52ba
Show file tree

Hide file tree

Showing 16 changed files with 199 additions and 146 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -32,6 +32,15 @@ Language changes
   * `{ }` expressions now use `braces` and `bracescat` as expression heads instead
     of `cell1d` and `cell2d`, and parse similarly to `vect` and `vcat` ([#8470]).
 
+  * 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
 ----------------
 

diff --git a/base/poll.jl b/base/poll.jl
@@ -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)

diff --git a/doc/src/manual/modules.md b/doc/src/manual/modules.md
@@ -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
 
@@ -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
 ```
 

diff --git a/doc/src/manual/variables-and-scoping.md b/doc/src/manual/variables-and-scoping.md
@@ -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,
@@ -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;
@@ -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.

diff --git a/src/codegen.cpp b/src/codegen.cpp
@@ -3000,8 +3000,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);
@@ -3736,7 +3743,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));

diff --git a/src/dump.c b/src/dump.c
@@ -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);
@@ -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)) {

diff --git a/src/interpreter.c b/src/interpreter.c
@@ -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);
@@ -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");
@@ -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;
@@ -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;
@@ -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);
@@ -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();
                 }

diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm
@@ -2704,8 +2704,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))
@@ -2848,35 +2848,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) "\"")))))
 
 ;; replace leading (function) argument type with `typ`
 (define (fix-function-arg-type te typ iskw namemap type-sp)
@@ -3063,9 +3073,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))
@@ -3107,10 +3115,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)))
                                 '()
@@ -3187,7 +3195,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)))))
@@ -3313,7 +3321,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)
@@ -3615,13 +3623,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)

diff --git a/src/julia.h b/src/julia.h
@@ -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);